Re: [PATCH net-next v2 2/6] net: devmem: support TX over NETMEM_TX_NO_DMA devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Double lock lockdep splat and potential deadlock in netdev_nl_bind_tx_doit()
- [High] Use-After-Free of dma_dev pointer in netdev_nl_bind_tx_doit()
- [High] ABA vulnerability and pointer aliasing due to untracked vdev lifecycle
--

commit fea3266a76d31b8278bf86dac679860b442a5bea
Author: Bobby Eshleman <bobbyeshleman@meta.com>

net: devmem: support TX over NETMEM_TX_NO_DMA devices

This patch enables devmem TX bindings on netkit virtual devices by routing
the binding to the underlying DMA-capable physical device. It extends the
skb validation and binding lookup to support the netkit architecture.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index cde4c89bc146..644c286b778f 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
[ ... ]
> @@ -212,6 +212,7 @@ net_devmem_bind_dmabuf(struct net_device *dev,
>  	}
>  
>  	binding->dev = dev;
> +	binding->vdev = vdev;
>  	xa_init_flags(&binding->bound_rxqs, XA_FLAGS_ALLOC);

Is it possible for this virtual device to be unregistered and freed while
the binding is still active? Since there is no reference count acquired on
vdev, and no NETDEV_UNREGISTER notifier seems to clean up the binding for
the virtual device, vdev could become a dangling pointer.

[ ... ]
> @@ -397,7 +398,8 @@ struct net_devmem_dmabuf_binding *net_devmem_get_binding(struct sock *sk,
>  	 */
>  	dst_dev = dst_dev_rcu(dst);
>  	if (unlikely(!dst_dev) ||
> -	    unlikely(dst_dev != READ_ONCE(binding->dev))) {
> +	    unlikely(dst_dev != READ_ONCE(binding->dev) &&
> +		     dst_dev != READ_ONCE(binding->vdev))) {

If vdev was freed as mentioned above, and a new, unrelated network device
is allocated at the exact same memory address, could this pointer comparison
incorrectly pass due to the ABA problem?

[ ... ]
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index b8f6076d8007..0e296c3bb677 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
[ ... ]
> @@ -1164,16 +1197,30 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
[ ... ]
> +	bind_dev = netdev_find_netmem_tx_dev(netdev);
> +	if (!bind_dev) {
> +		err = -EOPNOTSUPP;
> +		NL_SET_ERR_MSG(info->extack,
> +			       "No DMA-capable device found for netmem TX");
> +		goto err_unlock_netdev;
> +	}
> +
> +	if (bind_dev != netdev)
> +		netdev_lock(bind_dev);

Will acquiring netdev_lock() on bind_dev while already holding it on netdev
trigger a lockdep warning? Since both are struct net_device instances, their
locks belong to the exact same lockdep class. Without a mutex_lock_nested()
annotation, acquiring them sequentially might trigger a possible circular
locking dependency splat, and introduce a potential deadlock if locked in
reverse order elsewhere.

> +	dma_dev = netdev_queue_get_dma_dev(bind_dev, 0, NETDEV_QUEUE_TYPE_TX);
> +	if (bind_dev != netdev)
> +		netdev_unlock(bind_dev);
> +	binding = net_devmem_bind_dmabuf(bind_dev,
> +					 bind_dev != netdev ? netdev : NULL,
> +					 dma_dev, DMA_TO_DEVICE, dmabuf_fd,
> +					 priv, info->extack);

Is it safe to use dma_dev after dropping the bind_dev lock? Since
netdev_queue_get_dma_dev() returns a raw device pointer without acquiring a
reference via get_device(), what prevents a concurrent hardware hot-unplug
or driver unbind from freeing the dma_dev between netdev_unlock() and the
subsequent use in net_devmem_bind_dmabuf()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504-tcp-dm-netkit-v2-0-56d52ac72fd4@meta.com?part=2