[PATCH net v2] net: cap skb->queue_mapping when the tx queue is picked

[PATCH net v2] net: cap skb->queue_mapping when the tx queue is picked

[Jamal Hadi Salim]

skbedit can set skb->queue_mapping and raise the per-CPU skip_txqueue
flag so __dev_queue_xmit() honours the mapping. __dev_queue_xmit()
cleared the flag before sch_handle_egress() and only read it afterwards,
so the flag was not confined to the xmit that set it: a nested xmit
(mirred redirect or mirror, or a drop after skbedit) could set the flag
and the outer xmit would consume it for an skb that never went through
skbedit.

A forwarded packet still carries the ingress NIC's rx_queue + 1 in
skb->queue_mapping, so the outer device then indexes its tx queue state
with that stale value. Taprio's child array q->qdiscs[] is sized to the
device's queue count, so taprio_enqueue() indexes past its allocation
and dereferences the result as a struct Qdisc *.

Own the flag for the whole xmit frame: save the incoming value and clear
it before any of the frame's egress work can recurse, and restore it only
when the frame exits. A transmit-qdisc classifier is a documented flag
producer too (it runs in q->enqueue(), after sch_handle_egress()), so the
flag must be owned for the whole frame, not just around the clsact hook.
The flag then never crosses an xmit boundary in either direction. Also
store the value netdev_cap_txqueue() selected back into skb->queue_mapping
in netdev_tx_queue_mapping(), as netdev_core_pick_tx() already does, so a
mapping rewritten later in the same egress run (for example a tc BPF
store) cannot leave an out-of-range index for the later readers on the
xmit path.

A local user in a network namespace can redirect a packet from a device
with more TX queues to one with fewer after setting a mapping valid only
on the larger device. That reaches these reads and, under KASAN, faults
with "slab-out-of-bounds in taprio_enqueue".

Conditions to recreate the bug: with CONFIG_NET_SCH_TAPRIO=y,
CONFIG_NET_ACT_SKBEDIT=y, CONFIG_NET_ACT_MIRRED=y,
CONFIG_NET_CLS_MATCHALL=y, CONFIG_NET_SCH_PRIO=y and KASAN enabled,
create qa (3 queues), qb (2 queues) and qc (1 queue) as dummy devices;
put a taprio root on qb and clsact on all three; then add an egress
matchall filter on every device. On qa: "action skbedit queue_mapping 2
pipe action mirred egress redirect dev qb". On qb: "action mirred egress
mirror dev qc". On qc: "action skbedit queue_mapping 0 pipe". Send one
packet out qa. qc's skbedit sets the flag while qb's outer xmit is in
flight; without the fix qb consumes it and reads its two-entry taprio
child array with the forwarded packet's stale mapping. A qc whose skbedit
is instead installed in a transmit-qdisc classifier (a matchall filter on
the qc root qdisc) reaches the same read the same way.

Testing: on a KASAN build with panic_on_warn=1 the unfixed kernel panics
with "BUG: KASAN: slab-out-of-bounds in taprio_enqueue", a read 0 bytes
past a 16-byte taprio_init() allocation; the fixed kernel runs both the
clsact-setter and the transmit-qdisc-classifier reproducers with no report
and no clamp notice, and the taprio/multiq/matchall/skbedit/mirred tdc
tests pass (117 ok, 0 fail).

Fixes: 2f1e85b1aee4 ("net: sched: use queue_mapping to pick tx queue")
Reported-by: Zero Day Initiative <zdi-disclosures@trendmicro.com>
Link: https://lore.kernel.org/netdev/CANn89iLwYx8nCVf0pCEk_MmEiyC6kQaMwCQT9WkQVeeNzNQHqQ@mail.gmail.com/
Link: https://lore.kernel.org/netdev/179008581937.2160803.7117814290574262942@kernel.org/
Suggested-by: Eric Dumazet <edumazet@google.com>
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v1 -> v2:
- Rework the root cause to the per-CPU skip_txqueue flag lifetime: it was
  cleared before sch_handle_egress() and read after, so a nested xmit could
  set it and the outer xmit consume it for an skb that never went through
  skbedit (Eric Dumazet, nipa Sashiko).
