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

On Thu, Oct 29, 2026 at 2:16 PM [off-list ref] wrote:

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

Critical: 0 · High: 0 · Medium: 1 · 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] The patch silently changes behaviour on stable kernels, and
  its commit message and docs do not describe the change accurately.

sigh. Exhausting. I will send v6 with the commit message fixed.

cheers,
jamal

--

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

A tc BPF store can leave skb->queue_mapping out of range. This patch caps
it in the qdisc dequeue paths in sch_generic.c and in taprio_enqueue(), so
it can no longer index dev->_tx[] or q->qdiscs[] out of bounds. On a
TCQ_F_ONETXQUEUE qdisc the mapping is pinned to the qdisc's own
q->dev_queue instead of being capped to queue 0.

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

quoted

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 6f6a6f0d5eb0..5dc1d47f811a 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c

@@ -96,9 +96,34 @@ static void qdisc_maybe_clear_missed(struct Qdisc *q,

 #define SKB_XOFF_MAGIC ((struct sk_buff *)1UL)

+/* Re-cap a stale queue_mapping; an ONETXQUEUE qdisc pins q->dev_queue. */
+static u16 qdisc_cap_skb_tx_queue(struct Qdisc *q, struct sk_buff *skb)
+{
+     u16 queue = skb_get_queue_mapping(skb);
+     struct net_device *dev = qdisc_dev(q);
+     u16 capped;
+
+     if (q->flags & TCQ_F_ONETXQUEUE)
+             capped = q->dev_queue - netdev_get_tx_queue(dev, 0);
+     else
+             capped = netdev_cap_txqueue(dev, queue);

[Severity: Medium]

On a TCQ_F_ONETXQUEUE qdisc, this branch rewrites the mapping of every skb,
not just the out-of-range ones. Is that intended for in-range mappings that
a classifier sets on purpose?

The rewrite happens in several places. dequeue_skb() applies it to the head
skb and to gso_skb. try_bulk_dequeue_skb() applies it to each follower.
__skb_dequeue_bad_txq() applies it to skbs taken from skb_bad_txq.

Here is one way to hit it. Take a per-queue child of mq, mqprio, taprio or
htb offload whose q->dev_queue is txq i. A filter on that child can run
"action skbedit queue_mapping j" with j != i and j < real_num_tx_queues.
tcf_skbedit_act() accepts that 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) {
                if (skb_at_tc_egress(skb))
                        netdev_xmit_skip_txqueue(true);
                skb_set_queue_mapping(skb, tcf_skbedit_hash(params, skb));
        }
    ...
}

A tc BPF store of an in-range value behaves the same way.

Before this patch, qdisc_restart() sent the packet on
skb_get_tx_queue(dev, skb), which is queue j. With this patch,
dequeue_skb()->qdisc_cap_skb_tx_queue() first rewrites j back to i. The
packet then goes out on queue i, and nothing is logged.

The commit message says:

    netdev_cap_txqueue() already logs a ratelimited notice and selects
    queue 0.

The TCQ_F_ONETXQUEUE branch never calls netdev_cap_txqueue(), though. That
also covers the 1-queue reproducer in the commit message, because
qdisc_create() sets the flag on every non-multiqueue device:

net/sched/sch_api.c:qdisc_create() {
    ...
                if (!netif_is_multiqueue(dev))
                        sch->flags |= TCQ_F_ONETXQUEUE;
    ...
}

So in that reproducer, a store of 1 from the filter on the prio root is
quietly pinned to 0, with no notice.

The only place that says in-range mappings from a filter on a per-queue
child are now overridden is the v4 -> v5 note below the "---". git am drops
that part, and the patch is tagged for stable.

Documentation/networking/tc-queue-filters.rst also still says:

    2) TC filter directs traffic to a transmit queue with the action
       skbedit queue_mapping $tx_qid.

It does not limit where the filter can be attached.

Could the commit message describe the pinning of in-range mappings, and the
missing log message on the TCQ_F_ONETXQUEUE path? Should the documentation
be updated to match?

quoted

+
+     if (unlikely(capped != queue))
+             skb_set_queue_mapping(skb, capped);
+
+     return capped;
+}

[ ... ]

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