Re: [PATCH net-next v13 2/5] tun/tap: add ptr_ring consume helper with netdev queue wakeup

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] vhost-net bypasses the new queue wakeup logic (`tun_ring_consume`), leading to a permanent TX queue stall when IFF_BACKPRESSURE is used.

Pre-existing issues:
- [High] A structural flaw in `tun_set_iff` allows changing a non-multiqueue persistent TUN device into a multiqueue device, leading to out-of-bounds accesses on the `dev->_tx` array via the newly introduced `__tun_wake_queue`.
--

--- Patch [2]: [PATCH net-next v13 2/5] tun/tap: add ptr_ring consume helper with netdev queue wakeup ---
commit 537cb0b1726915d53bc505e79f9dac64d03106a9
Author: Simon Schippers <[email protected]>

tun/tap: add ptr_ring consume helper with netdev queue wakeup

Introduce tun_ring_consume() that wraps ptr_ring_consume() and calls
__tun_wake_queue(). The latter wakes the stopped netdev subqueue once
half of the ring capacity has been consumed, tracked via the new
cons_cnt field in tun_file.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index ea8573efb8489..d9db6d2e4c56a 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -2117,13 +2142,50 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  	return total;
>  }
>  
> -static void *tun_ring_recv(struct tun_file *tfile, int noblock, int *err)
> +/* Callers must hold ring.consumer_lock */
> +static void __tun_wake_queue(struct tun_struct *tun,
> +			     struct tun_file *tfile, int consumed)
> +{
> +	struct netdev_queue *txq;
> +
> +	if (!(tun->flags & IFF_BACKPRESSURE))
> +		return;
> +
> +	txq = netdev_get_tx_queue(tun->dev, tfile->queue_index);

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds access
on the dev->_tx array?

If a persistent TUN device is initially created without IFF_MULTI_QUEUE, the
underlying net_device is allocated with only 1 TX queue. If the file
descriptor is closed and TUNSETIFF is called again with IFF_MULTI_QUEUE
enabled, tun_set_iff() allows the change but the net_device still only has
a single queue.

When a second file descriptor is attached, it receives a queue_index of 1.
Will this cause netdev_get_tx_queue() to access dev->_tx[1], resulting in
a heap out-of-bounds access when evaluated here?

> +
> +	/* Paired with smp_mb__after_atomic() in tun_net_xmit() */
> +	smp_mb();
> +	if (netif_tx_queue_stopped(txq)) {
> +		tfile->cons_cnt += consumed;
> +		if (tfile->cons_cnt >= tfile->tx_ring.size / 2 ||
> +		    __ptr_ring_empty(&tfile->tx_ring)) {
> +			netif_tx_wake_queue(txq);
> +			tfile->cons_cnt = 0;
> +		}
> +	}
> +}
> +
> +static void *tun_ring_consume(struct tun_struct *tun, struct tun_file *tfile)
> +{
> +	void *ptr;
> +
> +	spin_lock(&tfile->tx_ring.consumer_lock);
> +	ptr = __ptr_ring_consume(&tfile->tx_ring);
> +	if (ptr)
> +		__tun_wake_queue(tun, tfile, 1);

[Severity: High]
Will this cause a permanent TX queue stall when IFF_BACKPRESSURE is used
with vhost-net?

vhost-net directly accesses the transmit ring via tun_get_tx_ring() and
consumes packets natively, completely bypassing tun_ring_recv() and
tun_ring_consume().

Since the queue wakeup logic is exclusively placed here inside
tun_ring_consume(), does this mean the netdev subqueue will never be woken
up when vhost-net is consuming the packets?

> +
> +	spin_unlock(&tfile->tx_ring.consumer_lock);
> +	return ptr;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2