From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: udp, sysctl: unlock UDPCTL_ROOTONLY and UDPCTL_BADDYNAMIC
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 12 May 2025 14:57:51 +0200

On Sat, May 10, 2025 at 06:24:24AM +0300, Vitaliy Makkoveev wrote:
> sysctl(2) is not the hot path, but I want to remove kernel lock with
> vslock() and relax netlock pressure here.
> 
> Use temporary buffer to exchange data with the userland. So we need to
> take exclusive netlock only if we modify values. The shared netlock is
> still required even if we read values, but it doesn't stop packets
> processing in the most paths.
> 
> I have more complicated version of this diff which introduces the mutex
> for `baddynamicports' protection, so the netlock could be avoided here.
> However this requires to take this mutex in the in_pcbbind() path and I
> see no reason to do this now. sysctl(2) is not the hot path, the most
> common read-only case doesn't stop the netstack and doesn't take kernel
> lock, I'm fine with it. I like to use producer/consumer locking here
> instead of mutexes.

Some style nits.

> +		const size_t buflen = bufitems * sizeof(u_int32_t);
> +		uint32_t *buf;

We prefer the newer uint32_t over u_int32_t.  Please use that
consistently here.

> +		if ((error == 0) && newp) {

The extra () within an if condition tell the compler not to issue
an warning if = is uses instead of ==.

if ((error = 0) && newp) compiles perfectly
if (error = 0 && newp) gives compiler warning
if (error == 0 && newp) should be used here

OK bluhm@

> The TCP related `rootonlyports' and `baddynamicports' could be
> refactored in the same way.
> 
> Index: sys/netinet/udp_usrreq.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/udp_usrreq.c,v
> retrieving revision 1.336
> diff -u -p -r1.336 udp_usrreq.c
> --- sys/netinet/udp_usrreq.c	11 Mar 2025 15:31:03 -0000	1.336
> +++ sys/netinet/udp_usrreq.c	10 May 2025 02:23:08 -0000
> @@ -174,7 +174,6 @@ void	udp_sbappend(struct inpcb *, struct
>  	    u_int32_t, struct netstack *);
>  int	udp_output(struct inpcb *, struct mbuf *, struct mbuf *, struct mbuf *);
>  void	udp_notify(struct inpcb *, int);
> -int	udp_sysctl_locked(int *, u_int, void *, size_t *, void *, size_t);
>  int	udp_sysctl_udpstat(void *, size_t *, void *);
>  
>  #ifndef	UDB_INITIAL_HASH_SIZE
> @@ -1278,16 +1277,37 @@ udp_sysctl(int *name, u_int namelen, voi
>  		return (ENOTDIR);
>  
>  	switch (name[0]) {
> -	case UDPCTL_BADDYNAMIC:
> -	case UDPCTL_ROOTONLY: {
> -		size_t savelen = *oldlenp;
> +	case UDPCTL_ROOTONLY:
> +		if (newp && (int)atomic_load_int(&securelevel) > 0)
> +			return (EPERM);
> +		/* FALLTHROUGH */
> +	case UDPCTL_BADDYNAMIC: {
> +		struct baddynamicports *ports = (name[0] == UDPCTL_ROOTONLY ?
> +		    &rootonlyports : &baddynamicports);
> +		const size_t bufitems = DP_MAPSIZE;
> +		const size_t buflen = bufitems * sizeof(u_int32_t);
> +		size_t i;
> +		uint32_t *buf;
>  		int error;
>  
> -		if ((error = sysctl_vslock(oldp, savelen)))
> -			return (error);
> -		error = udp_sysctl_locked(name, namelen, oldp, oldlenp,
> -		    newp, newlen);
> -		sysctl_vsunlock(oldp, savelen);
> +		buf = malloc(buflen, M_SYSCTL, M_WAITOK | M_ZERO);
> +
> +		NET_LOCK_SHARED();
> +		for (i = 0; i < bufitems; ++i)
> +			buf[i] = ports->udp[i];
> +		NET_UNLOCK_SHARED();
> +
> +		error = sysctl_struct(oldp, oldlenp, newp, newlen,
> +		    buf, buflen);
> +
> +		if ((error == 0) && newp) {
> +			NET_LOCK();
> +			for (i = 0; i < bufitems; ++i)
> +				ports->udp[i] = buf[i];
> +			NET_UNLOCK();
> +		}
> +
> +		free(buf, M_SYSCTL, buflen);
>  
>  		return (error);
>  	}
> @@ -1302,33 +1322,6 @@ udp_sysctl(int *name, u_int namelen, voi
>  		    name, namelen, oldp, oldlenp, newp, newlen));
>  	}
>  	/* NOTREACHED */
> -}
> -
> -int
> -udp_sysctl_locked(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> -    void *newp, size_t newlen)
> -{
> -	int error = ENOPROTOOPT;
> -
> -	switch (name[0]) {
> -	case UDPCTL_BADDYNAMIC:
> -		NET_LOCK();
> -		error = sysctl_struct(oldp, oldlenp, newp, newlen,
> -		    baddynamicports.udp, sizeof(baddynamicports.udp));
> -		NET_UNLOCK();
> -		break;
> -
> -	case UDPCTL_ROOTONLY:
> -		if (newp && securelevel > 0)
> -			return (EPERM);
> -		NET_LOCK();
> -		error = sysctl_struct(oldp, oldlenp, newp, newlen,
> -		    rootonlyports.udp, sizeof(rootonlyports.udp));
> -		NET_UNLOCK();
> -		break;
> -	}
> -
> -	return (error);
>  }
>  
>  int