From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: udp, sysctl: unlock UDPCTL_ROOTONLY and UDPCTL_BADDYNAMIC
To: Alexander Bluhm <bluhm@openbsd.org>, tech@openbsd.org
Date: Sat, 10 May 2025 06:24:24 +0300

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.

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