Netdev List
 help / color / mirror / Atom feed
* [PATCH net] net: cap skb->queue_mapping when the tx queue is picked
@ 2026-09-21 14:02 Jamal Hadi Salim
  2026-09-21 14:42 ` Eric Dumazet
  2026-09-22 14:03 ` netdev-bot+sashiko
  0 siblings, 2 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-21 14:02 UTC (permalink / raw)
  To: netdev
  Cc: Jamal Hadi Salim, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Jiri Pirko, Vinicius Costa Gomes, Simon Horman,
	Victor Nogueira, Tonghao Zhang, Zero Day Initiative, hybris,
	stable

skbedit can set skb->queue_mapping and __dev_queue_xmit() honors it
through the skip_txqueue flag; netdev_tx_queue_mapping() clamps the
index it uses to select the netdev_queue but leaves the out-of-range
value in skb->queue_mapping.

Every later consumer of skb_get_queue_mapping()/skb_get_tx_queue() on
that path then reads past the device's queues. Taprio's child array
q->qdiscs[] is sized to the device's queue count, so taprio_enqueue()
indexes past its allocation; qdisc_restart() likewise dereferences
dev->_tx[queue_mapping].

A local user in a network namespace can redirect a packet from a device
with more TX queues to one with fewer (mirred action) 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".

Store the clamped value back into skb->queue_mapping, as
netdev_core_pick_tx() already does for the mapping it picks, so the
whole egress path observes an in-range queue index.

I have looked at other alternative places to put this "fix", none
appealing: an out-of-range queue_mapping is read by every consumer
on the xmit path, not just by taprio. For example, upon testing
an approach that only bounds-checked taprio_enqueue() I observed
the fault relocated to sch_direct_xmit()/qdisc_restart() instead
(because dev->_tx[queue_mapping] is still indexed with the raw value).
Another approach was to cap it in skbedit;  cannot work: the
redirect target, whose queue count bounds the mapping, is not known
when the action runs, and act_mirred sets skb->dev afterwards.
So the decision is to cap the value where it is first trusted and
result is it fixes all downstream readers at once.

Conditions to recreate the bug: with CONFIG_NET_SCH_TAPRIO=y,
CONFIG_NET_ACT_SKBEDIT=y, CONFIG_NET_ACT_MIRRED=y and KASAN enabled,
create a 3-queue dummy qa and a 2-queue dummy qb, put a taprio root on
qb, then on qa's clsact add matchall with "action skbedit queue_mapping
2 pipe action mirred egress redirect dev qb" and send one packet out
qa. Mapping 2 is valid for qa but past qb's two-entry taprio child
array.

Reproduction: reproducer ran on a KASAN build with panic_on_warn=1:
the unfixed control faults with "BUG: KASAN: slab-out-of-bounds in
taprio_enqueue", a read 0 bytes past a 16-byte taprio_init allocation,
and panics.
The fixed kernel runs the same reproducer without a report, only the
expected ratelimited "qb selects TX queue 2, but real number of TX
queues is 2" notice.

Fixes: 2f1e85b1aee4 ("net: sched: use queue_mapping to pick tx queue")
Reported-by: Zero Day Initiative <zdi-disclosures@trendmicro.com>
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/core/dev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index c67900354fa6..736b3664b635 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
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: cap skb->queue_mapping when the tx queue is picked
  2026-09-21 14:02 [PATCH net] net: cap skb->queue_mapping when the tx queue is picked Jamal Hadi Salim
