[PATCH net] net: devmem: allow peer-to-peer dma-buf attach for bindings

[PATCH net] net: devmem: allow peer-to-peer dma-buf attach for bindings

[Erfan Sharafzadeh]

net_devmem_bind_dmabuf() uses dma_buf_attach(), a static attachment:

- peer2peer is false, so exporters must keep the buffer reachable
  without P2P. amdgpu migrates a VRAM buffer to GTT (system memory)
  for that, so device memory bindings silently land in host memory.
- the core pins the buffer on map (dma_buf_pin_on_map()), so it never
  moves while bound.

Switch to dma_buf_dynamic_attach() with allow_peer2peer set, so the
exporter can keep the buffer in device memory. Dynamic attachments are
not pinned on map, and devmem cannot handle the buffer moving, so:

- pin it explicitly with dma_buf_pin() before mapping, which keeps it
  non-movable for the lifetime of the binding, as before;
- wait for the exporter's kernel fences after mapping, which the core
  only does for static attachments.

This matches the pinned RDMA umem path. Exporters that cannot do P2P
to the device fall back as before. RX and TX bindings both go through
this function.

Fixes: 170aafe35cb9 ("netdev: support binding dma-buf to netdevice")
Signed-off-by: Erfan Sharafzadeh <erfansh@meta.com>
---
 net/core/devmem.c | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index f4d60654ce7f..3721a47426fa 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/dma-buf.h>
+#include <linux/dma-resv.h>
 #include <linux/genalloc.h>
 #include <linux/mm.h>
 #include <linux/netdevice.h>
@@ -49,6 +50,10 @@ static dma_addr_t net_devmem_get_dma_addr(const struct net_iov *niov)
 	       ((dma_addr_t)net_iov_idx(niov) << owner->binding->niov_shift);
 }
 
