Re: [PATCH v1 net-next 4/4] net: Hold rtnl_net_lock() in (un)?register_netdevice_notifier_dev_net().

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Yael Chemla <ychemla@nvidia.com>
Date: Mon, 20 Jan 2025 20:55:07 +0200
> >>> diff --git a/net/core/dev.c b/net/core/dev.c
> >>> index f6c6559e2548..a0dd34463901 100644
> >>> --- a/net/core/dev.c
> >>> +++ b/net/core/dev.c
> >>> @@ -1943,15 +1943,17 @@ int register_netdevice_notifier_dev_net(struct net_device *dev,
> >>>    					struct notifier_block *nb,
> >>>    					struct netdev_net_notifier *nn)
> >>>    {
> >>> +	struct net *net = dev_net(dev);
> >>
> >> it seems to happen since the net pointer is acquired here without a lock.
> >> Note that KASAN issue is not triggered when executing with rtnl_lock()
> >> taken before this line. and our kernel .config expands
> >> rtnl_net_lock(net) to rtnl_lock() (CONFIG_DEBUG_NET_SMALL_RTNL is not set).
> > 
> > It sounds like the device was being moved to another netns while
> > unregister_netdevice_notifier_dev_net() was called.
> > 
> > Could you check if dev_net() is changed before/after rtnl_lock() in
> > 
> >    * register_netdevice_notifier_dev_net()
> >    * unregister_netdevice_notifier_dev_net()
> > 
> > ?
> 
> When checking dev_net before and after taking the lock the issue won’t 
> reproduce.
> note that when issue reproduce we arrive to 
> unregister_netdevice_notifier_dev_net with an invalid net pointer 
> (verified it with prints of its value, and it's not the same consistent 
> value as is throughout rest of the test).

Does an invalid net pointer means a dead netns pointer ?
dev_net() and dev_net_set() use rcu_dereference() and rcu_assign_pointer(),
so I guess it should not be an invalid address at least.


> we suspect the issue related to the async ns deletion.

I think async netns change would trigger the issue too.

Could you try this patch ?

---8<---
diff --git a/net/core/dev.c b/net/core/dev.c
index afa2282f2604..f4438ec24683 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2070,20 +2070,50 @@ static void __move_netdevice_notifier_net(struct net *src_net,
 	__register_netdevice_notifier_net(dst_net, nb, true);
 }
 
+static void rtnl_net_dev_lock(struct net_device *dev)
+{
+	struct net *net;
+
+again:
+	/* netns might be being dismantled. */
+	net = maybe_get_net(dev_net(dev));
+	if (!net) {
+		cond_resched();
+		goto again;
+	}
+
+	rtnl_net_lock(net);
+
+	/* dev might be moved to another netns. */
+	if (!net_eq(net, dev_net(dev))) {
+		rtnl_net_unlock(net);
+		put_net(net);
+		cond_resched();
+		goto again;
+	}
+}
+
+static void rtnl_net_dev_unlock(struct net_device *dev)
+{
+	struct net *net = dev_net(dev);
+
+	rtnl_net_unlock(net);
+	put_net(net);
+}
+
 int register_netdevice_notifier_dev_net(struct net_device *dev,
 					struct notifier_block *nb,
 					struct netdev_net_notifier *nn)
 {
-	struct net *net = dev_net(dev);
 	int err;
 
-	rtnl_net_lock(net);
-	err = __register_netdevice_notifier_net(net, nb, false);
+	rtnl_net_dev_lock(dev);
+	err = __register_netdevice_notifier_net(dev_net(dev), nb, false);
 	if (!err) {
 		nn->nb = nb;
 		list_add(&nn->list, &dev->net_notifier_list);
 	}
-	rtnl_net_unlock(net);
+	rtnl_net_dev_unlock(dev);
 
 	return err;
 }
@@ -2093,13 +2123,12 @@ int unregister_netdevice_notifier_dev_net(struct net_device *dev,
 					  struct notifier_block *nb,
 					  struct netdev_net_notifier *nn)
 {
-	struct net *net = dev_net(dev);
 	int err;
 
-	rtnl_net_lock(net);
+	rtnl_net_dev_lock(dev);
 	list_del(&nn->list);
-	err = __unregister_netdevice_notifier_net(net, nb);
-	rtnl_net_unlock(net);
+	err = __unregister_netdevice_notifier_net(dev_net(dev), nb);
+	rtnl_net_dev_unlock(dev);
 
 	return err;
 }
---8<---