@ 2026-09-21 14:42 ` Eric Dumazet
  2026-09-21 19:57   ` Jamal Hadi Salim
  2026-09-22 14:03 ` netdev-bot+sashiko
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2026-09-21 14:42 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: netdev, David S. Miller, Jakub Kicinski, Paolo Abeni, Jiri Pirko,
	Vinicius Costa Gomes, Simon Horman, Victor Nogueira,
	Tonghao Zhang, Zero Day Initiative, hybris, stable

On Mon, Sep 21, 2026 at 4:02 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> skbedit can set skb->queue_mapping and __dev_queue_xmit() honors it
> through the skip_txqueue flag; netdev_tx_queue_mapping() clamps the
> index it uses to select the netdev_queue but leaves the out-of-range
> value in skb->queue_mapping.
>
> Every later consumer of skb_get_queue_mapping()/skb_get_tx_queue() on
> that path then reads past the device's queues. Taprio's child array
> q->qdiscs[] is sized to the device's queue count, so taprio_enqueue()
> indexes past its allocation; qdisc_restart() likewise dereferences
> dev->_tx[queue_mapping].
>
> A local user in a network namespace can redirect a packet from a device
> with more TX queues to one with fewer (mirred action) 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".
>
> Store the clamped value back into skb->queue_mapping, as
> netdev_core_pick_tx() already does for the mapping it picks, so the
> whole egress path observes an in-range queue index.
>
> I have looked at other alternative places to put this "fix", none
> appealing: an out-of-range queue_mapping is read by every consumer
> on the xmit path, not just by taprio. For example, upon testing
> an approach that only bounds-checked taprio_enqueue() I observed
> the fault relocated to sch_direct_xmit()/qdisc_restart() instead
> (because dev->_tx[queue_mapping] is still indexed with the raw value).
> Another approach was to cap it in skbedit;  cannot work: the
> redirect target, whose queue count bounds the mapping, is not known
> when the action runs, and act_mirred sets skb->dev afterwards.
> So the decision is to cap the value where it is first trusted and
> result is it fixes all downstream readers at once.
>
> Conditions to recreate the bug: with CONFIG_NET_SCH_TAPRIO=y,
> CONFIG_NET_ACT_SKBEDIT=y, CONFIG_NET_ACT_MIRRED=y and KASAN enabled,
> create a 3-queue dummy qa and a 2-queue dummy qb, put a taprio root on
> qb, then on qa's clsact add matchall with "action skbedit queue_mapping
> 2 pipe action mirred egress redirect dev qb" and send one packet out
> qa. Mapping 2 is valid for qa but past qb's two-entry taprio child
> array.
>
> Reproduction: reproducer ran on a KASAN build with panic_on_warn=1:
> the unfixed control faults with "BUG: KASAN: slab-out-of-bounds in
> taprio_enqueue", a read 0 bytes past a 16-byte taprio_init allocation,
> and panics.

I do not think this reproducer works. mirred egress redirect re-enters
__dev_queue_xmit() for qb, which does netdev_xmit_skip_txqueue(false)
before sch_handle_egress(). qb has no egress filter here, so
netdev_tx_queue_mapping() is not called for qb at all, and
netdev_core_pick_tx() rewrites skb->queue_mapping in range before
taprio_enqueue() runs.

I suspect the real issue is that the per-cpu flag is cleared *before*
sch_handle_egress(), so a nested dev_queue_xmit() (mirred mirror, or a
drop after skbedit) can set it and have the *outer* __dev_queue_xmit()
consume it, for an skb that never went through skbedit. For a forwarded
packet, skb->queue_mapping is then still skb_record_rx_queue()'s
rx_queue + 1 from the ingress NIC. That reaches taprio with a garbage
index, and clamping it only hides the fact that an innocent packet got
pinned to a random TX queue.

If that is confirmed, I would rather consume the flag where it is read:

    skip_txq = netdev_xmit_txqueue_skipped();
    netdev_xmit_skip_txqueue(false);

right after sch_handle_egress(), dropping the pre-clear. Same number of
stores, and the request can no longer escape the xmit that set it.

Also, taprio_enqueue() is the only qdisc indexing a num_tx_queues sized
array with skb->queue_mapping. A bounds check there is free and worth
having regardless.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: cap skb->queue_mapping when the tx queue is picked
  2026-09-21 14:42 ` Eric Dumazet