- Own the flag for the whole xmit frame: save/clear it before any of the
  frame's egress work can recurse, and restore it only when the frame exits.
- Retain the v1 producer-side cap (netdev_tx_queue_mapping() writes the
  clamped value back), covering the residual in-frame rewrite nipa
  identified (e.g. a tc BPF store).
- Correct the description of the reproducer to a three-device chain
  (qa 3q -> qb 2q taprio -> qc 1q);

 net/core/dev.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 0292a16e16c2..e72a5c6dc63a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4404,9 +4404,14 @@ EXPORT_SYMBOL(dev_loopback_xmit);
 static struct netdev_queue *
 netdev_tx_queue_mapping(struct net_device *dev, struct sk_buff *skb)
 {
-	int qm = skb_get_queue_mapping(skb);
+	int queue = skb_get_queue_mapping(skb);
+	int capped;
 
-	return netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, qm));
+	capped = netdev_cap_txqueue(dev, queue);
+	if (unlikely(capped != queue))
+		skb_set_queue_mapping(skb, capped);
+
+	return netdev_get_tx_queue(dev, capped);
 }
 
 #ifndef CONFIG_PREEMPT_RT
@@ -4824,6 +4829,9 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
 	int cpu, rc = -ENOMEM;
 	bool again = false;
 	struct Qdisc *q;
+#ifdef CONFIG_NET_EGRESS
+	bool skip_txq;
+#endif
 
 	skb_reset_mac_header(skb);
 	skb_assert_len(skb);
@@ -4847,6 +4855,14 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
 
 	tcx_set_ingress(skb, false);
 #ifdef CONFIG_NET_EGRESS
+	/* The flag is per-CPU and a nested xmit can set it from its own
+	 * clsact hook or transmit qdisc. Own it for the whole frame: this
+	 * frame cannot consume a nested xmit's flag and a nested xmit
+	 * cannot inherit this frame's.
+	 */
+	skip_txq = netdev_xmit_txqueue_skipped();
+	netdev_xmit_skip_txqueue(false);
+
 	if (static_branch_unlikely(&egress_needed_key)) {
 		if (nf_hook_egress_active()) {
 			skb = nf_hook_egress(skb, &rc, dev);
@@ -4854,8 +4870,6 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
 				goto out;
 		}
 
-		netdev_xmit_skip_txqueue(false);
-
 		nf_skip_egress(skb, true);
 		skb = sch_handle_egress(skb, &rc, dev);
 		if (!skb)
@@ -4952,12 +4966,18 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
 
 	reason = SKB_DROP_REASON_RECURSION_LIMIT;
 drop:
+#ifdef CONFIG_NET_EGRESS
+	netdev_xmit_skip_txqueue(skip_txq);
+#endif
 	rcu_read_unlock_bh();
 
 	dev_core_stats_tx_dropped_inc(dev);
 	kfree_skb_list_reason(skb, reason);
 	return rc;
 out:
+#ifdef CONFIG_NET_EGRESS
+	netdev_xmit_skip_txqueue(skip_txq);
+#endif
 	rcu_read_unlock_bh();
 	return rc;
 }
-- 
2.43.0

Re: [PATCH net v2] net: cap skb->queue_mapping when the tx queue is picked

[netdev-bot+sashiko]

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

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

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 does not close the whole tc-BPF queue_mapping case its
  commit message claims to cover.
- [Low] The comment this patch adds in __dev_queue_xmit()
  (net/core/dev.c) says "The flag is per-CPU and a nested xmit can set…

--

net: cap skb->queue_mapping when the tx queue is picked

This patch changes __dev_queue_xmit() to save and clear the skip_txqueue
flag when a transmit frame starts and to restore it when the frame exits.
A nested xmit can then no longer hand the flag to an outer xmit. The patch
also stores the value picked by netdev_cap_txqueue() back into
skb->queue_mapping in netdev_tx_queue_mapping().

