Re: [PATCH v2] net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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] Single `Fixes: a41851bea7bf` tag plus `Cc: stable` for three
  independent defects, none of which was introduced by the tagged commit.
- [Low] The changelog justifies change 3 with "post-enqueue mangling
  (such as BPF packet trimming via bpf_skb_net_shrink())", but that…

--

net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()

This changes cake_overhead() to bail out to cake_calc_overhead() for
segs <= 1, to skip the multi-segment path when the transport header was
never set, and to treat a negative header offset as a fallback case by
making hdr_len signed. The stated goal is to avoid inflated lengths being
charged to the CAKE shaper.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..45969c1b95fc9 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1413,21 +1413,28 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
>  static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  {
>  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> -	unsigned int hdr_len, last_len = 0;
> +	unsigned int last_len = 0;
>  	u32 off = skb_network_offset(skb);
>  	u16 segs = qdisc_pkt_segs(skb);
>  	u32 len = qdisc_pkt_len(skb);
> +	int hdr_len;
>  
>  	WRITE_ONCE(q->avg_netoff, cake_ewma(q->avg_netoff, off << 16, 8));
>  
> -	if (segs == 1)
> +	if (segs <= 1)
>  		return cake_calc_overhead(q, len, off);
>  
>  	/* borrowed from qdisc_pkt_len_segs_init() */
> -	if (!skb->encapsulation)
> +	if (!skb->encapsulation) {
> +		if (unlikely(!skb_transport_header_was_set(skb)))
> +			return cake_calc_overhead(q, len, off);
>  		hdr_len = skb_transport_offset(skb);
> -	else
> +	} else {
>  		hdr_len = skb_inner_transport_offset(skb);
> +	}

[Severity: Medium]
Should the three changes here carry their own Fixes tags rather than a
single Fixes: a41851bea7bf plus Cc: stable?

None of the three defects look like they were introduced by
a41851bea7bf. git show a41851bea7bf only replaces

	hdr_len = skb_transport_offset(skb);

with the encapsulation if/else, so it neither added the segs handling nor
the missing sentinel check.

The segs early return that changes from segs == 1 to segs <= 1 came in
with c5d34f4583ea88 ("net_sched: cake: use qdisc_pkt_segs()"). Before
that commit cake_overhead() recomputed the count itself for dodgy GSO
skbs:

	segs = DIV_ROUND_UP(skb->len - hdr_len, shinfo->gso_size);

so a cached segs == 0 could not be observed in older trees.

The missing skb_transport_header_was_set() guard, on the other hand,
dates back to a729b7f0bd5bf4, where the same
hdr_len = skb_transport_offset(skb); had the identical ~0U sentinel
behaviour.

With only the a41851bea7bf tag, doesn't the segs <= 1 hunk get applied to
trees that never had the cached count, while the sentinel fix never
reaches the trees between a729b7f0bd5bf4 and a41851bea7bf that are
exposed to the ~66 KB per segment accounting the changelog describes?

Would splitting this into one logical fix per patch, each with its own
Fixes tag, make the stable ranges match the defects?

> +
> +	if (unlikely(hdr_len < 0))
> +		return cake_calc_overhead(q, len, off);
>  
>  	/* + transport layer */
>  	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 |

[Severity: Low]
The changelog justifies this guard with:

    If post-enqueue mangling (such as BPF packet trimming via
    bpf_skb_net_shrink()) produces a negative offset, hdr_len wraps to
    near UINT_MAX

Is that ordering possible? __dev_queue_xmit() calls

	reason = qdisc_pkt_len_segs_init(skb);

near its top, and only afterwards runs sch_handle_egress() and
__dev_xmit_skb() -> cake_enqueue() -> cake_overhead(), which is the only
caller. So cake_overhead() runs during enqueue, not after it.

The two windows that do exist look like:

  - between qdisc_pkt_len_segs_init() and enqueue, i.e. tc/BPF egress in
    sch_handle_egress()
  - inside cake_enqueue() itself, since cake_classify() -> tcf_classify()
    (act_bpf, act_pedit, act_mpls, act_vlan) runs before
    get_cobalt_cb(skb)->adjusted_len = cake_overhead(q, skb)

The underlying mechanism still holds: bpf_skb_adjust_room() with
BPF_ADJ_ROOM_MAC ends up in bpf_skb_net_hdr_pop(), which adjusts only
mac_header/network_header and leaves transport_header stale, so
skb_transport_offset() becomes old_offset - len_diff and can go negative.

Could the changelog name that ordering and a concrete path instead of
"post-enqueue mangling", given the patch is tagged Cc: stable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922084124.36858-1-ndaugoing%40gmail.com