@ 2026-09-21 19:57   ` Jamal Hadi Salim
  0 siblings, 0 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-21 19:57 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, David S. Miller, Jakub Kicinski, Paolo Abeni, Jiri Pirko,
	Vinicius Costa Gomes, Simon Horman, Victor Nogueira,
	Tonghao Zhang, Zero Day Initiative, hybris, stable

On Mon, Sep 21, 2026 at 10:42 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Sep 21, 2026 at 4:02 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > skbedit can set skb->queue_mapping and __dev_queue_xmit() honors it
> > through the skip_txqueue flag; netdev_tx_queue_mapping() clamps the
> > index it uses to select the netdev_queue but leaves the out-of-range
> > value in skb->queue_mapping.
> >
> > Every later consumer of skb_get_queue_mapping()/skb_get_tx_queue() on
> > that path then reads past the device's queues. Taprio's child array
> > q->qdiscs[] is sized to the device's queue count, so taprio_enqueue()
> > indexes past its allocation; qdisc_restart() likewise dereferences
> > dev->_tx[queue_mapping].
> >
> > A local user in a network namespace can redirect a packet from a device
> > with more TX queues to one with fewer (mirred action) 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".
> >
> > Store the clamped value back into skb->queue_mapping, as
> > netdev_core_pick_tx() already does for the mapping it picks, so the
> > whole egress path observes an in-range queue index.
> >
> > I have looked at other alternative places to put this "fix", none
> > appealing: an out-of-range queue_mapping is read by every consumer
> > on the xmit path, not just by taprio. For example, upon testing
> > an approach that only bounds-checked taprio_enqueue() I observed
> > the fault relocated to sch_direct_xmit()/qdisc_restart() instead
> > (because dev->_tx[queue_mapping] is still indexed with the raw value).
> > Another approach was to cap it in skbedit;  cannot work: the
> > redirect target, whose queue count bounds the mapping, is not known
> > when the action runs, and act_mirred sets skb->dev afterwards.
> > So the decision is to cap the value where it is first trusted and
> > result is it fixes all downstream readers at once.
> >
> > Conditions to recreate the bug: with CONFIG_NET_SCH_TAPRIO=y,
> > CONFIG_NET_ACT_SKBEDIT=y, CONFIG_NET_ACT_MIRRED=y and KASAN enabled,
> > create a 3-queue dummy qa and a 2-queue dummy qb, put a taprio root on
> > qb, then on qa's clsact add matchall with "action skbedit queue_mapping
> > 2 pipe action mirred egress redirect dev qb" and send one packet out
> > qa. Mapping 2 is valid for qa but past qb's two-entry taprio child
> > array.
> >
> > Reproduction: reproducer ran on a KASAN build with panic_on_warn=1:
> > the unfixed control faults with "BUG: KASAN: slab-out-of-bounds in
> > taprio_enqueue", a read 0 bytes past a 16-byte taprio_init allocation,
> > and panics.
>
> I do not think this reproducer works. mirred egress redirect re-enters
> __dev_queue_xmit() for qb, which does netdev_xmit_skip_txqueue(false)
> before sch_handle_egress(). qb has no egress filter here, so
> netdev_tx_queue_mapping() is not called for qb at all, and
> netdev_core_pick_tx() rewrites skb->queue_mapping in range before
> taprio_enqueue() runs.
>

The repro from zdi works (and produced the kasan) - but i think you
caught a bad description on how to reproduce.
It reads like it is two devices but it is infact 3:

  qa (3 queues) - clsact egress matchall:
          action skbedit queue_mapping 2 pipe
           action mirred egress redirect dev qb
  qb (2 queues, taprio root) - clsact egress matchall:
       action mirred egress mirror dev qc
  qc (1 queue) - clsact egress matchall:
       action skbedit queue_mapping 0 pipe

The KASAN trace shows exactly that stack (tcf_mirred_act ->
tcf_skbedit_act on qc, then taprio_enqueue on the outer qb xmit).

At a minimum, the commit log needs fixing to describe as above:

> I suspect the real issue is that the per-cpu flag is cleared *before*
> sch_handle_egress(), so a nested dev_queue_xmit() (mirred mirror, or a
> drop after skbedit) can set it and have the *outer* __dev_queue_xmit()
> consume it, for an skb that never went through skbedit.

That could be the root cause or part of it....

> For a forwarded
> packet, skb->queue_mapping is then still skb_record_rx_queue()'s
> rx_queue + 1 from the ingress NIC. That reaches taprio with a garbage
> index, and clamping it only hides the fact that an innocent packet got
> pinned to a random TX queue.
>
> If that is confirmed, I would rather consume the flag where it is read:
>
>     skip_txq = netdev_xmit_txqueue_skipped();
>     netdev_xmit_skip_txqueue(false);
>
> right after sch_handle_egress(), dropping the pre-clear. Same number of
> stores, and the request can no longer escape the xmit that set it.
>

I believe your suggestion would fix the poc repro but opens other issues.

How about: save it, clear it before sch_handle_egress(), and restore
it on every exit? Something like:

        skip_txq = netdev_xmit_txqueue_skipped();
        netdev_xmit_skip_txqueue(false);
        ...
        skb = sch_handle_egress(skb, &rc, dev);
        if (!skb) {
                netdev_xmit_skip_txqueue(skip_txq);
                goto out;
        }
        ...
        if (netdev_xmit_txqueue_skipped())
                txq = netdev_tx_queue_mapping(dev, skb);
        netdev_xmit_skip_txqueue(skip_txq);


> Also, taprio_enqueue() is the only qdisc indexing a num_tx_queues sized
> array with skb->queue_mapping. A bounds check there is free and worth
> having regardless.

Likely missing something: how will taprio code be hit if we cover the
egress direction hole above?

cheers,
jamal

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: cap skb->queue_mapping when the tx queue is picked
  2026-09-21 14:02 [PATCH net] net: cap skb->queue_mapping when the tx queue is picked Jamal Hadi Salim
  2026-09-21 14:42 ` Eric Dumazet
