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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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] RX bindings made through a leased queue (a netkit virtual device
  leasing a physical NIC's rxq) still hit the dma_unmap_sg_attrs() use-…

Pre-existing issues:
- [Low] This patch handles only DMA_FROM_DEVICE bindings when a device is
  unregistered.

--

net: devmem: unmap dma_buf synchronously on queue uninstall

This commit unmaps the devmem dma_buf attachment when the last bound RX
queue is uninstalled. Previously the unmap waited for the deferred binding
free, which could run after dma_dev had already been freed. It also keeps
bindings in net_devmem_dmabuf_bindings until they are freed, hides unbound
ones from TX lookups with an xarray mark, and unmaps unbound RX bindings
on NETDEV_UNREGISTER, except for leased queues.

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

[ ... ]

> @@ -153,6 +176,38 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)

[ ... ]

> +void net_devmem_dev_unregister(struct net_device *dev)
> +{
> +	struct net_devmem_dmabuf_binding *binding;
> +	unsigned long xa_idx;
> +
> +	if (dev->reg_state != NETREG_UNREGISTERING)
> +		return;
> +
> +	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);

[Severity: Low]
This isn't a bug introduced by this patch, but TX bindings are skipped
here. They are never installed on an rxq either, so
mp_dmabuf_devmem_uninstall() never runs for them.

After the NIC is unregistered, a TX binding created through bind-tx still
has three things: its dma_buf mapping against the NIC's dma_dev, the
NET_DEVMEM_BINDING_LIVE mark, and stale binding->dev and binding->vdev
pointers.

Can a later netdev_nl_sock_priv_destroy() then call netdev_hold() and
netdev_lock() on the freed net_device? Could
__net_devmem_dmabuf_binding_free() also unmap against the freed dma_dev?
net_devmem_get_binding() would compare against the stale dev and vdev
pointers too.

The next patch in the series, "net: devmem: detach TX bindings on
NETDEV_UNREGISTER", appears to fix this. It adds
net_devmem_dev_unregister_tx(), which clears the LIVE mark and dev/vdev.
It also adds net_devmem_unmap_detached_tx(), which unmaps after
synchronize_net().

> +	}
> +	mutex_unlock(&net_devmem_lock);
> +}

[ ... ]

> @@ -529,16 +588,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]
Can RX bindings on a leased queue still hit the dma_unmap_sg_attrs()
use-after-free described in the commit message?

Take a bind-rx on a netkit rxq leased from a physical NIC.
netdev_nl_bind_rx_doit() gets dma_dev from netdev_queue_get_dma_dev(),
which returns the physical NIC's DMA device. But binding->dev is the
netkit device, and bound_rxqs holds the netkit rxq.

On unlease, the lease pointers are cleared only after cleanup has run:

netdev_rx_queue_unlease()
    netif_rxq_cleanup_unlease(rxq_src, rxq_dst)
        __netif_mp_uninstall_rxq(virt_rxq, &p)
            mp_dmabuf_devmem_uninstall()
                WRITE_ONCE(binding->dev, NULL);
                if (!rxq->lease)   <-- lease still set, unmap skipped
    WRITE_ONCE(rxq_src->lease, NULL);
    WRITE_ONCE(rxq_dst->lease, NULL);
    netdev_put(rxq_src->dev, &rxq_src->lease_tracker);

net_devmem_dev_unregister_rx() does the same for unbound bindings,
because netif_is_queue_leasee() is true for netkit:

	WRITE_ONCE(binding->dev, NULL);
	if (!netif_is_queue_leasee(dev))
		net_devmem_dmabuf_binding_unmap(binding);

Once binding->dev is NULL, it seems nothing can unmap the binding before
the final free. When the physical NIC unregisters later,
dev_memory_provider_uninstall() passes the physical rxq, which is not in
bound_rxqs. net_devmem_dev_unregister() matches only on binding->dev,
which is never the physical device.

For example: with the netlink socket open, delete the netkit device,
remove the physical NIC, and then close the socket:

net_devmem_unbind_dmabuf()
  percpu_ref_kill()
    __net_devmem_dmabuf_binding_free()
      net_devmem_dmabuf_binding_unmap()     <-- sgt still non-NULL
        dma_buf_unmap_attachment_unlocked() <-- freed physical dma_dev

The commit message gives this reason for skipping the unmap:

  "the physical device is still registered"

Is that still accurate when the physical NIC is unregistered first? In
that case netkit_check_lease_unregister() unregisters the netkit device
from inside the physical NIC's own NETDEV_UNREGISTER. The unlease skip
then happens while the physical device is already NETREG_UNREGISTERING.

Closing the socket first while RX tokens keep the binding alive seems to
end the same way. The netkit unregister clears binding->dev through
net_devmem_dev_unregister_rx() without unmapping. Once the physical NIC
is removed, the last token release unmaps against the freed dma_dev.

For leased queues, would the binding need to track the physical device,
or be unmapped against it?

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

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