Re: [PATCH net-next v5 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] Kernel panic (DoS) by bypassing unreadable SKB validation for netkit devices.
- [High] Use-After-Free and ABA pointer reuse vulnerability due to dangling `vdev` and `dev` in TX bindings.
- [High] Use-After-Free in `netdev_nl_bind_tx_doit` due to unprotected access to `phys_dev`.
- [Medium] Lockdep warning (recursive locking) when locking both virtual and physical devices.
--

commit fc62474407bd468f37ed050ed11229d597ff605b
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.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2da2688fe490b..bbc93b181ef9a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3993,7 +3993,8 @@ static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
>  	struct skb_shared_info *shinfo;
>  	struct net_iov *niov;
>  
> -	if (likely(skb_frags_readable(skb)))
> +	if (likely(skb_frags_readable(skb) ||
> +		   dev->netmem_tx == NETMEM_TX_NO_DMA))
>  		goto out;

If the netkit device is configured to pass packets to its peer (such as via
NETKIT_PASS or NETKIT_NEXT), could this allow an unreadable devmem SKB to be
injected into the host's local receive stack via __netif_rx()?

If software paths expecting valid struct pages attempt to read the payload,
such as local sockets calling kmap_local_page(), will this result in a crash?

[ ... ]

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 468344739db29..893643909f6a4 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);

Does this code need to take a reference count on dev and vdev?

Since TX bindings remain active until the user closes the netlink socket,
if the physical or virtual device is unregistered and freed while the
socket is open, these pointers might become dangling.

If a new net_device is subsequently allocated at the exact same memory
address, could the pointer comparison dst_dev != READ_ONCE(binding->vdev)
in net_devmem_get_binding() falsely succeed?

[ ... ]

> 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

[ ... ]

> +static struct net_device *
> +netdev_find_netmem_tx_dev(struct net_device *dev)
> +{
> +	struct netdev_rx_queue *lease_rxq;
> +	struct net_device *phys_dev;
> +	int i;
> +
> +	if (dev->netmem_tx == NETMEM_TX_DMA)
> +		return dev;
> +
> +	if (dev->netmem_tx != NETMEM_TX_NO_DMA)
> +		return NULL;
> +
> +	for (i = 0; i < dev->real_num_rx_queues; i++) {
> +		lease_rxq = READ_ONCE(__netif_get_rx_queue(dev, i)->lease);
> +		if (!lease_rxq)
> +			continue;
> +
> +		phys_dev = lease_rxq->dev;
> +		if (netif_device_present(phys_dev) &&
> +		    phys_dev->netmem_tx == NETMEM_TX_DMA)
> +			return phys_dev;

Is it safe to return phys_dev here without holding an RCU read lock or taking
a reference on the device?

Because the caller netdev_nl_bind_tx_doit() operates without the RTNL lock,
could the physical device be unregistered and freed concurrently between this
pointer read and the subsequent lock acquisition?

> +	}
> +
> +	return NULL;
> +}
> +
>  int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
>  {

[ ... ]

> +	if (bind_dev != netdev)
> +		netdev_lock(bind_dev);

Since both the virtual device and the physical device are likely initialized
as Ethernet devices, they will share the same lock class.

Will acquiring netdev_lock(bind_dev) while already holding the lock for
netdev trigger a lockdep warning for possible recursive locking?

> +
> +	dma_dev = netdev_queue_get_dma_dev(bind_dev, 0, NETDEV_QUEUE_TYPE_TX);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-tcp-dm-netkit-v5-0-408c59b91e66@meta.com?part=3