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

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 [off-list ref]

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.

quoted

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

[ ... ]

quoted

+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;

[ ... ]

quoted

 	err = netif_set_real_num_tx_queues(netdev, num_tx_queues + 1);
 	if (err) {

[ ... ]

quoted

-	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.

[ ... ]

quoted

 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

quoted

-	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