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

[PATCH net v1 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(),
   preventing slab-use-after-free in dma_unmap_sg_attrs() when the netlink
   socket closes after dma_dev is freed.

2. net: devmem: detach TX bindings on NETDEV_UNREGISTER
   Add net_devmem_dev_unregister() on NETDEV_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>

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                        |  68 +++++++++++++-
 net/core/devmem.h                        |   5 ++
 net/core/netdev-genl.c                   |   1 +
 tools/testing/selftests/net/nl_netdev.py | 108 ++++++++++++++++++++++-
 6 files changed, 192 insertions(+), 12 deletions(-)


base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH net v1 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().

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>
---
 net/core/devmem.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index f4d60654ce7fd..5e4070b82df44 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -58,6 +58,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 +89,10 @@ 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(&binding->lock);
+	net_devmem_dmabuf_binding_unmap(binding);
+	mutex_unlock(&binding->lock);
+
 	dma_buf_put(binding->dmabuf);
 	xa_destroy(&binding->bound_rxqs);
 	percpu_ref_exit(&binding->ref);
@@ -535,6 +551,7 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv,
 				mutex_lock(&binding->lock);
 				ASSERT_EXCLUSIVE_WRITER(binding->dev);
 				WRITE_ONCE(binding->dev, NULL);
+				net_devmem_dmabuf_binding_unmap(binding);
 				mutex_unlock(&binding->lock);
 			}
 			break;
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH net v1 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

Add net_devmem_dev_unregister() called on NETDEV_UNREGISTER:
- When binding->dev (the physical DMA-capable device) unregisters,
  erase the binding ID, 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>
---
 net/core/devmem.c      | 45 ++++++++++++++++++++++++++++++++++++++++++
 net/core/devmem.h      |  5 +++++
 net/core/netdev-genl.c |  1 +
 3 files changed, 51 insertions(+)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index 5e4070b82df44..da7b3e8abaa13 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -169,6 +169,51 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
 	percpu_ref_kill(&binding->ref);
 }
 
+void net_devmem_dev_unregister(struct net_device *dev)
+{
+	struct net_devmem_dmabuf_binding *binding;
+	unsigned long xa_idx;
+
+	rcu_read_lock();
+	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
+		if (!percpu_ref_tryget(&binding->ref))
+			continue;
+		rcu_read_unlock();
+
+		if (xa_empty(&binding->bound_rxqs)) {
+			if (READ_ONCE(binding->dev) == dev) {
+				xa_erase(&net_devmem_dmabuf_bindings,
+					 binding->id);
+
+				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();
+			}
+		}
+
+		net_devmem_dmabuf_binding_put(binding);
+		rcu_read_lock();
+	}
+	rcu_read_unlock();
+}
+
 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)
diff --git a/net/core/devmem.h b/net/core/devmem.h
index 4a293a7d1149c..b572013d602c7 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h
@@ -100,6 +100,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 +197,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

[PATCH net v1 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

[PATCH net v1 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 v1 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[Pavel Begunkov]

On 10/10/26 03:55, 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().
> 
> Fixes: f8350a4358fc ("net: page_pool: add a mp hook to unregister_netdevice*")

It looks more like 170aafe35cb98e0f3fbacb446ea86389fbce22ea
("netdev: support binding dma-buf to netdevice"). The one in the
tag didn't do anything interesting.

The fix looks good as well, might be a good idea to clear
net_iov's dma_addr as well.

Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>

-- 
Pavel Begunkov

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

[netdev-bot+sashiko]

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

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

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] Incomplete fix: a binding that has already been unbound (netlink
  socket closed) but is still alive because a TCP socket or page pool…
- [High] The patch unmaps and detaches the dma-buf in
  mp_dmabuf_devmem_uninstall() as soon as bound_rxqs becomes empty.
- [Low] The comment on struct net_devmem_dmabuf_binding::lock in
  net/core/devmem.h still reads '/* Protect dev */'.
- [Low] The comment on struct net_devmem_dmabuf_binding::ref in
  net/core/devmem.h still says 'The binding undos itself and unmaps the…

--

net: devmem: unmap dma_buf synchronously on queue uninstall

This patch adds net_devmem_dmabuf_binding_unmap(), which runs under
binding->lock and clears binding->sgt and binding->attachment so that
unmapping is idempotent. It is called from mp_dmabuf_devmem_uninstall()
when the last bound RX queue is uninstalled, so the dma_buf is no longer
unmapped against a freed dma_dev after device unregister.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index f4d60654ce7fd..5e4070b82df44 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -58,6 +58,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);

