Re: [PATCH net-next v5] net/sched: cap the accounted backlog before it can wrap

["Toke Høiland-Jørgensen" <toke@toke.dk>]

> This is a follow-up to an issue found by Sashiko (nipa) during review of
> commit d9ebd8f9aa8b ("net/sched: fq_codel: clamp default quantum and
> mtu"), part of the quantum/mtu overflow series merged as a687f2ae995f.
> Clamping the per-flow quantum and the CoDel mtu does not bound the
> accounted backlog, so the per-flow backlog wrap the review flagged
> remained.

This, and the rest of the commit message is obtuse LLM vomit, and close
to unreadable. Please rewrite the commit message without the verbosity,
so it's actually something a human reader can comprehend. For the cake
bits in particular:

[..]

> CAKE's SPLIT_GSO path accounts the sum of the segment lengths, which a
> stab recomputes from skb->len and can exceed the pre-split
> qdisc_pkt_len(). The list is summed before any segment is linked and
> rejected as a whole before any flow state is resolved: the flow/host keys
> are computed from the unsegmented packet, but the set-associative
> resolution that commits q->tags[], the flow host indices, the way
> counters and the host bulk-flow counts runs only after the whole
> segment-sum admission test passes. A packet rejected by that test
> therefore leaves no persistent state behind. The whole list is dropped
> via to_free so the tail segment's sock_wfree() never runs under the qdisc
> lock. A fraglist GSO is the exception: skb_segment_list() returns the
> original skb as the list head carrying an extra reference, so segs ==
> skb; dropping the caller's reference and queueing the list once avoids
> self-linking *to_free, which would otherwise free the same skb repeatedly
> in the deferred teardown.

I read this paragraph multiple times, and it's still not clear to me
what it's trying to say.

Also:

>  net/sched/sch_cake.c      | 198 ++++++++++++++++++++++++++++----------

You're changing hundreds of lines of code which is partly a refactor,
partly a fix for the actual overflow issue, and partly unrelated
changes. Please split them into separate patches.

[..]

> +/* Compute the flow/host hash keys from the packet as received. Must run
> + * before skb_gso_segment(), which rewrites an encapsulated GSO packet's
> + * headers to the inner ones and would change the keys. Reads only the skb
> + * and commits no qdisc state, so it may run before admission is known.
> + */

And drop the weird LLM comments, please. If you split the series
properly they are not needed.

[..]
> -		skb_list_walk_safe(segs, segs, nskb) {
> -			skb_mark_not_on_list(segs);
> -			qdisc_skb_cb(segs)->pkt_len = segs->len;
> -			qdisc_skb_cb(segs)->pkt_segs = 1;
> -			cobalt_set_enqueue_time(segs, now);
> -			get_cobalt_cb(segs)->adjusted_len = cake_overhead(q,
> -									  segs);
> -			flow_queue_add(flow, segs);
> +		skb_list_walk_safe(segs, seg, nskb) {

The skb_list_walk_safe(segs, segs) thing is admittedly a bit weird, and
fixing it is fine. But that's the "separate fix" I was referring to
above, so split it out; it's just making the actual fix harder to
comprehend here.

-Toke

pw-bot: cr