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

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

[Jamal Hadi Salim]

This issue was caught by Sashiko (nipa). A producer-side path cap is
already merged (ea4d4b5dddb5); this patch adds the matching consumer-side
cap.

The issue:
The tx queue is picked and capped in netdev_core_pick_tx(), before the
__dev_xmit_skb() qdisc enqueue path. A tc BPF program running inside
egress qdisc (not clsact) can then change __sk_buff->queue_mapping;
that store is never re-checked. bpf_convert_ctx_access() bounds it only
by NO_QUEUE_MAPPING, not by the device's queue count.

On dequeue, qdisc_restart() calls netdev_get_tx_queue(), which
indexes dev->_tx[] with that value. netdev_get_tx_queue() guards it with
just a DEBUG_NET_WARN_ON_ONCE, so sch_direct_xmit() takes the out-of-bounds
txq->_xmit_lock and writes txq->xmit_lock_owner past the end of the
allocation on non-lltx devices such as ifb. Drivers that index their rings
from skb_get_queue_mapping() (ex: ifb) go out of range the same way.

The Fix:
Cap the mapping at the consumer and write the capped value back, so both
the qdisc's _tx[] lookup and any later driver read stay in range. The cap
folds into the dequeue path instead of a separate walk over the returned
list: dequeue_skb() caps the head, and each bulk helper caps the element it
appends. netdev_cap_txqueue() already logs a ratelimited notice and selects
queue 0. The single-packet case pays one extra compare against
real_num_tx_queues.

A bulk-dequeued list is transmitted under the single tx queue lock of the
head skb, so every element must belong to that same queue. The fast bulk
helper (TCQ_F_ONETXQUEUE, taken on BQL devices by the per-queue children
of mq, mqprio, taprio and htb, and by any qdisc on a single-queue device
such as the prio root in the reproducer) must therefore compare each
capped follower against the head and requeue a mismatch, exactly as the
slow helper already does; appending a follower that caps to a different
queue would send it under the wrong _xmit_lock.

Conditions to recreate the bug:
Load a SCHED_CLS or SCHED_ACT program that stores __sk_buff->queue_mapping
(a tc BPF action is enough), attach it to a transmit qdisc (for example a
matchall filter on a prio root), and send a packet. With a 1-queue device a
store of 1 indexes one past dev->_tx[]. Under KASAN with panic_on_warn=1
the unfixed kernel faults with "BUG: KASAN: slab-out-of-bounds in
sch_direct_xmit". Loading the program needs CAP_BPF/CAP_SYS_ADMIN, so the
trigger is root.

Fixes: 74e31ca850c1 ("bpf: add skb->queue_mapping write access from tc clsact")
Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
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/
Cc: stable@vger.kernel.org
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v2 -> v3:
- Considered Comments from Simon Horman (hunk counts did not match the
  diff content; patch did not apply):
  https://lore.kernel.org/netdev/20261015125822.GH83879@horms.kernel.org/

v1 -> v2:
- try_bulk_dequeue_skb(): the fast bulk helper (TCQ_F_ONETXQUEUE) capped
  each follower but appended it without comparing it against the head, so
  a mismatched follower was still sent under the head's txq lock. Compare
  the capped follower with the head and requeue a mismatch to skb_bad_txq,
  matching try_bulk_dequeue_skb_slow(). Reported by Sashiko (nipa).
 net/sched/sch_generic.c | 49 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 6f6a6f0d5eb0..6b548099b55d 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;
+}
+
+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));
+}
+
 static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
 {
-	const struct netdev_queue *txq = q->dev_queue;
+	const struct netdev_queue *txq;
 	spinlock_t *lock = NULL;
 	struct sk_buff *skb;
 
@@ -110,7 +132,8 @@ static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
 	skb = skb_peek(&q->skb_bad_txq);
 	if (skb) {
 		/* check the reason of requeuing without tx lock first */
-		txq = skb_get_tx_queue(txq->dev, skb);
+		/* re-cap: real_num_tx_queues may have shrunk */
+		txq = skb_cap_tx_queue(qdisc_dev(q), skb);
 		if (!netif_xmit_frozen_or_stopped(txq)) {
 			skb = __skb_dequeue(&q->skb_bad_txq);
 			if (qdisc_is_percpu_stats(q)) {
@@ -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;
+		}
 		bytelimit -= nskb->len; /* covers GSO len */
 		skb->next = nskb;
 		skb = nskb;
@@ -233,7 +266,7 @@ static void try_bulk_dequeue_skb_slow(struct Qdisc *q,
 				      struct sk_buff *skb,
 				      int *packets)
 {
-	int mapping = skb_get_queue_mapping(skb);
+	struct net_device *dev = qdisc_dev(q);
 	struct sk_buff *nskb;
 	int cnt = 0;
 
@@ -241,7 +274,11 @@ static void try_bulk_dequeue_skb_slow(struct Qdisc *q,
 		nskb = q->dequeue(q);
 		if (!nskb)
 			break;
-		if (unlikely(skb_get_queue_mapping(nskb) != mapping)) {
+		/* A tc BPF store may have changed the mapping after
+		 * pick; cap it and keep the list on one tx queue.
+		 */
+		if (unlikely(qdisc_cap_skb_tx_queue(dev, nskb) !=
+			     skb_get_queue_mapping(skb))) {
 			qdisc_enqueue_skb_bad_txq(q, nskb);
 			break;
 		}
@@ -286,7 +323,7 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
 		if (xfrm_offload(skb))
 			*validate = true;
 		/* check the reason of requeuing without tx lock first */
-		txq = skb_get_tx_queue(txq->dev, skb);
+		txq = skb_cap_tx_queue(qdisc_dev(q), skb);
 		if (!netif_xmit_frozen_or_stopped(txq)) {
 			skb = __skb_dequeue(&q->gso_skb);
 			if (qdisc_is_percpu_stats(q)) {
@@ -322,6 +359,8 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
 	skb = q->dequeue(q);
 	if (skb) {
 bulk:
+		/* cap the head; the bulk helpers cap every appended element */
+		qdisc_cap_skb_tx_queue(qdisc_dev(q), skb);
 		if (qdisc_may_bulk(q))
 			try_bulk_dequeue_skb(q, skb, txq, packets, budget);
 		else
-- 
2.43.0

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

[netdev-bot+sashiko]

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