Link: https://lore.kernel.org/netdev/CANn89iLwYx8nCVf0pCEk_MmEiyC6kQaMwCQT9WkQVeeNzNQHqQ@mail.gmail.com/
Link: https://lore.kernel.org/netdev/179008581937.2160803.7117814290574262942@kernel.org/

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0292a16e16c2c..e72a5c6dc63af 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4404,9 +4404,14 @@ EXPORT_SYMBOL(dev_loopback_xmit);
>  static struct netdev_queue *
>  netdev_tx_queue_mapping(struct net_device *dev, struct sk_buff *skb)
>  {
> -	int qm = skb_get_queue_mapping(skb);
> +	int queue = skb_get_queue_mapping(skb);
> +	int capped;
>  
> -	return netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, qm));
> +	capped = netdev_cap_txqueue(dev, queue);
> +	if (unlikely(capped != queue))
> +		skb_set_queue_mapping(skb, capped);
> +
> +	return netdev_get_tx_queue(dev, capped);
>  }

[Severity: High]

The commit message says this write-back means "a mapping rewritten later
in the same egress run (for example a tc BPF store) cannot leave an
out-of-range index for the later readers on the xmit path". Is that true
for a BPF store made by a transmit-qdisc classifier?

In __dev_queue_xmit(), this write-back runs after sch_handle_egress() and
before q->enqueue(). The commit message itself notes that transmit-qdisc
classifiers run inside q->enqueue(), which is later in the same frame.

tc_cls_act_is_valid_access() lets any SCHED_CLS/SCHED_ACT program write
__sk_buff->queue_mapping. That includes a cls_bpf filter or act_bpf on a
transmit qdisc such as prio, multiq, htb, or a child of mq or taprio. The
ctx rewrite in net/core/filter.c only rejects values at or above
NO_QUEUE_MAPPING:

    if (BPF_CLASS(si->code) == BPF_ST && si->imm >= NO_QUEUE_MAPPING) {
        *insn++ = BPF_JMP_A(0); /* noop */
        break;
    }

Take a 1-queue device with a prio root and a cls_bpf filter that sets
skb->queue_mapping = 100 and returns TC_ACT_OK:

__dev_queue_xmit()
  netdev_core_pick_tx()            <- mapping capped here
  __dev_xmit_skb()
    dev_qdisc_enqueue()
      prio_enqueue()
        prio_classify()
          tcf_classify_qdisc()     <- BPF sets queue_mapping = 100
    qdisc_run()
      __qdisc_run()
        qdisc_restart()
          skb_get_tx_queue(dev, skb)
            netdev_get_tx_queue()  <- &dev->_tx[100]

The only check in netdev_get_tx_queue() is a debug warning:

    DEBUG_NET_WARN_ON_ONCE(index >= dev->num_tx_queues);
    return &dev->_tx[index];

sch_direct_xmit() then calls HARD_TX_LOCK(), netif_xmit_frozen_or_stopped()
and dev_hard_start_xmit() on that out-of-bounds netdev_queue. Drivers then
index their rings with the same value.

multiq behaves the same way. After classification it checks the band
against q->bands, but skb->queue_mapping stays out of range when
qdisc_restart() reads it.

This path does not use the skip_txqueue flag, so saving, clearing and
restoring the flag does not help here either.

The unchecked read in qdisc_restart() already existed before this patch.
However, the commit message and the v1->v2 notes ("covering the residual
in-frame rewrite nipa identified (e.g. a tc BPF store)") say this case is
fixed.

Would a cap on the consumer side be needed to close it, in qdisc_restart(),
skb_get_tx_queue() or sch_direct_xmit()? A bound on the BPF store could
also work.

If not, could the commit message make a narrower claim, so backporters do
not assume every queue_mapping out-of-bounds case is fixed?

[ ... ]

> @@ -4847,6 +4855,14 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
>  
>  	tcx_set_ingress(skb, false);
>  #ifdef CONFIG_NET_EGRESS
> +	/* The flag is per-CPU and a nested xmit can set it from its own
> +	 * clsact hook or transmit qdisc. Own it for the whole frame: this
> +	 * frame cannot consume a nested xmit's flag and a nested xmit
> +	 * cannot inherit this frame's.
> +	 */

[Severity: Low]

This isn't a bug, but is "per-CPU" accurate on every config? With
CONFIG_PREEMPT_RT, the helpers in the same file store the flag per task:

    static bool netdev_xmit_txqueue_skipped(void)
    {
        return current->net_xmit.skip_txqueue;
    }

Saving and restoring the flag still works on RT, because nested xmits run
in the same task. Could the comment say "per-CPU (per-task on
PREEMPT_RT)"?

