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

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/ open here

Link https://lore.kernel.org/netdev/[email protected]/ open here

Quoted 9 lines

net/core/dev.c +0 -1

index 0292a16e16c2c..e72a5c6dc63af 100644
@@ -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);

net/core/dev.c EXPORT_SYMBOL(dev_loopback_xmit); · line 4415

Quoted 10 lines

net/core/dev.c +7 -1

+     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?

[ ... ]

net/core/dev.c int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) · line 4862

Quoted 9 lines

net/core/dev.c +5 -0

@@ -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)"?

net/core/dev.c int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) · line 4865

Quoted 3 lines

net/core/dev.c +3 -0

+     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