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

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.

[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