Re: [PATCH net] net/sched: clamp skb->queue_mapping before indexing the tx queue

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch fixes the out-of-range index, but on the
  TCQ_F_ONETXQUEUE fast bulk path a packet whose queue_mapping does not…

--

net/sched: clamp skb->queue_mapping before indexing the tx queue

A tc BPF program running inside an egress qdisc can store an out-of-range
skb->queue_mapping after netdev_core_pick_tx() has already capped it. This
patch adds the cap on the qdisc dequeue side. dequeue_skb() caps the head
skb and the bulk helpers cap each skb they append. The capped value is
written back to the skb, and an out-of-range mapping becomes 0.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-9R8V.v2.20260924072708%40mojatatu.com

> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0d..e3e76e794aae2 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -96,9 +96,33 @@ static void qdisc_maybe_clear_missed(struct Qdisc *q,
>  
>  #define SKB_XOFF_MAGIC ((struct sk_buff *)1UL)
>  
> +/* A tc BPF program attached to a transmit qdisc runs inside q->enqueue(),
> + * after the tx queue has been picked and capped, so its store to
> + * __sk_buff->queue_mapping is not re-capped. Bound it against the device at
> + * the consumer and write the capped value back, so a driver that re-reads
> + * skb_get_queue_mapping() in ndo_start_xmit() also indexes in range.
> + */

[ ... ]

> @@ -208,6 +232,7 @@ static void try_bulk_dequeue_skb(struct Qdisc *q,
>  				 int *packets, int budget)
>  {
>  	int bytelimit = qdisc_avail_bulklimit(txq) - skb->len;
> +	struct net_device *dev = qdisc_dev(q);
>  	int cnt = 0;
>  
>  	while (bytelimit > 0) {
> @@ -216,6 +241,10 @@ static void try_bulk_dequeue_skb(struct Qdisc *q,
>  		if (!nskb)
>  			break;
>  
> +		/* A tc BPF store may have poisoned the mapping after pick;
> +		 * cap every element before it reaches the driver.
> +		 */
> +		qdisc_cap_skb_tx_queue(dev, nskb);
>  		bytelimit -= nskb->len; /* covers GSO len */
>  		skb->next = nskb;
>  		skb = nskb;

[Severity: High]
Can a follower whose queue_mapping differs from the head's still reach the
driver under the wrong tx queue lock here?

The return value of qdisc_cap_skb_tx_queue() is ignored, and nskb is
appended no matter what. try_bulk_dequeue_skb_slow() compares each
follower with the head and diverts mismatches:

    if (unlikely(qdisc_cap_skb_tx_queue(dev, nskb) !=
                 skb_get_queue_mapping(skb))) {
        qdisc_enqueue_skb_bad_txq(q, nskb);
        break;
    }

This fast path has no such check. It is the path taken on BQL devices by
TCQ_F_ONETXQUEUE qdiscs. That includes the per-queue children of mq,
mqprio, taprio and htb offload, which get the flag in mq_init_common()
and mq_graft().

The whole list is then sent under the head's txq only:

qdisc_restart()
    txq = skb_get_tx_queue(dev, skb);
  sch_direct_xmit()
    HARD_TX_LOCK(dev, txq, smp_processor_id());
    dev_hard_start_xmit(skb, dev, txq, &ret);

Capping to 0 can create a mismatch in two ways:

  - In an mq child bound to txq N, a tc BPF program sets a follower's
    mapping out of range. The follower is rewritten to 0, but it is still
    sent under txq N's lock.

  - A poisoned head is capped to 0 in dequeue_skb(). The whole list then
    goes out under txq 0's lock, while unpoisoned followers keep mapping
    N.

Drivers that pick their ring from the skb mapping would then touch
another queue's ring and BQL state without holding that queue's
_xmit_lock. virtio_net start_xmit() is one example:

    int qnum = skb_get_queue_mapping(skb);
    struct send_queue *sq = &vi->sq[qnum];

igb is another. igb_tx_queue_mapping() picks the ring, and
netdev_tx_sent_queue(txring_txq(tx_ring)) updates BQL.

Meanwhile another CPU can legitimately hold the right lock: the mq child
that owns that txq, PACKET_QDISC_BYPASS, or netpoll. Could that race on
the descriptor ring and the dql counters? It would also let packets
bypass a stopped txq 0.

igb also changes behaviour in one case. Take a 2-ring device and a
follower stored as 3 inside mq child 1. igb's modulo used to map it to
ring 1, which matched the held lock. With this patch it goes to ring 0
while txq 1's lock is held.

There is also a possible stall. dequeue_skb() still runs the ONETXQUEUE
frozen pre-check and computes the bulk byte limit against q->dev_queue
(txq N). A capped head that is requeued into child N's gso_skb now
depends on txq 0 instead. When txq 0 wakes, netif_tx_wake_queue()
reschedules txq 0's qdisc, not child N. Could child N stall until
something else is enqueued to it?

The new comment says a driver that re-reads skb_get_queue_mapping()
"also indexes in range". Staying in range doesn't look sufficient,
because per-queue driver state is protected by the matching txq lock.

ifb is not affected by this fast path. It doesn't use BQL, so
qdisc_avail_bulklimit() returns 0 and no followers are appended. The
drivers exposed are BQL multi-queue ones such as virtio_net, igb and
ixgbe.

Would it work to compare each follower's capped mapping with the head's
here, and push mismatches to skb_bad_txq the way the slow path does?
Another option for ONETXQUEUE qdiscs would be to cap to the index of
q->dev_queue instead of 0.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-MD1X.v1.20261005193231%40mojatatu.com