> +	skip_txq = netdev_xmit_txqueue_skipped();
> +	netdev_xmit_skip_txqueue(false);
> +

[ ... ]

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

Re: [PATCH net v2] net: cap skb->queue_mapping when the tx queue is picked

[Jakub Kicinski]

On Thu, 24 Sep 2026 07:49:06 -0400 Jamal Hadi Salim wrote:
> +#ifdef CONFIG_NET_EGRESS
> +	netdev_xmit_skip_txqueue(skip_txq);
> +#endif

Can we have netdev_xmit_skip_txqueue() return the previous value,
and also define an empty version of it if !CONFIG_NET_EGRESS
to make the code less ifdefy please?
-- 
pw-bot: cr

Re: [PATCH net v2] net: cap skb->queue_mapping when the tx queue is picked

[Jamal Hadi Salim]

On Fri, Sep 25, 2026 at 7:52 AM netdev-bot+sashiko via Hyper-Yielding
Back-end Review & Insight System <hybris@mojatatu.com> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
>
> 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 does not close the whole tc-BPF queue_mapping case its
>   commit message claims to cover.

Excellent catch (should i call you sashiko or clashiko?);  _but_ it is
not something we introduced or something the patch tries to fix.

1) If we are to resend this fix (likely if we can resolve what Jakub
suggested) then we will narrow down what the commit says so you dont
find a loophole to nitpick on ;->
2) we added a followup to our list of followups to resolve this issue
so we dont forget. A couple of initial approaches we are considering
are:
a) Cap at netdev_get_tx_queue() (or qdisc_restart()  /
skb_get_tx_queue()) index to dev->num_tx_queues - 1, or b) reject the
BPF store bound more tightly.

#a is our favorite so far but when we get to implementation/testing #b
may be simpler.

> - [Low] The comment this patch adds in __dev_queue_xmit()
>   (net/core/dev.c) says "The flag is per-CPU and a nested xmit can set…
>

If we resend we'll fix the comment per your suggestion.

cheers,
jamal

