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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 2 · 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).

- [Medium] skb_at_tc_egress() treats skb->nf_skip_egress &&
  !tc_at_ingress as meaning "inside sch_handle_egress()".
- [Medium] The fix keeps skip_txqueue inside one xmit frame only if every
  nested __dev_queue_xmit() takes the egress_needed_key branch.
- [Low] After this patch, skb->nf_skip_egress also gates tc behaviour:
  skb_at_tc_egress() decides whether tcf_skbedit_act() arms skip_txqueue.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is high
  severity.

--

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

This patch keeps the per-CPU skip_txqueue flag inside the xmit frame that
runs sch_handle_egress(). It saves the flag before the hook and restores
it afterwards. tcf_skbedit_act() may now arm the flag only when
skb_at_tc_egress() reports tc egress. The patch also writes the capped
value back into skb->queue_mapping in netdev_tx_queue_mapping(), and
builds skb->nf_skip_egress under CONFIG_NET_EGRESS.

Link: https://lore.kernel.org/netdev/CANn89iLwYx8nCVf0pCEk_MmEiyC6kQaMwCQT9WkQVeeNzNQHqQ@mail.gmail.com/
Link: https://lore.kernel.org/netdev/179008581937.2160803.7117814290574262942@kernel.org/
Link: https://lore.kernel.org/netdev/179033713973.2160803.4914570693994398206@kernel.org/
Link: https://lore.kernel.org/netdev/20260925180407.63647514@kernel.org/
Link: https://lore.kernel.org/netdev/CANn89i+k-mZKDQVtvws_MEXeuMTAdaCcOXFZE-RfhcGTu90sjA@mail.gmail.com/

[ ... ]

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 84308498a3a8..a3ff380d43f0 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1020,7 +1020,7 @@ struct sk_buff {
>  #ifdef CONFIG_NET_REDIRECT
>  	__u8			from_ingress:1;
>  #endif
> -#ifdef CONFIG_NETFILTER_SKIP_EGRESS
> +#ifdef CONFIG_NET_EGRESS
>  	__u8			nf_skip_egress:1;
>  #endif

[Severity: Low]
This isn't a bug, but skb_at_tc_egress() now reads this bit to gate
tcf_skbedit_act(), and the documentation still describes a flag used only
by netfilter.

The struct sk_buff kernel-doc says:

 *	@nf_skip_egress: packet shall skip nf egress - see netfilter_netdev.h

The comment above nf_hook_egress() in include/linux/netfilter_netdev.h
says:

 * The nf_skip_egress flag controls whether netfilter is applied on egress.
 * It is updated by __netif_receive_skb_core() and __dev_queue_xmit() when the

Neither comment mentions the tc egress meaning. The second one also leaves
out ifb_ri_tasklet() and netkit_prep_forward(), which write the bit too.
The new skb_at_tc_egress() has no comment about reusing this bit or about
the precondition it depends on.

Could these comments be updated along with this change?

>  #ifdef CONFIG_SKB_DECRYPTED
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index f35bd06a6bad..1acaadb3e2cf 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -810,6 +810,15 @@ static inline bool skb_at_tc_ingress(const struct sk_buff *skb)
>  #endif
>  }
>  
> +static inline bool skb_at_tc_egress(const struct sk_buff *skb)
> +{
> +#ifdef CONFIG_NET_EGRESS
> +	return skb->nf_skip_egress && !skb_at_tc_ingress(skb);

[Severity: Medium]
Is nf_skip_egress only ever set around sch_handle_egress()? The commit
message says:

  "the marker is set only around the single sch_handle_egress() call and
  ingress path is guarded by tc_at_ingress"

Some other writers seem to leave the bit set on skbs that later reach
__dev_queue_xmit().

__netif_receive_skb_core() sets the bit before sch_handle_ingress(), but
clears it only on the path where the skb survives:

		nf_skip_egress(skb, true);
		skb = sch_handle_ingress(skb, &pt_prev, &ret, orig_dev,
					 &another);
		...
		if (!skb)
			goto out;

		nf_skip_egress(skb, false);

An ingress mirred redirect consumes the skb and passes it to
dev_queue_xmit() with the bit still set. A mirror does the same with a
clone, which copies the bit through the headers group.

ifb_ri_tasklet() sets the bit and then calls dev_queue_xmit(). On the
from_ingress path it reinjects the skb with tc_skip_classify set, so
__netif_receive_skb_core() never resets the bit:

		skb->tc_skip_classify = 1;
		...
		nf_skip_egress(skb, true);

netkit_prep_forward() also sets the bit. After that, netkit_xmit() calls
skb_do_redirect(), which can reach dev_queue_xmit().

In __dev_queue_xmit(), tcx_set_ingress(skb, false) always runs. The only
reset of nf_skip_egress is inside the egress_needed_key static branch.

Suppose only an ingress clsact is installed, for example one doing
"mirred egress redirect dev qc". The skb then reaches qc's root qdisc
enqueue with nf_skip_egress=1 and tc_at_ingress=0. A skbedit queue_mapping
action in a prio, multiq or htb classifier passes skb_at_tc_egress() and
calls netdev_xmit_skip_txqueue(true). The v4 changelog says this kind of
arming from a transmit-qdisc classifier no longer happens.

Nothing seems to consume this stale value today. Every reader inside the
static branch saves, clears and restores the flag. The one exception is
the static key race described further down.

There is also a config effect. nf_skip_egress() is now built under
CONFIG_NET_EGRESS instead of CONFIG_NETFILTER_SKIP_EGRESS. With
CONFIG_NET_EGRESS=y and CONFIG_NETFILTER_EGRESS=n, the ingress, ifb and
netkit calls used to compile to nothing. Now they write a bit that
skb_at_tc_egress() reads.

Should the reasoning in the commit message be revisited? Otherwise the
planned rename to skb->skip_egress would carry the same assumption
forward.

> +#else
> +	return false;
> +#endif
> +}