@ 2026-09-22 14:03 ` netdev-bot+sashiko
  2026-09-22 21:33   ` Jamal Hadi Salim
  1 sibling, 1 reply; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-09-22 14:03 UTC (permalink / raw)
  To: jhs
  Cc: netdev, davem, edumazet, kuba, pabeni, jiri, vinicius.gomes,
	horms, victor, xiangxia.m.yue, zdi-disclosures, hybris, stable

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 commit message's justification and reproducer cannot reach
  the code the patch changes, so the patch's stated bug (and its…

--

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

This stores the value returned by netdev_cap_txqueue() back into
skb->queue_mapping in netdev_tx_queue_mapping(), so that later readers of
skb_get_queue_mapping() on the xmit path see an in-range index. The
changelog attributes the out-of-range mapping to an skbedit + mirred
egress redirect from a device with more TX queues to one with fewer.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index c67900354fa64..736b3664b6354 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]
Can the reproducer in the changelog actually reach this function?

netdev_tx_queue_mapping() has a single caller, and it is gated on the
per-CPU (or per-task, on PREEMPT_RT) skip_txqueue flag:

net/core/dev.c:__dev_queue_xmit() {
	...
	if (static_branch_unlikely(&egress_needed_key)) {
		...
		netdev_xmit_skip_txqueue(false);

		nf_skip_egress(skb, true);
		skb = sch_handle_egress(skb, &rc, dev);
		if (!skb)
			goto out;
		nf_skip_egress(skb, false);

		if (netdev_xmit_txqueue_skipped())
			txq = netdev_tx_queue_mapping(dev, skb);
	}
	...
}

The clear at netdev_xmit_skip_txqueue(false) runs unconditionally at the
top of that block, in every egress-enabled frame.

The described redirect enters the target device through a nested
__dev_queue_xmit():

tcf_mirred_forward()
  tcf_dev_queue_xmit(skb, dev_queue_xmit)
    dev_queue_xmit()
      __dev_queue_xmit()   /* for qb */

Since clsact on qa has already raised egress_needed_key, that nested
frame for qb takes the same block and clears the flag that skbedit set
while running on qa. netdev_xmit_txqueue_skipped() is then false, txq
stays NULL, and qb's queue comes from netdev_core_pick_tx(), which caps
and rewrites the mapping itself:

net/core/dev.c:netdev_core_pick_tx() {
	...
		queue_index = netdev_cap_txqueue(dev, queue_index);
	}

	skb_set_queue_mapping(skb, queue_index);
	return netdev_get_tx_queue(dev, queue_index);
}

So by the time taprio_enqueue() runs:

net/sched/sch_taprio.c:taprio_enqueue() {
	queue = skb_get_queue_mapping(skb);

	child = q->qdiscs[queue];

the index it reads looks already in range for qb, on the patched and the
unpatched kernel alike. Does the quoted "BUG: KASAN: slab-out-of-bounds
in taprio_enqueue", the read 0 bytes past the 16-byte taprio_init
allocation, really come from this skbedit + mirred sequence?

A second point on the rationale:

    So the decision is to cap the value where it is first trusted and
    result is it fixes all downstream readers at once.

Is this site where the value is first trusted? The only in-tree setter of
the flag validates against the same device before arming it, and stores an
already capped value:

net/sched/act_skbedit.c:tcf_skbedit_act() {
	...
	if (params->flags & SKBEDIT_F_QUEUE_MAPPING &&
	    skb->dev->real_num_tx_queues > params->queue_mapping) {
#ifdef CONFIG_NET_EGRESS
		netdev_xmit_skip_txqueue(true);
#endif
		skb_set_queue_mapping(skb, tcf_skbedit_hash(params, skb));
	}

and tcf_skbedit_hash() ends with:

	return netdev_cap_txqueue(skb->dev, queue_mapping);

With that, capped != queue in the new code seems to require either
real_num_tx_queues shrinking concurrently (the index still being below
dev->num_tx_queues, so still inside dev->_tx[] and taprio's q->qdiscs[]),
or something rewriting skb->queue_mapping later in the same egress run.

A path that does look able to deliver a genuinely out-of-range value to
the new statement is a tc BPF program writing skb->queue_mapping after
skbedit armed the flag on the same device, since
tc_cls_act_is_valid_access() permits BPF_WRITE of that field and
bpf_convert_ctx_access() emits the store. Would describing that (or a
similar in-frame rewrite) instead of the cross-device mirred path make the
Fixes: tag and the reported scenario verifiable for reviewers and stable
backporters?

Also, this isn't a bug, but the changelog says:

    Another approach was to cap it in skbedit;  cannot work: the
    redirect target, whose queue count bounds the mapping, is not known
    when the action runs, and act_mirred sets skb->dev afterwards.

there is a double space after the semicolon.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: cap skb->queue_mapping when the tx queue is picked
  2026-09-22 14:03 ` netdev-bot+sashiko
@ 2026-09-22 21:33   ` Jamal Hadi Salim
  2026-09-24  8:06     ` Jamal Hadi Salim
  0 siblings, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-22 21:33 UTC (permalink / raw)
  To: netdev-bot+sashiko
  Cc: netdev, davem, edumazet, kuba, pabeni, jiri, vinicius.gomes,
	horms, victor, xiangxia.m.yue, zdi-disclosures, hybris, stable

On Tue, Sep 22, 2026 at 10:03 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> 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 commit message's justification and reproducer cannot reach
>   the code the patch changes, so the patch's stated bug (and its…
>
> --
>
> net: cap skb->queue_mapping when the tx queue is picked
>
> This stores the value returned by netdev_cap_txqueue() back into
> skb->queue_mapping in netdev_tx_queue_mapping(), so that later readers of
> skb_get_queue_mapping() on the xmit path see an in-range index. The
> changelog attributes the out-of-range mapping to an skbedit + mirred
> egress redirect from a device with more TX queues to one with fewer.
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index c67900354fa64..736b3664b6354 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]
> Can the reproducer in the changelog actually reach this function?
>

This is the same comment as Eric made. See my response in this thread:
https://lore.kernel.org/netdev/QDISC-9R8V.v1.20260921065103@mojatatu.com/

I will be sending a V2 with the save-clear-restore proposal I made on
that thread.

> netdev_tx_queue_mapping() has a single caller, and it is gated on the
> per-CPU (or per-task, on PREEMPT_RT) skip_txqueue flag:
>
> net/core/dev.c:__dev_queue_xmit() {
>         ...
>         if (static_branch_unlikely(&egress_needed_key)) {
>                 ...
>                 netdev_xmit_skip_txqueue(false);
>
>                 nf_skip_egress(skb, true);
>                 skb = sch_handle_egress(skb, &rc, dev);
>                 if (!skb)
>                         goto out;
>                 nf_skip_egress(skb, false);
>
>                 if (netdev_xmit_txqueue_skipped())
>                         txq = netdev_tx_queue_mapping(dev, skb);
>         }
>         ...
> }
>
> The clear at netdev_xmit_skip_txqueue(false) runs unconditionally at the
> top of that block, in every egress-enabled frame.
>
> The described redirect enters the target device through a nested
> __dev_queue_xmit():
>
> tcf_mirred_forward()
>   tcf_dev_queue_xmit(skb, dev_queue_xmit)
>     dev_queue_xmit()
>       __dev_queue_xmit()   /* for qb */
>
> Since clsact on qa has already raised egress_needed_key, that nested
> frame for qb takes the same block and clears the flag that skbedit set
> while running on qa. netdev_xmit_txqueue_skipped() is then false, txq
> stays NULL, and qb's queue comes from netdev_core_pick_tx(), which caps
> and rewrites the mapping itself:
>
> net/core/dev.c:netdev_core_pick_tx() {
>         ...
>                 queue_index = netdev_cap_txqueue(dev, queue_index);
>         }
>
>         skb_set_queue_mapping(skb, queue_index);
>         return netdev_get_tx_queue(dev, queue_index);
> }
>
> So by the time taprio_enqueue() runs:
>
> net/sched/sch_taprio.c:taprio_enqueue() {
>         queue = skb_get_queue_mapping(skb);
>
>         child = q->qdiscs[queue];
>
> the index it reads looks already in range for qb, on the patched and the
> unpatched kernel alike. Does the quoted "BUG: KASAN: slab-out-of-bounds
> in taprio_enqueue", the read 0 bytes past the 16-byte taprio_init
> allocation, really come from this skbedit + mirred sequence?
>
> A second point on the rationale:
>
>     So the decision is to cap the value where it is first trusted and
>     result is it fixes all downstream readers at once.
>
> Is this site where the value is first trusted? The only in-tree setter of
> the flag validates against the same device before arming it, and stores an
> already capped value:
>
> net/sched/act_skbedit.c:tcf_skbedit_act() {
>         ...
>         if (params->flags & SKBEDIT_F_QUEUE_MAPPING &&
>             skb->dev->real_num_tx_queues > params->queue_mapping) {
> #ifdef CONFIG_NET_EGRESS
>                 netdev_xmit_skip_txqueue(true);
> #endif
>                 skb_set_queue_mapping(skb, tcf_skbedit_hash(params, skb));
>         }
>
> and tcf_skbedit_hash() ends with:
>
>         return netdev_cap_txqueue(skb->dev, queue_mapping);
>
> With that, capped != queue in the new code seems to require either
> real_num_tx_queues shrinking concurrently (the index still being below
> dev->num_tx_queues, so still inside dev->_tx[] and taprio's q->qdiscs[]),
> or something rewriting skb->queue_mapping later in the same egress run.
>
> A path that does look able to deliver a genuinely out-of-range value to
> the new statement is a tc BPF program writing skb->queue_mapping after
> skbedit armed the flag on the same device, since
> tc_cls_act_is_valid_access() permits BPF_WRITE of that field and
> bpf_convert_ctx_access() emits the store. Would describing that (or a
> similar in-frame rewrite) instead of the cross-device mirred path make the
> Fixes: tag and the reported scenario verifiable for reviewers and stable
> backporters?
>
> Also, this isn't a bug, but the changelog says:
>
>     Another approach was to cap it in skbedit;  cannot work: the
>     redirect target, whose queue count bounds the mapping, is not known
>     when the action runs, and act_mirred sets skb->dev afterwards.
>
> there is a double space after the semicolon.

Sheesh - bikeshed much?

cheers,
jamal
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-9R8V.v1.20260921065103%40mojatatu.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: cap skb->queue_mapping when the tx queue is picked
  2026-09-22 21:33   ` Jamal Hadi Salim
@ 2026-09-24  8:06     ` Jamal Hadi Salim
  0 siblings, 0 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2026-09-24  8:06 UTC (permalink / raw)
  To: netdev-bot+sashiko
  Cc: netdev, davem, edumazet, kuba, pabeni, jiri, vinicius.gomes,
	horms, victor, xiangxia.m.yue, zdi-disclosures, hybris, stable

On Tue, Sep 22, 2026 at 5:33 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Tue, Sep 22, 2026 at 10:03 AM <netdev-bot+sashiko@kernel.org> wrote:
> >

> >
> > Also, this isn't a bug, but the changelog says:
> >
> >     Another approach was to cap it in skbedit;  cannot work: the
> >     redirect target, whose queue count bounds the mapping, is not known
> >     when the action runs, and act_mirred sets skb->dev afterwards.
> >
> > there is a double space after the semicolon.
>
> Sheesh - bikeshed much?
>

pw-bot: cr

cheers,
jamal

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-24  8:06 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-21 14:02 [PATCH net] net: cap skb->queue_mapping when the tx queue is picked Jamal Hadi Salim
2026-09-21 14:42 ` Eric Dumazet
2026-09-21 19:57   ` Jamal Hadi Salim
2026-09-22 14:03 ` netdev-bot+sashiko
2026-09-22 21:33   ` Jamal Hadi Salim
2026-09-24  8:06     ` Jamal Hadi Salim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox