[PATCH net v2 0/4] net: devmem: fix RX and TX binding teardown on device unregistration

[PATCH net v2 0/4] net: devmem: fix RX and TX binding teardown on device unregistration

[Mina Almasry]

Fix two device-unregistration lifecycle bugs in devmem TCP bindings and
add netdevsim support plus selftests covering both RX and TX device
unregistration:

1. net: devmem: unmap dma_buf synchronously on queue uninstall
   Synchronously unmap and detach binding->attachment under binding->lock
   when the last RX queue is uninstalled in mp_dmabuf_devmem_uninstall(),
   and unmap RX bindings that were already unbound on NETDEV_UNREGISTER,
   preventing slab-use-after-free in dma_unmap_sg_attrs() when the binding
   is freed after dma_dev.

2. net: devmem: detach TX bindings on NETDEV_UNREGISTER
   Extend net_devmem_dev_unregister() to fully detach and unmap TX
   bindings when binding->dev unregisters, or clear binding->vdev alone
   when only a virtual device unregisters while binding->dev lives.

3. netdevsim: support devmem RX and TX bindings
   Enable PP_FLAG_DMA_MAP | PP_FLAG_ALLOW_UNREADABLE_NETMEM, 64-bit DMA mask
   on nsim_bus_dev, and NETMEM_TX_DMA on netdevsim so devmem RX/TX bindings
   can be exercised in software selftests.

4. selftests: net: add devmem RX and TX netdev unregister tests
   Add devmem_bind_rx_unregister_check and devmem_bind_tx_unregister_check
   to tools/testing/selftests/net/nl_netdev.py.

Cc: Tariq Toukan <tariqt@nvidia.com>
Cc: Kaifeng Wang <kaifengw@google.com>

---
v2:
- 1/4: Don't unmap when a leased queue is unleased. Keep bindings in
  net_devmem_dmabuf_bindings until freed, hidden from TX lookups by an
  xarray mark, and unmap unbound RX bindings on NETDEV_UNREGISTER. Update
  the binding->lock and binding->ref comments. (Sashiko)
- 2/4: Detach TX bindings from the same net_devmem_lock-protected walk,
  which now also covers unbound TX bindings, and clear the LIVE mark
  instead of erasing the ID. The walk no longer uses RCU or takes
  binding references (Stanislav).
- Link to v1: https://lore.kernel.org/netdev/20261010025532.839559-1-almasrymina@google.com/

Mina Almasry (4):
  net: devmem: unmap dma_buf synchronously on queue uninstall
  net: devmem: detach TX bindings on NETDEV_UNREGISTER
  netdevsim: support devmem RX and TX bindings
  selftests: net: add devmem RX and TX netdev unregister tests

 drivers/net/netdevsim/bus.c              |   1 +
 drivers/net/netdevsim/netdev.c           |  21 ++--
 net/core/devmem.c                        | 121 ++++++++++++++++++++---
 net/core/devmem.h                        |  10 +-
 net/core/netdev-genl.c                   |   1 +
 tools/testing/selftests/net/nl_netdev.py | 108 +++++++++++++++++++-
 6 files changed, 236 insertions(+), 26 deletions(-)


base-commit: 4f6ac74fa594a918d41255c941256e1d51592d3b
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH net v2 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[Mina Almasry]

When a bound net_device is unregistered while userspace still holds a
netlink or TCP socket reference on a devmem binding,
mp_dmabuf_devmem_uninstall() clears binding->dev to NULL so
netdev_nl_sock_priv_destroy() will not touch the unregistered
net_device, but leaves the dma_buf attachment mapped until
__net_devmem_dmabuf_binding_free() runs later on socket close.

Because dma_buf_attach() does not take a reference on dma_dev, device
removal frees dma_dev while the attachment is still mapped. Closing the
netlink socket afterwards triggers a slab use-after-free in
dma_unmap_sg_attrs():

  BUG: KASAN: slab-use-after-free in dma_unmap_sg_attrs+0x76/0x2b0
  Read of size 8 at addr ffff88810fae64b0 by task kworker/3:1/53
  Workqueue: events __net_devmem_dmabuf_binding_free
  Call Trace:
   dma_unmap_sg_attrs+0x76/0x2b0
   unmap_udmabuf+0x8e/0x100
   dma_buf_unmap_attachment+0x145/0x330
   dma_buf_unmap_attachment_unlocked+0x96/0x130
   __net_devmem_dmabuf_binding_free+0x156/0x2e0
  Freed by task 9:
   kfree+0x1b8/0x550
   nsim_bus_dev_release+0xe/0x60
   device_release+0xcf/0x250

Add net_devmem_dmabuf_binding_unmap() serialized under binding->lock
and null out binding->sgt and binding->attachment after unmapping so
unmapping is idempotent across queue uninstall and deferred binding
destruction. Call it synchronously when the last bound RX queue is
uninstalled in mp_dmabuf_devmem_uninstall(), except when a leased
queue is unleased: the physical device is still registered and, if
restarting its queue failed, may still be using the binding.

The same use-after-free happens when the netlink socket is closed
before the device is removed but RX tokens or the pending release work
keep the binding alive. Such bindings are no longer installed on any
queue and were already erased from net_devmem_dmabuf_bindings. Keep
bindings in net_devmem_dmabuf_bindings until they are freed, hide
unbound ones from TX lookups with an xarray mark, and on
NETDEV_UNREGISTER clear binding->dev of unbound RX bindings for the
device and unmap them, unless the device is a virtual device leasing
queues, for the same reason as above. net_devmem_lock serializes that
walk with the final free.

Fixes: f8350a4358fc ("net: page_pool: add a mp hook to unregister_netdevice*")
Fixes: 0afc44d8cdf6 ("net: devmem: fix kernel panic when netlink socket close after module unload")
Cc: Taehee Yoo <ap420073@gmail.com>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Tariq Toukan <tariqt@nvidia.com>
Cc: Kaifeng Wang <kaifengw@google.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
v2:
- Don't unmap when a leased queue is unleased (Sashiko).
- Unmap unbound RX bindings on NETDEV_UNREGISTER; keep bindings in
  net_devmem_dmabuf_bindings until freed, hidden from TX lookups by
  NET_DEVMEM_BINDING_LIVE (Sashiko).
- Update the binding->lock and binding->ref comments (Sashiko).
---
 net/core/devmem.c      | 89 +++++++++++++++++++++++++++++++++++-------
 net/core/devmem.h      | 10 ++++-
 net/core/netdev-genl.c |  1 +
 3 files changed, 83 insertions(+), 17 deletions(-)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index f4d60654ce7fd..02d607cc87217 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -20,6 +20,7 @@
 #include <net/tcp.h>
 #include <trace/events/page_pool.h>
 
+#include "dev.h"
 #include "devmem.h"
 #include "mp_dmabuf_devmem.h"
 #include "page_pool_priv.h"
@@ -27,6 +28,8 @@
 /* Device memory support */
 
 static DEFINE_XARRAY_FLAGS(net_devmem_dmabuf_bindings, XA_FLAGS_ALLOC1);
+#define NET_DEVMEM_BINDING_LIVE XA_MARK_1
+static DEFINE_MUTEX(net_devmem_lock);
 
 static const struct memory_provider_ops dmabuf_devmem_ops;
 
@@ -58,6 +61,21 @@ static void net_devmem_dmabuf_binding_release(struct percpu_ref *ref)
 	schedule_work(&binding->unbind_w);
 }
 
+static void
+net_devmem_dmabuf_binding_unmap(struct net_devmem_dmabuf_binding *binding)
+{
+	lockdep_assert_held(&binding->lock);
+
+	if (!binding->sgt)
+		return;
+
+	dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
+					  binding->direction);
+	dma_buf_detach(binding->dmabuf, binding->attachment);
+	binding->sgt = NULL;
+	binding->attachment = NULL;
+}
+
 void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
 {
 	struct net_devmem_dmabuf_binding *binding = container_of(wq, typeof(*binding), unbind_w);
@@ -74,9 +92,13 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
 		  size, avail))
 		gen_pool_destroy(binding->chunk_pool);
 
-	dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
-					  binding->direction);
-	dma_buf_detach(binding->dmabuf, binding->attachment);
+	mutex_lock(&net_devmem_lock);
+	mutex_lock(&binding->lock);
+	net_devmem_dmabuf_binding_unmap(binding);
+	mutex_unlock(&binding->lock);
+	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
+	mutex_unlock(&net_devmem_lock);
+
 	dma_buf_put(binding->dmabuf);
 	xa_destroy(&binding->bound_rxqs);
 	percpu_ref_exit(&binding->ref);
@@ -129,10 +151,11 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
 	unsigned long xa_idx;
 	unsigned int rxq_idx;
 
-	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
+	xa_clear_mark(&net_devmem_dmabuf_bindings, binding->id,
+		      NET_DEVMEM_BINDING_LIVE);
 
 	/* Ensure no tx net_devmem_lookup_dmabuf() are in flight after the
-	 * erase.
+	 * mark is cleared.
 	 */
 	synchronize_net();
 
@@ -153,6 +176,35 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
 	percpu_ref_kill(&binding->ref);
 }
 
+static void
+net_devmem_dev_unregister_rx(struct net_devmem_dmabuf_binding *binding,
+			     unsigned long xa_idx, struct net_device *dev)
+{
+	if (xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
+			NET_DEVMEM_BINDING_LIVE) ||
+	    READ_ONCE(binding->dev) != dev)
+		return;
+
+	mutex_lock(&binding->lock);
+	WRITE_ONCE(binding->dev, NULL);
+	if (!netif_is_queue_leasee(dev))
+		net_devmem_dmabuf_binding_unmap(binding);
+	mutex_unlock(&binding->lock);
+}
+
+void net_devmem_dev_unregister(struct net_device *dev)
+{
+	struct net_devmem_dmabuf_binding *binding;
+	unsigned long xa_idx;
+
+	mutex_lock(&net_devmem_lock);
+	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
+		if (binding->direction == DMA_FROM_DEVICE)
+			net_devmem_dev_unregister_rx(binding, xa_idx, dev);
+	}
+	mutex_unlock(&net_devmem_lock);
+}
+
 int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 				    struct net_devmem_dmabuf_binding *binding,
 				    struct netlink_ext_ack *extack)
@@ -331,6 +383,8 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 			      GFP_KERNEL);
 	if (err < 0)
 		goto err_free_chunks;
+	xa_set_mark(&net_devmem_dmabuf_bindings, binding->id,
+		    NET_DEVMEM_BINDING_LIVE);
 
 	list_add(&binding->list, &priv->bindings);
 
@@ -359,9 +413,11 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id)
 {
 	struct net_devmem_dmabuf_binding *binding;
+	unsigned long xa_idx = id;
 
 	rcu_read_lock();
-	binding = xa_load(&net_devmem_dmabuf_bindings, id);
+	binding = xa_find(&net_devmem_dmabuf_bindings, &xa_idx, id,
+			  NET_DEVMEM_BINDING_LIVE);
 	if (binding) {
 		if (!net_devmem_dmabuf_binding_get(binding))
 			binding = NULL;
@@ -529,16 +585,19 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv,
 	unsigned long xa_idx;
 
 	xa_for_each(&binding->bound_rxqs, xa_idx, bound_rxq) {
-		if (bound_rxq == rxq) {
-			xa_erase(&binding->bound_rxqs, xa_idx);
-			if (xa_empty(&binding->bound_rxqs)) {
-				mutex_lock(&binding->lock);
-				ASSERT_EXCLUSIVE_WRITER(binding->dev);
-				WRITE_ONCE(binding->dev, NULL);
-				mutex_unlock(&binding->lock);
-			}
-			break;
+		if (bound_rxq != rxq)
+			continue;
+
+		xa_erase(&binding->bound_rxqs, xa_idx);
+		if (xa_empty(&binding->bound_rxqs)) {
+			mutex_lock(&binding->lock);
+			ASSERT_EXCLUSIVE_WRITER(binding->dev);
+			WRITE_ONCE(binding->dev, NULL);
+			if (!rxq->lease)
+				net_devmem_dmabuf_binding_unmap(binding);
+			mutex_unlock(&binding->lock);
 		}
+		break;
 	}
 }
 
diff --git a/net/core/devmem.h b/net/core/devmem.h
index 4a293a7d1149c..5c47c8c3c7d2d 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h
@@ -27,7 +27,7 @@ struct net_devmem_dmabuf_binding {
 	 */
 	void *vdev;
 	struct gen_pool *chunk_pool;
-	/* Protect dev */
+	/* Protect dev, attachment and sgt */
 	struct mutex lock;
 
 	/* The user holds a ref (via the netlink API) for as long as they want
@@ -38,7 +38,8 @@ struct net_devmem_dmabuf_binding {
 	 *
 	 * The binding undos itself and unmaps the underlying dmabuf once all
 	 * those refs are dropped and the binding is no longer desired or in
-	 * use.
+	 * use. The dmabuf may be unmapped earlier, when the device it is
+	 * mapped for is unregistered.
 	 *
 	 * net_devmem_get_net_iov() on dmabuf net_iovs will increment this
 	 * reference, making sure that the binding remains alive until all the
@@ -100,6 +101,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 		       struct netlink_ext_ack *extack);
 struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id);
 void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding);
+void net_devmem_dev_unregister(struct net_device *dev);
 int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 				    struct net_devmem_dmabuf_binding *binding,
 				    struct netlink_ext_ack *extack);
@@ -196,6 +198,10 @@ net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
 {
 }
 
+static inline void net_devmem_dev_unregister(struct net_device *dev)
+{
+}
+
 static inline int
 net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 				struct net_devmem_dmabuf_binding *binding,
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 33b9f4eb9565a..5bfcd513fb0e8 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -1487,6 +1487,7 @@ static int netdev_genl_netdevice_event(struct notifier_block *nb,
 		netdev_unlock_full_to_ops(netdev);
 		break;
 	case NETDEV_UNREGISTER:
+		net_devmem_dev_unregister(netdev);
 		netdev_lock(netdev);
 		netdev_genl_dev_notify(netdev, NETDEV_CMD_DEV_DEL_NTF);
 		netdev_unlock(netdev);
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH net v2 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The same dma_unmap_sg_attrs() slab-use-after-free the patch
  claims to fix can still happen for RX bindings made on a virtual…
- [High] net_devmem_dev_unregister_rx() adds a new writer of binding->dev
  (and a new unmap).

Pre-existing issues:
- [Low] Not a valid finding against the final series state; kept only
  because requires_validation=true.

--

net: devmem: unmap dma_buf synchronously on queue uninstall

This adds net_devmem_dmabuf_binding_unmap(). The dma-buf attachment is
now unmapped when the last bound RX queue is uninstalled, and also on
NETDEV_UNREGISTER for unbound RX bindings, to avoid a use-after-free of
dma_dev in dma_unmap_sg_attrs(). Bindings now stay in
net_devmem_dmabuf_bindings until they are freed, and an xarray mark hides
unbound ones from TX lookups.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index f4d60654ce7fd..02d607cc87217 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -129,10 +151,11 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  	unsigned long xa_idx;
>  	unsigned int rxq_idx;
>  
> -	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
> +	xa_clear_mark(&net_devmem_dmabuf_bindings, binding->id,
> +		      NET_DEVMEM_BINDING_LIVE);
>  
>  	/* Ensure no tx net_devmem_lookup_dmabuf() are in flight after the
> -	 * erase.
> +	 * mark is cleared.
>  	 */
>  	synchronize_net();
>  

[ ... ]

> @@ -153,6 +176,35 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  	percpu_ref_kill(&binding->ref);
>  }
>  
> +static void
> +net_devmem_dev_unregister_rx(struct net_devmem_dmabuf_binding *binding,
> +			     unsigned long xa_idx, struct net_device *dev)
> +{
> +	if (xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +			NET_DEVMEM_BINDING_LIVE) ||
> +	    READ_ONCE(binding->dev) != dev)
> +		return;
> +
> +	mutex_lock(&binding->lock);
> +	WRITE_ONCE(binding->dev, NULL);
> +	if (!netif_is_queue_leasee(dev))
> +		net_devmem_dmabuf_binding_unmap(binding);
> +	mutex_unlock(&binding->lock);
> +}

[Severity: High]

Can this race with net_devmem_unbind_dmabuf() and set binding->dev to
NULL while the unbind is still walking bound_rxqs?

net_devmem_dev_unregister() is called from netdev_genl_netdevice_event()
before netdev_lock(netdev) is taken. That means this writer holds only
RTNL, net_devmem_lock and binding->lock.

The socket close path holds netdev_lock(dev) but not binding->lock:

netdev_nl_sock_priv_destroy()
    netdev_lock(dev);
    net_devmem_unbind_dmabuf(binding);
        xa_clear_mark(..., NET_DEVMEM_BINDING_LIVE);
        synchronize_net();
        xa_for_each(&binding->bound_rxqs, xa_idx, rxq) {
            ...
            netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params);
        }

Clearing the mark is what makes this walk treat the binding as unbound.
At least two paths deliver NETDEV_UNREGISTER after the instance lock has
been dropped and while bound_rxqs is still populated:

- __dev_change_net_namespace() calls netdev_unlock_ops() after
  dev_shutdown(), then call_netdevice_notifiers(NETDEV_UNREGISTER, dev).
  It never calls dev_memory_provider_uninstall().

- For a netkit leasee, unregister_netdevice_many_notify() runs the
  notifiers after netdev_unlock_ops(). The bound rxq is only uninstalled
  later, from ndo_uninit (netkit_uninit()->netkit_queue_unlease()).

Suppose the unbind is sleeping in synchronize_net() during that window.
This function then sees !LIVE and binding->dev == dev, and sets
binding->dev to NULL. For a physical device it also unmaps the dma-buf
while the binding is still the installed memory provider on the RX
queues.

The unbind loop then calls netif_mp_close_rxq(NULL, ...), which
dereferences dev straight away:

	if (WARN_ON_ONCE(rxq_idx >= dev->real_num_rx_queues))
		return;

Before this patch, binding->dev was only written by
mp_dmabuf_devmem_uninstall() under the netdev instance lock. Does
anything else serialize this walk with an unbind that is in progress?

> +
> +void net_devmem_dev_unregister(struct net_device *dev)
> +{
> +	struct net_devmem_dmabuf_binding *binding;
> +	unsigned long xa_idx;
> +
> +	mutex_lock(&net_devmem_lock);
> +	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
> +		if (binding->direction == DMA_FROM_DEVICE)
> +			net_devmem_dev_unregister_rx(binding, xa_idx, dev);
> +	}
> +	mutex_unlock(&net_devmem_lock);
> +}

[Severity: Low]

This is a pre-existing issue, not one introduced by this patch, but only
DMA_FROM_DEVICE bindings are handled here.

After NETDEV_UNREGISTER, TX bindings created by netdev_nl_bind_tx_doit()
still keep their dma-buf mapping. Their binding->dev also still points at
the unregistered net_device, and netdev_nl_sock_priv_destroy() reads it
later.

The next patch in the series, "net: devmem: detach TX bindings on
NETDEV_UNREGISTER", adds net_devmem_dev_unregister_tx() and an else
branch here for TX bindings. That appears to resolve this by the end of
the series.

[ ... ]

