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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3945 bytes --]

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Off-by-one in airoha_tc_remove_htb_queue() clears the hardware rate limit of the wrong QDMA channel, corrupting cross-device shared state.
> - [High] Dynamic real_num_tx_queues accounting causes out-of-bounds queue indices and bypasses hardware QoS entirely.
> --
> 
> commit cfd982e85e1dc0ffc8dc017589e1128ecce7248e
> 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 overwriting configurations from multiple
> net_devices sharing the same GDM port. It also introduces
> qos_channel_map in airoha_qdma to track shared QDMA channel usage.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 847f432b0a2b0..594fbebcb12ce 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> [ ... ]
> > +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;
> > +	int err, num_tx_queues = netdev->real_num_tx_queues;
> [ ... ]
> >  	err = netif_set_real_num_tx_queues(netdev, num_tx_queues + 1);
> >  	if (err) {
> [ ... ]
> > -	set_bit(channel, port->qos_sq_bmap);
> > +	set_bit(channel, dev->qos_sq_bmap);
> >  	opt->qid = AIROHA_NUM_TX_RING + channel;
> 
> Can this dynamic real_num_tx_queues accounting cause out-of-bounds queue
> indices?
> 
> The driver tracks allocated QoS leaf queues by incrementing
> dev->real_num_tx_queues by 1. However, the assigned hardware queue index is
> derived from a sparse hash of the user-provided classid.
> 
> If a user allocates a non-sequential classid that hashes to a higher channel
> number while having only a few queues active, won't opt->qid exceed the
> incremented dev->real_num_tx_queues?
> 
> During transmission, would netdev_core_pick_tx() then detect this
> out-of-bounds queue mapping, reset the queue index, and route the traffic to
> the default ring 0, silently bypassing the hardware HTB rate limits?

As pointed out before this issue has not been introduced by this series and I
will fix it with a dedicated patch.

> 
> [ ... ]
> >  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);
> 
> Is there an off-by-one error when clearing the hardware rate limit here?
> 
> The queue + 1 offset causes it to clear the configuration of the adjacent
> channel instead of the one actually being deleted, as the allocation path
> uses channel without the offset.
> 
> Since this patch enables multiple net_devices to share the same QDMA block,
> could clearing queue + 1 silently destroy the active hardware QoS
> configuration belonging to a completely different net_device?
> 
> Does this also leave the rate limit for the actual deleted queue active,
> and potentially cause an out-of-bounds register write if the deleted queue is
> the maximum channel index (15)?

As pointed out before this issue has not been introduced by this series and I
will fix it with a dedicated patch.

Regards,
Lorenzo

> 
> > -	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/20260519-airoha-eth-multi-serdes-v8-0-6bd70e329df6@kernel.org?part=5

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]