[PATCH net-next 1/2] net: use a single lockdep class for the netdev instance lock

[PATCH net-next 1/2] net: use a single lockdep class for the netdev instance lock

[Jakub Kicinski]

netdev_lockdep_set_classes() puts dev->lock in a separate lockdep class,
by type (netkit vs dummy etc), with the intent of keeping the instance
locks of individual devices as independent from each other as possible.
In practice it does the opposite. lockdep only calls the cmp_fn for locks
of the same class, so netdev_lock_cmp_fn() never gets a say when devices
from different classes are nested. Instead lockdep records a dependency
between the classes, and reports a circular locking problem as soon as
the nesting happens the other way round, e.g. when devices are unregistered
in a batch.

  ======================================================
  WARNING: possible circular locking dependency detected
  7.3.0-rc3+ #26 Not tainted
  ------------------------------------------------------
  kworker/u256:1/326 is trying to acquire lock:
  ff110000104fce28 (&dev_instance_lock_key#6){+.+.}-{4:4}, at:
  unregister_netdevice_many_notify+0x1141/0x1c30

  but task is already holding lock:
  ff110000127f2e28 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
  unregister_netdevice_many_notify+0x1141/0x1c30

  -> #1 (&dev_instance_lock_key#7){+.+.}-{4:4}:
         __lock_acquire+0x767/0xd60
         lock_acquire.part.0+0xd0/0x260
         __mutex_lock+0x17d/0x1f20
         unregister_netdevice_many_notify+0x1141/0x1c30
         default_device_exit_batch+0x3ee/0x520
         ops_undo_list+0x2cc/0x8a0
         cleanup_net+0x442/0x9c0
         process_one_work+0x951/0x1ab0
         worker_thread+0x5a6/0xd10
         kthread+0x339/0x430
         ret_from_fork+0x4a4/0x6f0
         ret_from_fork_asm+0x1a/0x30

  -> #0 (&dev_instance_lock_key#6){+.+.}-{4:4}:
         check_prev_add+0xeb/0xe60
         validate_chain+0x598/0x900
         __lock_acquire+0x767/0xd60
         lock_acquire.part.0+0xd0/0x260
         __mutex_lock+0x17d/0x1f20
         unregister_netdevice_many_notify+0x1141/0x1c30
         default_device_exit_batch+0x3ee/0x520
         ops_undo_list+0x2cc/0x8a0
         cleanup_net+0x442/0x9c0
         process_one_work+0x951/0x1ab0
         worker_thread+0x5a6/0xd10
         kthread+0x339/0x430
         ret_from_fork+0x4a4/0x6f0
         ret_from_fork_asm+0x1a/0x30

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    lock(&dev_instance_lock_key#7);
                                 lock(&dev_instance_lock_key#6);
                                 lock(&dev_instance_lock_key#7);
    lock(&dev_instance_lock_key#6);

   *** DEADLOCK ***

  locks held by kworker/u256:1/326: 6, last CPU#6:
   #0: ff11000001c2b540 ((wq_completion)netns){+.+.}-{0:0}, at:
  process_one_work+0x117c/0x1ab0
   #1: ffa0000001a3fd18 (net_cleanup_work){+.+.}-{0:0}, at:
  process_one_work+0x8ce/0x1ab0
   #2: ffffffff98f53288 (pernet_ops_rwsem){++++}-{4:4}, at:
  cleanup_net+0xc1/0x9c0
   #3: ffffffff98f6ede0 (rtnl_mutex){+.+.}-{4:4}, at:
  default_device_exit_batch+0x92/0x520
   #4: ff1100001321ae28 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
  unregister_netdevice_many_notify+0x1141/0x1c30

Keep all instance locks in the class from mutex_init() and let
the cmp_fn run. It allows nesting under rtnl_lock, which covers
the most common case.

The cmp function itself also its address gets compared by lockdep
so it can't be a static inline (that'd give each device type it's
own cmp_fn).

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
This is a fix for what was tripping up my recent loopback patch:

https://lore.kernel.org/20260921190452.1467853-1-kuba@kernel.org

Not marking for net since it's a false positive splat, which,
apparently, nobody is hitting.

CC: sdf.kernel@gmail.com
---
 include/net/netdev_lock.h | 18 ------------------
 net/core/dev.c            | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
index 9fb3e93857c3..8baa27266317 100644
--- a/include/net/netdev_lock.h
+++ b/include/net/netdev_lock.h
@@ -110,34 +110,16 @@ static inline int netdev_is_locked_ops_compat(const struct net_device *dev)
 	return lockdep_rtnl_is_held();
 }
 
-static inline int netdev_lock_cmp_fn(const struct lockdep_map *a,
-				     const struct lockdep_map *b)
-{
-	if (a == b)
-		return 0;
-
-	/* 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 in batched device removal for now.
-	 */
-	return lockdep_rtnl_is_held() ? -1 : 1;
-}
-
 #define netdev_lockdep_set_classes(dev)				\
 {								\
 	static struct lock_class_key qdisc_tx_busylock_key;	\
 	static struct lock_class_key qdisc_xmit_lock_key;	\
 	static struct lock_class_key dev_addr_list_lock_key;	\
-	static struct lock_class_key dev_instance_lock_key;	\
 	unsigned int i;						\
 								\
 	(dev)->qdisc_tx_busylock = &qdisc_tx_busylock_key;	\
 	lockdep_set_class(&(dev)->addr_list_lock,		\
 			  &dev_addr_list_lock_key);		\
-	lockdep_set_class(&(dev)->lock,				\
-			  &dev_instance_lock_key);		\
-	lock_set_cmp_fn(&dev->lock, netdev_lock_cmp_fn, NULL);	\
 	for (i = 0; i < (dev)->num_tx_queues; i++)		\
 		lockdep_set_class(&(dev)->_tx[i]._xmit_lock,	\
 				  &qdisc_xmit_lock_key);	\
diff --git a/net/core/dev.c b/net/core/dev.c
index c67900354fa6..bca7b267f937 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -568,6 +568,22 @@ static inline void netdev_set_addr_lockdep_class(struct net_device *dev)
 }
 #endif
 
+#ifdef CONFIG_PROVE_LOCKING
+static int netdev_lock_cmp_fn(const struct lockdep_map *a,
+			      const struct lockdep_map *b)
+{
+	if (a == b)
+		return 0;
+
+	/* 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 in batched device removal for now.
+	 */
+	return lockdep_rtnl_is_held() ? -1 : 1;
+}
+#endif
+
 /*******************************************************************************
  *
  *		Protocol management and registration routines
@@ -12172,6 +12188,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 #endif
 
 	mutex_init(&dev->lock);
+	lock_set_cmp_fn(&dev->lock, netdev_lock_cmp_fn, NULL);
 	netif_rx_mode_init(dev);
 
 	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
-- 
2.55.0

[PATCH net-next 2/2] selftests: net: add test for netdev instance lock ordering on unregister

[Jakub Kicinski]

Dismantle two netns holding a dummy and a netkit pair registered in
opposite order. unregister_netdevice_many_notify() takes the instance
locks of all the devices at once, nesting the two kinds of devices one
way for the first netns and the other way for the second. With lockdep
enabled this used to trigger a circular locking report.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/Makefile       |  1 +
 tools/testing/selftests/net/netdev_lock.py | 42 ++++++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100755 tools/testing/selftests/net/netdev_lock.py

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 3ee3378f8b26..d4ca82fec0b4 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -67,6 +67,7 @@ TEST_PROGS := \
 	nat6to4.sh \
 	ndisc_unsolicited_na_test.sh \
 	netdev-l2addr.sh \
+	netdev_lock.py \
 	netdevice.sh \
 	netns-name.sh \
 	netns-sysctl.sh \
diff --git a/tools/testing/selftests/net/netdev_lock.py b/tools/testing/selftests/net/netdev_lock.py
new file mode 100755
index 000000000000..0015b987eb9d
--- /dev/null
+++ b/tools/testing/selftests/net/netdev_lock.py
@@ -0,0 +1,42 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""Tests for the netdev instance lock."""
+
+from lib.py import ksft_run, ksft_exit
+from lib.py import ip
+from lib.py import NetNS
+
+
+def test_unreg_order() -> None:
+    """Dismantle two netns holding the same two kinds of ops locked device,
+    registered in opposite order.
+
+    unregister_netdevice_many_notify() takes the instance lock of every
+    device in the batch and holds them all at once, so the two kinds end up
+    nested one way round for the first netns and the other way round for
+    the second.
+    """
+    with NetNS() as ns1, NetNS() as ns2:
+        net1, net2 = str(ns1), str(ns2)
+
+        ip("link add du0 type dummy", ns=net1)
+        ip("link add nk0 type netkit peer name nk1", ns=net1)
+
+        ip("link add nk0 type netkit peer name nk1", ns=net2)
+        ip("link add du0 type dummy", ns=net2)
+
+        # only devices which are up get locked during unregister
+        for net in (net1, net2):
+            for dev in ("du0", "nk0", "nk1"):
+                ip(f"link set {dev} up", ns=net)
+
+
+def main() -> None:
+    """Ksft boilerplate main."""
+    ksft_run([test_unreg_order])
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.55.0

Re: [PATCH net-next 1/2] net: use a single lockdep class for the netdev instance lock

[Eric Dumazet]

On Thu, Sep 24, 2026 at 12:35 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> netdev_lockdep_set_classes() puts dev->lock in a separate lockdep class,
> by type (netkit vs dummy etc), with the intent of keeping the instance
> locks of individual devices as independent from each other as possible.
> In practice it does the opposite. lockdep only calls the cmp_fn for locks
> of the same class, so netdev_lock_cmp_fn() never gets a say when devices
> from different classes are nested. Instead lockdep records a dependency
> between the classes, and reports a circular locking problem as soon as
> the nesting happens the other way round, e.g. when devices are unregistered
> in a batch.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

Re: [PATCH net-next 2/2] selftests: net: add test for netdev instance lock ordering on unregister

[Eric Dumazet]

On Thu, Sep 24, 2026 at 12:35 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Dismantle two netns holding a dummy and a netkit pair registered in
> opposite order. unregister_netdevice_many_notify() takes the instance
> locks of all the devices at once, nesting the two kinds of devices one
> way for the first netns and the other way for the second. With lockdep
> enabled this used to trigger a circular locking report.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

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

Re: [PATCH net-next 1/2] net: use a single lockdep class for the netdev instance lock

[Stanislav Fomichev]

On 09/23, Jakub Kicinski wrote:
> netdev_lockdep_set_classes() puts dev->lock in a separate lockdep class,
> by type (netkit vs dummy etc), with the intent of keeping the instance
> locks of individual devices as independent from each other as possible.
> In practice it does the opposite. lockdep only calls the cmp_fn for locks
> of the same class, so netdev_lock_cmp_fn() never gets a say when devices
> from different classes are nested. Instead lockdep records a dependency
> between the classes, and reports a circular locking problem as soon as
> the nesting happens the other way round, e.g. when devices are unregistered
> in a batch.
> 
>   ======================================================
>   WARNING: possible circular locking dependency detected
>   7.3.0-rc3+ #26 Not tainted
>   ------------------------------------------------------
>   kworker/u256:1/326 is trying to acquire lock:
>   ff110000104fce28 (&dev_instance_lock_key#6){+.+.}-{4:4}, at:
>   unregister_netdevice_many_notify+0x1141/0x1c30
> 
>   but task is already holding lock:
>   ff110000127f2e28 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
>   unregister_netdevice_many_notify+0x1141/0x1c30
> 
>   -> #1 (&dev_instance_lock_key#7){+.+.}-{4:4}:
>          __lock_acquire+0x767/0xd60
>          lock_acquire.part.0+0xd0/0x260
>          __mutex_lock+0x17d/0x1f20
>          unregister_netdevice_many_notify+0x1141/0x1c30
>          default_device_exit_batch+0x3ee/0x520
>          ops_undo_list+0x2cc/0x8a0
>          cleanup_net+0x442/0x9c0
>          process_one_work+0x951/0x1ab0
>          worker_thread+0x5a6/0xd10
>          kthread+0x339/0x430
>          ret_from_fork+0x4a4/0x6f0
>          ret_from_fork_asm+0x1a/0x30
> 
>   -> #0 (&dev_instance_lock_key#6){+.+.}-{4:4}:
>          check_prev_add+0xeb/0xe60
>          validate_chain+0x598/0x900
>          __lock_acquire+0x767/0xd60
>          lock_acquire.part.0+0xd0/0x260
>          __mutex_lock+0x17d/0x1f20
>          unregister_netdevice_many_notify+0x1141/0x1c30
>          default_device_exit_batch+0x3ee/0x520
>          ops_undo_list+0x2cc/0x8a0
>          cleanup_net+0x442/0x9c0
>          process_one_work+0x951/0x1ab0
>          worker_thread+0x5a6/0xd10
>          kthread+0x339/0x430
>          ret_from_fork+0x4a4/0x6f0
>          ret_from_fork_asm+0x1a/0x30
> 
>    Possible unsafe locking scenario:
> 
>          CPU0                    CPU1
>          ----                    ----
>     lock(&dev_instance_lock_key#7);
>                                  lock(&dev_instance_lock_key#6);
>                                  lock(&dev_instance_lock_key#7);
>     lock(&dev_instance_lock_key#6);
> 
>    *** DEADLOCK ***
> 
>   locks held by kworker/u256:1/326: 6, last CPU#6:
>    #0: ff11000001c2b540 ((wq_completion)netns){+.+.}-{0:0}, at:
>   process_one_work+0x117c/0x1ab0
>    #1: ffa0000001a3fd18 (net_cleanup_work){+.+.}-{0:0}, at:
>   process_one_work+0x8ce/0x1ab0
>    #2: ffffffff98f53288 (pernet_ops_rwsem){++++}-{4:4}, at:
>   cleanup_net+0xc1/0x9c0
>    #3: ffffffff98f6ede0 (rtnl_mutex){+.+.}-{4:4}, at:
>   default_device_exit_batch+0x92/0x520
>    #4: ff1100001321ae28 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
>   unregister_netdevice_many_notify+0x1141/0x1c30
> 
> Keep all instance locks in the class from mutex_init() and let
> the cmp_fn run. It allows nesting under rtnl_lock, which covers
> the most common case.
> 
> The cmp function itself also its address gets compared by lockdep
> so it can't be a static inline (that'd give each device type it's
> own cmp_fn).
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 2/2] selftests: net: add test for netdev instance lock ordering on unregister

[Stanislav Fomichev]

On 09/23, Jakub Kicinski wrote:
> Dismantle two netns holding a dummy and a netkit pair registered in
> opposite order. unregister_netdevice_many_notify() takes the instance
> locks of all the devices at once, nesting the two kinds of devices one
> way for the first netns and the other way for the second. With lockdep
> enabled this used to trigger a circular locking report.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next 1/2] net: use a single lockdep class for the netdev instance lock

[Jakub Kicinski]

On Wed, 23 Sep 2026 15:35:44 -0700 Jakub Kicinski wrote:
>  	(dev)->qdisc_tx_busylock = &qdisc_tx_busylock_key;	\
>  	lockdep_set_class(&(dev)->addr_list_lock,		\
>  			  &dev_addr_list_lock_key);		\
> -	lockdep_set_class(&(dev)->lock,				\
> -			  &dev_instance_lock_key);		\
> -	lock_set_cmp_fn(&dev->lock, netdev_lock_cmp_fn, NULL);	\
>  	for (i = 0; i < (dev)->num_tx_queues; i++)		\
>  		lockdep_set_class(&(dev)->_tx[i]._xmit_lock,	\
>  				  &qdisc_xmit_lock_key);	\
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c67900354fa6..bca7b267f937 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -568,6 +568,22 @@ static inline void netdev_set_addr_lockdep_class(struct net_device *dev)
>  }
>  #endif
>  
> +#ifdef CONFIG_PROVE_LOCKING
> +static int netdev_lock_cmp_fn(const struct lockdep_map *a,
> +			      const struct lockdep_map *b)
> +{
> +	if (a == b)
> +		return 0;
> +
> +	/* 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 in batched device removal for now.
> +	 */
> +	return lockdep_rtnl_is_held() ? -1 : 1;
> +}
> +#endif

ugh, clashiko points out this is weakening the condition more than
I thought. Apparently lockdep_rtnl_is_held() is always UNKNOWN (-1)
when called from inside lockdep code ;( So this lockdep_rtnl_is_held()
is always true, and we would never fire a lockdep warning on instance
locks.

I need to switch to non-lockdep rtnl_is_held(), I think, and we either
bring back the classes for virt devices (but one class for all) or
we explicitly handle the virt->phys order that queue leasing needs.
Coding it up now, let me see which one is less ugly.