| Age | Commit message (Collapse) | Author |
|---|
|
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>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://patch.msgid.link/20260929191543.3295633-1-kuba@kernel.org
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
|
|
netdev_queue_get_dma_dev() uses "compat" locking assert which wants
either the rtnl_lock or netdev instance lock. This is not right,
the callers are taking the instance lock unconditionally. All entry
points for queue config are purely instance locked.
In other words the callers use netdev_get_by_index_lock(), not
netdev_get_by_index_lock_ops_compat(). All the state we will
access is effectively instance lock protected (it's const for
devices which are not ops-locked).
Update the assert to avoid false positive warnings.
Cc: stable@vger.kernel.org
Fixes: b6c5f9454ef34 ("io_uring/zcrx: call netdev_queue_get_dma_dev() under instance lock")
Reported-by: syzbot+a78926bdac2adb52dc0e@syzkaller.appspotmail.com
Reviewed-by: Simon Horman <horms@kernel.org>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://patch.msgid.link/20260806225627.3998672-1-kuba@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
Jakub suggests renaming the existing assert to match
the netdev_lock_ops_compat() semantics.
We want netdev_assert_locked_ops() to mean - if the driver
is ops locked - check that it's holding the device lock.
The existing helper check for either ops lock or rtnl_lock,
which is the locking behavior of netdev_lock_ops_compat().
The reason for naming divergence is likely that
netdev_ops_assert_locked() predated the _compat() helpers.
Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Link: https://patch.msgid.link/20260603012840.2254293-2-kuba@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
The netif_get_rx_queue_lease_locked() API hides the locking
and the descend onto the leased queue. Making the code
harder to follow (at least to me). Remove the API and open
code the descend a bit. Most of the code now looks like:
if (!leased)
return __helper(x);
hw_rxq = ..
netdev_lock(hw_rxq->dev);
ret = __helper(x);
netdev_unlock(hw_rxq->dev);
return ret;
Of course if we have more code paths that need the wrapping
we may need to revisit. For now, IMHO, having to know what
netif_get_rx_queue_lease_locked() does is not worth the 20LoC
it saves.
Link: https://patch.msgid.link/20260408151251.72bd2482@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
Extend netdev_queue_get_dma_dev to return the physical device of the
real rxq for DMA in case the queue was leased. This allows memory
providers like io_uring zero-copy or devmem to bind to the physically
leased rxq via virtual devices such as netkit.
Signed-off-by: David Wei <dw@davidwei.uk>
Co-developed-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Link: https://patch.msgid.link/20260402231031.447597-8-daniel@iogearbox.net
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
Implement netdev_nl_queue_create_doit which creates a new rx queue in a
virtual netdev and then leases it to a rx queue in a physical netdev.
Example with ynl client:
# ynl --family netdev --output-json --do queue-create \
--json '{"ifindex": 8, "type": "rx", "lease": {"ifindex": 4, "queue": {"type": "rx", "id": 15}}}'
{'id': 1}
Note that the netdevice locking order is always from the virtual to
the physical device.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Co-developed-by: David Wei <dw@davidwei.uk>
Signed-off-by: David Wei <dw@davidwei.uk>
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Link: https://patch.msgid.link/20260402231031.447597-3-daniel@iogearbox.net
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
For zerocopy (io_uring, devmem), there is an assumption that the
parent device can do DMA. However that is not always the case:
- Scalable Function netdevs [1] have the DMA device in the grandparent.
- For Multi-PF netdevs [2] queues can be associated to different DMA
devices.
This patch introduces the a queue based interface for allowing drivers
to expose a different DMA device for zerocopy.
[1] Documentation/networking/device_drivers/ethernet/mellanox/mlx5/switchdev.rst
[2] Documentation/networking/multi-pf-netdev.rst
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
Reviewed-by: Mina Almasry <almasrymina@google.com>
Link: https://patch.msgid.link/20250827144017.1529208-3-dtatulea@nvidia.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