> @@ -529,16 +585,19 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv,
>  	unsigned long xa_idx;
>  
>  	xa_for_each(&binding->bound_rxqs, xa_idx, bound_rxq) {
> -		if (bound_rxq == rxq) {
> -			xa_erase(&binding->bound_rxqs, xa_idx);
> -			if (xa_empty(&binding->bound_rxqs)) {
> -				mutex_lock(&binding->lock);
> -				ASSERT_EXCLUSIVE_WRITER(binding->dev);
> -				WRITE_ONCE(binding->dev, NULL);
> -				mutex_unlock(&binding->lock);
> -			}
> -			break;
> +		if (bound_rxq != rxq)
> +			continue;
> +
> +		xa_erase(&binding->bound_rxqs, xa_idx);
> +		if (xa_empty(&binding->bound_rxqs)) {
> +			mutex_lock(&binding->lock);
> +			ASSERT_EXCLUSIVE_WRITER(binding->dev);
> +			WRITE_ONCE(binding->dev, NULL);
> +			if (!rxq->lease)
> +				net_devmem_dmabuf_binding_unmap(binding);

[Severity: High]

Is the same use-after-free still reachable for RX bindings made on a
netkit device that leases a queue from a physical NIC?

In netdev_nl_bind_rx_doit(), dma_dev is resolved through the lease to
the physical device's DMA device
(netdev_nl_get_dma_dev()->netdev_queue_get_dma_dev()). However,
binding->dev is the virtual netdev, and bound_rxqs holds the virtual rxq.

The only uninstall this binding gets comes from the unlease path, and it
runs while rxq->lease is still set:

netkit_uninit()
  netkit_queue_unlease()
    netdev_rx_queue_unlease()
      netif_rxq_cleanup_unlease()
        __netif_mp_uninstall_rxq(virt_rxq, &p)
          mp_dmabuf_devmem_uninstall()  <- rxq->lease != NULL, no unmap
      WRITE_ONCE(rxq_src->lease, NULL);
      WRITE_ONCE(rxq_dst->lease, NULL);
      netdev_put(rxq_src->dev, &rxq_src->lease_tracker);

So binding->dev becomes NULL and the unmap is skipped.

net_devmem_dev_unregister_rx() doesn't catch it later either. On the
virtual device's unregister it skips the unmap because
netif_is_queue_leasee() is true. On the physical device's unregister,
binding->dev is the virtual device or NULL, so it never matches.

Once netdev_put() drops the lease's reference on the physical device,
nothing holds dma_dev, since dma_buf_attach() doesn't take a reference.
dma_dev can then be freed, for example by netdevsim del_device or PCI
hot-remove. The later
__net_devmem_dmabuf_binding_free()->net_devmem_dmabuf_binding_unmap()
then unmaps against the freed device.

The commit message says:

  except when a leased queue is unleased: the physical device is still
  registered and, if restarting its queue failed, may still be using
  the binding.

netkit_check_lease_unregister() triggers this unlease from inside the
physical device's own NETDEV_UNREGISTER:

	if (READ_ONCE(dev->reg_state) != NETREG_UNREGISTERING ||
	    !dev->dev.parent)
		return;
	...
	netdev_unlock_ops(dev);
	unregister_netdevice_many(&list_kill);

Is the physical device still registered in that case? Would this need to
track the device that owns dma_dev, or hold a reference on dma_dev,
instead of keying on binding->dev?

> +			mutex_unlock(&binding->lock);
>  		}
> +		break;
>  	}
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012174300.2338784-1-almasrymina%40google.com

Re: [PATCH net v2 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[Mina Almasry]

On Tue, Oct 13, 2026 at 11:14 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · Medium: 0 · Low: 0 (1)
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The same dma_unmap_sg_attrs() slab-use-after-free the patch
>   claims to fix can still happen for RX bindings made on a virtual…
> - [High] net_devmem_dev_unregister_rx() adds a new writer of binding->dev
>   (and a new unmap).
>

For both of these, they're issues with leased devices AFAIU. My LLM
found just too many issues with the leased devices. I'm hoping to have
this series merged to fix the lifetime/revoke issues with the physical
devices binding->dev == binding->vdev, and rework how the leasted
devices work properly in another series. It was just too much to fix
all the bugs in one go.

-- 
Thanks,
Mina

Re: [PATCH net v2 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[Stanislav Fomichev]

On 10/12, Mina Almasry wrote:
> When a bound net_device is unregistered while userspace still holds a
> netlink or TCP socket reference on a devmem binding,
> mp_dmabuf_devmem_uninstall() clears binding->dev to NULL so
> netdev_nl_sock_priv_destroy() will not touch the unregistered
> net_device, but leaves the dma_buf attachment mapped until
> __net_devmem_dmabuf_binding_free() runs later on socket close.
> 
> Because dma_buf_attach() does not take a reference on dma_dev, device
> removal frees dma_dev while the attachment is still mapped. Closing the
> netlink socket afterwards triggers a slab use-after-free in
> dma_unmap_sg_attrs():
> 
>   BUG: KASAN: slab-use-after-free in dma_unmap_sg_attrs+0x76/0x2b0
>   Read of size 8 at addr ffff88810fae64b0 by task kworker/3:1/53
>   Workqueue: events __net_devmem_dmabuf_binding_free
>   Call Trace:
>    dma_unmap_sg_attrs+0x76/0x2b0
>    unmap_udmabuf+0x8e/0x100
>    dma_buf_unmap_attachment+0x145/0x330
>    dma_buf_unmap_attachment_unlocked+0x96/0x130
>    __net_devmem_dmabuf_binding_free+0x156/0x2e0
>   Freed by task 9:
>    kfree+0x1b8/0x550
>    nsim_bus_dev_release+0xe/0x60
>    device_release+0xcf/0x250
> 
> Add net_devmem_dmabuf_binding_unmap() serialized under binding->lock
> and null out binding->sgt and binding->attachment after unmapping so
> unmapping is idempotent across queue uninstall and deferred binding
> destruction. Call it synchronously when the last bound RX queue is
> uninstalled in mp_dmabuf_devmem_uninstall(), except when a leased
> queue is unleased: the physical device is still registered and, if
> restarting its queue failed, may still be using the binding.
> 
> The same use-after-free happens when the netlink socket is closed
> before the device is removed but RX tokens or the pending release work
> keep the binding alive. Such bindings are no longer installed on any
> queue and were already erased from net_devmem_dmabuf_bindings. Keep
> bindings in net_devmem_dmabuf_bindings until they are freed, hide
> unbound ones from TX lookups with an xarray mark, and on
> NETDEV_UNREGISTER clear binding->dev of unbound RX bindings for the
> device and unmap them, unless the device is a virtual device leasing

[..]

> queues, for the same reason as above. net_devmem_lock serializes that
> walk with the final free.

Can you explain net_devmem_lock a bit more? In
net_devmem_dev_unregister, why can you not net_devmem_lookup_dmabuf and
then rely on the binding->lock? The rest looks good, but the
new lock seems a bit excessive?

Re: [PATCH net v2 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[Mina Almasry]

On Wed, Oct 14, 2026 at 9:34 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
>
> On 10/12, Mina Almasry wrote:
> > When a bound net_device is unregistered while userspace still holds a
> > netlink or TCP socket reference on a devmem binding,
> > mp_dmabuf_devmem_uninstall() clears binding->dev to NULL so
> > netdev_nl_sock_priv_destroy() will not touch the unregistered
> > net_device, but leaves the dma_buf attachment mapped until
> > __net_devmem_dmabuf_binding_free() runs later on socket close.
> >
> > Because dma_buf_attach() does not take a reference on dma_dev, device
> > removal frees dma_dev while the attachment is still mapped. Closing the
> > netlink socket afterwards triggers a slab use-after-free in
> > dma_unmap_sg_attrs():
> >
> >   BUG: KASAN: slab-use-after-free in dma_unmap_sg_attrs+0x76/0x2b0
> >   Read of size 8 at addr ffff88810fae64b0 by task kworker/3:1/53
> >   Workqueue: events __net_devmem_dmabuf_binding_free
> >   Call Trace:
> >    dma_unmap_sg_attrs+0x76/0x2b0
> >    unmap_udmabuf+0x8e/0x100
> >    dma_buf_unmap_attachment+0x145/0x330
> >    dma_buf_unmap_attachment_unlocked+0x96/0x130
> >    __net_devmem_dmabuf_binding_free+0x156/0x2e0
> >   Freed by task 9:
> >    kfree+0x1b8/0x550
> >    nsim_bus_dev_release+0xe/0x60
> >    device_release+0xcf/0x250
> >
> > Add net_devmem_dmabuf_binding_unmap() serialized under binding->lock
> > and null out binding->sgt and binding->attachment after unmapping so
> > unmapping is idempotent across queue uninstall and deferred binding
> > destruction. Call it synchronously when the last bound RX queue is
> > uninstalled in mp_dmabuf_devmem_uninstall(), except when a leased
> > queue is unleased: the physical device is still registered and, if
> > restarting its queue failed, may still be using the binding.
> >
> > The same use-after-free happens when the netlink socket is closed
> > before the device is removed but RX tokens or the pending release work
> > keep the binding alive. Such bindings are no longer installed on any
> > queue and were already erased from net_devmem_dmabuf_bindings. Keep
> > bindings in net_devmem_dmabuf_bindings until they are freed, hide
> > unbound ones from TX lookups with an xarray mark, and on
> > NETDEV_UNREGISTER clear binding->dev of unbound RX bindings for the
> > device and unmap them, unless the device is a virtual device leasing
>
> [..]
>
> > queues, for the same reason as above. net_devmem_lock serializes that
> > walk with the final free.
>
> Can you explain net_devmem_lock a bit more? In
> net_devmem_dev_unregister, why can you not net_devmem_lookup_dmabuf and
> then rely on the binding->lock? The rest looks good, but the
> new lock seems a bit excessive?

That problem that my feeble human brain can barely understand is that,
in order to revoke bindings when devices go away, we need to hold the
device somewhere. I decided the a good design would be to keep them in
the xarray but mark them as unalive; that seems nice.

Now xa_erase the binding got moved from unbind time to free time,
which makes sense, but that generates this pathological race, where
net_devmem_lookup_dmabuf for unregister can actually find a binding
for which the last ref has been dropped, but _binding_free() was not
called yet. Fun. I tried to work with the LLM to find another simpler
approach that gets around that but no luck, the overlords find a race
with every suggestion; this seems like the simplest thing to do,
although it is depressing to add yet another lock.

I uploaded v3 btw to address the 1 non-lease devices issue. Seems
we've crossed paths again, let me know what you think and I can spin
V4.

-- 
Thanks,
Mina

Re: [PATCH net v2 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[Stanislav Fomichev]

On 10/14, Mina Almasry wrote:
> On Wed, Oct 14, 2026 at 9:34 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> >
> > On 10/12, Mina Almasry wrote:
> > > When a bound net_device is unregistered while userspace still holds a
> > > netlink or TCP socket reference on a devmem binding,
> > > mp_dmabuf_devmem_uninstall() clears binding->dev to NULL so
> > > netdev_nl_sock_priv_destroy() will not touch the unregistered
> > > net_device, but leaves the dma_buf attachment mapped until
> > > __net_devmem_dmabuf_binding_free() runs later on socket close.
> > >
> > > Because dma_buf_attach() does not take a reference on dma_dev, device
> > > removal frees dma_dev while the attachment is still mapped. Closing the
> > > netlink socket afterwards triggers a slab use-after-free in
> > > dma_unmap_sg_attrs():
> > >
> > >   BUG: KASAN: slab-use-after-free in dma_unmap_sg_attrs+0x76/0x2b0
> > >   Read of size 8 at addr ffff88810fae64b0 by task kworker/3:1/53
> > >   Workqueue: events __net_devmem_dmabuf_binding_free
> > >   Call Trace:
> > >    dma_unmap_sg_attrs+0x76/0x2b0
> > >    unmap_udmabuf+0x8e/0x100
> > >    dma_buf_unmap_attachment+0x145/0x330
> > >    dma_buf_unmap_attachment_unlocked+0x96/0x130
> > >    __net_devmem_dmabuf_binding_free+0x156/0x2e0
> > >   Freed by task 9:
> > >    kfree+0x1b8/0x550
> > >    nsim_bus_dev_release+0xe/0x60
> > >    device_release+0xcf/0x250
> > >
> > > Add net_devmem_dmabuf_binding_unmap() serialized under binding->lock
> > > and null out binding->sgt and binding->attachment after unmapping so
> > > unmapping is idempotent across queue uninstall and deferred binding
> > > destruction. Call it synchronously when the last bound RX queue is
> > > uninstalled in mp_dmabuf_devmem_uninstall(), except when a leased
> > > queue is unleased: the physical device is still registered and, if
> > > restarting its queue failed, may still be using the binding.
> > >
> > > The same use-after-free happens when the netlink socket is closed
> > > before the device is removed but RX tokens or the pending release work
> > > keep the binding alive. Such bindings are no longer installed on any
> > > queue and were already erased from net_devmem_dmabuf_bindings. Keep
> > > bindings in net_devmem_dmabuf_bindings until they are freed, hide
> > > unbound ones from TX lookups with an xarray mark, and on
> > > NETDEV_UNREGISTER clear binding->dev of unbound RX bindings for the
> > > device and unmap them, unless the device is a virtual device leasing
> >
> > [..]
> >
> > > queues, for the same reason as above. net_devmem_lock serializes that
> > > walk with the final free.
> >
> > Can you explain net_devmem_lock a bit more? In
> > net_devmem_dev_unregister, why can you not net_devmem_lookup_dmabuf and
> > then rely on the binding->lock? The rest looks good, but the
> > new lock seems a bit excessive?
> 
> That problem that my feeble human brain can barely understand is that,
> in order to revoke bindings when devices go away, we need to hold the
> device somewhere. I decided the a good design would be to keep them in
> the xarray but mark them as unalive; that seems nice.
> 
> Now xa_erase the binding got moved from unbind time to free time,
> which makes sense, but that generates this pathological race, where
> net_devmem_lookup_dmabuf for unregister can actually find a binding
> for which the last ref has been dropped, but _binding_free() was not
> called yet. Fun. I tried to work with the LLM to find another simpler
> approach that gets around that but no luck, the overlords find a race
> with every suggestion; this seems like the simplest thing to do,
> although it is depressing to add yet another lock.
> 
> I uploaded v3 btw to address the 1 non-lease devices issue. Seems
> we've crossed paths again, let me know what you think and I can spin
> V4.

Let's chat a bit more about this :-) Since it's not a super pressing
issue (yanking device out at runtime), would be nice to understand if we
have exhausted other options..

I'm thinking: can we move, for DMA_FROM_DEVICE (rx) bindings, the unmap
back to net_devmem_unbind_dmabuf? IIRC __net_devmem_dmabuf_binding_free is a
scheduled work because tx can drop the last ref from atomic context, but
for rx, once the queues are down, there is nothing to land into those
mappings.. (there is queue restart failure, but we can make
netif_mp_close_rxq return an error and keep it mapped in this case)

Then we are left only with lingering TX bindings you'll have to care
about in net_devmem_dev_unregister.. For those, the race with
__net_devmem_dmabuf_binding_free can hopefully be sorted out by grabbing
binding->lock and doing the same netdev_hold + netdev_lock bs we do in
netdev_nl_sock_priv_destroy before unmapping and erasing. If we make
__net_devmem_dmabuf_binding_free grab netdev lock then it's gonna
synchronize properly with net_devmem_dev_unregister without any extra locks?

Seems like it should do it? NET_DEVMEM_BINDING_LIVE stays, net_devmem_lock
goes away because unmap for rx happens in unbind and
net_devmem_dev_unregister_tx is mostly the same, just under xa_lock and
unmapping right away (the device is already closed at that point) instead
of doing a second pass.

(tangential: might be also worth it targeting net-next since Linus was not
supper happy with the amount of non-critical fixes going via net; maintainers
please chime in)

The below is what I get if I feed the above in an LLM. Mostly your
version with simplified RX and no new locks. Have not run it through
a proper review, but seems less hairy? Will going in this direction make
it easier to reason about for you as well? (what I do like here is
the consistent locking, even if it's that strange dance of lock/hold/lock)

diff --git a/include/net/page_pool/memory_provider.h b/include/net/page_pool/memory_provider.h
index 255ce4cfd975..a999a9b28bc7 100644
--- a/include/net/page_pool/memory_provider.h
+++ b/include/net/page_pool/memory_provider.h
@@ -26,8 +26,8 @@ void net_mp_niov_clear_page_pool(struct net_iov *niov);
 int netif_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
 		      const struct pp_memory_provider_params *p,
 		      struct netlink_ext_ack *extack);
-void netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
-			const struct pp_memory_provider_params *old_p);
+int netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
+		       const struct pp_memory_provider_params *old_p);
 
 /**
   * net_mp_netmem_place_in_cache() - give a netmem to a page pool
diff --git a/net/core/devmem.c b/net/core/devmem.c
index 71c83730d73d..0c6e308a837d 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -11,6 +11,7 @@
 #include <linux/mm.h>
 #include <linux/netdevice.h>
 #include <linux/types.h>
+#include <net/netdev_lock.h>
 #include <net/netdev_queues.h>
 #include <net/netdev_rx_queue.h>
 #include <net/page_pool/helpers.h>
@@ -23,7 +24,13 @@
 
 /* Device memory support */
 
+/* Bindings stay in net_devmem_dmabuf_bindings until they are freed, so
+ * net_devmem_dev_unregister() can find TX bindings that are kept alive by
+ * skbs after their netlink socket is gone. Only bindings with the LIVE mark
+ * are visible to TX lookups.
+ */
 static DEFINE_XARRAY_FLAGS(net_devmem_dmabuf_bindings, XA_FLAGS_ALLOC1);
+#define NET_DEVMEM_BINDING_LIVE XA_MARK_1
 
 static const struct memory_provider_ops dmabuf_devmem_ops;
 
@@ -59,19 +66,62 @@ static void net_devmem_dmabuf_binding_release(struct percpu_ref *ref)
 	schedule_work(&binding->unbind_w);
 }
 
+static void
+net_devmem_dmabuf_binding_unmap(struct net_devmem_dmabuf_binding *binding)
+{
+	lockdep_assert_held(&binding->lock);
+
+	if (!binding->sgt)
+		return;
+
+	dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
+					  binding->direction);
+	dma_buf_detach(binding->dmabuf, binding->attachment);
+	binding->sgt = NULL;
+	binding->attachment = NULL;
+}
+
+/* Runs from a work because the last reference can be dropped from atomic
+ * context (TX skbs, or the percpu_ref RCU callback after percpu_ref_kill()),
+ * and unmapping the dma-buf may sleep.
+ */
 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;
 
 	if (binding->freelist)
 		WARN(binding->free_count != binding->area.num_niovs,
 		     "destroying dmabuf binding with outstanding net_iovs: total=%zu, free=%u",
 		     binding->area.num_niovs, binding->free_count);
 
+	/* Only TX bindings still have a device here; RX bindings are detached
+	 * in unbind or uninstall. Same dance as netdev_nl_sock_priv_destroy():
+	 * net_devmem_dev_unregister() works on bindings of the dying device
+	 * under its instance lock without holding a reference, so take that
+	 * lock before unmapping and erasing the binding. dev stays valid until
+	 * we hold it because net_devmem_dev_unregister() clears binding->dev
+	 * under binding->lock before the device can go away.
+	 */
+	mutex_lock(&binding->lock);
+	dev = binding->dev;
+	if (dev)
+		netdev_hold(dev, &dev_tracker, GFP_KERNEL);
+	mutex_unlock(&binding->lock);
+
+	if (dev)
+		netdev_lock(dev);
+	mutex_lock(&binding->lock);
+	net_devmem_dmabuf_binding_unmap(binding);
+	mutex_unlock(&binding->lock);
+	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
+	if (dev) {
+		netdev_unlock(dev);
+		netdev_put(dev, &dev_tracker);
+	}
+
 	kvfree(binding->area.niovs);
-	dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
-					  binding->direction);
-	dma_buf_detach(binding->dmabuf, binding->attachment);
 	dma_buf_put(binding->dmabuf);
 	xa_destroy(&binding->bound_rxqs);
 	percpu_ref_exit(&binding->ref);