[Severity: Low]
This isn't a bug, but the comment on binding->lock in net/core/devmem.h
still reads:

	/* Protect dev */
	struct mutex lock;

The lock now also protects binding->sgt and binding->attachment, and the
lockdep_assert_held() here enforces that. Should the comment in the header
be updated as well?

> +
> +	if (!binding->sgt)
> +		return;

[ ... ]

> @@ -74,9 +89,10 @@ 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(&binding->lock);
> +	net_devmem_dmabuf_binding_unmap(binding);
> +	mutex_unlock(&binding->lock);

[Severity: High]
Can this deferred unmap still run against a freed dma_dev when the binding
was already unbound before the device went away?

The commit message says the fix covers unregister "while userspace still
holds a netlink or TCP socket reference". However, the only synchronous
unmap is in mp_dmabuf_devmem_uninstall(), and dev_memory_provider_uninstall()
only reaches that for RX queues whose rxq->mp_params is still set.

Closing the netlink socket does this, with no unmap:

netdev_nl_sock_priv_destroy()
  net_devmem_unbind_dmabuf()
    xa_erase(&net_devmem_dmabuf_bindings, binding->id);
    netif_mp_close_rxq()
      __netif_mp_close_rxq()
        memset(&rxq->mp_params, 0, sizeof(rxq->mp_params));
    percpu_ref_kill(&binding->ref);

Other references can keep the binding alive past that point. On RX,
unreturned devmem tokens keep the old page pool alive. On TX, unacked
devmem skbs in a TCP retransmit queue hold refs taken via
net_devmem_get_net_iov(). The TX binding can be created through
netdev_nl_bind_tx_doit(), which does not check for CAP_NET_ADMIN.

A later unregister then finds no installed provider, so
mp_dmabuf_devmem_uninstall() is never called for this binding. After the
parent device is freed (for example in nsim_bus_dev_release()), dropping
the last reference goes through:

__net_devmem_dmabuf_binding_free()
  net_devmem_dmabuf_binding_unmap()
    dma_buf_unmap_attachment_unlocked()
      unmap_udmabuf()
        dma_unmap_sg_attrs()    <-- freed attach->dev

That looks like the same KASAN report quoted in the commit message.

The follow-up commit "net: devmem: detach TX bindings on NETDEV_UNREGISTER"
fixes the TX case while the netlink socket is still open. Its
net_devmem_dev_unregister() only walks net_devmem_dmabuf_bindings, though,
and unbound bindings are no longer in that xarray.

A narrower variant needs no extra references. The unbind_w work queued by
net_devmem_dmabuf_binding_release() runs asynchronously, and nothing makes
it run before device removal.

Would a complete fix need to track every binding that is still mapped
against a device, and not only the ones installed on an RX queue?

> +
>  	dma_buf_put(binding->dmabuf);
>  	xa_destroy(&binding->bound_rxqs);
>  	percpu_ref_exit(&binding->ref);
> @@ -535,6 +551,7 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv,
>  				mutex_lock(&binding->lock);
>  				ASSERT_EXCLUSIVE_WRITER(binding->dev);
>  				WRITE_ONCE(binding->dev, NULL);
> +				net_devmem_dmabuf_binding_unmap(binding);

[Severity: High]
Is it safe to unmap here just because bound_rxqs became empty? Nothing
here checks whether the hardware queue has actually stopped using the
binding.

On queue lease teardown the physical NIC is still registered and running:

netkit_queue_unlease()
  netdev_rx_queue_unlease()
    netif_rxq_cleanup_unlease()
      __netif_mp_close_rxq(phys_rxq->dev, rxq_idx, &p);
      __netif_mp_uninstall_rxq(virt_rxq, &p);
        mp_dmabuf_devmem_uninstall()
          net_devmem_dmabuf_binding_unmap()

