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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: devmem: leased RX bindings never unmapped before dma_dev is freed
--

commit b250f1e4ffd826ff4e6f5c24e078e06c36365baf
Author: Mina Almasry <almasrymina@google.com>

net: devmem: unmap dma_buf synchronously on queue uninstall

This patch adds synchronous dma_buf unmapping when RX queues are closed,
uninstalled, or when the bound net_device is unregistered, to avoid a
slab use-after-free in dma_unmap_sg_attrs() if dma_dev is freed while the
devmem binding is still alive.

> 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;
> +
> +		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]
As reported by Sashiko on v2 [1], for an RX binding on a leased queue
(for example via netkit), can the binding still be left mapped after the
backing physical device is unregistered and its dma_dev is freed?

When netdev_nl_bind_rx_doit() binds to a virtual queue-leasee device, the
dmabuf is attached to the physical device's dma_dev, but binding->dev is
set to the virtual device and net_devmem_bind_dmabuf_to_queue() records
the virtual rxq in binding->bound_rxqs while netif_mp_open_rxq() installs
mp_params on the physical rxq.

When the virtual device unregisters (either directly or via
netkit_check_lease_unregister() when the physical device unregisters),
ndo_uninit runs:

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

Because netdev_rx_queue_unlease() only clears rxq_src->lease and
rxq_dst->lease after netif_rxq_cleanup_unlease() 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);
      ...
  }

rxq->lease is still non-NULL in mp_dmabuf_devmem_uninstall(), so it skips
net_devmem_dmabuf_binding_unmap(binding) while emptying
binding->bound_rxqs and setting binding->dev to NULL. If the netlink
socket is closed afterwards, net_devmem_unbind_dmabuf() sees
binding->dev == NULL and also skips unmapping.

Similarly, if the netlink socket was closed earlier and
netif_mp_close_rxq() failed (busy == true), net_devmem_dev_unregister()
on the virtual device skips net_devmem_dmabuf_binding_unmap(binding)
because netif_is_queue_leasee(dev) is true, and then clears binding->dev
to NULL.

In both cases, when the physical device finishes unregistering,
dev_memory_provider_uninstall() and net_devmem_dev_unregister() on the
physical device cannot find or unmap the binding because binding->bound_rxqs
is empty and binding->dev was the virtual device (and is now NULL). Once
the physical device frees its dma_dev, won't
__net_devmem_dmabuf_binding_free() later hit the same slab use-after-free
in dma_unmap_sg_attrs()?

[1] https://lore.kernel.org/r/179191526299.434549.7888465945343049764@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030083539.777877-1-almasrymina@google.com?part=1