[PATCH v4 net 0/3] net: Fix race of rtnl_net_lock(dev_net(dev)).

[PATCH v4 net 0/3] net: Fix race of rtnl_net_lock(dev_net(dev)).

[Kuniyuki Iwashima]

Yael Chemla reported that commit 7fb1073300a2 ("net: Hold rtnl_net_lock()
in (un)?register_netdevice_notifier_dev_net().") started to trigger KASAN's
use-after-free splat.

The problem is that dev_net(dev) fetched before rtnl_net_lock() might be
different after rtnl_net_lock().

The patch 2 fixes the issue by checking dev_net(dev) after rtnl_net_lock(),
and the patch 3 fixes the same potential issue that would emerge once RTNL
is removed.


Changes:
  v4:
    * Add patch 1
    * Fix build failure for !CONFIG_NET_NS in patch 2

  v3:
    * Bump net->passive instead of maybe_get_net()
    * Remove msleep(1) loop
    * Use rcu_access_pointer() instead of rcu_read_lock().

  v2:
    * Use dev_net_rcu()
    * Use msleep(1) instead of cond_resched() after maybe_get_net()
    * Remove cond_resched() after net_eq() check

  v1: https://lore.kernel.org/netdev/20250130232435.43622-1-kuniyu@amazon.com/


Kuniyuki Iwashima (3):
  net: Add net_passive_inc() and net_passive_dec().
  net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().
  dev: Use rtnl_net_dev_lock() in unregister_netdev().

 include/net/net_namespace.h | 11 ++++++++
 net/core/dev.c              | 51 +++++++++++++++++++++++++++++++------
 net/core/net_namespace.c    |  8 +++---
 3 files changed, 58 insertions(+), 12 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v4 net 1/3] net: Add net_passive_inc() and net_passive_dec().

[Kuniyuki Iwashima]

net_drop_ns() is NULL when CONFIG_NET_NS is disabled.

The next patch introduces a function that increments
and decrements net->passive.

As a prep, let's rename and export net_free() to
net_passive_dec() and add net_passive_inc().

Suggested-by: Eric Dumazet <edumazet@google.com>
Link: https://lore.kernel.org/netdev/CANn89i+oUCt2VGvrbrweniTendZFEh+nwS=uonc004-aPkWy-Q@mail.gmail.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/net_namespace.h | 11 +++++++++++
 net/core/net_namespace.c    |  8 ++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 7ba1402ca779..f467a66abc6b 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -297,6 +297,7 @@ static inline int check_net(const struct net *net)
 }
 
 void net_drop_ns(void *);
+void net_passive_dec(struct net *net);
 
 #else
 
@@ -326,8 +327,18 @@ static inline int check_net(const struct net *net)
 }
 
 #define net_drop_ns NULL
+
+static inline void net_passive_dec(struct net *net)
+{
+	refcount_dec(&net->passive);
+}
 #endif
 
+static inline void net_passive_inc(struct net *net)
+{
+	refcount_inc(&net->passive);
+}
+
 /* Returns true if the netns initialization is completed successfully */
 static inline bool net_initialized(const struct net *net)
 {
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index cb39a12b2f82..4303f2a49262 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -464,7 +464,7 @@ static void net_complete_free(void)
 
 }
 
