Re: [PATCH net v3 1/2] net/sched: sch_cake: fix shaper stall on segs == 0 in cake_overhead()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (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] The commit message gets the mechanism and impact of the segs
  == 0 bug wrong.
- [Low] The commit message never says how an skb with qdisc_pkt_segs(skb)
  == 0 reaches the multi-segment arithmetic in cake_overhead().

Pre-existing issues:
- [Low] cake_overhead() says its header-length logic is "borrowed from
  qdisc_pkt_len_segs_init()", but it lacks that function's checks.

--

net/sched: sch_cake: fix shaper stall on segs == 0 in cake_overhead()

This patch changes the single-segment early return in cake_overhead() from
segs == 1 to segs <= 1. The goal is to stop skbs with a cached segment count
of 0 from reaching the multi-segment overhead arithmetic.

> Because segs is an unsigned 16-bit integer, (segs - 1) underflows to
> 65535 (and is promoted to 4294967295 in the 32-bit unsigned arithmetic
> above). This multiplies the per-segment overhead by UINT32_MAX, so
> cake_overhead() returns a length close to 4.29 GB.

[Severity: Medium]
Is this description of the arithmetic accurate? segs is a u16, but it is
promoted to int before the subtraction, so segs - 1 is -1 rather than 65535.

The last-segment term also wraps when segs == 0:

    last_len = skb->len - shinfo->gso_size * (segs - 1);

This gives skb->len + gso_size. Writing F() for cake_calc_overhead(), the
old return value reduces to:

    F(skb->len + gso_size) - F(gso_size + hdr_len)    (mod 2^32)

F() never decreases as its argument grows. So when hdr_len <= skb->len,
wouldn't the old code charge roughly skb->len - hdr_len rather than
something close to 4.29 GB?

The large wrap seems to need hdr_len > skb->len. At this SHA that looks
possible only when the transport header is unset. In that case
qdisc_pkt_len_segs_init() returns early and leaves pkt_segs == gso_segs:

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
	if (!skb->encapsulation) {
		if (unlikely(!skb_transport_header_was_set(skb)))
			return SKB_NOT_DROPPED_YET;
    ...
}

cake_overhead() then computes hdr_len from the unset transport offset,
which is around 0xFFFF. The next patch in the series, "net/sched: sch_cake:
validate transport header offset in cake_overhead()", targets that same
condition.

The 4.29 GB figure is used to justify Cc: stable. Could the commit message
describe the arithmetic correctly and state the condition needed for the
stall?

> Commit c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()") switched
> cake to retrieve the cached segmentation count via qdisc_pkt_segs(skb)
> instead of calculating it locally for dodgy GSO packets. If an skb with
> segs == 0 reaches cake_overhead(), it skips the segs == 1 early return
> and enters the multi-segment arithmetic:

[Severity: Low]
How does an skb with qdisc_pkt_segs(skb) == 0 get here? For every
SKB_GSO_DODGY skb that has a transport header, qdisc_pkt_len_segs_init()
already recomputes pkt_segs, and it drops the skb when payload <= 0:

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
	if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) {
		int payload = skb->len - hdr_len;

		/* Malicious packet. */
		if (payload <= 0)
			return SKB_DROP_REASON_SKB_BAD_GSO;
		gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size);
		shinfo->gso_segs = gso_segs;
		qdisc_skb_cb(skb)->pkt_segs = gso_segs;
	}
    ...
}

pkt_segs == 0 seems to survive in only two cases:

  - the early return for a non-encapsulated GSO skb with no transport
    header set
  - a non-DODGY GSO skb whose gso_segs was left at 0

The in-tree virtio_net_hdr producers set DODGY. On af_packet they also set
a transport header.

In the second case, with a valid transport header, the new early return
charges F(qdisc_pkt_len()), which is F(skb->len - hdr_len). That is about
what the old wrapped path charged.

Could the commit message name the producer or path that leads here? That
would let the reproducer and the "tens of seconds" stall be checked, and
would help stable maintainers judge exposure.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..b0d604a7052a9 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1420,7 +1420,7 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  
>  	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() */

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The code
after this comment in cake_overhead() lacks several of the checks in
qdisc_pkt_len_segs_init():

net/sched/sch_cake.c:cake_overhead() {
    ...
	/* borrowed from qdisc_pkt_len_segs_init() */
	if (!skb->encapsulation)
		hdr_len = skb_transport_offset(skb);
	else
		hdr_len = skb_inner_transport_offset(skb);
    ...
}

Compared with qdisc_pkt_len_segs_init(), it is missing:

  - a skb_transport_header_was_set() guard
  - a check that __tcp_hdrlen(th) >= sizeof(struct tcphdr)
  - a pskb_may_pull() of the full header

It also adds sizeof(struct udphdr) for every non-TCP GSO type, not just
SKB_GSO_UDP_L4.

Of these, only the missing transport header guard can cause a large
overcharge. With an unset header, hdr_len is around 64 KiB, and each
segment is charged tens of KB when segs >= 2. The follow-up patch
"net/sched: sch_cake: validate transport header offset in cake_overhead()"
adds that guard and a hdr_len < 0 check.

After the full series, is the remaining difference from the core helper
intended? What is left is small misaccounting: the TCP header length term is
bounded at 60 bytes, and the extra UDP header is 8 bytes. This code dates
back to a729b7f0bd5b.

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