* Re: [PATCH net-next v2 2/6] net: devmem: support TX over NETMEM_TX_NO_DMA devices
[not found] ` <20260506002905.DF681C2BCB4@smtp.kernel.org>
@ 2026-05-06 1:18 ` Bobby Eshleman
2026-05-06 15:00 ` Bobby Eshleman
0 siblings, 1 reply; 2+ messages in thread
From: Bobby Eshleman @ 2026-05-06 1:18 UTC (permalink / raw)
To: sashiko; +Cc: bpf
On Wed, May 06, 2026 at 12:29:05AM +0000, sashiko-bot@kernel.org 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 <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?
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
^ permalink raw reply [flat|nested] 2+ messages in thread