@@ -116,11 +166,13 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
 	struct netdev_rx_queue *rxq;
 	unsigned long xa_idx;
 	unsigned int rxq_idx;
+	bool busy = false;
 
-	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
+	xa_clear_mark(&net_devmem_dmabuf_bindings, binding->id,
+		      NET_DEVMEM_BINDING_LIVE);
 
 	/* Ensure no tx net_devmem_lookup_dmabuf() are in flight after the
-	 * erase.
+	 * mark is cleared.
 	 */
 	synchronize_net();
 
@@ -135,12 +187,86 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
 
 		rxq_idx = get_netdev_rx_queue_index(rxq);
 
-		netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params);
+		if (netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params))
+			busy = true;
+	}
+
+	/* Once its queues are closed, nothing DMAs into an RX binding anymore.
+	 * Outstanding tokens and skbs only need the niovs, not the mapping,
+	 * so unmap now while the device is known to be alive. RX bindings thus
+	 * never outlive their netlink owner while mapped, and
+	 * net_devmem_dev_unregister() can ignore them. If a queue failed to
+	 * restart, it may still be using the binding, keep it mapped.
+	 */
+	if (binding->direction == DMA_FROM_DEVICE) {
+		mutex_lock(&binding->lock);
+		if (!busy)
+			net_devmem_dmabuf_binding_unmap(binding);
+		WRITE_ONCE(binding->dev, NULL);
+		mutex_unlock(&binding->lock);
 	}
 
 	percpu_ref_kill(&binding->ref);
 }
 
+/* Called on NETDEV_UNREGISTER under the instance lock of @dev. Detach and
+ * unmap TX bindings of @dev, including ones whose netlink socket is gone but
+ * which are still kept alive by skbs. RX bindings are handled by
+ * mp_dmabuf_devmem_uninstall() and net_devmem_unbind_dmabuf().
+ */
+void net_devmem_dev_unregister(struct net_device *dev)
+{
+	XA_STATE(xas, &net_devmem_dmabuf_bindings, 0);
+	struct net_devmem_dmabuf_binding *binding;
+
+	netdev_assert_locked(dev);
+
+	/* Also sent when the device only moves to another netns. */
+	if (dev->reg_state != NETREG_UNREGISTERING)
+		return;
+
+	/* Bindings of other devices may be freed concurrently, their free
+	 * erases them under xa_lock before kfree(), so look at them only under
+	 * xa_lock.
+	 */
+	xas_lock(&xas);
+	xas_for_each(&xas, binding, ULONG_MAX) {
+		if (binding->direction != DMA_TO_DEVICE)
+			continue;
+
+		/* vdev is only compared against, never dereferenced. */
+		if (READ_ONCE(binding->vdev) == dev)
+			WRITE_ONCE(binding->vdev, NULL);
+
+		if (READ_ONCE(binding->dev) != dev)
+			continue;
+
+		xas_clear_mark(&xas, NET_DEVMEM_BINDING_LIVE);
+		xas_pause(&xas);
+		xas_unlock(&xas);
+
+		/* binding->dev == dev, so the free work blocks in
+		 * netdev_lock(dev) and can't free the binding under us, even
+		 * if its last reference is already gone.
+		 *
+		 * Clear dev and unmap in one critical section: once dev is NULL
+		 * the free work no longer takes our lock. No need to wait for
+		 * readers in between: dev is already closed, so nothing can hand
+		 * these DMA addresses to the hardware, and concurrent
+		 * net_devmem_get_binding() / validate_xmit_unreadable_skb()
+		 * only compare against binding->dev.
+		 */
+		mutex_lock(&binding->lock);
+		WRITE_ONCE(binding->dev, NULL);
+		WRITE_ONCE(binding->vdev, NULL);
+		net_devmem_dmabuf_binding_unmap(binding);
+		mutex_unlock(&binding->lock);
+
+		xas_lock(&xas);
+	}
+	xas_unlock(&xas);
+}
+
 int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 				    struct net_devmem_dmabuf_binding *binding,
 				    struct netlink_ext_ack *extack)
@@ -319,6 +445,8 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 			      GFP_KERNEL);
 	if (err < 0)
 		goto err_free_niovs;
+	xa_set_mark(&net_devmem_dmabuf_bindings, binding->id,
+		    NET_DEVMEM_BINDING_LIVE);
 
 	list_add(&binding->list, &priv->bindings);
 
@@ -346,9 +474,15 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id)
 {
 	struct net_devmem_dmabuf_binding *binding;
+	unsigned long xa_idx = id;
 
+	/* LIVE is cleared before the netlink reference is dropped, followed
+	 * by synchronize_net(), so a binding found here can't be freed before
+	 * we get a reference.
+	 */
 	rcu_read_lock();
-	binding = xa_load(&net_devmem_dmabuf_bindings, id);
+	binding = xa_find(&net_devmem_dmabuf_bindings, &xa_idx, id,
+			  NET_DEVMEM_BINDING_LIVE);
 	if (binding) {
 		if (!net_devmem_dmabuf_binding_get(binding))
 			binding = NULL;
@@ -520,16 +654,22 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv,
 	unsigned long xa_idx;
 
 	xa_for_each(&binding->bound_rxqs, xa_idx, bound_rxq) {
-		if (bound_rxq == rxq) {
-			xa_erase(&binding->bound_rxqs, xa_idx);
-			if (xa_empty(&binding->bound_rxqs)) {
-				mutex_lock(&binding->lock);
-				ASSERT_EXCLUSIVE_WRITER(binding->dev);
-				WRITE_ONCE(binding->dev, NULL);
-				mutex_unlock(&binding->lock);
-			}
-			break;
+		if (bound_rxq != rxq)
+			continue;
+
+		xa_erase(&binding->bound_rxqs, xa_idx);
+		if (xa_empty(&binding->bound_rxqs)) {
+			mutex_lock(&binding->lock);
+			ASSERT_EXCLUSIVE_WRITER(binding->dev);
+			WRITE_ONCE(binding->dev, NULL);
+			/* On unlease the physical queue may still be using
+			 * the binding if it failed to restart.
+			 */
+			if (!rxq->lease)
+				net_devmem_dmabuf_binding_unmap(binding);
+			mutex_unlock(&binding->lock);
 		}
+		break;
 	}
 }
 
diff --git a/net/core/devmem.h b/net/core/devmem.h
index 28d6fa7bcb57..5c3564d29b03 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h
@@ -27,7 +27,7 @@ struct net_devmem_dmabuf_binding {
 	 * dereferenced.
 	 */
 	void *vdev;
-	/* Protect dev */
+	/* Protect dev, attachment and sgt */
 	struct mutex lock;
 
 	/* The user holds a ref (via the netlink API) for as long as they want
@@ -38,7 +38,9 @@ struct net_devmem_dmabuf_binding {
 	 *
 	 * The binding undos itself and unmaps the underlying dmabuf once all
 	 * those refs are dropped and the binding is no longer desired or in
-	 * use.
+	 * use. RX bindings are unmapped earlier, once they are no longer
+	 * installed on any queue. TX bindings are unmapped earlier if their
+	 * device is unregistered.
 	 *
 	 * net_devmem_get_net_iov() on dmabuf net_iovs will increment this
 	 * reference, making sure that the binding remains alive until all the
@@ -91,6 +93,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 		       struct netlink_ext_ack *extack);
 struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id);
 void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding);
+void net_devmem_dev_unregister(struct net_device *dev);
 int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 				    struct net_devmem_dmabuf_binding *binding,
 				    struct netlink_ext_ack *extack);
@@ -178,6 +181,10 @@ net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
 {
 }
 
+static inline void net_devmem_dev_unregister(struct net_device *dev)
+{
+}
+
 static inline int
 net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 				struct net_devmem_dmabuf_binding *binding,
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 690c27f63d26..993c368ecde8 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -1500,6 +1500,7 @@ static int netdev_genl_netdevice_event(struct notifier_block *nb,
 		break;
 	case NETDEV_UNREGISTER:
 		netdev_lock(netdev);
+		net_devmem_dev_unregister(netdev);
 		netdev_genl_dev_notify(netdev, NETDEV_CMD_DEV_DEL_NTF);
 		netdev_unlock(netdev);
 		break;
diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
index 00a7011eb4d5..c66347a2f82a 100644
--- a/net/core/netdev_rx_queue.c
+++ b/net/core/netdev_rx_queue.c
@@ -275,8 +275,11 @@ int netif_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
 	return ret;
 }
 
-static void __netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
-				 const struct pp_memory_provider_params *old_p)
+/* Returns 0 once the queue no longer uses @old_p, or an error if the old
+ * queue may still be running with it.
+ */
+static int __netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
+				const struct pp_memory_provider_params *old_p)
 {
 	struct netdev_queue_config qcfg[2];
 	struct netdev_rx_queue *rxq;
@@ -289,35 +292,41 @@ static void __netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
 	 */
 	if (dev->reg_state > NETREG_REGISTERED &&
 	    !rxq->mp_params.mp_ops)
-		return;
+		return 0;
 
 	if (WARN_ON_ONCE(rxq->mp_params.mp_ops != old_p->mp_ops ||
 			 rxq->mp_params.mp_priv != old_p->mp_priv))
-		return;
+		return -EINVAL;
 
 	netdev_queue_config(dev, rxq_idx, &qcfg[0]);
 	memset(&rxq->mp_params, 0, sizeof(rxq->mp_params));
 	netdev_queue_config(dev, rxq_idx, &qcfg[1]);
 
 	err = netdev_rx_queue_reconfig(dev, rxq_idx, &qcfg[0], &qcfg[1]);
-	WARN_ON(err && err != -ENETDOWN);
+	if (err == -ENETDOWN)
+		return 0;
+	WARN_ON(err);
+	return err;
 }
 
-void netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
-			const struct pp_memory_provider_params *old_p)
+int netif_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
+		       const struct pp_memory_provider_params *old_p)
 {
+	int ret;
+
 	if (WARN_ON_ONCE(rxq_idx >= dev->real_num_rx_queues))
-		return;
+		return -EINVAL;
 	if (!netif_rxq_is_leased(dev, rxq_idx))
 		return __netif_mp_close_rxq(dev, rxq_idx, old_p);
 
 	if (!__netif_get_rx_queue_lease(&dev, &rxq_idx, NETIF_VIRT_TO_PHYS)) {
 		WARN_ON_ONCE(1);
-		return;
+		return -EINVAL;
 	}
 	netdev_lock(dev);
-	__netif_mp_close_rxq(dev, rxq_idx, old_p);
+	ret = __netif_mp_close_rxq(dev, rxq_idx, old_p);
 	netdev_unlock(dev);
+	return ret;
 }
 
 void __netif_mp_uninstall_rxq(struct netdev_rx_queue *rxq,

[PATCH net v2 2/4] net: devmem: detach TX bindings on NETDEV_UNREGISTER

[Mina Almasry]

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() 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, clear both
  binding->dev and binding->vdev, wait for in-flight RCU/TX readers via
  synchronize_net(), and unmap the dma_buf under binding->lock.
- 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.

Fixes: bd61848900bf ("net: devmem: Implement TX path")
Fixes: 1abe839b34ae ("net: devmem: support TX over NETMEM_TX_NO_DMA devices")
Cc: Kaiyuan Zhang <kaiyuanz@google.com>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Bobby Eshleman <bobbyeshleman@meta.com>
Cc: Tariq Toukan <tariqt@nvidia.com>
Cc: Kaifeng Wang <kaifengw@google.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
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 | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index 02d607cc87217..a1c029f6ae47f 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -192,6 +192,36 @@ net_devmem_dev_unregister_rx(struct net_devmem_dmabuf_binding *binding,
 	mutex_unlock(&binding->lock);
 }
 
+static void
+net_devmem_dev_unregister_tx(struct net_devmem_dmabuf_binding *binding,
+			     unsigned long xa_idx, struct net_device *dev)
+{
+	if (READ_ONCE(binding->dev) == dev) {
+		xa_clear_mark(&net_devmem_dmabuf_bindings, xa_idx,
+			      NET_DEVMEM_BINDING_LIVE);
+
+		mutex_lock(&binding->lock);
+		WRITE_ONCE(binding->dev, NULL);
+		WRITE_ONCE(binding->vdev, NULL);
+		mutex_unlock(&binding->lock);
+
+		/* Ensure no tx net_devmem_lookup_dmabuf() or
+		 * validate_xmit_unreadable_skb() are in flight after detach.
+		 */
+		synchronize_net();
+
+		mutex_lock(&binding->lock);
+		net_devmem_dmabuf_binding_unmap(binding);
+		mutex_unlock(&binding->lock);
+	} else if (READ_ONCE(binding->vdev) == dev) {
+		mutex_lock(&binding->lock);
+		WRITE_ONCE(binding->vdev, NULL);
+		mutex_unlock(&binding->lock);
+
+		synchronize_net();
+	}
+}
+
 void net_devmem_dev_unregister(struct net_device *dev)
 {
 	struct net_devmem_dmabuf_binding *binding;
@@ -201,6 +231,8 @@ void net_devmem_dev_unregister(struct net_device *dev)
 	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
 		if (binding->direction == DMA_FROM_DEVICE)
 			net_devmem_dev_unregister_rx(binding, xa_idx, dev);
+		else
+			net_devmem_dev_unregister_tx(binding, xa_idx, dev);
 	}
 	mutex_unlock(&net_devmem_lock);
 }
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH net v2 2/4] net: devmem: detach TX bindings on NETDEV_UNREGISTER

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Commit "net: devmem: detach TX bindings on NETDEV_UNREGISTER"
  leaves the dangling binding->dev use-after-free it says it fixes…