If the reconfig fails, __netif_mp_close_rxq() only warns:

	err = netdev_rx_queue_reconfig(dev, rxq_idx, &qcfg[0], &qcfg[1]);
	WARN_ON(err && err != -ENETDOWN);

netdev_rx_queue_reconfig() can fail in ndo_queue_mem_alloc() before
ndo_queue_stop() is called. It can also fail in stop or start, with the
old queue restored. In both cases the old devmem page pool keeps running,
and its ring holds DMA addresses from binding->sgt.

netif_rxq_cleanup_unlease() still calls uninstall unconditionally. The
virtual rxq is the one recorded in bound_rxqs, so removing it empties the
xarray. The dma-buf is then unmapped and detached while the NIC can still
DMA into those IOVAs, and the pool can keep refilling from
binding->chunk_pool.

Before this patch the mapping lasted until __net_devmem_dmabuf_binding_free()
ran, which only happens after every page pool has dropped its binding ref.
The commit message only talks about unregistration. Was the lease
teardown path also meant to unmap synchronously here?

[Severity: Low]
This isn't a bug, but the comment on the ref field of struct
net_devmem_dmabuf_binding in net/core/devmem.h still says:

	 * 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.

With this call, the mapping can go away while netlink, page pool or
net_iov refs are still held. The follow-up commit "net: devmem: detach TX
bindings on NETDEV_UNREGISTER" adds another early unmap in
net_devmem_dev_unregister(). Should this comment be updated to match?

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

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 2 · 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).

- [High] net_devmem_dev_unregister() can miss a TX binding created during
  unregistration, which leaves binding->dev dangling.
- [High] If the netlink socket closes before the device is removed, a TX
  binding can keep its DMA mapping past device removal, and the later…
- [Medium] net_devmem_dev_unregister() treats every NETDEV_UNREGISTER as
  device removal, but __dev_change_net_namespace() sends…
- [Medium] net_devmem_dev_unregister() erases binding->id from
  net_devmem_dmabuf_bindings early (devmem.c:185) but leaves the binding…
- [Low] net_devmem_dev_unregister() calls synchronize_net() once per
  matching binding inside the xa_for_each loop, in both the dev and vdev…

--

net: devmem: detach TX bindings on NETDEV_UNREGISTER

This adds net_devmem_dev_unregister(), which the netdev-genl
NETDEV_UNREGISTER notifier calls to detach TX devmem bindings from
devices that are going away.