> --
>
> net: cap skb->queue_mapping when the tx queue is picked
>
> This patch changes __dev_queue_xmit() to save and clear the skip_txqueue
> flag when a transmit frame starts and to restore it when the frame exits.
> A nested xmit can then no longer hand the flag to an outer xmit. The patch
> also stores the value picked by netdev_cap_txqueue() back into
> skb->queue_mapping in netdev_tx_queue_mapping().
>
> Link: https://lore.kernel.org/netdev/CANn89iLwYx8nCVf0pCEk_MmEiyC6kQaMwCQT9WkQVeeNzNQHqQ@mail.gmail.com/
> Link: https://lore.kernel.org/netdev/179008581937.2160803.7117814290574262942@kernel.org/
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 0292a16e16c2c..e72a5c6dc63af 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4404,9 +4404,14 @@ EXPORT_SYMBOL(dev_loopback_xmit);
> >  static struct netdev_queue *
> >  netdev_tx_queue_mapping(struct net_device *dev, struct sk_buff *skb)
> >  {
> > -     int qm = skb_get_queue_mapping(skb);
> > +     int queue = skb_get_queue_mapping(skb);
> > +     int capped;
> >
> > -     return netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, qm));
> > +     capped = netdev_cap_txqueue(dev, queue);
> > +     if (unlikely(capped != queue))
> > +             skb_set_queue_mapping(skb, capped);
> > +
> > +     return netdev_get_tx_queue(dev, capped);
> >  }
>
> [Severity: High]
>
> The commit message says this write-back means "a mapping rewritten later
> in the same egress run (for example a tc BPF store) cannot leave an
> out-of-range index for the later readers on the xmit path". Is that true
> for a BPF store made by a transmit-qdisc classifier?
>
> In __dev_queue_xmit(), this write-back runs after sch_handle_egress() and
> before q->enqueue(). The commit message itself notes that transmit-qdisc
> classifiers run inside q->enqueue(), which is later in the same frame.
>
> tc_cls_act_is_valid_access() lets any SCHED_CLS/SCHED_ACT program write
> __sk_buff->queue_mapping. That includes a cls_bpf filter or act_bpf on a
> transmit qdisc such as prio, multiq, htb, or a child of mq or taprio. The
> ctx rewrite in net/core/filter.c only rejects values at or above
> NO_QUEUE_MAPPING:
>
>     if (BPF_CLASS(si->code) == BPF_ST && si->imm >= NO_QUEUE_MAPPING) {
>         *insn++ = BPF_JMP_A(0); /* noop */
>         break;
>     }
>
> Take a 1-queue device with a prio root and a cls_bpf filter that sets
> skb->queue_mapping = 100 and returns TC_ACT_OK:
>
> __dev_queue_xmit()
>   netdev_core_pick_tx()            <- mapping capped here
>   __dev_xmit_skb()
>     dev_qdisc_enqueue()
>       prio_enqueue()
>         prio_classify()
>           tcf_classify_qdisc()     <- BPF sets queue_mapping = 100
>     qdisc_run()
>       __qdisc_run()
>         qdisc_restart()
>           skb_get_tx_queue(dev, skb)
>             netdev_get_tx_queue()  <- &dev->_tx[100]
>
> The only check in netdev_get_tx_queue() is a debug warning:
>
>     DEBUG_NET_WARN_ON_ONCE(index >= dev->num_tx_queues);
>     return &dev->_tx[index];
>
> sch_direct_xmit() then calls HARD_TX_LOCK(), netif_xmit_frozen_or_stopped()
> and dev_hard_start_xmit() on that out-of-bounds netdev_queue. Drivers then
> index their rings with the same value.
>
> multiq behaves the same way. After classification it checks the band
> against q->bands, but skb->queue_mapping stays out of range when
> qdisc_restart() reads it.
>
> This path does not use the skip_txqueue flag, so saving, clearing and
> restoring the flag does not help here either.
>
> The unchecked read in qdisc_restart() already existed before this patch.
> However, the commit message and the v1->v2 notes ("covering the residual
> in-frame rewrite nipa identified (e.g. a tc BPF store)") say this case is
> fixed.
>
> Would a cap on the consumer side be needed to close it, in qdisc_restart(),
> skb_get_tx_queue() or sch_direct_xmit()? A bound on the BPF store could
> also work.
>
> If not, could the commit message make a narrower claim, so backporters do
> not assume every queue_mapping out-of-bounds case is fixed?
>
> [ ... ]
>
> > @@ -4847,6 +4855,14 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
> >
> >       tcx_set_ingress(skb, false);
> >  #ifdef CONFIG_NET_EGRESS
> > +     /* The flag is per-CPU and a nested xmit can set it from its own
> > +      * clsact hook or transmit qdisc. Own it for the whole frame: this
> > +      * frame cannot consume a nested xmit's flag and a nested xmit
> > +      * cannot inherit this frame's.
> > +      */
>
> [Severity: Low]
>
> This isn't a bug, but is "per-CPU" accurate on every config? With
> CONFIG_PREEMPT_RT, the helpers in the same file store the flag per task:
>
>     static bool netdev_xmit_txqueue_skipped(void)
>     {
>         return current->net_xmit.skip_txqueue;
>     }
>
> Saving and restoring the flag still works on RT, because nested xmits run
> in the same task. Could the comment say "per-CPU (per-task on
> PREEMPT_RT)"?
>
> > +     skip_txq = netdev_xmit_txqueue_skipped();
> > +     netdev_xmit_skip_txqueue(false);
> > +
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-9R8V.v2.20260924072708%40mojatatu.com