Re: [PATCH v1 net-next 0/4] net: Hold per-netns RTNL during netdev notifier registration.

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Jakub Kicinski <kuba@kernel.org>
Date: Sat, 4 Jan 2025 07:37:40 -0800
> On Sat, 4 Jan 2025 15:37:31 +0900 Kuniyuki Iwashima wrote:
> > Patch 1 converts the global netdev notifier to blocking_notifier,
> > which will be called under per-netns RTNL without RTNL, then we
> > need to protect the ongoing netdev_chain users from unregistration.
> > 
> > Patch 2 ~ 4 adds per-netns RTNL for registration of the global
> > and per-netns netdev notifiers.
> 
> Lockdep is not happy:
> 
> [  249.261403][   T11] ============================================ 
> [  249.261592][   T11] WARNING: possible recursive locking detected
> [  249.261769][   T11] 6.13.0-rc5-virtme #1 Not tainted
> [  249.261920][   T11] --------------------------------------------
> [  249.262094][   T11] kworker/u16:0/11 is trying to acquire lock:
> [  249.262293][   T11] ffffffff8a7f6a70 ((netdev_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x50/0x90
> [  249.262591][   T11] 
> [  249.262591][   T11] but task is already holding lock:
> [  249.262810][   T11] ffffffff8a7f6a70 ((netdev_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x50/0x90
> [  249.263100][   T11] 
> [  249.263100][   T11] other info that might help us debug this:
> [  249.263310][   T11]  Possible unsafe locking scenario:
> [  249.263310][   T11] 
> [  249.263522][   T11]        CPU0
> [  249.263624][   T11]        ----
> [  249.263728][   T11]   lock((netdev_chain).rwsem);
> [  249.263875][   T11]   lock((netdev_chain).rwsem);

Ah, lockdep annotaion was needed for the nested notifier calls.

But this will not be a meaningful annotation and needs to be changed
once rtnl_setlink/dellink supports per-netns RTNL.

I'll drop patch 1 and just leave a comment around RTNL in
register_netdevice_notifier() in patch 2.

Another option would be clone each netdev notifier during registration
and unshare(2)/clone(2) and force notifiers to be namespacified ?

---8<---
diff --git a/net/core/dev.c b/net/core/dev.c
index a0dd34463901..8bf8d565f42d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -446,6 +446,17 @@ static void unlist_netdevice(struct net_device *dev)
 
 static BLOCKING_NOTIFIER_HEAD(netdev_chain);
 
+#ifdef CONFIG_PROVE_LOCKING
+static int netdev_chain_lock_cmp_fn(const struct lockdep_map *a,
+				    const struct lockdep_map *b)
+{
+	if (rtnl_is_locked())
+		return -1;
+
+	return 1;
+}
+#endif
+
 /*
  *	Device drivers call our routines to queue packets here. We empty the
  *	queue in the local softnet handler.
@@ -12229,6 +12240,8 @@ static int __init net_dev_init(void)
 
 	net_dev_struct_check();
 
+	lock_set_cmp_fn(&netdev_chain.rwsem, netdev_chain_lock_cmp_fn, NULL);
+
 	if (dev_proc_init())
 		goto out;
 
---8<---