-static void net_free(struct net *net)
+void net_passive_dec(struct net *net)
 {
 	if (refcount_dec_and_test(&net->passive)) {
 		kfree(rcu_access_pointer(net->gen));
@@ -482,7 +482,7 @@ void net_drop_ns(void *p)
 	struct net *net = (struct net *)p;
 
 	if (net)
-		net_free(net);
+		net_passive_dec(net);
 }
 
 struct net *copy_net_ns(unsigned long flags,
@@ -523,7 +523,7 @@ struct net *copy_net_ns(unsigned long flags,
 		key_remove_domain(net->key_domain);
 #endif
 		put_user_ns(user_ns);
-		net_free(net);
+		net_passive_dec(net);
 dec_ucounts:
 		dec_net_namespaces(ucounts);
 		return ERR_PTR(rv);
@@ -672,7 +672,7 @@ static void cleanup_net(struct work_struct *work)
 		key_remove_domain(net->key_domain);
 #endif
 		put_user_ns(net->user_ns);
-		net_free(net);
+		net_passive_dec(net);
 	}
 	cleanup_net_task = NULL;
 }
-- 
2.39.5 (Apple Git-154)

[PATCH v4 net 2/3] net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().

[Kuniyuki Iwashima]

After the cited commit, dev_net(dev) is fetched before holding RTNL
and passed to __unregister_netdevice_notifier_net().

However, dev_net(dev) might be different after holding RTNL.

In the reported case [0], while removing a VF device, its netns was
being dismantled and the VF was moved to init_net.

So the following sequence is basically illegal when dev was fetched
without lookup:

  net = dev_net(dev);
  rtnl_net_lock(net);

Let's use a new helper rtnl_net_dev_lock() to fix the race.

It fetches dev_net_rcu(dev), bumps its net->passive, and checks if
dev_net_rcu(dev) is changed after rtnl_net_lock().

[0]:
BUG: KASAN: slab-use-after-free in notifier_call_chain (kernel/notifier.c:75 (discriminator 2))
Read of size 8 at addr ffff88810cefb4c8 by task test-bridge-lag/21127
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl (lib/dump_stack.c:123)
 print_report (mm/kasan/report.c:379 mm/kasan/report.c:489)
 kasan_report (mm/kasan/report.c:604)
 notifier_call_chain (kernel/notifier.c:75 (discriminator 2))
 call_netdevice_notifiers_info (net/core/dev.c:2011)
 unregister_netdevice_many_notify (net/core/dev.c:11551)
 unregister_netdevice_queue (net/core/dev.c:11487)
 unregister_netdev (net/core/dev.c:11635)
 mlx5e_remove (drivers/net/ethernet/mellanox/mlx5/core/en_main.c:6552 drivers/net/ethernet/mellanox/mlx5/core/en_main.c:6579) mlx5_core
 auxiliary_bus_remove (drivers/base/auxiliary.c:230)
 device_release_driver_internal (drivers/base/dd.c:1275 drivers/base/dd.c:1296)
 bus_remove_device (./include/linux/kobject.h:193 drivers/base/base.h:73 drivers/base/bus.c:583)
 device_del (drivers/base/power/power.h:142 drivers/base/core.c:3855)
 mlx5_rescan_drivers_locked (./include/linux/auxiliary_bus.h:241 drivers/net/ethernet/mellanox/mlx5/core/dev.c:333 drivers/net/ethernet/mellanox/mlx5/core/dev.c:535 drivers/net/ethernet/mellanox/mlx5/core/dev.c:549) mlx5_core
 mlx5_unregister_device (drivers/net/ethernet/mellanox/mlx5/core/dev.c:468) mlx5_core
 mlx5_uninit_one (./include/linux/instrumented.h:68 ./include/asm-generic/bitops/instrumented-non-atomic.h:141 drivers/net/ethernet/mellanox/mlx5/core/main.c:1563) mlx5_core
 remove_one (drivers/net/ethernet/mellanox/mlx5/core/main.c:965 drivers/net/ethernet/mellanox/mlx5/core/main.c:2019) mlx5_core
 pci_device_remove (./include/linux/pm_runtime.h:129 drivers/pci/pci-driver.c:475)
 device_release_driver_internal (drivers/base/dd.c:1275 drivers/base/dd.c:1296)
 unbind_store (drivers/base/bus.c:245)
 kernfs_fop_write_iter (fs/kernfs/file.c:338)
 vfs_write (fs/read_write.c:587 (discriminator 1) fs/read_write.c:679 (discriminator 1))
 ksys_write (fs/read_write.c:732)
 do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1))
 entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
RIP: 0033:0x7f6a4d5018b7

Fixes: 7fb1073300a2 ("net: Hold rtnl_net_lock() in (un)?register_netdevice_notifier_dev_net().")
Reported-by: Yael Chemla <ychemla@nvidia.com>
Closes: https://lore.kernel.org/netdev/146eabfe-123c-4970-901e-e961b4c09bc3@nvidia.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
v4:
  * Fix build failure when !CONFIG_NET_NS
  * Use net_passive_dec()

v3:
  * Bump net->passive instead of maybe_get_net()
  * Remove msleep(1) loop
  * Use rcu_access_pointer() instead of rcu_read_lock().

v2:
  * Use dev_net_rcu().
  * Use msleep(1) instead of cond_resched() after maybe_get_net()
  * Remove cond_resched() after net_eq() check

v1: https://lore.kernel.org/netdev/20250130232435.43622-2-kuniyu@amazon.com/
---
 net/core/dev.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 55e356a68db6..6a39fb5baa92 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2070,6 +2070,39 @@ 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;
+
+#ifdef CONFIG_NET_NS
+again:
+#endif
+	/* netns might be being dismantled. */
+	rcu_read_lock();
+	net = dev_net_rcu(dev);
+	net_passive_inc(net);
+	rcu_read_unlock();
+
+	rtnl_net_lock(net);
+
+#ifdef CONFIG_NET_NS
+	/* dev might have been moved to another netns. */
+	if (!net_eq(net, rcu_access_pointer(dev->nd_net.net))) {
+		rtnl_net_unlock(net);
+		net_passive_dec(net);
+		goto again;
+	}
+#endif
+}
+
+static void rtnl_net_dev_unlock(struct net_device *dev)
+{
+	struct net *net = dev_net(dev);
+
+	rtnl_net_unlock(net);
+	net_passive_dec(net);
+}
+
 int register_netdevice_notifier_dev_net(struct net_device *dev,
 					struct notifier_block *nb,
 					struct netdev_net_notifier *nn)
