Re: [PATCH net-next v2 5/6] net: devmem: resolve physical map_dev for leased RX bindings

[netdev-bot+sashiko]

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

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

- [Medium] This patch fixes a use-after-free that predates the series,
  but the commit message understates it and has no Fixes: tag.
- [Medium] The series is not bisectable: the previous patch breaks leased
  RX bindings, and this patch fixes that without saying so.

--

net: devmem: resolve physical map_dev for leased RX bindings

For leased RX queues, netdev_nl_bind_rx_doit() now passes the physical
backing netdev (rxq->lease->dev) as map_dev, and locks it around
net_devmem_bind_dmabuf(). net_devmem_bind_dmabuf() now asserts that
both devices are locked. It also refuses to bind when map_dev is no
longer NETREG_REGISTERED.

> Previously, netdev_nl_bind_rx_doit() passed netdev as both map_dev and
> bind_dev to net_devmem_bind_dmabuf(), so leased RX bindings stored the
> virtual netdev in binding->map_dev without tracking the physical backing
> netdev at all. Unregistering the physical backing netdev while a leased
> RX binding was active failed to detach and unmap the dmabuf.

[Severity: Medium]
Does "failed to detach and unmap the dmabuf" understate the problem?

Before this series, a leased RX binding only referenced the netkit
device. Unregistering the physical device then goes like this:

net_devmem_dev_unregister(phys)
    binding->dev never matches phys
dev_memory_provider_uninstall(phys)
    mp_dmabuf_devmem_uninstall(phys_rxq)
        bound_rxqs only holds the virtual rxq, so nothing happens

Later, netkit_uninit()->netkit_queue_unlease()->netif_rxq_cleanup_unlease()
calls mp_dmabuf_devmem_uninstall(virt_rxq) while rxq->lease is still set:

    if (!rxq->lease)
        net_devmem_dmabuf_binding_unmap(binding);

The unmap is skipped and binding->dev is set to NULL. On socket close,
net_devmem_unbind_dmabuf() skips the unmap too, because binding->dev is
NULL.

The attachment stays mapped against the physical device's dma_dev
until __net_devmem_dmabuf_binding_free() calls
net_devmem_dmabuf_binding_unmap().

c4466d2c3e77 ("net: devmem: unmap dma_buf synchronously on queue
uninstall") notes that dma_buf_dynamic_attach() takes no reference on
dma_dev. Once the physical device is gone, isn't that final unmap a
use-after-free in dma_unmap_sg_attrs()?

If so, could the commit message name the use-after-free and carry
Fixes: tags? Candidates are 222b5566a02d ("net: Proxy
netdev_queue_get_dma_dev for leased queues"), which enabled devmem
binding through leased queues, and c4466d2c3e77, which kept the leasee
exception in mp_dmabuf_devmem_uninstall(). Without a Fixes: tag, stable
selection would likely miss this.

> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 9e56c26fc65f9..442362f23fe88 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c

[ ... ]

> @@ -1104,16 +1112,21 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
>  	if (err)
>  		goto err_rxq_bitmap;
>  
> -	dma_dev = netdev_nl_get_dma_dev(netdev, rxq_bitmap, info->extack);
> +	dma_dev = netdev_nl_get_dma_dev(netdev, rxq_bitmap, &map_dev,
> +					info->extack);
>  	if (IS_ERR(dma_dev)) {
>  		err = PTR_ERR(dma_dev);
>  		goto err_rxq_bitmap;
>  	}
>  
> -	binding = net_devmem_bind_dmabuf(netdev, netdev, dma_dev,
> +	if (map_dev != netdev)
> +		netdev_lock(map_dev);
> +	binding = net_devmem_bind_dmabuf(map_dev, netdev, dma_dev,

[Severity: Medium]
Is the series bisectable at the previous patch, "net: devmem: always
populate bind_dev and detach both devices on unregister"?

That patch dropped the netif_is_queue_leasee(dev) guard from
net_devmem_dev_unregister(). It now unmaps whenever map_dev matches:

    if (READ_ONCE(binding->map_dev) != dev)
        continue;
    ...
    net_devmem_dmabuf_binding_unmap(binding);

At that commit, this call still passed (netdev, netdev). For a leased
RX binding, map_dev == bind_dev == the netkit device.

Now delete the netkit device while the physical NIC stays up:

unregister_netdevice_many_notify()
    NETDEV_UNREGISTER
        netdev_genl_netdevice_event()
            net_devmem_dev_unregister(netkit)
                net_devmem_dmabuf_binding_unmap()  <- unmap, unpin, detach
    ndo_uninit
        netkit_uninit()
            netkit_queue_unlease()
                netif_rxq_cleanup_unlease()
                    __netif_mp_close_rxq(phys)

In that window, can the physical NIC's page pool still hand out
net_iovs whose DMA addresses come from the unmapped, unpinned sgt?

If __netif_mp_close_rxq() fails to restart the queue (the case
c4466d2c3e77 guarded against), the window never closes.

This patch fixes that by moving map_dev to rxq->lease->dev. The commit
message only describes the physical-unregister case, though.

Could the two patches be squashed or reordered, so the leasee guard is
dropped only once map_dev is the physical device? Failing that, could
the commit message mention this fix?

>  					 DMA_FROM_DEVICE, dmabuf_fd,
>  					 niov_shift, priv, NETLINK_CB(skb).sk,
>  					 info->extack);
> +	if (map_dev != netdev)
> +		netdev_unlock(map_dev);
>  	if (IS_ERR(binding)) {
>  		err = PTR_ERR(binding);
>  		goto err_rxq_bitmap;

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