[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: use-after-free in dma_unmap_sg_attrs() for devmem leased RX bindings
--

commit 0978772617865658c49d5d8aff7d5a2c206deaf4
Author: Mina Almasry <almasrymina@google.com>
net: devmem: unmap dma_buf synchronously on queue uninstall

This commit changes the unmapping of the dma_buf attachment to happen
synchronously when the queue is uninstalled. It introduces
net_devmem_dmabuf_binding_unmap() to clear the attachment, preventing a
use-after-free if the physical device is unregistered while references
are still held.

> 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

[ ... ]

> @@ -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;

[Severity: High]
For leased RX queues, if the physical device unregisters, could binding->dev
still point to the virtual device instead of the physical device?

If the physical device unregisters without prior unleasing, or if the queue
was previously uninstalled (setting binding->dev to NULL), this check would
evaluate to true and skip the unmapping. This would leave the dma_buf
mapped to the physical device's dma_dev, which is subsequently freed.

When the binding is ultimately destroyed via
__net_devmem_dmabuf_binding_free(), could this trigger a use-after-free in
dma_buf_unmap_attachment_unlocked() when it accesses the freed dma_dev?

> +
> +	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);
> +}

[ ... ]

> @@ -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);

[Severity: High]
Here binding->dev is unconditionally set to NULL, but for leased queues,
net_devmem_dmabuf_binding_unmap() is explicitly skipped.

If the physical device is later unregistered, won't
net_devmem_dev_unregister_rx() fail to unmap the binding because
binding->dev was already set to NULL?

Could this result in the dma_buf remaining mapped to the physical device
until the binding is eventually freed, causing a use-after-free at that
time?

>  		}
> +		break;
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012174300.2338784-1-almasrymina@google.com?part=1

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

[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: O(N) synchronize_net() calls under rtnl_lock cause RTNL stall and IPI storm
- [High] net: devmem: NULL deref in netif_mp_close_rxq() on concurrent unregister
--

commit 92717c23489a6c952c6219d6248685cf5a364b91
Author: Mina Almasry <almasrymina@google.com>
Subject: net: devmem: detach TX bindings on NETDEV_UNREGISTER

This patch extends net_devmem_dev_unregister() to detach TX bindings when
a device is unregistered. It waits for in-flight readers via
synchronize_net() and unmaps the dma_buf to prevent dangling pointers.

> 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
[ ... ]
> +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();

[Severity: High]
Does this O(N) synchronize_net() inside a loop cause a potential denial of
service?

Because net_devmem_dev_unregister() iterates over all bindings using
xa_for_each() and calls net_devmem_dev_unregister_tx() for each, this
synchronize_net() executes sequentially for every matching TX binding.

This executes in the NETDEV_UNREGISTER notifier chain under the global
rtnl_lock, which causes synchronize_net() to fall back to
synchronize_rcu_expedited(). For a large number of bindings, this could force
numerous consecutive expedited RCU grace periods, sending a massive amount of
IPIs system-wide and stalling the RTNL lock.

Could we lift the synchronize_net() out of the per-binding loop, batch the
pointer clearing, issue a single synchronize_net(), and then perform the
unmapping?

> +
> +		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);

[Severity: High]
Is there a NULL pointer dereference risk here if net_devmem_unbind_dmabuf()
runs concurrently with device unregistration?

If unregister_netdevice_many_notify() calls dev_memory_provider_uninstall()
and then drops the lock, the following race could occur:

1. netdev_nl_sock_priv_destroy() calls net_devmem_unbind_dmabuf(), clearing
   the LIVE mark and entering synchronize_net().

2. The unregister notifier triggers net_devmem_dev_unregister_rx() here.

3. Because the LIVE mark was cleared in step 1, net_devmem_dev_unregister_rx()
   bypasses its early return and unconditionally sets binding->dev = NULL.

4. When net_devmem_unbind_dmabuf() resumes, it executes
   xa_for_each(&binding->bound_rxqs) and calls
   netif_mp_close_rxq(binding->dev) using the now NULL binding->dev.

Could this crash the kernel at dev->real_num_rx_queues in netif_mp_close_rxq()?

> +		else
> +			net_devmem_dev_unregister_tx(binding, xa_idx, dev);
>  	}
>  	mutex_unlock(&net_devmem_lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012174300.2338784-1-almasrymina@google.com?part=2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] netmem: devmem leak and kernel warning in nsim_pp_hold_write()
--

commit b58e0da7daeddfad13177ed902471523d6b96cbf
Author: Mina Almasry <almasrymina@google.com>

netdevsim: support devmem RX and TX bindings

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.

> 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]
When PP_FLAG_ALLOW_UNREADABLE_NETMEM is set and devmem is bound, will this
cause a leak and kernel warning in the debugfs handler?

If nsim_pp_hold_write() is called to test allocating memory from the pool,
it uses page_pool_dev_alloc_pages():

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

Because devmem may be bound, page_pool_alloc_netmems() can allocate an
unreadable net_iov. When this is passed to netmem_to_page(), it detects an
IOV, triggers a WARN_ON_ONCE(), and returns NULL.

nsim_pp_hold_write() receives NULL and returns -ENOMEM, leaking the
allocated devmem netmem_ref because it is neither stored nor freed. 

While this is restricted to root via debugfs, does this break the exact
devmem testing functionality this patch intends to support?

> +		.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,
>  	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012174300.2338784-1-almasrymina@google.com?part=3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: net: test falsely assumes global dma_buf attachments drop to zero
- [Medium] selftests: net: netdevsim device leak on exception paths
--

commit 3ebc183bfb941c2f36dca38f1bd05b883cc61a30
Author: Mina Almasry <almasrymina@google.com>

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

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.

> diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
> --- a/tools/testing/selftests/net/nl_netdev.py
> +++ b/tools/testing/selftests/net/nl_netdev.py
[ ... ]
> +def _dmabuf_attached_devs():
> +    """Return attached device names from debugfs dma_buf/bufinfo if available."""
> +    path = "/sys/kernel/debug/dma_buf/bufinfo"
[ ... ]
> +    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())

[Severity: Medium]
Will this parsing approach unconditionally aggregate all attachments across all
dma_bufs in the system?

Because this does not filter the debugfs output to the specific dma_buf
created by the test, if the system executing the test has any other active
dma_bufs with attachments (e.g., from DRM graphics drivers), the global
attached count will remain non-zero.

> +            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]

[Severity: Medium]
If an exception occurs during the test, will this leak the netdevsim device?

The netdevsim device is instantiated without registering a teardown deferral
like defer(nsimdev.remove) or using a try/finally block. If any step before
the manual nsimdev.remove() call raises an exception, the cleanup is
bypassed. This permanently leaks the kernel netdevsim device and its sysfs
entries in the kernel namespace, potentially causing subsequent tests to fail.

[ ... ]
> +    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)

[Severity: Medium]
Can this assertion spuriously fail if other dma_bufs exist system-wide?

Since _dmabuf_attached_devs() aggregates all attachments without filtering
for the specific dma_buf created by the test, the attached count will not
drop to zero if the system has other active dma_bufs, causing the test to
fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012174300.2338784-1-almasrymina@google.com?part=4

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