@@ -2077,6 +2110,11 @@ int register_netdevice_notifier_dev_net(struct net_device *dev,
 	struct net *net = dev_net(dev);
 	int err;
 
+	/* rtnl_net_lock() assumes dev is not yet published by
+	 * register_netdevice().
+	 */
+	DEBUG_NET_WARN_ON_ONCE(!list_empty(&dev->dev_list));
+
 	rtnl_net_lock(net);
 	err = __register_netdevice_notifier_net(net, nb, false);
 	if (!err) {
@@ -2093,13 +2131,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;
 }
-- 
2.39.5 (Apple Git-154)

[PATCH v4 net 3/3] dev: Use rtnl_net_dev_lock() in unregister_netdev().

[Kuniyuki Iwashima]

The following sequence is basically illegal when dev was fetched
without lookup because dev_net(dev) might be different after holding
rtnl_net_lock():

  net = dev_net(dev);
  rtnl_net_lock(net);

Let's use rtnl_net_dev_lock() in unregister_netdev().

Note that there is no real bug in unregister_netdev() for now
because RTNL protects the scope even if dev_net(dev) is changed
before/after RTNL.

Fixes: 00fb9823939e ("dev: Hold per-netns RTNL in (un)?register_netdev().")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6a39fb5baa92..b6b1f597935f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11934,11 +11934,9 @@ EXPORT_SYMBOL(unregister_netdevice_many);
  */
 void unregister_netdev(struct net_device *dev)
 {
-	struct net *net = dev_net(dev);
-
-	rtnl_net_lock(net);
+	rtnl_net_dev_lock(dev);
 	unregister_netdevice(dev);
-	rtnl_net_unlock(net);
+	rtnl_net_dev_unlock(dev);
 }
 EXPORT_SYMBOL(unregister_netdev);
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v4 net 1/3] net: Add net_passive_inc() and net_passive_dec().

[Eric Dumazet]

On Wed, Feb 12, 2025 at 7:42 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> net_drop_ns() is NULL when CONFIG_NET_NS is disabled.
>
> The next patch introduces a function that increments
> and decrements net->passive.
>
> As a prep, let's rename and export net_free() to
> net_passive_dec() and add net_passive_inc().
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Link: https://lore.kernel.org/netdev/CANn89i+oUCt2VGvrbrweniTendZFEh+nwS=uonc004-aPkWy-Q@mail.gmail.com/
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v4 net 2/3] net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().

[Eric Dumazet]

On Wed, Feb 12, 2025 at 7:43 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> After the cited commit, dev_net(dev) is fetched before holding RTNL
> and passed to __unregister_netdevice_notifier_net().
>
> However, dev_net(dev) might be different after holding RTNL.
>
> In the reported case [0], while removing a VF device, its netns was
> being dismantled and the VF was moved to init_net.
>
> So the following sequence is basically illegal when dev was fetched
> without lookup:
>
>   net = dev_net(dev);
>   rtnl_net_lock(net);
>
> Let's use a new helper rtnl_net_dev_lock() to fix the race.
>
> It fetches dev_net_rcu(dev), bumps its net->passive, and checks if
> dev_net_rcu(dev) is changed after rtnl_net_lock().
>
>

> Fixes: 7fb1073300a2 ("net: Hold rtnl_net_lock() in (un)?register_netdevice_notifier_dev_net().")
> Reported-by: Yael Chemla <ychemla@nvidia.com>
> Closes: https://lore.kernel.org/netdev/146eabfe-123c-4970-901e-e961b4c09bc3@nvidia.com/
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v4 net 3/3] dev: Use rtnl_net_dev_lock() in unregister_netdev().

[Eric Dumazet]

On Wed, Feb 12, 2025 at 7:43 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> The following sequence is basically illegal when dev was fetched
> without lookup because dev_net(dev) might be different after holding
> rtnl_net_lock():
>
>   net = dev_net(dev);
>   rtnl_net_lock(net);
>
> Let's use rtnl_net_dev_lock() in unregister_netdev().
>
> Note that there is no real bug in unregister_netdev() for now
> because RTNL protects the scope even if dev_net(dev) is changed
> before/after RTNL.
>
> Fixes: 00fb9823939e ("dev: Hold per-netns RTNL in (un)?register_netdev().")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v4 net 2/3] net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().

[Jakub Kicinski]

