From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: net lock kernel lock order
To: Claudio Jeker <cjeker@diehard.n-r-g.com>
Cc: alexander.bluhm@gmx.net, tech@openbsd.org
Date: Fri, 05 Jan 2024 18:50:03 +0100

> Date: Fri, 5 Jan 2024 17:32:27 +0100
> From: Claudio Jeker <cjeker@diehard.n-r-g.com>
> 
> On Fri, Jan 05, 2024 at 03:24:44PM +0100, Alexander Bluhm wrote:
> > On Fri, Jan 05, 2024 at 03:05:44PM +0100, Claudio Jeker wrote:
> > > On Fri, Jan 05, 2024 at 01:26:15PM +0100, Alexander Bluhm wrote:
> > > > 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
> > > 
> > > this is incorrect. the kernel lock is aquired before the rwlock in a sleep
> > > call.
> > 
> > Yes.  The kernel lock dance is done in mi_switch().  So the correct
> > old sequence is:
> > 
> > take kernel lock -> try net lock -> release kernel lock -> sleep
> > -> take kernel lock -> take net lock
> > 
> > But that does not change my argument that the sequence with my diff
> > is cheaper.
> 
> But is this lock reversal safe? Is there no NET_LOCK somewhere in an
> interrupt handler that could run while KERNEL_LOCK is held?

Since NET_LOCK is an rwlock and may sleep, it shouldn't be used in
interrupt context.  Doing so would panic the kernel as soon as we hit
mi_switch().

> > > > 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);
> > > >  }
> > > >  
> > > > 
> > > 
> > > -- 
> > > :wq Claudio
> > 
> 
> -- 
> :wq Claudio
> 
>