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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message's explanation of the failure is wrong, and it
  leaves out what actually triggers the stall.

--

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. A GSO skb whose cached segment count is 0 then
skips the multi-segment overhead arithmetic, which could charge a huge
length to the shaper.

> 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: Low]
Is this description of the arithmetic accurate?

segs is a u16, so (segs - 1) is promoted to int and evaluates to -1. It
never becomes 65535. It only turns into 0xffffffff when it is multiplied
by the u32 that cake_calc_overhead() returns.

The description also leaves out the last-segment term. With segs == 0:

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

last_len becomes skb->len + gso_size, so the old return value was in
effect:

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

cake_calc_overhead() never decreases as len grows. So whenever
hdr_len <= skb->len, the result is small, roughly the payload size and a
slight undercharge, and the shaper does not stall.

The ~4.29 GB charge only happens when hdr_len > skb->len. In practice that
means a non-encapsulated skb whose transport header was never set. In that
case skb_transport_offset() is around 65300 and skb_header_pointer() fails.

That is the same case where qdisc_pkt_len_segs_init() returns early and
leaves pkt_segs at the raw gso_segs value, which can be 0:

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

When the transport header is set, the later pskb_may_pull() calls in
qdisc_pkt_len_segs_init() guarantee hdr_len <= skb->len for TCP and
UDP_L4. For SKB_GSO_DODGY, gso_segs is recomputed with DIV_ROUND_UP() and
ends up at 1 or more. So an skb with segs == 0 and a valid transport header
does not stall the shaper.

The code change itself looks correct. Could the commit message correct the
promotion explanation? Could it also state that an unset transport header
is the precondition for the stall, so stable backporters can judge their
exposure?

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