Re: [PATCH net v3] 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] net/sched/sch_generic.c, try_bulk_dequeue_skb(): the patch
  adds a new requeue of a mismatched follower to q->skb_bad_txq in the…
- [Medium] net/sched/sch_generic.c, qdisc_cap_skb_tx_queue(): an out-of-
  range queue_mapping is sent to device queue 0 through…

--

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

A tc BPF program on an egress qdisc can store an out-of-range value in
skb->queue_mapping after netdev_core_pick_tx() has already picked the
queue. This patch caps that value against real_num_tx_queues on the qdisc
dequeue side. The head is capped in dequeue_skb(), and both bulk helpers
cap each follower and requeue it to skb_bad_txq if it no longer matches
the head.

Closes: https://lore.kernel.org/netdev/179033713973.2160803.4914570693994398206@kernel.org/
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-9R8V.v2.20260924072708%40mojatatu.com
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-MD1X.v1.20261005193231%40mojatatu.com
Link: https://lore.kernel.org/netdev/20261015125822.GH83879@horms.kernel.org/

> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0d..6b548099b55d8 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -96,9 +96,31 @@ static void qdisc_maybe_clear_missed(struct Qdisc *q,
>  
>  #define SKB_XOFF_MAGIC ((struct sk_buff *)1UL)
>  
> +/* A tc BPF program on a transmit qdisc can change __sk_buff->queue_mapping
> + * after the tx queue was picked. Re-cap it against the device and write it
> + * back, so a driver that re-reads skb_get_queue_mapping() stays in range.
> + */
> +static u16 qdisc_cap_skb_tx_queue(struct net_device *dev, struct sk_buff *skb)
> +{
> +	u16 queue = skb_get_queue_mapping(skb);
> +	u16 capped;
> +
> +	capped = netdev_cap_txqueue(dev, queue);
> +	if (unlikely(capped != queue))
> +		skb_set_queue_mapping(skb, capped);
> +
> +	return capped;
> +}

[Severity: Medium]

Does sending an out-of-range mapping to queue 0 here break the assumption
documented for TCQ_F_ONETXQUEUE per-queue children (mq, mqprio, taprio,
htb offload)?

include/net/sch_generic.h:
#define TCQ_F_ONETXQUEUE	0x10 /* dequeue_skb() can assume all skbs are for
				      * q->dev_queue : It can test
				      * netif_xmit_frozen_or_stopped() before
				      * dequeueing next packet.
				      * Its true for MQ/MQPRIO slaves, or non
				      * multiqueue device.

Say a filter on the child attached to txq i (i != 0) stores a
queue_mapping >= real_num_tx_queues. The head is then rewritten to 0
here.

dequeue_skb() still uses q->dev_queue in three places: the ONETXQUEUE
frozen check, the BQL limit passed to try_bulk_dequeue_skb(), and
trace_qdisc_dequeue(). qdisc_restart(), though, transmits on
skb_get_tx_queue(dev, skb), which is txq 0.

If txq 0 is stopped at that point, sch_direct_xmit()->dev_requeue_skb()
puts the list on q->gso_skb. The gso_skb path in dequeue_skb() then
returns NULL for as long as txq 0 is frozen. When txq 0 wakes:

net/core/dev.c:netif_tx_wake_queue() {
    ...
	q = rcu_dereference(dev_queue->qdisc);
	__netif_schedule(q);
    ...
}

this schedules child 0, not q. Can q then stay idle until another packet
is enqueued to it?

Capping to 0 is consistent in netdev_core_pick_tx(), because the qdisc
is then taken from txq 0. Here the qdisc has already been chosen.

For ONETXQUEUE qdiscs, would capping to q->dev_queue's index keep the
documented invariant and avoid this?

> +
> +static struct netdev_queue *skb_cap_tx_queue(struct net_device *dev,
> +					     struct sk_buff *skb)
> +{
> +	return netdev_get_tx_queue(dev, qdisc_cap_skb_tx_queue(dev, skb));
> +}
> +

[ ... ]

> @@ -208,6 +231,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 +240,15 @@ static void try_bulk_dequeue_skb(struct Qdisc *q,
>  		if (!nskb)
>  			break;
>  
> +		/* Re-cap a mapping changed after pick; requeue a
> +		 * follower that no longer matches the head, which owns
> +		 * the _xmit_lock for the whole list.
> +		 */
> +		if (unlikely(qdisc_cap_skb_tx_queue(dev, nskb) !=
> +			     skb_get_queue_mapping(skb))) {
> +			qdisc_enqueue_skb_bad_txq(q, nskb);
> +			break;
> +		}

[Severity: Medium]

Can this stall a per-queue child qdisc of mq, mqprio, taprio offload or
htb offload on a multiqueue BQL device?

A follower can fail to match the head here in two ways. A filter on the
child (BPF, or act_skbedit queue_mapping) can set a valid queue X that
differs from the head's queue Y. Or an out-of-range value can be capped
to 0 by qdisc_cap_skb_tx_queue() while the head maps to another queue.

In both cases the follower is parked on q->skb_bad_txq, and nothing
reschedules q or sets the MISSED bit. On the next dequeue_skb(), the
ONETXQUEUE check only looks at q->dev_queue (Y), which is running. If X
is stopped:

dequeue_skb()
  qdisc_dequeue_skb_bad_txq()
    __skb_dequeue_bad_txq()
      txq = skb_cap_tx_queue(qdisc_dev(q), skb);   /* X, stopped */
      skb = SKB_XOFF_MAGIC;
      qdisc_maybe_clear_missed(q, txq);
  if (skb == SKB_XOFF_MAGIC)
      return NULL;

So q->dequeue() is never called. All of q's backlog then waits behind
the parked skb, including traffic for q's own running txq Y.

When X wakes, netif_tx_wake_queue() schedules rcu_dereference(X->qdisc).
mq_attach() grafted X's own child there, not q, and Y never stopped.

Does anything reschedule q before a new packet is enqueued to it?

The commit message says the fast helper must requeue a mismatch:

    exactly as the slow helper already does

The slow helper only runs for non-ONETXQUEUE root qdiscs. There every
txq->qdisc is q, so a wake on any queue reschedules q.

Does that reasoning carry over to per-queue children, where a wake on X
cannot reach the child attached to Y?

>  		bytelimit -= nskb->len; /* covers GSO len */
>  		skb->next = nskb;
>  		skb = nskb;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-MD1X.v3.r0.20261015165943-1%40mojatatu.com