+static const struct dma_buf_attach_ops net_devmem_dmabuf_attach_ops = {
+	.allow_peer2peer = true,
+};
+
 static void net_devmem_dmabuf_binding_release(struct percpu_ref *ref)
 {
 	struct net_devmem_dmabuf_binding *binding =
@@ -74,8 +79,11 @@ 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_resv_lock(binding->dmabuf->resv, NULL);
+	dma_buf_unmap_attachment(binding->attachment, binding->sgt,
+				 binding->direction);
+	dma_buf_unpin(binding->attachment);
+	dma_resv_unlock(binding->dmabuf->resv);
 	dma_buf_detach(binding->dmabuf, binding->attachment);
 	dma_buf_put(binding->dmabuf);
 	xa_destroy(&binding->bound_rxqs);
@@ -200,7 +208,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 	struct dma_buf *dmabuf;
 	unsigned int sg_idx, i;
 	unsigned long virtual;
-	int err;
+	long err;
 
 	if (!dma_dev) {
 		NL_SET_ERR_MSG(extack, "Device doesn't support DMA");
@@ -234,19 +242,35 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 	binding->dmabuf = dmabuf;
 	binding->direction = direction;
 
-	binding->attachment = dma_buf_attach(binding->dmabuf, dma_dev);
+	binding->attachment =
+		dma_buf_dynamic_attach(binding->dmabuf, dma_dev,
+				       &net_devmem_dmabuf_attach_ops, binding);
 	if (IS_ERR(binding->attachment)) {
 		err = PTR_ERR(binding->attachment);
 		NL_SET_ERR_MSG(extack, "Failed to bind dmabuf to device");
 		goto err_exit_ref;
 	}
 
-	binding->sgt = dma_buf_map_attachment_unlocked(binding->attachment,
-						       direction);
+	dma_resv_lock(dmabuf->resv, NULL);
+	err = dma_buf_pin(binding->attachment);
+	if (err) {
+		NL_SET_ERR_MSG(extack, "Failed to pin dmabuf");
+		goto err_unlock;
+	}
+
+	binding->sgt = dma_buf_map_attachment(binding->attachment, direction);
 	if (IS_ERR(binding->sgt)) {
 		err = PTR_ERR(binding->sgt);
 		NL_SET_ERR_MSG(extack, "Failed to map dmabuf attachment");
-		goto err_detach;
+		goto err_unpin;
+	}
+	dma_resv_unlock(dmabuf->resv);
+
+	err = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, false,
+				    MAX_SCHEDULE_TIMEOUT);
+	if (err < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to wait for dmabuf fences");
+		goto err_unmap;
 	}
 
 	if (direction == DMA_TO_DEVICE) {
@@ -343,9 +367,12 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 err_tx_vec:
 	kvfree(binding->tx_vec);
 err_unmap:
-	dma_buf_unmap_attachment_unlocked(binding->attachment, binding->sgt,
-					  direction);
-err_detach:
+	dma_resv_lock(dmabuf->resv, NULL);
+	dma_buf_unmap_attachment(binding->attachment, binding->sgt, direction);
+err_unpin:
+	dma_buf_unpin(binding->attachment);
+err_unlock:
+	dma_resv_unlock(dmabuf->resv);
 	dma_buf_detach(dmabuf, binding->attachment);
 err_exit_ref:
 	percpu_ref_exit(&binding->ref);
-- 
2.53.0-Meta

Re: [PATCH net] net: devmem: allow peer-to-peer dma-buf attach for bindings

[netdev-bot+sashiko]

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

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

- [Medium] The commit carries 'Fixes: 170aafe35cb9 ("netdev: support
  binding dma-buf to netdevice")', but it is an enhancement, not a bug…
- [Medium] The commit message says exporters that cannot do P2P 'fall
  back as before'.
- [Medium] net_devmem_bind_dmabuf() now waits with
  `dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, false,…
- [Medium] net_devmem_bind_dmabuf() now calls dma_buf_pin() for every
  netlink bind-rx/bind-tx request on a userspace-supplied dmabuf fd, and…
- [Medium] The patch makes devmem a peer-to-peer importer, so a binding's
  sg_table can now hold PCI BAR bus addresses (exporter VRAM aperture or…

Pre-existing issues:
- [Low] io_uring zcrx's io_import_dmabuf() still uses a static
  `dma_buf_attach(mem->dmabuf, ifq->dev)` plus…

--

net: devmem: allow peer-to-peer dma-buf attach for bindings

This commit switches net_devmem_bind_dmabuf() from a static
dma_buf_attach() to dma_buf_dynamic_attach() with allow_peer2peer set.
The goal is to let exporters such as amdgpu keep the buffer in device
memory. Because the core does neither for dynamic attachments, the buffer
is now pinned explicitly with dma_buf_pin() and the exporter's kernel
fences are waited on after mapping.

> Fixes: 170aafe35cb9 ("netdev: support binding dma-buf to netdevice")

[Severity: Medium]
Is this a fix for 170aafe35cb9? The old static dma_buf_attach() path looks
correct under the dma-buf contract. amdgpu_dma_buf_pin() drops VRAM when
any attachment has peer2peer == false:

drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c:amdgpu_dma_buf_pin() {
    ...
	list_for_each_entry(attach, &dmabuf->attachments, node)
		if (!attach->peer2peer)
			domains &= ~AMDGPU_GEM_DOMAIN_VRAM;
    ...
}

So the BO was pinned in GTT and payloads were delivered correctly, just
into host memory. This patch seems to add a new capability (P2P DMA into
exporter VRAM) rather than fix a bug.

The Fixes: tag also makes this a stable candidate. However, the patch
depends on recent dma-buf core changes. Commit 575157b1b5fc ("dma-buf:
Make .invalidate_mapping() truly optional") removed this check from
dma_buf_dynamic_attach():

	if (WARN_ON(importer_ops && !importer_ops->invalidate_mappings))
		return ERR_PTR(-EINVAL);

In older trees the callback is named move_notify. The patch also relies on
commit 609fc8766d68, which made dma_buf_pin_on_map() skip all dynamic
attachments.

net_devmem_dmabuf_attach_ops only sets .allow_peer2peer. On v6.12+ stable
trees, wouldn't a backport hit that WARN_ON? Every bind-rx and bind-tx
would then fail with -EINVAL at goto err_exit_ref. Even where the patch
applies cleanly, it adds new VRAM pinning and P2P DMA behaviour.

Should the Fixes: tag be dropped?

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

[ ... ]

> @@ -49,6 +50,10 @@ static dma_addr_t net_devmem_get_dma_addr(const struct net_iov *niov)
>  	       ((dma_addr_t)net_iov_idx(niov) << owner->binding->niov_shift);
>  }
>  
> +static const struct dma_buf_attach_ops net_devmem_dmabuf_attach_ops = {
> +	.allow_peer2peer = true,
> +};

[Severity: Medium]
With allow_peer2peer set, the binding's sg_table can now hold PCI BAR bus
addresses. There is no .invalidate_mappings callback here, and
dma_buf_invalidate_mappings() skips such attachments:

drivers/dma-buf/dma-buf.c:dma_buf_invalidate_mappings() {
    ...
	list_for_each_entry(attach, &dmabuf->attachments, node)
		if (attach->importer_ops &&
		    attach->importer_ops->invalidate_mappings)
			attach->importer_ops->invalidate_mappings(attach);
    ...
}

What happens if the exporter loses or reassigns the BAR window while a
binding is live, for example on GPU reset or hot-unplug? Bound RX queues
would keep posting DMA writes to a range the exporter may no longer
decode, and TX would keep issuing reads from it. This could show up as
AER errors on the NIC. After a reassignment, NIC writes could land in
another device's MMIO.

The binding is released only after all page_pool and in-flight skb
references drop, and the exporter has no control over that. Before this
patch, the binding was always backed by GTT system pages, which stay
valid.

I could not fully confirm how amdgpu handles reset and hot-unplug here.
The RDMA code that the commit message refers to does have a separate
revocable variant for this case:

drivers/infiniband/core/umem_dmabuf.c:
static const struct dma_buf_attach_ops ib_umem_dmabuf_attach_pinned_revocable_ops = {
	.allow_peer2peer = true,
	.invalidate_mappings = ib_umem_dmabuf_revoke_locked,
};

Exporters like VFIO can use dma_buf_attach_revocable() to reject
non-revocable importers, but amdgpu does not make that check.

Does devmem need a way to revoke a P2P binding?

[ ... ]

> @@ -234,19 +242,35 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  	binding->dmabuf = dmabuf;
>  	binding->direction = direction;
>  
> -	binding->attachment = dma_buf_attach(binding->dmabuf, dma_dev);
> +	binding->attachment =
> +		dma_buf_dynamic_attach(binding->dmabuf, dma_dev,
> +				       &net_devmem_dmabuf_attach_ops, binding);

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch.
io_uring zcrx's io_import_dmabuf() still uses the static attach path:

io_uring/zcrx.c:io_import_dmabuf() {
    ...
	mem->attach = dma_buf_attach(mem->dmabuf, ifq->dev);
    ...
	mem->sgt = dma_buf_map_attachment_unlocked(mem->attach, DMA_FROM_DEVICE);
    ...
}

So with amdgpu exporters, zcrx dma-buf areas still fall back to GTT. Is
that difference from devmem intended?

>  	if (IS_ERR(binding->attachment)) {
>  		err = PTR_ERR(binding->attachment);
>  		NL_SET_ERR_MSG(extack, "Failed to bind dmabuf to device");
>  		goto err_exit_ref;
>  	}
>  
> -	binding->sgt = dma_buf_map_attachment_unlocked(binding->attachment,
> -						       direction);
> +	dma_resv_lock(dmabuf->resv, NULL);
> +	err = dma_buf_pin(binding->attachment);
> +	if (err) {
> +		NL_SET_ERR_MSG(extack, "Failed to pin dmabuf");
> +		goto err_unlock;
> +	}

[Severity: Medium]
The commit message says:

  Exporters that cannot do P2P to the device fall back as before.

Does this still hold when the same dma-buf has more than one attachment?
With amdgpu, the first devmem binding over a P2P-capable path now pins the
BO in VRAM, because every attachment has peer2peer == true at that point.

A later attachment with peer2peer == false then asks for GTT only. One
case is a second devmem bind to a NIC with no P2P path, where
amdgpu_dma_buf_attach() clears peer2peer:

	if (!amdgpu_dmabuf_is_xgmi_accessible(attach_adev, bo) &&
	    pci_p2pdma_distance(adev->pdev, attach->dev, false) < 0)
		attach->peer2peer = false;

Static importers such as io_uring zcrx or v4l2 are another case.

amdgpu_bo_pin() then rejects the request, because the BO is already pinned
in VRAM:

drivers/gpu/drm/amd/amdgpu/amdgpu_object.c:amdgpu_bo_pin() {
    ...
	if (bo->tbo.pin_count) {
		uint32_t mem_type = bo->tbo.resource->mem_type;
		...
		if (!(domain & amdgpu_mem_type_to_domain(mem_type)))
			return -EINVAL;
    ...
}

The second devmem bind fails with "Failed to pin dmabuf", and static
importers fail in dma_buf_map_attachment(). Before this patch all devmem
pins went to GTT, so these combinations worked.

Is this regression expected?

[Severity: Medium]
The dma_buf_pin() kernel-doc says:

 * Only dynamic importers (who set up @attach with dma_buf_dynamic_attach()) may
 * call this, and only for limited use cases like scanout and not for temporary
 * pin operations. It is not permitted to allow userspace to pin arbitrary
 * amounts of buffers through this interface.

Here, every netlink bind-rx or bind-tx request pins a userspace-supplied
dma-buf for the lifetime of the binding. Devmem has no per-user or global
limit on bindings. NETDEV_CMD_BIND_TX is registered with only
GENL_CMD_CAP_DO, so no privilege is needed to reach it.

With allow_peer2peer, amdgpu_dma_buf_pin() keeps VRAM and also sets:

	if (domains & AMDGPU_GEM_DOMAIN_VRAM)
		bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;

So the pin lands in CPU-visible VRAM, which is small on systems without
resizable BAR.

Could an unprivileged user pin arbitrary amounts of non-evictable visible
VRAM this way and starve other GPU clients or display scanout? The pinned
RDMA path breaks the same rule, but this adds another unbounded caller.

> +
> +	binding->sgt = dma_buf_map_attachment(binding->attachment, direction);
>  	if (IS_ERR(binding->sgt)) {
>  		err = PTR_ERR(binding->sgt);
>  		NL_SET_ERR_MSG(extack, "Failed to map dmabuf attachment");
> -		goto err_detach;
> +		goto err_unpin;
> +	}
> +	dma_resv_unlock(dmabuf->resv);
> +
> +	err = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_KERNEL, false,
> +				    MAX_SCHEDULE_TIMEOUT);

[Severity: Medium]
The commit message presents this as the same wait the core does for static
attachments. The core version is interruptible, though:

drivers/dma-buf/dma-buf.c:dma_buf_map_attachment() {
    ...
	if (!dma_buf_attachment_is_dynamic(attach)) {
		ret = dma_resv_wait_timeout(attach->dmabuf->resv,
					    DMA_RESV_USAGE_KERNEL, true,
					    MAX_SCHEDULE_TIMEOUT);
    ...
}

Here intr is false and the timeout is MAX_SCHEDULE_TIMEOUT. A bind stuck
on a slow or stalled exporter kernel fence, such as a TTM move or clear on
a hung GPU, can't be aborted with SIGKILL.

The wait also runs while the bind paths hold their locks.
netdev_nl_bind_rx_doit() holds priv->lock and the netdev instance lock,
and netdev_nl_bind_tx_doit() also holds netdev_lock(bind_dev):

netdev_nl_bind_tx_doit()
    mutex_lock(&priv->lock)
    netdev_get_by_index_lock()
    netdev_lock(bind_dev)
    net_devmem_bind_dmabuf()
        dma_resv_wait_timeout(..., false, MAX_SCHEDULE_TIMEOUT)

BIND_TX is unprivileged. Should this pass intr=true to match the old
behaviour? As written, a signal can never reach the err < 0 branch below.

> +	if (err < 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to wait for dmabuf fences");
> +		goto err_unmap;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012170754.669462-1-erfansh%40meta.com

Re: [PATCH net] net: devmem: allow peer-to-peer dma-buf attach for bindings

[Erfan Sharafzadeh]

Replying to the Sashiko AI review of this patch. Three of the five
points are addressed in v2, which targets net-next:
https://lore.kernel.org/netdev/20261013210954.2170175-1-erfansh@meta.com/

1. Fixes: tag on an enhancement

Agreed. The static attach is correct under the dma-buf contract, and
the patch relies on 575157b1b5fc and 609fc8766d68, so it must not go to
stable. v2 drops the Fixes: tag and targets net-next.

2. "fall back as before"

Agreed, that was too broad. With an exporter that pins in a single
placement, a later attachment that cannot do P2P can fail to pin a
buffer that a P2P binding has already pinned in device memory, as with
pinned RDMA umem. v2 states this limit in the commit message.

3. Uninterruptible fence wait

Agreed. v2 waits interruptibly, as dma_buf_map_attachment() does for
static attachments.

4. dma_buf_pin() on every bind of a userspace-supplied dma-buf

Bindings were already pinned for their lifetime before this patch:
dma_buf_map_attachment() pins static attachments through
dma_buf_pin_on_map() and unpins them at unmap. The patch keeps the same
lifetime and only lets the exporter choose device memory. The amount is
bounded by what the user can allocate from the exporter in the first
place, and it is the same model as pinned RDMA umem. Whether BIND_TX
should require CAP_NET_ADMIN, as BIND_RX does, is a separate,
pre-existing question.

5. P2P bus addresses and no revocation

The binding is non-revocable, like ib_umem_dmabuf_attach_pinned_ops.
Devmem has no way to tear down a live binding today, since page_pool
and in-flight skbs hold references to it. Supporting revocable
bindings would be a separate change; v2 notes the limit in the commit
message.

pw-bot: cr