Re: [PATCH net-next] net: only give queue leasing devices a separate instance lock class

[Nikolay Aleksandrov <razor@blackwall.org>]

On 29/09/2026 22:15, Jakub Kicinski wrote:
> Commit b6f74dff6d26 ("net: use two lockdep classes for the netdev
> instance lock") put every device without a parent in the virtual class.
> It also restricted the locking order for the virtual class, because
> queue leasing has hard requirements on the exact order.
> 
> This bites us back on bond, which is "virtual" and needs to be taken
> before taking the locks of the lowers. NIPA hit the following on the
> new test I recently posted for XDP+bond:
> 
>    WARNING: possible circular locking dependency detected
>    ------------------------------------------------------
>    python3/18635 is trying to acquire lock:
>    ff11000120b8ce30 (&dev->lock){+.+.}-{4:4}, at:
>    netdev_put_lock+0x2d/0x1a0
> 
>    but task is already holding lock:
>    ff110001ef77ae30 (&netdev_virt_instance_lock_key){+.+.}-{4:4}, at:
>    netdev_put_lock+0x2d/0x1a0
> 
>    which lock already depends on the new lock.
> 
>    the existing dependency chain (in reverse order) is:
> 
>    -> #1 (&netdev_virt_instance_lock_key){+.+.}-{4:4}:
>           __mutex_lock+0x1ae/0x1f10
>           xdp_set_features_flag+0x2b/0x50
>           bond_xdp_set_features+0x1eb/0x360
>           bond_netdev_event+0x13f/0x300
>           notifier_call_chain+0xae/0x300
>           call_netdevice_notifiers+0x70/0xa0
>           bnxt_xdp_set+0x2f6/0x620
>           netif_xdp_propagate+0x503/0xc60
>           dev_xdp_propagate+0xa1/0x230
>           bond_xdp_set+0x234/0x700
>           dev_xdp_install+0x592/0xd70
>           dev_xdp_attach+0x355/0xf50
>           dev_change_xdp_fd+0x176/0x210
>           do_setlink.isra.0+0x220d/0x2b20
>           rtnl_newlink+0x9f1/0x11b0
> 
>    -> #0 (&dev->lock){+.+.}-{4:4}:
>           __mutex_lock+0x1ae/0x1f10
>           netdev_put_lock+0x2d/0x1a0
>           netdev_nl_queue_create_doit+0x801/0x1a70
>           genl_family_rcv_msg_doit+0x206/0x300
> 
>     Possible unsafe locking scenario:
> 
>           CPU0                    CPU1
>           ----                    ----
>      lock(&netdev_virt_instance_lock_key);
>                                   lock(&dev->lock);
>                                   lock(&netdev_virt_instance_lock_key);
>      lock(&dev->lock);
> 
> Let's narrow down the "virtual" class to only the devices which
> can actually create a queue. More LoC and complexity, but that
> is what we actually care about here. The rest needs to nest
> under rtnl_lock, which bond does (famous last words?)
> 
> Take the instance locks in two passes, first the netkits then
> the rest (matching the queue leasing order).
> An alternative would be to make sure the close list is sorted
> correctly from the start (queue head/tail appropriately in
> unregister_netdevice_queue()). I think it works but feels
> a little more fragile. Happy to change, tho.
> 
> netdev_can_create_queue() will now be used on paths where we
> genuinely handle non-netkit, so we can't always set the extack.
> Unfortunately, the (recently) added tracepoint in extack fires
> even when extack is NULL.
> 
> Fixes: b6f74dff6d26 ("net: use two lockdep classes for the netdev instance lock")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: daniel@iogearbox.net
> CC: hawk@kernel.org
> CC: john.fastabend@gmail.com
> CC: sdf@fomichev.me
> CC: razor@blackwall.org
> CC: dw@davidwei.uk
> ---
>   net/core/dev.c           | 48 ++++++++++++++++++++++------------------
>   net/core/netdev_queues.c | 31 ++++++++++++++------------
>   2 files changed, 44 insertions(+), 35 deletions(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>