[ ... ]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index f660fccfc0db..8638c994bbcb 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]
This is a pre-existing issue and was not introduced by this patch. The
commit message leaves it for a separate followup. Can a cls_bpf program on
a transmit qdisc still index past the end of dev->_tx[]?

tc_cls_act_is_valid_access() allows writes to __sk_buff queue_mapping.
bpf_convert_ctx_access() only drops stores of NO_QUEUE_MAPPING or larger:

net/core/filter.c:bpf_convert_ctx_access() {
    ...
			if (BPF_CLASS(si->code) == BPF_ST && si->imm >= NO_QUEUE_MAPPING) {
				*insn++ = BPF_JMP_A(0); /* noop */
				break;
			}

			if (BPF_CLASS(si->code) == BPF_STX)
				*insn++ = BPF_JMP_IMM(BPF_JGE, si->src_reg, NO_QUEUE_MAPPING, 1);
			*insn++ = BPF_EMIT_STORE(BPF_H, si, offset);
    ...
}

The stored value is never compared with dev->num_tx_queues or
real_num_tx_queues. A program attached under a prio or htb root runs
inside q->enqueue(). That is after netdev_core_pick_tx() or
netdev_tx_queue_mapping() has already capped the mapping, so the program
can store any value up to 0xfffe.

On dequeue:

qdisc_restart()
  txq = skb_get_tx_queue(dev, skb)
    netdev_get_tx_queue()
      DEBUG_NET_WARN_ON_ONCE(index >= dev->num_tx_queues);
      return &dev->_tx[index];
  sch_direct_xmit(skb, q, dev, txq, ...)
    HARD_TX_LOCK(dev, txq, cpu)

The gso_skb and skb_bad_txq requeue paths in dequeue_skb() also call
skb_get_tx_queue().

That would take txq->_xmit_lock, write xmit_lock_owner and read txq->state
in memory past the end of the dev->_tx allocation. Loading the program
needs CAP_BPF and CAP_NET_ADMIN, or a delegated BPF token. Even so, the
program has passed the verifier and should not be able to corrupt kernel
memory.

[ ... ]

> @@ -4848,21 +4861,25 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
>  	tcx_set_ingress(skb, false);
>  #ifdef CONFIG_NET_EGRESS
>  	if (static_branch_unlikely(&egress_needed_key)) {
> +		bool skip_txq;
> +
>  		if (nf_hook_egress_active()) {
>  			skb = nf_hook_egress(skb, &rc, dev);
>  			if (!skb)
>  				goto out;
>  		}
>  
> -		netdev_xmit_skip_txqueue(false);
> +		skip_txq = netdev_xmit_skip_txqueue(false);
>  
>  		nf_skip_egress(skb, true);
>  		skb = sch_handle_egress(skb, &rc, dev);
> -		if (!skb)
> +		if (!skb) {
> +			netdev_xmit_skip_txqueue(skip_txq);
>  			goto out;
> +		}
>  		nf_skip_egress(skb, false);
>  
> -		if (netdev_xmit_txqueue_skipped())
> +		if (netdev_xmit_skip_txqueue(skip_txq))
>  			txq = netdev_tx_queue_mapping(dev, skb);
>  	}
>  #endif

[Severity: Medium]
Does the flag stay confined to this frame if egress_needed_key is turned
off while an outer frame is inside sch_handle_egress()?

Each nested __dev_queue_xmit() reads the static key again.
net_dec_egress_queue() is a plain static_branch_dec() and does not wait
for RCU-bh readers that are already inside the branch. Consider:

CPU1                                     CPU2
__dev_queue_xmit(qa)
  static key is on
  skip_txq = netdev_xmit_skip_txqueue(false)
  nf_skip_egress(skb, true)
  sch_handle_egress()
                                         clsact_destroy()
                                           net_dec_egress_queue()
                                             static_branch_dec()
    tcf_mirred_forward()
      (mirror clone has nf_skip_egress=1)
      dev_queue_xmit(qc)
        static key is off: no save/restore,
        no nf_skip_egress reset
        q->enqueue() -> prio classifier
          tcf_skbedit_act()
            skb_at_tc_egress() is true
            netdev_xmit_skip_txqueue(true)
  nf_skip_egress(skb, false)
  netdev_xmit_skip_txqueue(skip_txq) returns true
  txq = netdev_tx_queue_mapping(dev, skb)

In this sequence, qa's surviving skb picks its tx queue from its own
queue_mapping, even though it never went through skbedit. For a forwarded
packet that value is the ingress rx_queue + 1.

The capped write-back in netdev_tx_queue_mapping() still prevents the
taprio out-of-bounds access. The result would be a wrong but valid tx
queue, and possibly the ratelimited "selects TX queue" warning.

The commit message says the flag is now owned by the xmit frame the
egress hook runs in. Is this cross-frame leak meant to be covered as well?

[ ... ]

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