Re: [PATCH net-next v3 3/8] net: devmem: support TX over NETMEM_TX_NO_DMA devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Missing reference counting on `dev` and `vdev` leads to a Use-After-Free when the netlink socket is closed.
- [High] `validate_xmit_unreadable_skb()` completely skips validation for netkit guest devices, breaking virtual device isolation.
- [High] Acquiring multiple `net_device` locks sequentially triggers a lockdep recursive locking warning.
- [Medium] A netmem TX binding is leaked if `genlmsg_reply()` fails in `netdev_nl_bind_tx_doit()`.
--

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

net: devmem: support TX over NETMEM_TX_NO_DMA devices

When a netkit virtual device leases queues from a physical NIC, devmem
TX bindings created on the netkit device must still result in the dmabuf
being mapped for dma by the physical device. This patch accomplishes
this by teaching the bind handler to search for the underlying
DMA-capable device by looking it up via leased rx queues.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index fbe4c328a3674..268417c9ef224 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3999,6 +3999,9 @@ static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
>  	if (dev->netmem_tx == NETMEM_TX_NONE)
>  		goto out_free;
>  
> +	if (dev->netmem_tx == NETMEM_TX_NO_DMA)
> +		goto out;
> +

The commit message states that the skb is validated twice: once on the netkit
guest device and again on the physical NIC. However, this check seems to
completely bypass the validation for the guest device rather than validating
it.

Since all virtual devices leasing queues from the same physical NIC share the
same binding->dev, the physical NIC cannot distinguish between their devmem
payloads. Should this instead explicitly validate that
READ_ONCE(binding->vdev) == dev to preserve isolation?

>  	shinfo = skb_shinfo(skb);
>  
>  	if (shinfo->nr_frags > 0) {

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index cde4c89bc146a..644c286b778f1 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);

Are dev and vdev missing reference counting here?

Because TX bindings are not attached to device queues, their lifetime is
strictly tied to the netlink socket's priv->bindings list. If either network
device is unregistered and its memory freed while the socket remains open,
binding->dev and binding->vdev would become dangling pointers.

When the socket is later closed, netdev_nl_sock_priv_destroy() iterates over
the bindings and could dereference the freed memory, causing a use-after-free.
Also, net_devmem_get_binding() might compare dst_dev to the dangling vdev
pointer.

> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 4d2c49371cdb4..b4d48f3672a5b 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
[ ... ]
> @@ -1171,22 +1205,41 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto err_unlock_netdev;
>  	}
>  
> +	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 this second lock trigger a lockdep recursive locking warning?

The virtual device's lock is acquired implicitly via
netdev_get_by_index_lock() earlier. Here we explicitly call
netdev_lock(bind_dev) to lock the physical device.

Since all net_device locks (dev->lock) are initialized at the same call site
in alloc_netdev_mqs(), lockdep assigns them to the identical lock class.
Acquiring two instances of the same lock class sequentially without a nesting
annotation might violate locking rules.

> +	dma_dev = netdev_queue_get_dma_dev(bind_dev, 0, NETDEV_QUEUE_TYPE_TX);
> +
> +	binding = net_devmem_bind_dmabuf(bind_dev,
> +					 bind_dev != netdev ? netdev : NULL,
> +					 dma_dev, DMA_TO_DEVICE, dmabuf_fd,
> +					 priv, info->extack);
>  	if (IS_ERR(binding)) {
>  		err = PTR_ERR(binding);
> -		goto err_unlock_netdev;
> +		goto err_unlock_bind_dev;
>  	}
>  
>  	nla_put_u32(rsp, NETDEV_A_DMABUF_ID, binding->id);
>  	genlmsg_end(rsp, hdr);
>  
> +	if (bind_dev != netdev)
> +		netdev_unlock(bind_dev);
>  	netdev_unlock(netdev);
>  	mutex_unlock(&priv->lock);
>  
>  	return genlmsg_reply(rsp, info);

Does this leak the netmem TX binding if genlmsg_reply() fails?

After successfully establishing the TX binding, the function unlocks the
devices and calls genlmsg_reply(), directly returning its error code. If
genlmsg_reply() fails (e.g., due to socket buffer exhaustion), the user
application receives an error and assumes the bind operation failed.

However, the binding remains fully active in the kernel and is attached to the
socket's priv->bindings list. Because the user never received the binding->id,
they cannot explicitly unbind it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507-tcp-dm-netkit-v3-0-52821445867c@meta.com?part=3