Re: [PATCH net v5 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 (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).

- [High] The dma_dev use-after-free this patch says it fixes can still
  happen for RX bindings made on a virtual device that leases queues,…

Pre-existing issues:
- [Medium] At this commit, net_devmem_dev_unregister() skips TX
  (DMA_TO_DEVICE) bindings.

--

net: devmem: unmap dma_buf synchronously on queue uninstall

This commit unmaps RX devmem dma_buf bindings earlier: when the last
bound RX queue is uninstalled, when the binding is unbound, or on
NETDEV_UNREGISTER. Previously the unmap waited until the final binding
free. Bindings now stay in the global xarray until they are freed, with
an xarray mark hiding unbound ones from TX lookups, and
netif_mp_close_rxq() now reports queue restart failures. The aim is to
avoid a use-after-free of dma_dev in dma_unmap_sg_attrs() after the
device is removed.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 32fafbfa1359e..4610de805ef08 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -143,12 +228,45 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  
>  		rxq_idx = get_netdev_rx_queue_index(rxq);
>  
> -		netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params);
> +		if (netif_mp_close_rxq(binding->dev, rxq_idx, &mp_params))
> +			busy = true;
> +	}
> +
> +	if (binding->direction == DMA_FROM_DEVICE && binding->dev && !busy) {
> +		net_devmem_dmabuf_binding_unmap(binding);
> +		WRITE_ONCE(binding->dev, NULL);
>  	}
>  
>  	percpu_ref_kill(&binding->ref);
>  }
>  
> +void net_devmem_dev_unregister(struct net_device *dev)
> +{
> +	struct net_devmem_dmabuf_binding *binding;
> +	unsigned long xa_idx;
> +
> +	netdev_assert_locked(dev);
> +
> +	if (dev->reg_state != NETREG_UNREGISTERING)
> +		return;
> +
> +	xa_lock(&net_devmem_dmabuf_bindings);
> +	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
> +		if (binding->direction != DMA_FROM_DEVICE ||
> +		    READ_ONCE(binding->dev) != dev ||
> +		    xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +				NET_DEVMEM_BINDING_LIVE))
> +			continue;

[Severity: Medium]
This issue was not introduced by this patch, but at this commit TX
(DMA_TO_DEVICE) bindings are skipped here. Their binding->dev is never
cleared, and they stay mapped until __net_devmem_dmabuf_binding_free().

That has three consequences. The deferred dma_unmap_sg_attrs() can run
on a freed dma_dev. netdev_nl_sock_priv_destroy()->net_devmem_lock_dev()
can call netdev_hold() and netdev_lock() on a freed net_device. And the
invariant in the new net_devmem_lock_dev() kernel-doc does not hold for
TX bindings.

The old netdev_nl_sock_priv_destroy() already called netdev_hold() on a
stale TX binding->dev, so this predates the patch. The next patch in the
series, "net: devmem: detach TX bindings on NETDEV_UNREGISTER", appears
to fix it. That patch clears the LIVE mark, unmaps the binding and
clears dev and vdev for TX bindings with binding->dev == dev. It also
calls net_devmem_lock_dev() in __net_devmem_dmabuf_binding_free() for
both directions.

> +
> +		xa_unlock(&net_devmem_dmabuf_bindings);
> +		if (!netif_is_queue_leasee(dev))
> +			net_devmem_dmabuf_binding_unmap(binding);
> +		xa_lock(&net_devmem_dmabuf_bindings);
> +		WRITE_ONCE(binding->dev, NULL);
> +	}
> +	xa_unlock(&net_devmem_dmabuf_bindings);
> +}

[ ... ]

> @@ -551,16 +669,17 @@ 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)) {
> +			if (!rxq->lease)
> +				net_devmem_dmabuf_binding_unmap(binding);
> +			ASSERT_EXCLUSIVE_WRITER(binding->dev);
> +			WRITE_ONCE(binding->dev, NULL);
>  		}
> +		break;
>  	}
>  }

[Severity: High]
Can the dma_dev use-after-free still happen for an RX binding made on a
virtual device that leases queues, such as netkit leasing from a
physical NIC?

In that case binding->dev is the netkit device and bound_rxqs holds the
netkit rxq. dma_dev is the physical NIC's DMA device, which
netdev_queue_get_dma_dev() finds by following the lease.

On unlease, the call chain is:

netkit_uninit()
  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()

The lease pointers are cleared only after the cleanup returns:

net/core/netdev_rx_queue.c:netdev_rx_queue_unlease() {
    ...
	netif_rxq_cleanup_unlease(rxq_src, rxq_dst);

	WRITE_ONCE(rxq_src->lease, NULL);
	WRITE_ONCE(rxq_dst->lease, NULL);
    ...
}

So rxq->lease is always non-NULL here. The unmap is skipped, but
binding->dev is still set to NULL. netif_rxq_cleanup_unlease() also
ignores the new return value of __netif_mp_close_rxq(), so even a
successful close of the physical queue leaves the mapping in place.

After this, which path unmaps the binding?

net_devmem_unbind_dmabuf() unmaps only when binding->dev is non-NULL.

net_devmem_dev_unregister() on the physical device matches only
binding->dev == dev, so it never finds this binding.

dev_memory_provider_uninstall() on the physical device calls uninstall
with the physical rxq. That never matches the netkit rxq stored in
bound_rxqs.

The binding then stays mapped until __net_devmem_dmabuf_binding_free().
If the NIC was removed before that, the call to
dma_buf_unmap_attachment()->dma_unmap_sg_attrs() uses the freed physical
dma_dev. This is the same KASAN trace quoted in the commit message.

There are two ways to reach this:

Remove the physical NIC while the netlink socket is still open.
netkit_check_lease_unregister() runs from the physical device's
NETDEV_UNREGISTER and unregisters netkit. The unlease therefore runs
while the physical device is NETREG_UNREGISTERING, which seems to
contradict this part of the commit message:

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

Delete netkit first. The physical queue close succeeds, but the unmap
is skipped anyway. If the physical NIC is removed later, while the
socket or RX tokens still hold the binding, the deferred unmap runs on
the freed device.

The leasee branch in net_devmem_dev_unregister() has the same problem.
It skips the unmap when netif_is_queue_leasee(dev) is true but still
clears binding->dev, so nothing can unmap the binding before the
physical device goes away.

Could the lease case decide whether to unmap based on whether the
physical queue close succeeded, instead of skipping the unmap whenever
rxq->lease is set?

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