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

On Wed, May 06, 2026 at 12:29:05AM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

For reviewers, we could add a dev_hold() on the netkit to avoid the ABA
problem, but then a guest ns user can use bind-tx to prevent the the ns
manager user from being able to delete the netkit. This ->dev and ->vdev
check only exists to inform the sendmsg() caller early about the
mismatch, so I leaned towards this not being a great trade-off.
Especially since validate_xmit_unreadable_skb() does the validation
correctly. I'm open to adding this if that seems like a better approach,
though.

> 
> [ ... ]
> > 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.

The tests passed with CONFIG_LOCKDEP=y and CONFIG_PROVE_LOCKING=y. Since
they take the bind_dev != netdev path, I'm not not sure if this is
needed. Though I admit I don't fully understand mutex subclasses...

> 
> > +	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()?

Valid, will fix.

Best,
Bobby