Re: [PATCH net-next v7 05/10] net: airoha: Move qos_sq_bmap in airoha_gdm_dev struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Off-by-one error in airoha_tc_remove_htb_queue when clearing hardware rate limit.
- [High] Blind increment/decrement of real_num_tx_queues breaks HTB offload for non-sequential queue IDs.
--

commit 7088e1abda5f2f9f4a048fc5586684fdaf382845
Author: Lorenzo Bianconi <lorenzo@kernel.org>

net: airoha: Move qos_sq_bmap in airoha_gdm_dev struct

This commit moves the qos_sq_bmap field from airoha_gdm_port to
airoha_gdm_dev to prevent overwrites when multiple net_devices share a GDM
port. It also adds qos_channel_map to track QDMA channels in use.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index b90fc1cbfbc48..2a2a30dbad27e 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2597,44 +2597,61 @@ static int airoha_qdma_set_tx_rate_limit(struct net_device *netdev,

[ ... ]

>  	err = netif_set_real_num_tx_queues(netdev, num_tx_queues + 1);
>  	if (err) {
>  		airoha_qdma_set_tx_rate_limit(netdev, channel, 0,
>  					      opt->quantum);
>  		NL_SET_ERR_MSG_MOD(opt->extack,
>  				   "failed setting real_num_tx_queues");
> -		return err;
> +		goto error;
>  	}
>  
> -	set_bit(channel, port->qos_sq_bmap);
> +	set_bit(channel, dev->qos_sq_bmap);
>  	opt->qid = AIROHA_NUM_TX_RING + channel;

This problem wasn't introduced by this patch, but is a logic flaw that
breaks the offload functionality.

When allocating a QoS channel, real_num_tx_queues is blindly incremented by
1 while the qid is derived from the channel. If a user allocates a
non-sequential classid (for example, mapping to channel 3 when it is the
first allocated queue), real_num_tx_queues will become
AIROHA_NUM_TX_RING + 1, but qid will be AIROHA_NUM_TX_RING + 3.

When transmitting, netdev_cap_txqueue() strictly validates that the packet's
queue_mapping is < real_num_tx_queues.

Since qid exceeds the blindly incremented count, will the stack log a warning
and forcefully route the packet to queue 0, silently bypassing the HTB
hardware rate limit?

Additionally, does blindly decrementing the count when deleting queues
immediately invalidate any active higher-indexed queues?

> @@ -2815,11 +2832,13 @@ static int airoha_dev_setup_tc_block(struct net_device *dev,
>  static void airoha_tc_remove_htb_queue(struct net_device *netdev, int queue)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> -	struct airoha_gdm_port *port = dev->port;
> +	struct airoha_qdma *qdma = dev->qdma;
>  
>  	netif_set_real_num_tx_queues(netdev, netdev->real_num_tx_queues - 1);
>  	airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);

This problem wasn't introduced by this patch, but the patch exposes it to
cross-device corruption.

In airoha_tc_remove_htb_queue(), the driver attempts to disable the hardware
rate limit for the removed queue by calling airoha_qdma_set_tx_rate_limit()
with queue + 1. However, the queue is originally allocated and configured
using the channel index (where channel equals queue).

Passing queue + 1 fails to clear the rate limit of the actually deleted queue,
permanently leaking its hardware state. Furthermore, it incorrectly zeros out
the rate limit of the adjacent queue.

Because this patch allows multiple net_devices to share the same QDMA block
and its channels, could deleting a QoS queue on one interface unexpectedly
corrupt the QoS hardware state of another interface actively using the
adjacent channel?

> -	clear_bit(queue, port->qos_sq_bmap);
> +
> +	clear_bit(queue, qdma->qos_channel_map);
> +	clear_bit(queue, dev->qos_sq_bmap);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-airoha-eth-multi-serdes-v7-0-99e0093303e2@kernel.org?part=5