Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
Re: Unlock ip6_sysctl()
To:
Vitaliy Makkoveev <mvs@openbsd.org>
Cc:
tech@openbsd.org
Date:
Wed, 30 Jul 2025 22:12:40 +0200

Download raw body.

Thread
  • Vitaliy Makkoveev:

    Unlock ip6_sysctl()

    • Alexander Bluhm:

      Unlock ip6_sysctl()

On Sun, Jul 27, 2025 at 10:31:01PM +0300, Vitaliy Makkoveev wrote:
> Use temporary local buffer for lockless IO within ip6_sysctl_soiikey().
> Use existing `sysctl_lock' rwlock(9) to protect `ip6_soiikey'. Also,
> replace temporary `ip_sysctl_lock' with `sysctl_lock' rwlock(9). It was
> introduced as temporary to avoid recursive lock acquisition in
> ip*_sysctl() until upcoming unlocked. The usage path are not the hot
> paths, no reason to have dedicated locks for them.
> 
> I want to deny negative values for 'net.inet6.ip6.neighborgcthresh' and
> 'net.inet6.ip6.maxdynroutes' with the following diffs. Negative values
> mean unlimited count of in-kernel objects and this is definitely bad
> idea. During ip6_sysctl() unlocking I found some sysctl() variables
> already modified, but these were forgotten.
> 
> const struct sysctl_bounded_args ipv6ctl_vars[] = {
>     /* ... */
>     { IPV6CTL_NEIGHBORGCTHRESH, &ip6_neighborgcthresh, -1, 5 * 2048},
>     { IPV6CTL_MAXDYNROUTES, &ip6_maxdynroutes, -1, 5 * 4096},
> };

OK bluhm@

> Index: sys/netinet/ip_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.424
> diff -u -p -r1.424 ip_input.c
> --- sys/netinet/ip_input.c	24 Jul 2025 22:31:19 -0000	1.424
> +++ sys/netinet/ip_input.c	27 Jul 2025 18:58:30 -0000
> @@ -1723,9 +1723,6 @@ ip_forward(struct mbuf *m, struct ifnet 
>  
>  #ifndef SMALL_KERNEL
>  
> -/* Temporary, to avoid sysctl_lock recursion. */
> -struct rwlock ip_sysctl_lock = RWLOCK_INITIALIZER("ipslk");
> -
>  int
>  ip_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
>      size_t newlen)
> @@ -1759,10 +1756,10 @@ ip_sysctl(int *name, u_int namelen, void
>  		error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
>  		    &newval, 0, INT_MAX);
>  		if (error == 0 && oldval != newval) {
> -			rw_enter_write(&ip_sysctl_lock);
> +			rw_enter_write(&sysctl_lock);
>  			atomic_store_int(&ip_mtudisc_timeout, newval);
>  			rt_timer_queue_change(&ip_mtudisc_timeout_q, newval);
> -			rw_exit_write(&ip_sysctl_lock);
> +			rw_exit_write(&sysctl_lock);
>  		}
>  
>  		return (error);
> Index: sys/netinet/ip_var.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_var.h,v
> retrieving revision 1.122
> diff -u -p -r1.122 ip_var.h
> --- sys/netinet/ip_var.h	21 Jun 2025 14:21:17 -0000	1.122
> +++ sys/netinet/ip_var.h	27 Jul 2025 18:58:30 -0000
> @@ -285,9 +285,5 @@ int	 rip_send(struct socket *, struct mb
>  extern struct socket *ip_mrouter[];	/* multicast routing daemon */
>  #endif
>  
> -#ifndef SMALL_KERNEL
> -extern struct rwlock ip_sysctl_lock;
> -#endif
> -
>  #endif /* _KERNEL */
>  #endif /* _NETINET_IP_VAR_H_ */
> Index: sys/netinet6/in6_proto.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
> retrieving revision 1.148
> diff -u -p -r1.148 in6_proto.c
> --- sys/netinet6/in6_proto.c	27 Jul 2025 17:46:58 -0000	1.148
> +++ sys/netinet6/in6_proto.c	27 Jul 2025 18:58:30 -0000
> @@ -120,6 +120,7 @@ const struct protosw inet6sw[] = {
>  {
>    .pr_domain	= &inet6domain,
>    .pr_protocol	= IPPROTO_IPV6,
> +  .pr_flags	= PR_MPSYSCTL,
>    .pr_init	= ip6_init,
>    .pr_slowtimo	= frag6_slowtimo,
>    .pr_sysctl	= ip6_sysctl
> Index: sys/netinet6/ip6_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.293
> diff -u -p -r1.293 ip6_input.c
> --- sys/netinet6/ip6_input.c	27 Jul 2025 17:46:58 -0000	1.293
> +++ sys/netinet6/ip6_input.c	27 Jul 2025 18:58:30 -0000
> @@ -1484,14 +1484,27 @@ ip6_sysctl_ip6stat(void *oldp, size_t *o
>  int
>  ip6_sysctl_soiikey(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
>  {
> +	uint8_t soiikey[sizeof(ip6_soiikey)];
>  	int error;
>  
>  	error = suser(curproc);
>  	if (error != 0)
>  		return (error);
>  
> -	return (sysctl_struct(oldp, oldlenp, newp, newlen, ip6_soiikey,
> -	    sizeof(ip6_soiikey)));
> +	rw_enter_read(&sysctl_lock);
> +	memcpy(soiikey, ip6_soiikey, sizeof(ip6_soiikey));
> +	rw_exit_read(&sysctl_lock);
> +
> +	error = sysctl_struct(oldp, oldlenp, newp, newlen, soiikey,
> +	    sizeof(soiikey));
> +
> +	if (error == 0 && newp) {
> +		rw_enter_write(&sysctl_lock);
> +		memcpy(ip6_soiikey, soiikey, sizeof(soiikey));
> +		rw_exit_write(&sysctl_lock);
> +	}
> +
> +	return (error);
>  }
>  
>  int
> @@ -1533,10 +1546,10 @@ ip6_sysctl(int *name, u_int namelen, voi
>  		error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
>  		    &newval, 0, INT_MAX);
>  		if (error == 0 && oldval != newval) {
> -			rw_enter_write(&ip_sysctl_lock);
> +			rw_enter_write(&sysctl_lock);
>  			atomic_store_int(&ip6_mtudisc_timeout, newval);
>  			rt_timer_queue_change(&icmp6_mtudisc_timeout_q, newval);
> -			rw_exit_write(&ip_sysctl_lock);
> +			rw_exit_write(&sysctl_lock);
>  		}
>  
>  		return (error);
> Index: sys/sys/sysctl.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/sysctl.h,v
> retrieving revision 1.245
> diff -u -p -r1.245 sysctl.h
> --- sys/sys/sysctl.h	24 Jul 2025 19:42:41 -0000	1.245
> +++ sys/sys/sysctl.h	27 Jul 2025 18:58:30 -0000
> @@ -1030,6 +1030,8 @@ struct sysctl_bounded_args {
>   */
>  typedef int (sysctlfn)(int *, u_int, void *, size_t *, void *, size_t, struct proc *);
>  
> +extern struct rwlock sysctl_lock;
> +
>  int sysctl_vslock(void *, size_t);
>  void sysctl_vsunlock(void *, size_t);
>