On Wed, 12 Feb 2025 15:42:05 +0900 Kuniyuki Iwashima wrote:
> +static void rtnl_net_dev_lock(struct net_device *dev)
> +{
> +	struct net *net;
> +
> +#ifdef CONFIG_NET_NS
> +again:
> +#endif
> +	/* netns might be being dismantled. */
> +	rcu_read_lock();
> +	net = dev_net_rcu(dev);
> +	net_passive_inc(net);
> +	rcu_read_unlock();
> +
> +	rtnl_net_lock(net);
> +
> +#ifdef CONFIG_NET_NS
> +	/* dev might have been moved to another netns. */
> +	if (!net_eq(net, rcu_access_pointer(dev->nd_net.net))) {
> +		rtnl_net_unlock(net);
> +		net_passive_dec(net);
> +		goto again;
> +	}
> +#endif

Is there a plan to clean this up in net-next? Or perhaps after Eric's
dev_net() work? Otherwise I'm tempted to suggest to use a loop, maybe:

	bool again;

	do {
		again = false;

		/* netns might be being dismantled. */
		rcu_read_lock();
		net = dev_net_rcu(dev);
		net_passive_inc(net);
		rcu_read_unlock();

		rtnl_net_lock(net);

#ifdef CONFIG_NET_NS
		/* dev might have been moved to another netns. */
		if (!net_eq(net, rcu_access_pointer(dev->nd_net.net))) {
			rtnl_net_unlock(net);
			net_passive_dec(net);
			again = true;
		}
#endif
	} while (again);

Re: [PATCH v4 net 2/3] net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 13 Feb 2025 08:32:17 -0800
> On Wed, 12 Feb 2025 15:42:05 +0900 Kuniyuki Iwashima wrote:
> > +static void rtnl_net_dev_lock(struct net_device *dev)
> > +{
> > +	struct net *net;
> > +
> > +#ifdef CONFIG_NET_NS
> > +again:
> > +#endif
> > +	/* netns might be being dismantled. */
> > +	rcu_read_lock();
> > +	net = dev_net_rcu(dev);
> > +	net_passive_inc(net);
> > +	rcu_read_unlock();
> > +
> > +	rtnl_net_lock(net);
> > +
> > +#ifdef CONFIG_NET_NS
> > +	/* dev might have been moved to another netns. */
> > +	if (!net_eq(net, rcu_access_pointer(dev->nd_net.net))) {
> > +		rtnl_net_unlock(net);
> > +		net_passive_dec(net);
> > +		goto again;
> > +	}
> > +#endif
> 
> Is there a plan to clean this up in net-next? Or perhaps after Eric's
> dev_net() work? Otherwise I'm tempted to suggest to use a loop, maybe:

For sure, I will post a followup patch to net-next.

Thanks!


> 
> 	bool again;
> 
> 	do {
> 		again = false;
> 
> 		/* netns might be being dismantled. */
> 		rcu_read_lock();
> 		net = dev_net_rcu(dev);
> 		net_passive_inc(net);
> 		rcu_read_unlock();
> 
> 		rtnl_net_lock(net);
> 
> #ifdef CONFIG_NET_NS
> 		/* dev might have been moved to another netns. */
> 		if (!net_eq(net, rcu_access_pointer(dev->nd_net.net))) {
> 			rtnl_net_unlock(net);
> 			net_passive_dec(net);
> 			again = true;
> 		}
> #endif
> 	} while (again);

Re: [PATCH v4 net 2/3] net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().

[Jakub Kicinski]

On Fri, 14 Feb 2025 09:25:57 +0900 Kuniyuki Iwashima wrote:
> > Is there a plan to clean this up in net-next? Or perhaps after Eric's
> > dev_net() work? Otherwise I'm tempted to suggest to use a loop, maybe:  
> 
> For sure, I will post a followup patch to net-next.

Sorry, I meant that as distinct alternatives :)
The loop we can do already in net.
The question about net-next was more in case you're planning to rewrite
this entire function anyway, in which case the contents which land in
net are not as important.

Does that make sense?

Re: [PATCH v4 net 2/3] net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 14 Feb 2025 13:08:27 -0800
> On Fri, 14 Feb 2025 09:25:57 +0900 Kuniyuki Iwashima wrote:
> > > Is there a plan to clean this up in net-next? Or perhaps after Eric's
> > > dev_net() work? Otherwise I'm tempted to suggest to use a loop, maybe:  
> > 
> > For sure, I will post a followup patch to net-next.
> 
> Sorry, I meant that as distinct alternatives :)
> The loop we can do already in net.
> The question about net-next was more in case you're planning to rewrite
> this entire function anyway, in which case the contents which land in
> net are not as important.
> 
> Does that make sense?

Ah I see.
I didn't have the plan so will post v5 with the simple loop.

Thanks!