- [High] net: devmem: NULL deref in netif_mp_close_rxq() on concurrent
  unregister
- [Medium] net_devmem_dev_unregister_tx() treats every NETDEV_UNREGISTER
  as device destruction, but the same event is sent when a device moves…
- [Medium] net_devmem_dev_unregister_tx() calls synchronize_net() once
  for every TX binding that matches the device, in both the dev branch…

--

net: devmem: detach TX bindings on NETDEV_UNREGISTER

This patch extends net_devmem_dev_unregister() to cover TX devmem
bindings. When binding->dev unregisters, it clears the LIVE mark, clears
both dev and vdev, waits for a grace period and unmaps the dma_buf. When
only binding->vdev unregisters, it clears just vdev.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 02d607cc87217..a1c029f6ae47f 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -192,6 +192,36 @@ net_devmem_dev_unregister_rx(struct net_devmem_dmabuf_binding *binding,
>  	mutex_unlock(&binding->lock);
>  }
>  
> +static void
> +net_devmem_dev_unregister_tx(struct net_devmem_dmabuf_binding *binding,
> +			     unsigned long xa_idx, struct net_device *dev)
> +{
> +	if (READ_ONCE(binding->dev) == dev) {
> +		xa_clear_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +			      NET_DEVMEM_BINDING_LIVE);

[Severity: Medium]
Does NETDEV_UNREGISTER always mean the device is being destroyed here?
__dev_change_net_namespace() also sends it when a device only moves to
another netns:

net/core/dev.c:__dev_change_net_namespace() {
    ...
	 * Note that dev->reg_state stays at NETREG_REGISTERED.
	 * This is wanted because this way 8021q and macvlan know
	 * the device is just moving and can keep their slaves up.
	 */
	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
    ...
}

netdev_genl_netdevice_event() calls net_devmem_dev_unregister() on every
NETDEV_UNREGISTER, so a netns move ends up here too.

If the physical dev of a TX binding is moved, this code clears
NET_DEVMEM_BINDING_LIVE, sets binding->dev and binding->vdev to NULL and
unmaps the dma_buf, even though the device is still alive. If a netkit vdev
is moved, binding->vdev is cleared.

Nothing restores the mark, the pointers or the mapping when the device
re-registers in the new netns. After that, net_devmem_lookup_dmabuf() can't
find the binding, net_devmem_get_binding() returns -ENODEV, and
validate_xmit_unreadable_skb() drops devmem skbs.

netkit_check_lease_unregister() tells the two cases apart with:

	if (READ_ONCE(dev->reg_state) != NETREG_UNREGISTERING ||

Should net_devmem_dev_unregister_tx() check reg_state the same way? Bound
RX bindings already survive netns moves, because
net_devmem_dev_unregister_rx() skips LIVE bindings.

> +
> +		mutex_lock(&binding->lock);
> +		WRITE_ONCE(binding->dev, NULL);
> +		WRITE_ONCE(binding->vdev, NULL);
> +		mutex_unlock(&binding->lock);
> +
> +		/* Ensure no tx net_devmem_lookup_dmabuf() or
> +		 * validate_xmit_unreadable_skb() are in flight after detach.
> +		 */
> +		synchronize_net();
> +
> +		mutex_lock(&binding->lock);
> +		net_devmem_dmabuf_binding_unmap(binding);
> +		mutex_unlock(&binding->lock);
> +	} else if (READ_ONCE(binding->vdev) == dev) {
> +		mutex_lock(&binding->lock);
> +		WRITE_ONCE(binding->vdev, NULL);
> +		mutex_unlock(&binding->lock);
> +
> +		synchronize_net();
> +	}

[Severity: High]
Can a TX binding still end up with a dangling binding->dev? There seems to
be a race between bind-tx on a netkit and unregistration of the physical
device the netkit leases from.

netdev_nl_bind_tx_doit() only checks reg_state on the device the user
names, through netdev_get_by_index_lock() -> netdev_put_lock(). The physical
device comes from netdev_find_netmem_tx_dev():

	if (netif_device_present(phys_dev) &&
	    phys_dev->netmem_tx == NETMEM_TX_DMA)
		return phys_dev;

Both of these conditions still hold after unregister_netdevice_many_notify()
has set phys->reg_state to NETREG_UNREGISTERING.

netdev_nl_bind_tx_doit() then calls netdev_lock(bind_dev) without checking
reg_state again. net_devmem_bind_dmabuf() maps the dma_buf and publishes the
binding with xa_alloc_cyclic() and xa_set_mark(LIVE), without holding
net_devmem_lock.

In the NETDEV_UNREGISTER chain for phys, netdev_genl_nb (subsys_initcall)
is called before netkit's notifier (module_init). So this ordering looks
possible:

CPU1 unregistering phys:
  call_netdevice_notifiers(NETDEV_UNREGISTER, phys)
    netdev_genl_netdevice_event()
      net_devmem_dev_unregister(phys)
        /* walk completes, no binding for phys yet */

CPU2, before netkit's notifier runs:
  netdev_nl_bind_tx_doit()
    netdev_find_netmem_tx_dev()      /* returns phys */
    net_devmem_bind_dmabuf()         /* dev = phys, vdev = netkit */

CPU1 continues:
    netkit_notifier()
      netkit_check_lease_unregister()  /* unregisters netkit */

When the netkit unregisters, net_devmem_dev_unregister_tx() only takes the
vdev branch above, because binding->dev is phys and not the netkit.

Then netkit_uninit() -> netkit_queue_unlease() -> netdev_rx_queue_unlease()
drops the last lease reference on phys with netdev_put(). The binding holds
no netdev reference, so phys is freed while binding->dev still points to it
and the dma_buf is still mapped.

When the netlink socket is later closed, netdev_nl_sock_priv_destroy() reads
binding->dev and calls netdev_hold() and netdev_lock() on it. Isn't that the
same oops quoted in the commit message?

Would it make sense for netdev_nl_bind_tx_doit() to reject
bind_dev->reg_state != NETREG_REGISTERED under netdev_lock(bind_dev)? Or to
publish the binding under net_devmem_lock together with that check?

> +}
> +
>  void net_devmem_dev_unregister(struct net_device *dev)
>  {
>  	struct net_devmem_dmabuf_binding *binding;
> @@ -201,6 +231,8 @@ void net_devmem_dev_unregister(struct net_device *dev)
>  	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
>  		if (binding->direction == DMA_FROM_DEVICE)
>  			net_devmem_dev_unregister_rx(binding, xa_idx, dev);
> +		else
> +			net_devmem_dev_unregister_tx(binding, xa_idx, dev);
>  	}
>  	mutex_unlock(&net_devmem_lock);
>  }

[Severity: High]
Can net_devmem_unbind_dmabuf() end up calling netif_mp_close_rxq()
with a NULL dev when it races with unregistration of the device?

unregister_netdevice_many_notify() runs dev_memory_provider_uninstall()
under netdev_lock(dev). It then drops the instance lock before calling
the NETDEV_UNREGISTER notifiers. If bound_rxqs is not fully emptied at
that point, the binding still has rxqs to close when the netlink socket
goes away. That could happen with a queue above a reduced
real_num_rx_queues, or with a leased queue.

This ordering looks possible:

CPU1 unregistering dev:
  unregister_netdevice_many_notify()
    dev_memory_provider_uninstall(dev)  /* under netdev_lock */
    /* netdev_lock(dev) dropped */

CPU2 closing the netlink socket:
  netdev_nl_sock_priv_destroy()
    netdev_lock(dev)
    net_devmem_unbind_dmabuf()
      /* clears NET_DEVMEM_BINDING_LIVE */
      synchronize_net()

CPU1 continues:
  call_netdevice_notifiers(NETDEV_UNREGISTER, dev)
    netdev_genl_netdevice_event()
      net_devmem_dev_unregister(dev)
        net_devmem_dev_unregister_rx()
          /* LIVE is clear and binding->dev == dev */
          WRITE_ONCE(binding->dev, NULL)

CPU2 continues:
      xa_for_each(&binding->bound_rxqs, ...)
        netif_mp_close_rxq(binding->dev, ...)  /* now NULL */

netif_mp_close_rxq() reads dev->real_num_rx_queues straight away, so
this is a NULL pointer dereference. An unprivileged user can trigger it
by closing a devmem netlink socket while the device is being removed.
The notifier path takes only net_devmem_lock and binding->lock. The
netdev_lock(dev) held by the unbind side does not serialize against it.

The LIVE check in net_devmem_dev_unregister_rx() seems meant to leave
bindings alone while the unbind path still owns them. But
net_devmem_unbind_dmabuf() clears the mark before it is done using
binding->dev, and that opens this window.

I see a few possible fixes:

- net_devmem_unbind_dmabuf() could use a local copy of the dev that
  netdev_nl_sock_priv_destroy() already holds, instead of re-reading
  binding->dev.
- net_devmem_dev_unregister_rx() could also skip bindings that still
  have entries in bound_rxqs.
- The unbind side could hold net_devmem_lock or binding->lock across
  this window.

Which of these did you intend?

[Severity: Medium]
Does this run one expedited RCU grace period for each matching TX binding,
all while RTNL and net_devmem_lock are held?

net_devmem_dev_unregister_tx() calls synchronize_net() for every binding
that matches dev, in both branches. This runs from the NETDEV_UNREGISTER
notifier with RTNL held, so synchronize_net() picks the expedited variant:

net/core/dev.c:synchronize_net() {
	...
	if (from_cleanup_net() || rtnl_is_locked())
		synchronize_rcu_expedited();
	else
		synchronize_rcu();
}

NETDEV_CMD_BIND_TX is unprivileged and has no limit on the number of
bindings, so a user can create many bindings against one device. The next
removal or netns move of that device would then hold RTNL and
net_devmem_lock across N expedited grace periods. That also blocks
__net_devmem_dmabuf_binding_free(), which takes net_devmem_lock.

Could the walk first clear the mark and pointers on all matching bindings,
then call synchronize_net() once, and only then unmap them?

The vdev-only branch also doesn't seem to need a grace period for memory
safety, since vdev is never dereferenced.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012174300.2338784-1-almasrymina%40google.com

[PATCH net v2 3/4] netdevsim: support devmem RX and TX bindings

[Mina Almasry]

Set a 64-bit DMA mask on nsim_bus_dev, enable NETMEM_TX_DMA on the
netdevsim net_device, and configure page_pool instances with
PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | PP_FLAG_ALLOW_UNREADABLE_NETMEM
and queue_idx so devmem bind-rx and bind-tx netlink operations can be
tested against netdevsim in software selftests.

Cc: Tariq Toukan <tariqt@nvidia.com>
Cc: Kaifeng Wang <kaifengw@google.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 drivers/net/netdevsim/bus.c    |  1 +
 drivers/net/netdevsim/netdev.c | 21 +++++++++++++++------
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index 5c55c308487b4..f87f5e037b6fb 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -462,6 +462,7 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count, unsigned int num_queu
 	nsim_bus_dev->port_count = port_count;
 	nsim_bus_dev->num_queues = num_queues;
 	nsim_bus_dev->initial_net = current->nsproxy->net_ns;