When the physical device unregisters, the binding ID is erased, dev and
vdev are cleared, and the dma_buf is unmapped after synchronize_net().
When only the virtual device unregisters, only vdev is cleared.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 5e4070b82df44..da7b3e8abaa13 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -169,6 +169,51 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  	percpu_ref_kill(&binding->ref);
>  }
>  
> +void net_devmem_dev_unregister(struct net_device *dev)
> +{
> +	struct net_devmem_dmabuf_binding *binding;
> +	unsigned long xa_idx;
> +
> +	rcu_read_lock();
> +	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {

[Severity: High]
Can this walk miss TX bindings that are still mapped but are no longer
in net_devmem_dmabuf_bindings?

If the netlink socket closes before the device is removed,
netdev_nl_sock_priv_destroy()->net_devmem_unbind_dmabuf() does:

    xa_erase(&net_devmem_dmabuf_bindings, binding->id);
    ...
    percpu_ref_kill(&binding->ref);

TX skbs still waiting for retransmit hold binding references through
net_devmem_get_net_iov(). This includes skbs on orphaned TCP sockets
after the process exits. So the binding and its dma_buf attachment stay
alive.

When the physical device is unregistered after that, this loop only
looks at net_devmem_dmabuf_bindings and never sees that binding, so it
is not unmapped. When the last skb reference drops, the deferred free
does:

__net_devmem_dmabuf_binding_free() {
    ...
    mutex_lock(&binding->lock);
    net_devmem_dmabuf_binding_unmap(binding);
    mutex_unlock(&binding->lock);
    ...
}

That calls dma_buf_unmap_attachment_unlocked() and dma_buf_detach() on
an attachment whose ->dev belonged to the freed device.
dma_buf_dynamic_attach() stores attach->dev = dev without taking a
reference on it.

Isn't this the same dma_unmap_sg_attrs() use-after-free described in
"net: devmem: unmap dma_buf synchronously on queue uninstall"?

The commit message says the dma_buf no longer "remains attached and
mapped after device removal", but on this path it still looks mapped.
Does the tracking need to cover every binding that is still mapped,
rather than only the ones in the lookup table?

> +		if (!percpu_ref_tryget(&binding->ref))
> +			continue;
> +		rcu_read_unlock();
> +
> +		if (xa_empty(&binding->bound_rxqs)) {
> +			if (READ_ONCE(binding->dev) == dev) {
> +				xa_erase(&net_devmem_dmabuf_bindings,
> +					 binding->id);

[Severity: Medium]
Can this ID end up being erased twice?

The binding stays on the owning socket's priv->bindings list, and
binding->id does not change. When that socket closes later,
netdev_nl_sock_priv_destroy() sees binding->dev == NULL:

    if (!dev) {
        mutex_unlock(&binding->lock);
        net_devmem_unbind_dmabuf(binding);
        continue;
    }

net_devmem_unbind_dmabuf() then calls
xa_erase(&net_devmem_dmabuf_bindings, binding->id) again. It does not
check that the slot still holds this binding.

net_devmem_bind_dmabuf() allocates IDs with xa_alloc_cyclic() over
xa_limit_32b. Once the allocator wraps, a new binding B could get the
same ID while the old socket is still open. Closing the old socket would
then remove B from the global table.

After that, B's TX lookups would fail. A later net_devmem_dev_unregister()
scan would also miss B, so B->dev could dangle after B's device is
removed. It would then be passed to netdev_hold() and netdev_lock() when
B's socket closes.

This also conflicts with the comment in devmem.h:

    /* ID of this binding. Globally unique to all bindings currently
     * active.
     */

Would an xa_cmpxchg() against this binding, or erasing the ID only once,
avoid this?

> +
> +				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: Low]
How long can RTNL end up held here? synchronize_net() runs once per
matching binding, here and again in the vdev branch below.
NETDEV_UNREGISTER notifiers run under RTNL, so each call turns into
synchronize_rcu_expedited().

NETDEV_CMD_BIND_TX is unprivileged, and the same dmabuf can be bound
many times. A user with many TX bindings on a device therefore adds one
expedited grace period per binding to that device's unregistration, all
while RTNL is held.

Could the loop erase and clear all matching bindings first, call
synchronize_net() once, and then unmap them?

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

[Severity: High]
If a bind-tx races with the physical device's unregistration, can the
binding reach this branch with binding->dev still dangling?

net_devmem_dev_unregister() walks the table once, and nothing
serializes that walk against netdev_nl_bind_tx_doit(). Take a netkit
device V with a queue leased from physical device P.
netdev_find_netmem_tx_dev() picks P with:

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

netif_device_present() stays true during unregistration.
netdev_nl_bind_tx_doit() then does:

    if (bind_dev != netdev)
        netdev_lock(bind_dev);
    ...
    binding = net_devmem_bind_dmabuf(bind_dev, ...

It never checks bind_dev->reg_state.

During P's unregistration, the NETDEV_UNREGISTER notifiers run without
P's instance lock. netdev-genl registers its notifier from
subsys_initcall and netkit registers its notifier from module_init, so
the sequence looks like this:

unregister_netdevice_many_notify(P)
  call_netdevice_notifiers(NETDEV_UNREGISTER, P)
    netdev_genl_netdevice_event()
      net_devmem_dev_unregister(P)   <- walk finishes
                                       bind-tx on V publishes a
                                       binding with dev = P
    netkit_check_lease_unregister(P)
      unregisters V
        net_devmem_dev_unregister(V) <- takes this vdev branch and
                                        only clears vdev

netkit_uninit()->netkit_queue_unlease() then drops the lease's hold on
P, so P can be freed while binding->dev still points at it.

When the netlink socket closes, netdev_nl_sock_priv_destroy() calls
netdev_hold() and netdev_lock() on the freed net_device. That is the
same use-after-free shown in the commit message. The dma_buf mapping on
P's DMA device is also never unmapped.

Does netdev_nl_bind_tx_doit() need a reg_state check under
netdev_lock(bind_dev), or some other way to serialize bind against this
detach?

> +
> +				synchronize_net();
> +			}
> +		}
> +
> +		net_devmem_dmabuf_binding_put(binding);
> +		rcu_read_lock();
> +	}
> +	rcu_read_unlock();
> +}

