From: Alexander Bluhm <alexander.bluhm@gmx.net>
Subject: net lock kernel lock order
To: tech@openbsd.org
Date: Fri, 5 Jan 2024 13:26:15 +0100

Hi,

We have code that takes kernel lock just before net lock.  As net
lock is a rwlock that releases kernel lock while sleeping, there
is never deadlock due to lock ordering.

The order kernel lock -> net lock is suboptimal.  If net lock is
held by another thread the following happens:

take kernel lock -> try net lock -> release kernel lock -> sleep
-> take net lock -> take kernel lock

If we change the order we get a less expensive sequence:

try net lock -> sleep -> take net lock -> take kernel lock

ok?

bluhm

Index: net/if.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
diff -u -p -r1.714 if.c
--- net/if.c	29 Dec 2023 11:43:04 -0000	1.714
+++ net/if.c	5 Jan 2024 12:08:01 -0000
@@ -1774,16 +1774,16 @@ if_linkstate_task(void *xifidx)
 	unsigned int ifidx = (unsigned long)xifidx;
 	struct ifnet *ifp;
 
-	KERNEL_LOCK();
 	NET_LOCK();
+	KERNEL_LOCK();
 
 	ifp = if_get(ifidx);
 	if (ifp != NULL)
 		if_linkstate(ifp);
 	if_put(ifp);
 
-	NET_UNLOCK();
 	KERNEL_UNLOCK();
+	NET_UNLOCK();
 }
 
 void
Index: netinet/in.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v
diff -u -p -r1.185 in.c
--- netinet/in.c	28 Jun 2023 11:49:49 -0000	1.185
+++ netinet/in.c	5 Jan 2024 12:10:07 -0000
@@ -260,8 +260,8 @@ in_ioctl(u_long cmd, caddr_t data, struc
 			return (error);
 	}
 
-	KERNEL_LOCK();
 	NET_LOCK();
+	KERNEL_LOCK();
 
 	TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
 		if (ifa->ifa_addr->sa_family != AF_INET)
@@ -331,8 +331,8 @@ in_ioctl(u_long cmd, caddr_t data, struc
 		break;
 	}
 err:
-	NET_UNLOCK();
 	KERNEL_UNLOCK();
+	NET_UNLOCK();
 	return (error);
 }
 
@@ -353,8 +353,8 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t 
 	if (error)
 		return (error);
 
-	KERNEL_LOCK();
 	NET_LOCK();
+	KERNEL_LOCK();
 
 	TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
 		if (ifa->ifa_addr->sa_family != AF_INET)
@@ -387,8 +387,8 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t 
 	if (!error)
 		if_addrhooks_run(ifp);
 
-	NET_UNLOCK();
 	KERNEL_UNLOCK();
+	NET_UNLOCK();
 	return error;
 }
 
@@ -409,8 +409,8 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
 			return (error);
 	}
 
-	KERNEL_LOCK();
 	NET_LOCK();
+	KERNEL_LOCK();
 
 	TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
 		if (ifa->ifa_addr->sa_family != AF_INET)
@@ -527,8 +527,8 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
 		panic("%s: invalid ioctl %lu", __func__, cmd);
 	}
 
-	NET_UNLOCK();
 	KERNEL_UNLOCK();
+	NET_UNLOCK();
 	return (error);
 }