Re: [PATCH net-next] net: let lockdep compare instance locks

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 16 May 2025 10:14:41 -0700
> On Fri, 16 May 2025 08:22:43 -0700 Jakub Kicinski wrote:
> > On Thu, 15 May 2025 19:59:41 -0700 Kuniyuki Iwashima wrote:
> > > > Is the thinking that once the big rtnl lock disappears in cleanup_net
> > > > the devices are safe to destroy without any locking because there can't
> > > > be any live users trying to access them?    
> > > 
> > > I hope yes, but removing VF via sysfs and removing netns might
> > > race and need some locking ?  
> > 
> > I think we should take the small lock around default_device_exit_net()
> > and then we'd be safe?

Agree.  The 'queuing dev for destruction' part will be only racy.


> > Either a given VF gets moved to init_net first
> > or the sysfs gets to it and unregisters it safely in the old netns.
> 
> Thinking about it some more, we'll have to revisit this problem before
> removing the big lock, anyway. I'm leaning towards doing this for now:

This looks good to me.


> 
> diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
> index 2a753813f849..c345afecd4c5 100644
> --- a/include/net/netdev_lock.h
> +++ b/include/net/netdev_lock.h
> @@ -99,16 +99,15 @@ static inline void netdev_unlock_ops_compat(struct net_device *dev)
>  static inline int netdev_lock_cmp_fn(const struct lockdep_map *a,
>  				     const struct lockdep_map *b)
>  {
> -	/* Only lower devices currently grab the instance lock, so no
> -	 * real ordering issues can occur. In the near future, only
> -	 * hardware devices will grab instance lock which also does not
> -	 * involve any ordering. Suppress lockdep ordering warnings
> -	 * until (if) we start grabbing instance lock on pure SW
> -	 * devices (bond/team/veth/etc).
> -	 */
>  	if (a == b)
>  		return 0;
> -	return -1;
> +
> +	/* Allow locking multiple devices only under rtnl_lock,
> +	 * the exact order doesn't matter.
> +	 * Note that upper devices don't lock their ops, so nesting
> +	 * mostly happens during batched device removal for now.
> +	 */
> +	return lockdep_rtnl_is_held() ? -1 : 1;
>  }
>  
>  #define netdev_lockdep_set_classes(dev)				\