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

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] Incomplete fix: the patch re-caps a queue_mapping stored by tc
  BPF only at dequeue time, in dequeue_skb(), __skb_dequeue_bad_txq()…
- [Low] For TCQ_F_ONETXQUEUE qdiscs, qdisc_cap_skb_tx_queue() overwrites
  every skb's queue_mapping with q->dev_queue's index, including valid…

--

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

This patch re-caps skb->queue_mapping in the qdisc dequeue path
(dequeue_skb(), __skb_dequeue_bad_txq() and the bulk helpers). The goal is
to stop a tc BPF store made after netdev_core_pick_tx() from indexing past
dev->_tx[]. For TCQ_F_ONETXQUEUE qdiscs the mapping is pinned to
q->dev_queue; for other qdiscs it goes through netdev_cap_txqueue().

Closes: https://lore.kernel.org/netdev/179033713973.2160803.4914570693994398206@kernel.org/ (local)
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://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-MD1X.v3.r0.20261015165943-1%40mojatatu.com
Link: https://lore.kernel.org/netdev/20261015125822.GH83879@horms.kernel.org/ (local)

[Severity: Low]
This isn't a memory safety issue, and the commit message does describe
the pinning. Should the change in steering behaviour still be called out
for stable users?

For TCQ_F_ONETXQUEUE qdiscs, this overwrites every skb's queue_mapping
with the index of q->dev_queue. That includes valid in-range values set
by a filter on a per-queue child, for example in tcf_skbedit_act():

	if (params->flags & SKBEDIT_F_QUEUE_MAPPING &&
	    skb->dev->real_num_tx_queues > params->queue_mapping) {

Before this patch, qdisc_restart() used skb_get_tx_queue() and honoured
an in-range mapping on the head skb. Under mq, mqprio, htb offload or
taprio offload children on multiqueue devices, that steering is now
silently replaced with the child's own queue.

The pre-patch try_bulk_dequeue_skb() already sent followers under the
head's tx lock whatever their mapping was. The TCQ_F_ONETXQUEUE comment
in include/net/sch_generic.h also documents the invariant being restored.

However, Documentation/networking/tc-queue-filters.rst describes
"skbedit queue_mapping $tx_qid" without saying where the filter has to
sit. Would a note there help?

+	else
+		capped = netdev_cap_txqueue(dev, queue);

[ ... ]

[Severity: High]
This caps the mapping at dequeue time. Can the same BPF-stored value
still reach a consumer on the enqueue side that uses it as an array
index?

taprio_enqueue() in net/sched/sch_taprio.c indexes q->qdiscs[], which
has dev->num_tx_queues entries, without a bounds check:

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

	child = q->qdiscs[queue];
	if (unlikely(child == &noop_qdisc))
    ...
}

This path looks reachable with software-mode taprio as the root:

  - taprio_attach() installs the taprio root on every tx queue
  - a prio child is grafted onto a taprio class
  - a cls_bpf filter on that prio stores a large
    __sk_buff->queue_mapping (bpf_convert_ctx_access() only rejects
    values >= NO_QUEUE_MAPPING)
  - a duplicating netem sits on one of the prio bands

netem_enqueue() clones the skb, and __copy_skb_header() copies
queue_mapping. It then re-enqueues the clone at the root:

net/sched/sch_netem.c:netem_enqueue() {
    ...
	if (skb2) {
		struct Qdisc *rootq = qdisc_root_bh(sch);

		skb2->tc_depth++; /* prevent duplicating a dup... */
		rootq->enqueue(skb2, rootq, to_free);
    ...
}

netem inherits its dev_queue from prio, and taprio is grafted on every
tx queue, so rootq is the taprio root. That gives:

netem_enqueue()
  taprio_enqueue()
    child = q->qdiscs[queue]   /* out of bounds */
    taprio_enqueue_one()
      qdisc_enqueue()          /* calls child->enqueue */

All of this runs before dequeue_skb(), so the new cap never sees the
clone.

Commit ea4d4b5dddb5 cited this taprio_enqueue() out-of-bounds access. It
left the "tc BPF program attached to a transmit qdisc" store path for a
separate followup.

Does closing that case need a check on the store or enqueue side, or in
taprio_enqueue() itself?

 		if (qdisc_may_bulk(q))
 			try_bulk_dequeue_skb(q, skb, txq, packets, budget);
 		else

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