TX devmem bindings (NETDEV_CMD_BIND_TX) have no bound RX queues, so
mp_dmabuf_devmem_uninstall() never runs when binding->dev or
binding->vdev unregisters. As a result, binding->dev and binding->vdev
dangle pointing to freed struct net_device memory, and the dma_buf
remains attached and mapped after device removal.
Closing the netlink socket after netdev unregistration dereferences the
freed struct net_device in netdev_nl_sock_priv_destroy():
BUG: unable to handle page fault for address: ffff88824ca9b000
Oops: Oops: 0000 [#1] SMP KASAN NOPTI
RIP: 0010:netdev_nl_sock_priv_destroy+0xbd/0x1e0
Call Trace:
genl_release+0xf5/0x190
netlink_release+0xd01/0x18d0
__sock_release+0xb0/0x270
sock_close+0x18/0x20
Extend net_devmem_dev_unregister() and __net_devmem_dmabuf_binding_free()
to TX bindings, including ones whose netlink socket is already closed
but which TCP retransmits keep alive:
- When binding->dev (the physical DMA-capable device) unregisters,
clear the binding's NET_DEVMEM_BINDING_LIVE mark, unmap the dma_buf,
and clear both binding->vdev and binding->dev. No grace period is
needed: the device is already closed, so nothing hands the DMA
addresses to the hardware anymore, and concurrent senders only compare
against binding->dev.
- When only binding->vdev (virtual device) unregisters while
binding->dev lives, only clear binding->vdev so no new TX sends match
the virtual device while allowing in-flight TX packets on binding->dev
to finish DMA safely.
This issue was found via LLM-assisted code review (Sashiko) and
reproduced and verified with the new nl_netdev.py selftest on netdevsim
(not tested on real hardware).
Fixes: bd61848900bf ("net: devmem: Implement TX path")
Fixes: 1abe839b34ae ("net: devmem: support TX over NETMEM_TX_NO_DMA devices")
Suggested-by: Stanislav Fomichev <[email protected]>
Cc: Kaiyuan Zhang <[email protected]>
Cc: Bobby Eshleman <[email protected]>
Cc: Tariq Toukan <[email protected]>
Cc: Kaifeng Wang <[email protected]>
Signed-off-by: Mina Almasry <[email protected]>
Reviewed-by: Dragos Tatulea <[email protected]>
Acked-by: Stanislav Fomichev <[email protected]>
---
v5:
- Inline the TX unregistration check into net_devmem_dev_unregister()
(Stanislav), and clear binding->vdev under the xarray lock (Sashiko).
- Lock binding->dev in __net_devmem_dmabuf_binding_free() for TX
bindings as well now that net_devmem_dev_unregister() detaches them
(Sashiko).
v4:
- Detach and unmap TX bindings directly from the xarray walk, without a
grace period or a second pass (Stanislav).
v3:
- Detach all matching TX bindings first, then call synchronize_net()
once before unmapping them, instead of once per binding. Don't wait
for a grace period when only binding->vdev is cleared, since vdev is
never dereferenced (Sashiko).
- Don't detach TX bindings when the device only moves to another netns
(Sashiko).
v2:
- Build on the net_devmem_dev_unregister() walk from the previous patch,
so unbound TX bindings are also detached (Sashiko). Clear the
binding's LIVE mark instead of erasing its ID.
- The walk holds net_devmem_lock, which the final free also takes
before erasing the binding, instead of using RCU and
percpu_ref_tryget(), so it no longer takes a binding reference
(Stanislav).
---
net/core/devmem.c | 24 ++++++++++++++++--------
net/core/devmem.h | 6 ++++--
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/net/core/devmem.c b/net/core/devmem.c
index 4610de805ef08..5c7d9640e9d14 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -149,7 +149,7 @@ void __net_devmem_dmabuf_binding_free(struct work_struct
*wq)
{
struct net_devmem_dmabuf_binding *binding = container_of(wq,
typeof(*binding), unbind_w);
netdevice_tracker dev_tracker;
- struct net_device *dev = NULL;
+ struct net_device *dev;
if (binding->freelist)
WARN(binding->free_count != binding->area.num_niovs,
@@ -157,8 +157,7 @@ void __net_devmem_dmabuf_binding_free(struct work_struct
*wq)
binding->area.num_niovs, binding->free_count);
kvfree(binding->area.niovs);
- if (binding->direction == DMA_FROM_DEVICE)
- dev = net_devmem_lock_dev(binding, &dev_tracker);
+ dev = net_devmem_lock_dev(binding, &dev_tracker);
net_devmem_dmabuf_binding_unmap(binding);
xa_erase(&net_devmem_dmabuf_bindings, binding->id);
net_devmem_unlock_dev(dev, &dev_tracker);
@@ -252,11 +251,20 @@ void net_devmem_dev_unregister(struct net_device *dev)
xa_lock(&net_devmem_dmabuf_bindings);
xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
- if (binding->direction != DMA_FROM_DEVICE ||
- READ_ONCE(binding->dev) != dev ||
- xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
- NET_DEVMEM_BINDING_LIVE))
- continue;
+ if (binding->direction == DMA_FROM_DEVICE) {
+ if (READ_ONCE(binding->dev) != dev ||
+ xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
+ NET_DEVMEM_BINDING_LIVE))
+ continue;
+ } else {
+ if (READ_ONCE(binding->vdev) == dev)
+ WRITE_ONCE(binding->vdev, NULL);
+ if (READ_ONCE(binding->dev) != dev)
+ continue;
+ WRITE_ONCE(binding->vdev, NULL);
+ __xa_clear_mark(&net_devmem_dmabuf_bindings, xa_idx,
+ NET_DEVMEM_BINDING_LIVE);
+ }
xa_unlock(&net_devmem_dmabuf_bindings);
if (!netif_is_queue_leasee(dev))
diff --git a/net/core/devmem.h b/net/core/devmem.h
index a9c38ed6eaa44..c4f476947dfaa 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h
@@ -26,8 +26,8 @@ struct net_devmem_dmabuf_binding {
*/
struct net_device *dev;
/* Opaque cookie identifying the virtual device (e.g. netkit) the user
- * called bind-tx on. Used only for pointer comparison. Never
- * dereferenced.
+ * called bind-tx on. Used only for pointer comparison via READ_ONCE()
+ * and cleared with WRITE_ONCE() on unregistration. Never dereferenced.
*/
void *vdev;
possible_net_t net;
@@ -45,6 +45,8 @@ struct net_devmem_dmabuf_binding {
* those refs are dropped and the binding is no longer desired or in
* use. RX bindings are unmapped earlier when their queues are closed or
* uninstalled, unless a leased or busy queue may still use the binding.
+ * TX bindings are also unmapped earlier if their device is
+ * unregistered.
*
* net_devmem_net_iov_inc_ref() on dmabuf net_iovs will increment this
* reference, making sure that the binding remains alive until all the
--
2.56.0.385.gac671c819a-goog