+	dma_coerce_mask_and_coherent(&nsim_bus_dev->dev, DMA_BIT_MASK(64));
 	/* Disallow using nsim_bus_dev */
 	smp_store_release(&nsim_bus_dev->init, false);
 
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index b4a99f3ceac60..4415dc01c610b 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -414,16 +414,22 @@ static int nsim_poll(struct napi_struct *napi, int budget)
 	return done;
 }
 
-static int nsim_create_page_pool(struct page_pool **p, struct napi_struct *napi)
+static int nsim_create_page_pool(struct net_device *dev, struct page_pool **p,
+				 struct napi_struct *napi, int idx)
 {
+	struct netdevsim *ns = netdev_priv(dev);
 	struct page_pool_params params = {
 		.order = 0,
 		.pool_size = NSIM_RING_SIZE,
+		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV |
+			 PP_FLAG_ALLOW_UNREADABLE_NETMEM,
+		.max_len = PAGE_SIZE,
 		.nid = NUMA_NO_NODE,
-		.dev = &napi->dev->dev,
+		.dev = &ns->nsim_bus_dev->dev,
 		.napi = napi,
 		.dma_dir = DMA_BIDIRECTIONAL,
-		.netdev = napi->dev,
+		.netdev = dev,
+		.queue_idx = idx,
 	};
 	struct page_pool *pool;
 
@@ -450,7 +456,7 @@ static int nsim_init_napi(struct netdevsim *ns)
 	for (i = 0; i < dev->num_rx_queues; i++) {
 		rq = ns->rq[i];
 
-		err = nsim_create_page_pool(&rq->page_pool, &rq->napi);
+		err = nsim_create_page_pool(dev, &rq->page_pool, &rq->napi, i);
 		if (err)
 			goto err_pp_destroy;
 	}
@@ -762,14 +768,16 @@ nsim_queue_mem_alloc(struct net_device *dev,
 	if (ns->rq_reset_mode == 1) {
 		if (!netif_running(ns->netdev))
 			return -ENETDOWN;
-		return nsim_create_page_pool(&qmem->pp, &ns->rq[idx]->napi);
+		return nsim_create_page_pool(dev, &qmem->pp,
+					     &ns->rq[idx]->napi, idx);
 	}
 
 	qmem->rq = nsim_queue_alloc();
 	if (!qmem->rq)
 		return -ENOMEM;
 
-	err = nsim_create_page_pool(&qmem->rq->page_pool, &qmem->rq->napi);
+	err = nsim_create_page_pool(dev, &qmem->rq->page_pool, &qmem->rq->napi,
+				    idx);
 	if (err)
 		goto err_free;
 
@@ -1051,6 +1059,7 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
 	ns->netdev->netdev_ops = &nsim_netdev_ops;
 	ns->netdev->stat_ops = &nsim_stat_ops;
 	ns->netdev->queue_mgmt_ops = &nsim_queue_mgmt_ops;
+	ns->netdev->netmem_tx = NETMEM_TX_DMA;
 	netdev_lockdep_set_classes(ns->netdev);
 
 	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH net v2 3/4] netdevsim: support devmem RX and TX bindings

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The pp_hold debugfs knob leaks a devmem net_iov, and the page
  pool's inflight count never reaches zero, once rx queue 0 has a dmabuf…
- [Medium] Page-pool allocations always fail on CONFIG_HAS_DMA=n builds
  after this patch.
- [Medium] Enabling NETMEM_TX_DMA exposes netdevsim's software PSP
  checksum path to unreadable payloads.
- [Medium] Deferred page-pool cleanup can call the DMA API on a freed
  nsim_bus_dev on configurations where PP_DMA_INDEX_BITS is 0.

Pre-existing issues:
- [Medium] nsim_pp_hold_write() dereferences ns->rq[0]->page_pool and
  allocates from it while holding only rtnl_lock.

--

netdevsim: support devmem RX and TX bindings

This commit sets a 64-bit DMA mask on the netdevsim bus device and enables
NETMEM_TX_DMA on the netdev. It also creates page pools with DMA mapping
and unreadable netmem support, tied to a queue index, so that devmem
bind-rx and bind-tx can be tested against netdevsim in selftests.

> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> index 5c55c308487b4..f87f5e037b6fb 100644
> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -462,6 +462,7 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count, unsigned int num_queu
>  	nsim_bus_dev->port_count = port_count;
>  	nsim_bus_dev->num_queues = num_queues;
>  	nsim_bus_dev->initial_net = current->nsproxy->net_ns;
> +	dma_coerce_mask_and_coherent(&nsim_bus_dev->dev, DMA_BIT_MASK(64));

[Severity: Medium]
The return value of dma_coerce_mask_and_coherent() is ignored here.

NETDEVSIM has no HAS_DMA dependency, and UML selects NO_DMA unless
UML_DMA_EMULATION is set. On those builds the dma_set_mask stub returns
-EIO, and the device registers anyway.

nsim_create_page_pool() then sets PP_FLAG_DMA_MAP unconditionally. Every
slow-path allocation goes through page_pool_dma_map() ->
dma_map_page_attrs(). The stub returns DMA_MAPPING_ERROR, so the page is
freed.

Will page_pool_alloc_pages() always return NULL on CONFIG_HAS_DMA=n after
this change? If so, nsim_pp_hold_write() would always fail with -ENOMEM.
That would break the page_pool selftests that use pp_hold, which worked
before.

Should the DMA flags only be set when the mask setup succeeds? The other
option is to make NETDEVSIM depend on HAS_DMA.

>  	/* Disallow using nsim_bus_dev */
>  	smp_store_release(&nsim_bus_dev->init, false);
>  
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index b4a99f3ceac60..4415dc01c610b 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -414,16 +414,22 @@ static int nsim_poll(struct napi_struct *napi, int budget)
>  	return done;
>  }
>  
> -static int nsim_create_page_pool(struct page_pool **p, struct napi_struct *napi)
> +static int nsim_create_page_pool(struct net_device *dev, struct page_pool **p,
> +				 struct napi_struct *napi, int idx)
>  {
> +	struct netdevsim *ns = netdev_priv(dev);
>  	struct page_pool_params params = {
>  		.order = 0,
>  		.pool_size = NSIM_RING_SIZE,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV |
> +			 PP_FLAG_ALLOW_UNREADABLE_NETMEM,

[Severity: Medium]
With PP_FLAG_ALLOW_UNREADABLE_NETMEM and .queue_idx set, page_pool_init()
picks up the rx queue's mp_params. After a bind-rx on queue 0 and the
queue restart, ns->rq[0]->page_pool becomes a dmabuf devmem pool.

What happens if pp_hold is written while the device is up?
nsim_pp_hold_write() still uses the page-only API:

drivers/net/netdevsim/netdev.c:nsim_pp_hold_write() {
    ...
		ns->page = page_pool_dev_alloc_pages(ns->rq[0]->page_pool);
		if (!ns->page)
			ret = -ENOMEM;
    ...
}

The call chain would be:

nsim_pp_hold_write()
  page_pool_dev_alloc_pages()
    page_pool_alloc_pages()
      page_pool_alloc_netmems()
        mp_dmabuf_devmem_alloc_netmems()
          page_pool_set_pp_info()
          pool->pages_state_hold_cnt++
      netmem_to_page()
        WARN_ON_ONCE(netmem_is_net_iov(netmem)) -> return NULL

Does this leak the net_iov? The niov is already counted as inflight, but
the handle is dropped without a page_pool_put_netmem() and the write
returns -ENOMEM.

If so, the pool's inflight count can never reach zero. page_pool_destroy()
at close or queue restart would then loop forever in
page_pool_release_retry() with "stalled pool shutdown". Because
mp_ops->destroy never runs, the binding, its genpool chunk and the dma-buf
would not be released either.

Should nsim_pp_hold_write() switch to the netmem API, or refuse when the
pool has a memory provider?

> +		.max_len = PAGE_SIZE,
>  		.nid = NUMA_NO_NODE,
> -		.dev = &napi->dev->dev,
> +		.dev = &ns->nsim_bus_dev->dev,

[Severity: Medium]
Pool pages are now DMA-mapped against the nsim_bus_dev device, and the
pool holds no reference on that device. Can the deferred page_pool release
worker touch the bus device after it has been freed?

nsim_destroy() intentionally returns the pp_hold page late:

	/* Put this intentionally late to exercise the orphaning path */
	if (ns->page) {
		page_pool_put_full_page(pp_page_to_nmdesc(ns->page)->pp,
					ns->page, false);

By this point page_pool_destroy() has already scheduled
page_pool_release_retry().

Some configs have PP_DMA_INDEX_BITS == 0 (PAGE_OFFSET is not a compile-time
constant and POISON_POINTER_DELTA == 0). There,
page_pool_register_dma_index() does not track mappings, so
page_pool_scrub() cannot unmap the outstanding page synchronously through
pool->dma_mapped.

After nsim_destroy() returns, device_unregister() can drop the last
reference, and nsim_bus_dev_release() calls kfree(nsim_bus_dev). The
delayed worker would later drain the ring and call
dma_unmap_page_attrs(pool->p.dev, ...) on the freed device.

The existing orphaning path test seems to exercise this sequence. Whether
it actually triggers depends on which configs have PP_DMA_INDEX_BITS == 0,
and on whether the final put_device() runs before the HZ/10 worker.

>  		.napi = napi,
>  		.dma_dir = DMA_BIDIRECTIONAL,
> -		.netdev = napi->dev,
> +		.netdev = dev,
> +		.queue_idx = idx,
>  	};
>  	struct page_pool *pool;
>  

[ ... ]

> @@ -762,14 +768,16 @@ nsim_queue_mem_alloc(struct net_device *dev,
>  	if (ns->rq_reset_mode == 1) {
>  		if (!netif_running(ns->netdev))
>  			return -ENETDOWN;
> -		return nsim_create_page_pool(&qmem->pp, &ns->rq[idx]->napi);
> +		return nsim_create_page_pool(dev, &qmem->pp,
> +					     &ns->rq[idx]->napi, idx);
>  	}
>  
>  	qmem->rq = nsim_queue_alloc();
>  	if (!qmem->rq)
>  		return -ENOMEM;
>  
> -	err = nsim_create_page_pool(&qmem->rq->page_pool, &qmem->rq->napi);
> +	err = nsim_create_page_pool(dev, &qmem->rq->page_pool, &qmem->rq->napi,
> +				    idx);
>  	if (err)
>  		goto err_free;

[Severity: Medium]
This isn't a bug introduced by this patch, but nsim_pp_hold_write()
dereferences ns->rq[0]->page_pool and allocates from it while holding only
rtnl_lock.

Queue restarts swap ns->rq[idx] in nsim_queue_start(). They destroy and
free the old rq and pool in nsim_queue_mem_free() while holding only the
netdev instance lock:

	page_pool_destroy(qmem->rq->page_pool);
	nsim_queue_free(dev, qmem->rq);

Since rtnl_lock and the netdev instance lock never overlap here, can
pp_hold allocate from a destroyed pool or dereference a freed rq?

The queue_reset debugfs file already provides a trigger:
nsim_qreset_write() -> netdev_lock() -> netdev_rx_queue_restart(). Working
devmem bind-rx and unbind adds another one.

[ ... ]

> @@ -1051,6 +1059,7 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
>  	ns->netdev->netdev_ops = &nsim_netdev_ops;
>  	ns->netdev->stat_ops = &nsim_stat_ops;
>  	ns->netdev->queue_mgmt_ops = &nsim_queue_mgmt_ops;
> +	ns->netdev->netmem_tx = NETMEM_TX_DMA;

[Severity: Medium]
With NETMEM_TX_DMA set, validate_xmit_unreadable_skb() now lets devmem TX
skbs through to nsim_start_xmit(). psp_validate_xmit() only checks that
the association's psd matches dev->psp_dev, so it does not reject
unreadable frags.

For a PSP-associated socket, nsim_start_xmit() calls nsim_do_psp() before
any linearization. If the peer has no PSP device, or does not enable
pas->version, nsim_do_psp() takes the non-decapsulation branch:

drivers/net/netdevsim/psp.c:nsim_do_psp() {
    ...
		uh = udp_hdr(skb);
		udplen = udp_get_len(skb, uh, skb_transport_offset(skb));
		csum = skb_checksum(skb, skb_transport_offset(skb),
				    udplen, 0);
    ...
}

Would this hit WARN_ON_ONCE(!skb_frags_readable(skb)) in skb_checksum()?

skb_checksum() then returns 0, or a partial header-only checksum.
nsim_do_psp() uses that value to write uh->check, sets CHECKSUM_NONE, and
forwards the packet to the peer with a wrong checksum.

Should nsim_do_psp() check skb_frags_readable() before computing the
checksum in software?

>  	netdev_lockdep_set_classes(ns->netdev);
>  
>  	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012174300.2338784-1-almasrymina%40google.com

[PATCH net v2 4/4] selftests: net: add devmem RX and TX netdev unregister tests

[Mina Almasry]

Add devmem_bind_rx_unregister_check and devmem_bind_tx_unregister_check
to nl_netdev.py to verify that unregistering a netdevsim device while a
netlink socket holds an active RX or TX devmem binding synchronously
detaches the dma_buf attachment and cleanly closes the netlink socket
without use-after-free or page faults.

Cc: Tariq Toukan <tariqt@nvidia.com>
Cc: Kaifeng Wang <kaifengw@google.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 tools/testing/selftests/net/nl_netdev.py | 108 ++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
index ceb44c8e1fec5..bad230ee9dcd2 100755
--- a/tools/testing/selftests/net/nl_netdev.py
+++ b/tools/testing/selftests/net/nl_netdev.py
@@ -6,10 +6,14 @@ Tests for the netdev netlink family.
 """
 
 import errno
+import fcntl
+import mmap
+import os
+import struct
 from os import system
-from lib.py import ksft_run, ksft_exit
+from lib.py import ksft_run, ksft_exit, KsftSkipEx
 from lib.py import ksft_eq, ksft_ge, ksft_ne, ksft_raises, ksft_busy_wait
-from lib.py import NetdevFamily, NetdevSimDev, NlError, defer, ip
+from lib.py import EthtoolFamily, NetdevFamily, NetdevSimDev, NlError, defer, ip
 
 
 def empty_check(nf) -> None:
@@ -366,6 +370,102 @@ def page_pool_stats_ifindex_check(nf) -> None:
     ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.info.id')
 
 
+def _create_udmabuf(num_pages=64) -> int:
+    """Create a sealed memfd-backed udmabuf fd for devmem tests."""
+    if not os.path.exists("/dev/udmabuf"):
+        raise KsftSkipEx("/dev/udmabuf is not available")
+
+    size = num_pages * mmap.PAGESIZE
+    memfd = os.memfd_create("devmem-ksft", os.MFD_ALLOW_SEALING)
+    os.ftruncate(memfd, size)
+    fcntl.fcntl(memfd, 1033, 0x0002)  # F_ADD_SEALS, F_SEAL_SHRINK
+    devfd = os.open("/dev/udmabuf", os.O_RDWR)
+    req = bytearray(struct.pack("IIQQ", memfd, 0, 0, size))
+    dmabuf_fd = fcntl.ioctl(devfd, 0x40187542, req)  # UDMABUF_CREATE
+    os.close(devfd)
+    os.close(memfd)
+    return dmabuf_fd
+
+
+def _dmabuf_attached_devs():
+    """Return attached device names from debugfs dma_buf/bufinfo if available."""
+    path = "/sys/kernel/debug/dma_buf/bufinfo"
+    if not os.path.exists(path):
+        return None
+    with open(path, "r", encoding="utf-8") as f:
+        text = f.read()
+    attached = []
+    in_attached = False
+    for line in text.splitlines():
+        if line.strip() == "Attached Devices:":
+            in_attached = True
+            continue
+        if in_attached:
+            if line.startswith("\t") and line.strip():
+                attached.append(line.strip())
+            else:
+                in_attached = False
+    return attached
+
+
+def devmem_bind_rx_unregister_check(_nf) -> None:
+    """Verify RX devmem bindings detach dma_buf synchronously on netdev unregister."""
+    dmabuf_fd = _create_udmabuf()
+    nf_priv = NetdevFamily()
+    ef = EthtoolFamily()
+    nsimdev = NetdevSimDev(queue_count=2)
+    nsim = nsimdev.nsims[0]
+
+    ip(f"link set dev {nsim.ifname} up")
+    ef.rings_set({"header": {"dev-index": nsim.ifindex},
+                  "tcp-data-split": "enabled"})
+    nf_priv.bind_rx({
+        "ifindex": nsim.ifindex,
+        "fd": dmabuf_fd,
+        "queues": [{"id": 1, "type": "rx"}],
+    })
+
+    attached = _dmabuf_attached_devs()
+    if attached is not None:
+        ksft_ge(len(attached), 1)
+
+    nsimdev.remove()
+
+    attached = _dmabuf_attached_devs()
+    if attached is not None:
+        ksft_eq(len(attached), 0)
+
+    del nf_priv
+    os.close(dmabuf_fd)
+
+
+def devmem_bind_tx_unregister_check(_nf) -> None:
+    """Verify TX devmem bindings detach cleanly on netdev unregister without UAF."""
+    dmabuf_fd = _create_udmabuf()
+    nf_priv = NetdevFamily()
+    nsimdev = NetdevSimDev(queue_count=2)
+    nsim = nsimdev.nsims[0]
+
+    ip(f"link set dev {nsim.ifname} up")
+    nf_priv.bind_tx({
+        "ifindex": nsim.ifindex,
+        "fd": dmabuf_fd,
+    })
+
+    attached = _dmabuf_attached_devs()
+    if attached is not None:
+        ksft_ge(len(attached), 1)
+
+    nsimdev.remove()
+
+    attached = _dmabuf_attached_devs()
+    if attached is not None:
+        ksft_eq(len(attached), 0)
+
+    del nf_priv
+    os.close(dmabuf_fd)
+
+
 def main() -> None:
     """ Ksft boiler plate main """
     nf = NetdevFamily()
@@ -379,7 +479,9 @@ def main() -> None:
               page_pool_check,
               page_pool_dump_ifindex,
               page_pool_ifindex_leak_check,
-              page_pool_stats_ifindex_check
+              page_pool_stats_ifindex_check,
+              devmem_bind_rx_unregister_check,
+              devmem_bind_tx_unregister_check
               ],
              args=(nf, ))
     ksft_exit()
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH net v2 4/4] selftests: net: add devmem RX and TX netdev unregister tests

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 6 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] `del nf_priv` in devmem_bind_rx_unregister_check() and
  devmem_bind_tx_unregister_check()…
- [Medium] _dmabuf_attached_devs() (nl_netdev.py:390-408) collects the
  'Attached Devices:' entries for every dma-buf in…
- [Medium] The new tests in tools/testing/selftests/net/nl_netdev.py need
  /dev/udmabuf (CONFIG_UDMABUF) and CONFIG_NET_DEVMEM.
- [Medium] devmem_bind_rx_unregister_check() and
  devmem_bind_tx_unregister_check() create the netdevsim device…
- [Medium] The detach check is what these tests mainly claim to verify
  (RX docstring: 'Verify RX devmem bindings detach dma_buf synchronously…
- [Medium] _create_udmabuf() (nl_netdev.py:384) issues
  `fcntl.ioctl(devfd, 0x40187542, req)`, which hardcodes the asm-generic…
- [Low] _create_udmabuf() (nl_netdev.py:379-387) opens `memfd` (line 379)
  and `devfd` (line 382) without try/finally.

--

selftests: net: add devmem RX and TX netdev unregister tests

This adds devmem_bind_rx_unregister_check() and
devmem_bind_tx_unregister_check() to nl_netdev.py. Each test binds a
udmabuf to a netdevsim device over the netdev netlink family, then
removes the device. It checks debugfs dma_buf/bufinfo to confirm the
attachment is gone.

> diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
> index ceb44c8e1fec5..bad230ee9dcd2 100755
> --- a/tools/testing/selftests/net/nl_netdev.py
> +++ b/tools/testing/selftests/net/nl_netdev.py

[ ... ]

> @@ -366,6 +370,102 @@ def page_pool_stats_ifindex_check(nf) -> None:
>      ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.info.id')
>  
>  
> +def _create_udmabuf(num_pages=64) -> int:
> +    """Create a sealed memfd-backed udmabuf fd for devmem tests."""
> +    if not os.path.exists("/dev/udmabuf"):
> +        raise KsftSkipEx("/dev/udmabuf is not available")

[Severity: Medium]
Will these tests ever run with the config fragment for this directory?

tools/testing/selftests/net/config has CONFIG_NETDEVSIM=m. It has none of
CONFIG_UDMABUF, CONFIG_DMA_SHARED_BUFFER, CONFIG_NET_DEVMEM or
CONFIG_DEBUG_FS. NET_DEVMEM depends on DMA_SHARED_BUFFER:

net/Kconfig:
config NET_DEVMEM
	def_bool y
	select GENERIC_ALLOCATOR
	depends on DMA_SHARED_BUFFER

The existing devmem tests use tools/testing/selftests/drivers/net/hw/config.
That file sets CONFIG_NET_DEVMEM=y and CONFIG_UDMABUF=y.

On a kernel built from net/config, the skip above fires every time. The
regression coverage for "net: devmem: unmap dma_buf synchronously on queue
uninstall" and "net: devmem: detach TX bindings on NETDEV_UNREGISTER"
would then never run in CI for this directory.

Should CONFIG_UDMABUF=y, CONFIG_DMA_SHARED_BUFFER=y and CONFIG_DEBUG_FS=y
be added to tools/testing/selftests/net/config?

> +
> +    size = num_pages * mmap.PAGESIZE
> +    memfd = os.memfd_create("devmem-ksft", os.MFD_ALLOW_SEALING)
> +    os.ftruncate(memfd, size)
> +    fcntl.fcntl(memfd, 1033, 0x0002)  # F_ADD_SEALS, F_SEAL_SHRINK
> +    devfd = os.open("/dev/udmabuf", os.O_RDWR)
> +    req = bytearray(struct.pack("IIQQ", memfd, 0, 0, size))
> +    dmabuf_fd = fcntl.ioctl(devfd, 0x40187542, req)  # UDMABUF_CREATE

[Severity: Medium]
Is 0x40187542 correct on every architecture? The UAPI defines:

include/uapi/linux/udmabuf.h:
#define UDMABUF_CREATE       _IOW('u', 0x42, struct udmabuf_create)

and the _IOW() encoding depends on the architecture. On powerpc,
_IOC_WRITE is 4U and _IOC_SIZEBITS is 13, so the command works out to
0x80187542. mips, sparc and alpha use the same layout.

On those architectures, udmabuf_ioctl() falls through to its default case
and returns -ENOTTY:

drivers/dma-buf/udmabuf.c:udmabuf_ioctl() {
	switch (ioctl) {
	case UDMABUF_CREATE:
	...
	default:
		ret = -ENOTTY;
}

Python then raises OSError, and both tests report FAIL instead of SKIP
before any binding happens. Could the command value be computed from the
architecture's ioctl encoding instead?

> +    os.close(devfd)
> +    os.close(memfd)

[Severity: Low]
Does this leak memfd and devfd when something fails partway through? If
os.ftruncate(), the F_ADD_SEALS fcntl, os.open("/dev/udmabuf") or the
UDMABUF_CREATE ioctl raises, these two os.close() calls are skipped. The
descriptors, and the memfd pages, then stay open for the rest of the ksft
process. Would a try/finally around these steps help?

> +    return dmabuf_fd
> +
> +
> +def _dmabuf_attached_devs():
> +    """Return attached device names from debugfs dma_buf/bufinfo if available."""
> +    path = "/sys/kernel/debug/dma_buf/bufinfo"
> +    if not os.path.exists(path):
> +        return None

[Severity: Medium]
When bufinfo is missing, can these tests pass without checking anything?

The detach check is the main thing the RX docstring says this test
verifies. If CONFIG_DEBUG_FS is off, debugfs is not mounted, or the file
is not readable, this returns None. The callers' "if attached is not None:"
guards then skip both assertions, and the test reports PASS.

In that setup, the test would still pass if net_devmem_dev_unregister()
stopped calling net_devmem_dmabuf_binding_unmap(), which is where
dma_buf_detach() happens. net/config does not enable CONFIG_DEBUG_FS
either.

Should this raise KsftSkipEx instead of returning None?

> +    with open(path, "r", encoding="utf-8") as f:
> +        text = f.read()
> +    attached = []
> +    in_attached = False
> +    for line in text.splitlines():
> +        if line.strip() == "Attached Devices:":
> +            in_attached = True
> +            continue
> +        if in_attached:
> +            if line.startswith("\t") and line.strip():
> +                attached.append(line.strip())
> +            else:
> +                in_attached = False
> +    return attached

[Severity: Medium]
Does this count attachments from every dma-buf on the system, not just the
udmabuf the test created?

dma_buf_debug_show() walks the global dmabuf_list. For each buffer it
prints a header and an "Attached Devices:" block:

drivers/dma-buf/dma-buf.c:dma_buf_debug_show() {
	...
		list_for_each_entry(attach_obj, &buf_obj->attachments, node) {
			seq_printf(s, "\t%s\n", dev_name(attach_obj->dev));
	...
}

This loop adds every buffer's entries to one list. The before-bind
ksft_ge(len(attached), 1) check can then pass because of some other
buffer, even if the devmem bind never attached.

The after-remove ksft_eq(len(attached), 0) check would also fail on any
machine where another dma-buf has an attachment, even if the devmem
binding detached correctly. Examples include GPU or DRM prime, V4L2, a
parallel test, or a binding left behind by an earlier test.

Could the parser filter on the test buffer's inode, for example by
matching the ino column against os.fstat(dmabuf_fd).st_ino?

> +
> +
> +def devmem_bind_rx_unregister_check(_nf) -> None:
> +    """Verify RX devmem bindings detach dma_buf synchronously on netdev unregister."""
> +    dmabuf_fd = _create_udmabuf()
> +    nf_priv = NetdevFamily()
> +    ef = EthtoolFamily()
> +    nsimdev = NetdevSimDev(queue_count=2)
> +    nsim = nsimdev.nsims[0]
> +
> +    ip(f"link set dev {nsim.ifname} up")
> +    ef.rings_set({"header": {"dev-index": nsim.ifindex},
> +                  "tcp-data-split": "enabled"})
> +    nf_priv.bind_rx({
> +        "ifindex": nsim.ifindex,
> +        "fd": dmabuf_fd,
> +        "queues": [{"id": 1, "type": "rx"}],
> +    })

[Severity: Medium]
What happens to the netdevsim device if ip(), ef.rings_set() or
nf_priv.bind_rx() raises here?

nsimdev is created without "with" or defer(nsimdev.remove). ksft_run()
only flushes registered defers, so nothing removes the device. That leaves
it registered in the kernel even after the test process exits:

tools/testing/selftests/net/lib/py/nsim.py:NetdevSimDev.remove() {
        if not self.removed:
            self.ctrl_write("del_device", "%u" % (self.addr, ))
            self.removed = True
}

dmabuf_fd and the netlink socket also leak for the rest of the run.

Also, an NlError with EOPNOTSUPP from bind_rx() or bind_tx() is not turned
into KsftSkipEx. Kernels that have the devmem fixes but not "netdevsim:
support devmem RX and TX bindings" would then report FAIL rather than
SKIP.

defer is already imported in this file, and the other netdevsim tests use
"with NetdevSimDev(...)". The same pattern is in
devmem_bind_tx_unregister_check() around bind_tx().

> +
> +    attached = _dmabuf_attached_devs()
> +    if attached is not None:
> +        ksft_ge(len(attached), 1)
> +
> +    nsimdev.remove()
> +
> +    attached = _dmabuf_attached_devs()
> +    if attached is not None:
> +        ksft_eq(len(attached), 0)
> +
> +    del nf_priv

[Severity: Medium]
Does "del nf_priv" actually close the netlink socket that owns the
binding? YnlFamily.__init__() stores partials of bound methods on the
instance:

tools/net/ynl/pyynl/lib/ynl.py:YnlFamily.__init__() {
            bound_f = functools.partial(self._op, op_name)
            setattr(self, op.ident_name, bound_f)
}

Each partial refers back to self, which makes a reference cycle. Deleting
the local name drops only one reference. YnlFamily has no __del__, so
self.sock stays open until the cyclic GC happens to run or the process
exits.

The commit message says the tests verify that unregister:

    "cleanly closes the netlink socket without use-after-free or page
    faults."

As written, the genl release -> netdev_nl_sock_priv_destroy() path after
unregister never runs inside the test. That is the path fixed by "net:
devmem: detach TX bindings on NETDEV_UNREGISTER". A regression there would
show up later, or not be linked to this test at all, and the test would
still report PASS.

YnlFamily already has close() and __enter__/__exit__. Would nf_priv.close()
or "with NetdevFamily() as nf_priv:" work here? The same applies to
"del nf_priv" in devmem_bind_tx_unregister_check().

> +    os.close(dmabuf_fd)
> +
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012174300.2338784-1-almasrymina%40google.com

Re: [PATCH net v2 4/4] selftests: net: add devmem RX and TX netdev unregister tests

[Dragos Tatulea]

On 12.10.26 19:43, Mina Almasry wrote:
> Add devmem_bind_rx_unregister_check and devmem_bind_tx_unregister_check
> to nl_netdev.py to verify that unregistering a netdevsim device while a
> netlink socket holds an active RX or TX devmem binding synchronously
> detaches the dma_buf attachment and cleanly closes the netlink socket
> without use-after-free or page faults.
> 
> Cc: Tariq Toukan <tariqt@nvidia.com>
> Cc: Kaifeng Wang <kaifengw@google.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> ---
>  tools/testing/selftests/net/nl_netdev.py | 108 ++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
> index ceb44c8e1fec5..bad230ee9dcd2 100755
> --- a/tools/testing/selftests/net/nl_netdev.py
> +++ b/tools/testing/selftests/net/nl_netdev.py
> @@ -6,10 +6,14 @@ Tests for the netdev netlink family.
>  """
>  
>  import errno
> +import fcntl
> +import mmap
> +import os
> +import struct
>  from os import system
> -from lib.py import ksft_run, ksft_exit
> +from lib.py import ksft_run, ksft_exit, KsftSkipEx
>  from lib.py import ksft_eq, ksft_ge, ksft_ne, ksft_raises, ksft_busy_wait
> -from lib.py import NetdevFamily, NetdevSimDev, NlError, defer, ip
> +from lib.py import EthtoolFamily, NetdevFamily, NetdevSimDev, NlError, defer, ip
>  
>  
>  def empty_check(nf) -> None:
> @@ -366,6 +370,102 @@ def page_pool_stats_ifindex_check(nf) -> None:
>      ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.info.id')
>  
>  
> +def _create_udmabuf(num_pages=64) -> int:
> +    """Create a sealed memfd-backed udmabuf fd for devmem tests."""
> +    if not os.path.exists("/dev/udmabuf"):
> +        raise KsftSkipEx("/dev/udmabuf is not available")
> +
> +    size = num_pages * mmap.PAGESIZE
> +    memfd = os.memfd_create("devmem-ksft", os.MFD_ALLOW_SEALING)
> +    os.ftruncate(memfd, size)
> +    fcntl.fcntl(memfd, 1033, 0x0002)  # F_ADD_SEALS, F_SEAL_SHRINK
> +    devfd = os.open("/dev/udmabuf", os.O_RDWR)
> +    req = bytearray(struct.pack("IIQQ", memfd, 0, 0, size))
> +    dmabuf_fd = fcntl.ioctl(devfd, 0x40187542, req)  # UDMABUF_CREATE
Nit: this could be a constant

> +    os.close(devfd)
> +    os.close(memfd)
> +    return dmabuf_fd
> +
> +
> +def _dmabuf_attached_devs():
> +    """Return attached device names from debugfs dma_buf/bufinfo if available."""
> +    path = "/sys/kernel/debug/dma_buf/bufinfo"
> +    if not os.path.exists(path):
> +        return None
> +    with open(path, "r", encoding="utf-8") as f:
> +        text = f.read()
> +    attached = []
> +    in_attached = False
> +    for line in text.splitlines():
> +        if line.strip() == "Attached Devices:":
> +            in_attached = True
> +            continue
> +        if in_attached:
> +            if line.startswith("\t") and line.strip():
> +                attached.append(line.strip())
> +            else:
> +                in_attached = False
> +    return attached
> +
> +
> +def devmem_bind_rx_unregister_check(_nf) -> None:
> +    """Verify RX devmem bindings detach dma_buf synchronously on netdev unregister."""
> +    dmabuf_fd = _create_udmabuf()
> +    nf_priv = NetdevFamily()
> +    ef = EthtoolFamily()
> +    nsimdev = NetdevSimDev(queue_count=2)
> +    nsim = nsimdev.nsims[0]
> +
> +    ip(f"link set dev {nsim.ifname} up")
> +    ef.rings_set({"header": {"dev-index": nsim.ifindex},
> +                  "tcp-data-split": "enabled"})
> +    nf_priv.bind_rx({
> +        "ifindex": nsim.ifindex,
> +        "fd": dmabuf_fd,
> +        "queues": [{"id": 1, "type": "rx"}],
> +    })
> +
> +    attached = _dmabuf_attached_devs()
> +    if attached is not None:
> +        ksft_ge(len(attached), 1)
> +
> +    nsimdev.remove()
> +
> +    attached = _dmabuf_attached_devs()
> +    if attached is not None:
> +        ksft_eq(len(attached), 0)
> +
> +    del nf_priv
> +    os.close(dmabuf_fd)
> +
> +
> +def devmem_bind_tx_unregister_check(_nf) -> None:
> +    """Verify TX devmem bindings detach cleanly on netdev unregister without UAF."""
> +    dmabuf_fd = _create_udmabuf()
> +    nf_priv = NetdevFamily()
> +    nsimdev = NetdevSimDev(queue_count=2)
> +    nsim = nsimdev.nsims[0]
> +
> +    ip(f"link set dev {nsim.ifname} up")
> +    nf_priv.bind_tx({
> +        "ifindex": nsim.ifindex,
> +        "fd": dmabuf_fd,
> +    })
> +
> +    attached = _dmabuf_attached_devs()
> +    if attached is not None:
> +        ksft_ge(len(attached), 1)
> +
> +    nsimdev.remove()
> +
> +    attached = _dmabuf_attached_devs()
> +    if attached is not None:
> +        ksft_eq(len(attached), 0)
> +
> +    del nf_priv
> +    os.close(dmabuf_fd)
> +
> +
>  def main() -> None:
>      """ Ksft boiler plate main """
>      nf = NetdevFamily()
> @@ -379,7 +479,9 @@ def main() -> None:
>                page_pool_check,
>                page_pool_dump_ifindex,
>                page_pool_ifindex_leak_check,
> -              page_pool_stats_ifindex_check
> +              page_pool_stats_ifindex_check,
> +              devmem_bind_rx_unregister_check,
> +              devmem_bind_tx_unregister_check
>                ],
>               args=(nf, ))
>      ksft_exit()

Other than the above nit:
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>

Thanks,
Dragos

Re: [PATCH net v2 0/4] net: devmem: fix RX and TX binding teardown on device unregistration

[Dragos Tatulea]

On 12.10.26 19:42, Mina Almasry wrote:
> Fix two device-unregistration lifecycle bugs in devmem TCP bindings and
> add netdevsim support plus selftests covering both RX and TX device
> unregistration:
> 
> 1. net: devmem: unmap dma_buf synchronously on queue uninstall
>    Synchronously unmap and detach binding->attachment under binding->lock
>    when the last RX queue is uninstalled in mp_dmabuf_devmem_uninstall(),
>    and unmap RX bindings that were already unbound on NETDEV_UNREGISTER,
>    preventing slab-use-after-free in dma_unmap_sg_attrs() when the binding
>    is freed after dma_dev.
> 
> 2. net: devmem: detach TX bindings on NETDEV_UNREGISTER
>    Extend net_devmem_dev_unregister() to fully detach and unmap TX
>    bindings when binding->dev unregisters, or clear binding->vdev alone
>    when only a virtual device unregisters while binding->dev lives.
> 
> 3. netdevsim: support devmem RX and TX bindings
>    Enable PP_FLAG_DMA_MAP | PP_FLAG_ALLOW_UNREADABLE_NETMEM, 64-bit DMA mask
>    on nsim_bus_dev, and NETMEM_TX_DMA on netdevsim so devmem RX/TX bindings
>    can be exercised in software selftests.
> 
> 4. selftests: net: add devmem RX and TX netdev unregister tests
>    Add devmem_bind_rx_unregister_check and devmem_bind_tx_unregister_check
>    to tools/testing/selftests/net/nl_netdev.py.
> 
> Cc: Tariq Toukan <tariqt@nvidia.com>
> Cc: Kaifeng Wang <kaifengw@google.com>
> 
> ---
> v2:
> - 1/4: Don't unmap when a leased queue is unleased. Keep bindings in
>   net_devmem_dmabuf_bindings until freed, hidden from TX lookups by an
>   xarray mark, and unmap unbound RX bindings on NETDEV_UNREGISTER. Update
>   the binding->lock and binding->ref comments. (Sashiko)
> - 2/4: Detach TX bindings from the same net_devmem_lock-protected walk,
>   which now also covers unbound TX bindings, and clear the LIVE mark
>   instead of erasing the ID. The walk no longer uses RCU or takes
>   binding references (Stanislav).
> - Link to v1: https://lore.kernel.org/netdev/20261010025532.839559-1-almasrymina@google.com/
> 
> Mina Almasry (4):
>   net: devmem: unmap dma_buf synchronously on queue uninstall
>   net: devmem: detach TX bindings on NETDEV_UNREGISTER
>   netdevsim: support devmem RX and TX bindings
>   selftests: net: add devmem RX and TX netdev unregister tests
> 
>  drivers/net/netdevsim/bus.c              |   1 +
>  drivers/net/netdevsim/netdev.c           |  21 ++--
>  net/core/devmem.c                        | 121 ++++++++++++++++++++---
>  net/core/devmem.h                        |  10 +-
>  net/core/netdev-genl.c                   |   1 +
>  tools/testing/selftests/net/nl_netdev.py | 108 +++++++++++++++++++-
>  6 files changed, 236 insertions(+), 26 deletions(-)
> 
> 
> base-commit: 4f6ac74fa594a918d41255c941256e1d51592d3b

Went through the series and it looks good to my feeble human brain:

Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>

I am biased though because I need Mina's code to add support for
unbinding the dma device for data-direct.

Thanks,
Dragos