Re: [PATCH net-next v2 04/11] net: airoha: Rely on net_device pointer in HTB callbacks

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

Pre-existing issues:
- [High] An off-by-one error in `airoha_tc_remove_htb_queue` disables the hardware rate limit for the wrong queue.
- [High] Unconditional linear increment/decrement of `real_num_tx_queues` combined with sparse statically mapped queue IDs causes out-of-bounds queue indices and silent truncation of active queues.
- [High] Missing collision check during HTB leaf queue allocation leads to silent hardware state overwrites and permanent leaks of `real_num_tx_queues`.
--

--- Patch [4]: [PATCH net-next v2 04/11] net: airoha: Rely on net_device pointer in HTB callbacks ---
commit 358f6f560341ad816e004d993767ab86443f985e
Author: Lorenzo Bianconi <[email protected]>
Subject: net: airoha: Rely on net_device pointer in HTB callbacks

Remove airoha_gdm_port dependency in HTB tc callback signatures and rely
on net_device pointer instead. This patch does not introduce any logical
change and is a preliminary patch.

> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2503,21 +2504,21 @@ static int airoha_qdma_set_tx_rate_limit(struct airoha_gdm_port *port,
>  	return 0;
>  }
>  
> -static int airoha_tc_htb_alloc_leaf_queue(struct airoha_gdm_port *port,
> +static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
>  					  struct tc_htb_qopt_offload *opt)
>  {
>  	u32 channel = TC_H_MIN(opt->classid) % AIROHA_NUM_QOS_CHANNELS;
>  	u32 rate = div_u64(opt->rate, 1000) << 3; /* kbps */
> -	struct net_device *dev = port->dev;
> -	int num_tx_queues = dev->real_num_tx_queues;
> -	int err;
> +	int err, num_tx_queues = netdev->real_num_tx_queues;
> +	struct airoha_gdm_port *port = netdev_priv(netdev);
>  
>  	if (opt->parent_classid != TC_HTB_CLASSID_ROOT) {
>  		NL_SET_ERR_MSG_MOD(opt->extack, "invalid parent classid");
>  		return -EINVAL;
>  	}
>  
> -	err = airoha_qdma_set_tx_rate_limit(port, channel, rate, opt->quantum);
> +	err = airoha_qdma_set_tx_rate_limit(netdev, channel, rate,
> +					    opt->quantum);

[Severity: High]
This is a pre-existing issue, but is there a missing collision check when
multiple TC classes map to the same channel via the modulo operation?

If this occurs, could it silently overwrite the active hardware rate limit
and incorrectly increment real_num_tx_queues a second time for the same
channel? When the classes are eventually deleted, the first deletion will
clear the bit in qos_sq_bmap, causing subsequent deletions to fail the
check and permanently leak the inflated queue count.

>  	if (err) {
>  		NL_SET_ERR_MSG_MOD(opt->extack,
>  				   "failed configuring htb offload");
> @@ -2527,9 +2528,10 @@ static int airoha_tc_htb_alloc_leaf_queue(struct airoha_gdm_port *port,
>  	if (opt->command == TC_HTB_NODE_MODIFY)
>  		return 0;
>  
> -	err = netif_set_real_num_tx_queues(dev, num_tx_queues + 1);
> +	err = netif_set_real_num_tx_queues(netdev, num_tx_queues + 1);

[Severity: High]
This is a pre-existing issue, but does incrementing real_num_tx_queues
unconditionally by 1 cause out-of-bounds queue mapping in the network stack?

The qid is statically assigned based on the channel, which could result
in a sparse ID that exceeds the new real_num_tx_queues value, causing the
enqueue path to wrap around.

>  	if (err) {
> -		airoha_qdma_set_tx_rate_limit(port, channel, 0, opt->quantum);
> +		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;

[ ... ]

> @@ -2716,44 +2718,47 @@ static int airoha_dev_setup_tc_block(struct net_device *dev,
>  	}
>  }
>  
> -static void airoha_tc_remove_htb_queue(struct airoha_gdm_port *port, int queue)
> +static void airoha_tc_remove_htb_queue(struct net_device *netdev, int queue)
>  {
> -	struct net_device *dev = port->dev;
> +	struct airoha_gdm_port *port = netdev_priv(netdev);
>  
> -	netif_set_real_num_tx_queues(dev, dev->real_num_tx_queues - 1);
> -	airoha_qdma_set_tx_rate_limit(port, queue + 1, 0, 0);
> +	netif_set_real_num_tx_queues(netdev, netdev->real_num_tx_queues - 1);

[Severity: High]
This is a pre-existing issue, but could decrementing real_num_tx_queues
blindly during out-of-order queue deletions silently truncate higher-numbered
queues that are still active? 

This might result in qdisc_reset_all_tx_gt() improperly destroying active
queues.

> +	airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);

[Severity: High]
This is a pre-existing issue, but does this off-by-one index clear the
hardware rate limit for the wrong queue?

The allocation path configures the hardware using the exact channel index,
but the removal path passes queue + 1. Will this leave the deleted queue's
rate limit orphaned while unintentionally disabling an adjacent active
queue's rate limit?

>  	clear_bit(queue, port->qos_sq_bmap);
>  }

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