[ ... ]

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

[Severity: Medium]
Does this also tear down TX bindings when a device is only moving to
another netns?

__dev_change_net_namespace() sends NETDEV_UNREGISTER for a device that
stays alive:

    /* Notify protocols, that we are about to destroy
     * this device. They should clean all the things.
     *
     * 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);

net_devmem_dev_unregister() does not check reg_state. If the physical
device moves, the binding ID is erased, dev and vdev are cleared, and
the dma_buf is unmapped, all while the device still exists.

If only the netkit vdev moves into a container, binding->vdev is cleared
and sends through the vdev fail with -ENODEV. Userspace is not told, and
the binding stays on priv->bindings, so every later sendmsg with that
dmabuf ID fails.

netkit_check_lease_unregister(), which runs in the same notifier flow,
does check this:

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

A netns move leaves RX bindings alone, because
dev_memory_provider_uninstall() only runs from
unregister_netdevice_many_notify().

Should this path skip devices whose reg_state is not
NETREG_UNREGISTERING, so that TX and RX bindings behave the same way?

>  		netdev_lock(netdev);
>  		netdev_genl_dev_notify(netdev, NETDEV_CMD_DEV_DEL_NTF);
>  		netdev_unlock(netdev);

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · 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).

- [Medium] This patch adds PP_FLAG_ALLOW_UNREADABLE_NETMEM and .queue_idx
  to nsim_create_page_pool().
- [Medium] nsim_bus_dev_new() ignores the return value of
  dma_coerce_mask_and_coherent(&nsim_bus_dev->dev, DMA_BIT_MASK(64)) and…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  nsim_pp_hold_write() holds only rtnl_lock() while it loads ns->rq[0],…

--

netdevsim: support devmem RX and TX bindings

This patch sets a 64-bit DMA mask on the netdevsim bus device and enables
NETMEM_TX_DMA on the netdevsim net_device. It also creates the RX page_pools
with DMA mapping, unreadable netmem and queue_idx set, 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]
What happens here on CONFIG_HAS_DMA=n builds?

NETDEVSIM does not depend on HAS_DMA. Some configs select NO_DMA, for
example UML without UML_DMA_EMULATION. On those builds the dma_set_mask()
stub returns -EIO. The return value is ignored here, so the device
registers and no error is reported.

The PP_FLAG_DMA_MAP change in nsim_create_page_pool() below means every
slow-path page_pool allocation would then fail:

page_pool_alloc_netmems()
  ...
    page_pool_dma_map()
      dma_map_page_attrs()   <- stub returns DMA_MAPPING_ERROR
      if (dma_mapping_error(pool->p.dev, dma))
          return false;

After that, nsim_pp_hold_write() returns -ENOMEM:

	ns->page = page_pool_dev_alloc_pages(ns->rq[0]->page_pool);
	if (!ns->page)
		ret = -ENOMEM;

Would the existing page_pool selftests that use pp_hold now fail on these
builds? They passed there before this patch.

Should the error be checked here? Alternatively, should the DMA flags only
be set when the mask setup succeeds?

>  	/* 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,
>  	};

[Severity: Medium]
With PP_FLAG_ALLOW_UNREADABLE_NETMEM and queue_idx set, page_pool_init()
now copies rxq->mp_params.mp_ops and mp_priv into the pool. After a devmem
(or io_uring zcrx) bind-rx on queue 0, ns->rq[0]->page_pool is therefore
backed by a memory provider.

nsim_pp_hold_write() still allocates from that pool with the page-only API:

	ns->page = page_pool_dev_alloc_pages(ns->rq[0]->page_pool);

What happens to the net_iov in that case? The call chain looks like:

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, netmem);
          pool->pages_state_hold_cnt++;
      netmem_to_page()
        if (WARN_ON_ONCE(netmem_is_net_iov(netmem)))
            return NULL;

The WARN fires, and it is fatal with panic_on_warn. ns->page becomes NULL
and the write returns -ENOMEM. The net_iov is already counted in
pages_state_hold_cnt, but nothing holds a pointer to it anymore.

Is this net_iov leaked for good? If so, the pool's inflight count would
never reach zero. page_pool_destroy() at unbind or queue restart would then
keep deferring the release, and the provider's destroy hook would never
run. That would also leak the pool, its dmabuf binding reference and the
binding's genpool chunk.

Should nsim_pp_hold_write() use the netmem allocation API instead? Or
should it refuse to allocate when a memory provider is bound to the queue?

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

[Severity: High]
This is a pre-existing issue that this patch did not introduce. It seems
worth raising because bind-rx now gives netdevsim another way to restart
queues.

nsim_pp_hold_write() holds only rtnl_lock() while it loads ns->rq[0],
dereferences its page_pool and allocates from it. The RX queue restart
path replaces and frees those objects while holding only the netdev instance
lock:

netdev_rx_queue_restart()
  nsim_queue_mem_alloc()
  nsim_queue_stop()
  nsim_queue_start()
    ns->rq[idx] = qmem->rq;               (modes 0/2/3)
    ns->rq[idx]->page_pool = qmem->pp;    (mode 1)
  nsim_queue_mem_free()
    page_pool_destroy(qmem->rq->page_pool);
    nsim_queue_free(dev, qmem->rq);       <- kfree()s the old rq

nsim_qreset_write() takes only netdev_lock(), and so do devmem bind-rx and
unbind. None of them takes rtnl, so these paths share no lock with
nsim_pp_hold_write().

Can this interleaving lead to a use-after-free?

CPU0                                CPU1
nsim_pp_hold_write()
  rtnl_lock()
  loads old ns->rq[0]
                                    restart of queue 0
                                      ns->rq[0] = new rq
                                      page_pool_destroy(old pool)
                                      kfree(old rq)
  old rq->page_pool
  page_pool_dev_alloc_pages()

Should nsim_pp_hold_write() also take the netdev instance lock?

>  	if (err)
>  		goto err_free;
>

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

Re: [PATCH net v1 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: 5 · Low: 2

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] In devmem_bind_rx_unregister_check() and
  devmem_bind_tx_unregister_check()…
- [Medium] The new tests need /dev/udmabuf (CONFIG_UDMABUF) and debugfs
  (CONFIG_DEBUG_FS, for the bufinfo assertions).
- [Medium] _dmabuf_attached_devs() (nl_netdev.py:390-408) collects the
  'Attached Devices' entries of every dma_buf in the system, not only…
- [Medium] devmem_bind_rx_unregister_check() (lines 413-439) and
  devmem_bind_tx_unregister_check() (lines 444-466) do not use `with…
- [Medium] _create_udmabuf() (nl_netdev.py:384) passes the hardcoded
  number 0x40187542 as UDMABUF_CREATE.
- [Low] In both new tests, the only check for the behaviour the
  docstrings claim ('Verify RX devmem bindings detach dma_buf…
- [Low] _create_udmabuf() (nl_netdev.py:379-387) closes memfd and devfd
  only on the success path.

--

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

This adds two tests to nl_netdev.py. Each one creates a udmabuf, binds it
to a netdevsim device for devmem RX or TX through the netdev netlink
family, and then removes the netdevsim device. The tests check debugfs
dma_buf/bufinfo to confirm that the attachment is gone, and expect the
netlink socket to close cleanly afterwards.

> 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 in CI that builds from the net config fragment?

The only related entry in tools/testing/selftests/net/config is:

CONFIG_NETDEVSIM=m

It enables neither CONFIG_UDMABUF nor CONFIG_DEBUG_FS. So
_create_udmabuf() would always raise KsftSkipEx here, and both new tests
would always skip.

tools/testing/selftests/drivers/net/hw/config and drivers/dma-buf/config
both set CONFIG_UDMABUF=y. Should the net config fragment add
CONFIG_UDMABUF=y and CONFIG_DEBUG_FS=y too?

> +
> +    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 the right request number on every architecture?

The uapi header defines it as:

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

0x40187542 is the asm-generic encoding. Alpha, mips, powerpc and sparc
define _IOC_WRITE as 4U with 13 size bits, and parisc defines it as 2U.
On all of these the write bit lands at 0x80000000, so the request number
becomes 0x80187542.

On those architectures udmabuf_ioctl() hits its default case:

	default:
		ret = -ENOTTY;

Nothing catches the resulting OSError from fcntl.ioctl(), so both devmem
tests would fail instead of running or skipping.

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

[Severity: Low]
Does this leak memfd and devfd on the error paths?

If os.ftruncate(), the F_ADD_SEALS fcntl() or os.open() raises, memfd is
never closed. If the UDMABUF_CREATE ioctl raises (the ENOTTY case above,
for example), both memfd and devfd stay open. ksft_run() catches the
exception and keeps the process running.

A try/finally around these calls would close both.

> +
> +
> +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: Low]
What happens when bufinfo is missing (CONFIG_DEBUG_FS off, or debugfs not
mounted)?

Both tests guard every assertion with:

    if attached is not None:

With None they run no assertions at all. ksft_run() then records them as
pass, not skip.

The tests still exercise nsimdev.remove(), but they would pass even if
the dma_buf stayed attached after unregister. 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 in the system, rather than
only the udmabuf this test created?

dma_buf_debug_show() walks the whole dmabuf_list and prints one block for
each buffer:

drivers/dma-buf/dma-buf.c:dma_buf_debug_show() {
    ...
		seq_puts(s, "\tAttached Devices:\n");
		attach_count = 0;

		list_for_each_entry(attach_obj, &buf_obj->attachments, node) {
			seq_printf(s, "\t%s\n", dev_name(attach_obj->dev));
    ...
}

The parser collects entries after every header, so the result is a total
for the whole host.

This has two effects:

- ksft_eq(len(attached), 0) after nsimdev.remove() fails whenever any
  other dma_buf (GPU, display, V4L2) has an attachment, even if the
  kernel detached this one correctly.
- ksft_ge(len(attached), 1) passes even if this binding never attached.

An attachment left behind by the RX test would also make the TX test
fail.

Could this filter on the inode of dmabuf_fd (os.fstat(dmabuf_fd).st_ino)
or on the netdevsim device name?

> +
> +
> +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]
What happens to the netdevsim device if anything between here and
nsimdev.remove() raises?

The ip link set call, ef.rings_set(), nf_priv.bind_rx() or ksft_ge()
could all raise. ksft_run() catches the exception and moves on, but
nsimdev.remove() is never reached.

NetdevSimDev only cleans up through remove() or __exit__ and has no
__del__. So the device stays in the kernel after the selftest exits.
dmabuf_fd stays open, and if the bind succeeded, the dma_buf attachment
stays live too.

The other netdevsim tests in this file use "with NetdevSimDev(...)" or
defer(). Could these do the same? devmem_bind_tx_unregister_check() has
the same pattern.

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

[Severity: Medium]
Does del nf_priv actually close the netlink socket that holds the
binding?

YnlFamily.__init__() stores, on self, a partial for every op that refers
back to self:

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

That is a reference cycle, so refcounting never frees the object. del
only drops the local name, and self.sock stays open until the cyclic GC
happens to run. That could be during the next test, or at process exit
after ksft_exit() has already printed the results.

So the step the commit message describes, closing the socket after
unregister and reaching netdev_nl_sock_priv_destroy(), does not happen
at a fixed point inside this test. A failure on that path could be
blamed on another test, or show up after this one already reported ok.

YnlFamily already provides close() and __enter__/__exit__. Would
nf_priv.close(), or "with NetdevFamily() as nf_priv:", be better here?
The same applies to devmem_bind_tx_unregister_check().

[ ... ]

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