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

[netdev-bot+sashiko@kernel.org]

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