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

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

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> This is a followup of commit 709f34f7c28d ("net/sched: fq: add overflow
> bounds to quantum and initial quantum") as reported by Sashiko.

The rest of this commit message is somewhat obtuse - too much run-on
information. I think it can be pretty easily fixed by a couple of
rearrangements and paragraph breaks, like this:

> The issue: fq_codel, cake, codel, pie, fq_pie, dualpi2, RED, pfifo,
> plug and (non-WRED) gred account every enqueued packet's stab-adjusted
> length into the 32-bit sch->qstats.backlog. A crafted tc size table
> inflates qdisc_pkt_len() up to QDISC_PKT_LEN_MAX (1 MiB), and a dodgy
> GSO skb can account even more through qdisc_pkt_len_segs_init(), so a
> few thousand packets wrap the counter mod 2^32.
>
> The AQM then reads a small backlog and makes the wrong drop decision,
> and the dequeue-side subtractions leave it corrupt (for non-WRED gred
> it is the reported stats and the dequeue-side subtractions that go
> wrong instead). In fq_codel a wrapped per-flow backlog can also make
> fq_codel_drop()'s scan pick an empty flow, dereferencing a NULL
> flow->head.
>
> The Fix: drop at enqueue once the stored backlog cannot represent the
> incoming packet's accounted length.
>
> Ordinary packets (qdisc_pkt_len() at most QDISC_PKT_LEN_MAX) are
> covered by a standing ceiling: drop once the backlog is within
> QDISC_MAX_BACKLOG (U32_MAX - QDISC_PKT_LEN_MAX) of the wrap point, the
> largest backlog one more 1 MiB packet cannot wrap, so there is no
> per-enqueue 64-bit add.
>
> A larger wire estimate is checked against the space that actually
> remains (len > U32_MAX - backlog): qdisc_pkt_len_segs_init() adds the
> headers replicated on every output segment, so a supported small-MSS
> BIG TCP aggregate can carry a wire estimate above 1 MiB while its raw
> length stays within GSO_MAX_SIZE, and such traffic must pass on an
> empty queue.
>
> qdisc_pkt_len() itself stays truthful for every input: it is not
> clamped and the initializer keeps succeeding, so BPF
> __sk_buff.wire_len, act_police and the HTB/TBF shapers read the real
> length, and sch_handle_ingress() -- which ignores the initializer's
> return value and reads pkt_segs for packet statistics -- keeps
> complete metadata.
>
> Cover sch_plug and non-WRED sch_gred too: plug admits on skb->len
> while qdisc_enqueue_tail() adds qdisc_pkt_len(), and gred bounds each
> virtual queue but adds every VQ into the one aggregate. pfifo is
> included because RED reads a grafted child's backlog into
> red_calc_qavg(); it can be reached standalone or as the default child
> of SFB. In RED, account the packet to the aggregate before handing it
> to the child, because a duplicating netem child re-enqueues its clone
> at the root while the stored backlog is still un-updated -- one guard
> check must not authorize two additions.
>
> The guard bounds each qdisc's own enqueue path: a child that segments a
> GSO skb (tbf, netem) can still grow a parent's aggregate through
> qdisc_tree_reduce_backlog()'s unclamped byte delta, which is left to a
> separate change.
>
> Conditions to recreate the bug:
>   ip tuntap add tun0 mode tun
>   ip link set tun0 txqueuelen 32 up
>   ip addr add 10.99.0.1/24 dev tun0
>   tc qdisc add dev tun0 root handle 1: stab overhead 2000000000 \
>       fq_codel flows 1 limit 20000 ecn
>   # hold the tun fd open without reading so the backlog persists, then
>   # send 6000 packets: the counter wraps and the unfixed kernel reports
>   # a wrapped backlog, while the fixed one drops at the ceiling.
>
> Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
> Link: https://lore.kernel.org/netdev/CANn89iLfMJV7ancKH1Gjzzm7ZUG-gKcrczkJjJEWN5sCuWd-ug@mail.gmail.com/
> Link: https://lore.kernel.org/netdev/CANn89i+GOFH_8g+vSV0jUj0aBqLYVWuFOrHuDggZu9kPC_m6tQ@mail.gmail.com/
> Link: https://lore.kernel.org/netdev/87a4oottcj.fsf@toke.dk/
> Link: https://lore.kernel.org/netdev/87pkxks68f.fsf@toke.dk/
> Link: https://lore.kernel.org/netdev/179231932946.3135.7497478990186385688@kernel.org/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

With the above rewording:

Reviewed-by: Toke Høiland-Jørgensen <toke@toke.dk>