Re: [PATCH net-next v4 2/4] veth: implement Byte Queue Limits (BQL) for latency reduction

On 02/05/2026 09.18, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Out-of-bounds array access on peer_dev->_tx in veth_poll() when local RX queues exceed peer TX queues.
> - [High] Disabling XDP permanently stops peer TX queues if they were backpressured (DRV_XOFF leak).
> --
> 
> commit 9bc3ed022846dda6dd5cc131e798f949f63f491f
> Author: Jesper Dangaard Brouer <[email protected]>
> 
> veth: implement Byte Queue Limits (BQL) for latency reduction
> 
> This commit adds byte queue limits support to the veth driver to provide
> qdiscs with feedback and reduce latency. It charges a fixed unit per
> packet rather than byte-based charging.
> 
>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> index e35df717e65e2..3de25ba34a906 100644
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
> 
> [ ... ]
> 
>> @@ -975,7 +1016,7 @@ static int veth_poll(struct napi_struct *napi, int budget)
>>   	peer_txq = peer_dev ? netdev_get_tx_queue(peer_dev, queue_idx) : NULL;
>>   
>>   	xdp_set_return_frame_no_direct();
>> -	done = veth_xdp_rcv(rq, budget, &bq, &stats);
>> +	done = veth_xdp_rcv(rq, budget, &bq, &stats, peer_txq);
> 
> Could this lead to an out-of-bounds array access on the peer device's
> transmit queues?
> 
> If a veth pair is configured asymmetrically, the local device might have
> more receive queues than the peer has transmit queues.
> 
> If packets are redirected from another interface into a higher-indexed
> receive queue, the poll function will run with a queue_idx greater than or
> equal to peer_dev->real_num_tx_queues.
> 
> Since netdev_get_tx_queue() performs a direct array access without bounds
> checking, peer_txq would point out-of-bounds. This out-of-bounds pointer is
> then passed into the new veth_xdp_rcv() code paths and accessed by
> netif_tx_queue_stopped().
> 
> Should queue_idx be checked against peer_dev->real_num_tx_queues before
> fetching the transmit queue?
Yes, for the XDP redirect code path, which enqueues frames into the
peer's ptr_ring using smp_processor_id() % peer->real_num_rx_queues
as the ring index.  Then this is actually possible.

It is not a bug introduced in this patchset, but given we access
peer_txq a bit earlier, then a fix for this is needed as part of this
patchset. I have prepared a patch that fixes this.

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index e35df717e65e..0cfb19b760dd 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -972,7 +972,8 @@ static int veth_poll(struct napi_struct *napi, int 
budget)

         /* NAPI functions as RCU section */
         peer_dev = rcu_dereference_check(priv->peer, 
rcu_read_lock_bh_held());
-       peer_txq = peer_dev ? netdev_get_tx_queue(peer_dev, queue_idx) : 
NULL;
+       peer_txq = (peer_dev && queue_idx < peer_dev->real_num_tx_queues) ?
+                  netdev_get_tx_queue(peer_dev, queue_idx) : NULL;



> [ ... ]
> 
>> @@ -1091,6 +1133,17 @@ static void veth_napi_del_range(struct net_device *dev, int start, int end)
>>   		ptr_ring_cleanup(&rq->xdp_ring, veth_ptr_free);
>>   	}
>>   
>> +	/* Reset BQL on peer's txqs: remaining ring items were freed above
>> +	 * without BQL completion, so DQL state must be reset.
>> +	 */
>> +	peer = rtnl_dereference(priv->peer);
>> +	if (peer) {
>> +		int peer_end = min(end, (int)peer->real_num_tx_queues);
>> +
>> +		for (i = start; i < peer_end; i++)
>> +			netdev_tx_reset_queue(netdev_get_tx_queue(peer, i));
>> +	}
> 
> Does this cleanup leave peer transmit queues permanently stopped if they
> were backpressured when xdp was disabled?
> 
> When the ptr_ring is full, veth_xmit() stops the transmit queue using
> netif_tx_stop_queue(), which sets the __QUEUE_STATE_DRV_XOFF flag.
> 
> While netdev_tx_reset_queue() successfully clears the newly added
> __QUEUE_STATE_STACK_XOFF flag, it does not clear __QUEUE_STATE_DRV_XOFF.
> If a queue was backpressured exactly when xdp is disabled, it appears it
> would remain locked forever.
> 
> Would it be appropriate to wake the queues using netif_tx_wake_queue() or
> explicitly clear the __QUEUE_STATE_DRV_XOFF flag during this cleanup?

The watchdog timer in this patchset will call netif_tx_wake_queue().
So, we would recover from this unlikely case.  I guess there is no harm
in adding a netif_tx_wake_queue() call here (but we should likely guard
it with netif_running(dev)).

--Jesper