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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 4

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 subject ('net/sched: cap the accounted backlog before it
  can wrap') reads as a general fix, but only fq_codel, cake, codel,…
- [Low] The commit message has no Fixes: tag.
- [Low] The commit message describes the current length accounting
  incorrectly in two places, and it points to an in-series patch by…
- [Low] plug_enqueue() (net/sched/sch_plug.c) changes its byte-limit
  admission test from '(u64)sch->qstats.backlog + skb->len <= q->limit'…
- [Low] In red_enqueue() (net/sched/sch_red.c:81-84), the new comment
  explains the second check, qdisc_backlog_overflow(child, skb), with:…

--

net/sched: cap the accounted backlog before it can wrap

This commit adds qdisc_backlog_overflow(), which drops a packet at enqueue
when adding its qdisc_pkt_len() would wrap the 32-bit sch->qstats.backlog.
The helper is called from fq_codel, cake, codel, pie, fq_pie, dualpi2, RED,
pfifo, plug and gred. The commit also switches plug's byte limit to
qdisc_pkt_len(), and RED now reserves the length before the child enqueue
and re-derives its backlog from snapshots when the enqueue fails.

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/
Link: https://lore.kernel.org/netdev/179269577131.3135.3967915390403138015@kernel.org/

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

[Severity: Low]
Should this have a Fixes: tag? The message describes a NULL flow->head
dereference in fq_codel_drop() after the per-flow backlog wraps. It also
describes wrong AQM drop decisions in several qdiscs.

The only commit it references is 709f34f7c28d. That commit changes sch_fq,
which this patch doesn't touch, and it doesn't look like the commit that
introduced the u32 backlog wrap.

The tags include Reported-by, Closes, Link and Reviewed-by, but none of them
names the commit(s) that introduced the problem. Examples would be the stab
size table support or the AQMs that read qstats.backlog. There is no
Cc: stable either, so stable tooling can't tell which trees need this.

> 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.

[Severity: Low]
Is this accurate? The SKB_GSO_DODGY branch of qdisc_pkt_len_segs_init()
already rejects wire estimates above QDISC_PKT_LEN_MAX:

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
		if (unlikely(gso_segs > GSO_MAX_SEGS ||
			     skb->len + (u64)(gso_segs - 1) * hdr_len >
			     QDISC_PKT_LEN_MAX))
			return SKB_DROP_REASON_SKB_BAD_GSO;
    ...
}

If so, only trusted, locally built GSO can reach the
len > QDISC_PKT_LEN_MAX branch of qdisc_backlog_overflow(). That is the
BIG TCP case described later in the message.

> qdisc_pkt_len() itself stays truthful for every input: it is not
> clamped and the initializer keeps succeeding, so BPF

This doesn't match the code either. __qdisc_calculate_pkt_len() clamps
every stab-adjusted length, and the reproducer goes through that path:

net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
    ...
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
	qdisc_skb_cb(skb)->pkt_len = pkt_len;
}

The initializer also fails for oversized dodgy GSO, as shown above. Could
this paragraph be reworded?

> CAKE's split-GSO branch is the other exception; 4/4 sums and checks those
> segments before linking any of them.

The "4/4" refers to a position in the series, which won't mean anything once
this is in git history. Could that commit be named by its subject instead?

> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 1acaadb3e2cf0..a5a6bdf55e0a1 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -915,6 +915,26 @@ static inline unsigned int qdisc_pkt_len(const struct sk_buff *skb)
>  	return qdisc_skb_cb(skb)->pkt_len;
>  }
>  
> +#define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */
> +
> +/* Largest backlog that still has room for one more 1 MiB packet. */
> +#define QDISC_MAX_BACKLOG	(U32_MAX - QDISC_PKT_LEN_MAX)
> +
> +/* Would this packet's accounted length wrap sch->qstats.backlog?
> + * Packets up to QDISC_PKT_LEN_MAX fit below QDISC_MAX_BACKLOG; GSO
> + * can make the length larger, so compare against the real headroom.
> + */
> +static inline bool qdisc_backlog_overflow(const struct Qdisc *sch,
> +					  const struct sk_buff *skb)
> +{
> +	u32 backlog = READ_ONCE(sch->qstats.backlog);
> +	u32 len = qdisc_pkt_len(skb);
> +
> +	if (likely(len <= QDISC_PKT_LEN_MAX))
> +		return backlog > QDISC_MAX_BACKLOG;
> +	return len > U32_MAX - backlog;
> +}

[Severity: Medium]
The subject reads like a general fix, but this helper is only called from
fq_codel, cake, codel, pie, fq_pie, dualpi2, RED, pfifo, plug and gred. Can
sch->qstats.backlog still wrap the same way in other qdiscs?

Several leaves add the stab-inflated qdisc_pkt_len() without the guard:

  fq_enqueue()   -> qdisc_qstats_backlog_inc(sch, skb);
  sfq_enqueue()  -> qdisc_qstats_backlog_inc(sch, skb);
  hhf_enqueue()  -> qdisc_qstats_backlog_inc(sch, skb);

fq's default limit is 10000 packets per band, across 3 bands. That is far
more than the roughly 4096 1 MiB packets needed to wrap. sfq's limit can be
set as high as maxdepth * maxflows. skbprio, netem, choke, etf and taprio
look similar.

Classful parents such as prio and htb (and also drr, ets, qfq, hfsc, cbs and
sfb) add each child's admitted length into their own u32 backlog:

  prio_enqueue() -> qstats_backlog_add(sch, len);
  htb_enqueue()  -> qstats_backlog_add(sch, len);

Each guarded child can hold a backlog just below U32_MAX, so several of them
together can still wrap the parent. The commit message names only child GSO
segmentation (tbf, netem) and CAKE split-GSO as remaining exceptions.

The patch counts stats-only corruption as in scope, since it guards pfifo and
non-WRED gred. Should the qdiscs above be covered too, or listed as known
gaps? None of the unguarded leaves seem to use sch->qstats.backlog for drop
decisions, so there the result would be wrong backlog stats in dumps.

[ ... ]

> diff --git a/net/sched/sch_plug.c b/net/sched/sch_plug.c
> index b60ddfee6a68a..c90b5ca60bc0b 100644
> --- a/net/sched/sch_plug.c
> +++ b/net/sched/sch_plug.c
> @@ -89,7 +89,14 @@ static int plug_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  {
>  	struct plug_sched_data *q = qdisc_priv(sch);
>  
> -	if (likely((u64)sch->qstats.backlog + skb->len <= q->limit)) {
> +	if (unlikely(qdisc_backlog_overflow(sch, skb))) {
> +		qdisc_qstats_overlimit(sch);
> +		return qdisc_drop_reason(skb, sch, to_free,
> +					 QDISC_DROP_OVERLIMIT);
> +	}
> +
> +	if (likely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) <=
> +		   q->limit)) {

[Severity: Low]
The limit test in plug_enqueue() now uses qdisc_pkt_len(skb) instead of
skb->len, but the commit message only says "Cover sch_plug". Could this
change be mentioned?

qdisc_enqueue_tail() already counted the backlog in qdisc_pkt_len() units,
so only the measurement of the arriving packet changes (GSO replicated
headers, or stab overhead). Near the byte limit, some packets that were
admitted before will now be dropped.

This switch alone is enough to prevent the wrap. q->limit is a u32 and plug
is classless, so once this test passes,
backlog + qdisc_pkt_len(skb) <= q->limit <= U32_MAX holds.

Is the qdisc_backlog_overflow() check above it still needed? It only does
anything when q->limit > QDISC_MAX_BACKLOG. In that case it drops packets
that the limit would accept and that could not wrap the counter.

[ ... ]

> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
> index d7598214270b8..b5233d9fbf572 100644
> --- a/net/sched/sch_red.c
> +++ b/net/sched/sch_red.c
> @@ -73,9 +73,22 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	enum qdisc_drop_reason reason = QDISC_DROP_CONGESTED;
>  	struct red_sched_data *q = qdisc_priv(sch);
>  	struct Qdisc *child = q->qdisc;
> +	u32 child_backlog;
> +	u32 red_backlog;
>  	unsigned int len;
>  	int ret;
>  
> +	/* RED reads the child backlog into red_calc_qavg(), so bound both
> +	 * counters: a child that evicts on NET_XMIT_CN can otherwise grow
> +	 * its own backlog while RED's stays put.
> +	 */
> +	if (unlikely(qdisc_backlog_overflow(sch, skb) ||
> +		     qdisc_backlog_overflow(child, skb))) {

[Severity: Low]
Is this comment still accurate, given the re-derive logic added later in
red_enqueue()? On the non-success path the patch now does:

		grew = READ_ONCE(child->qstats.backlog) - child_backlog;
		WRITE_ONCE(sch->qstats.backlog, red_backlog + grew);

The later comment names the NET_XMIT_CN cases. hhf and skbprio return CN
without calling qdisc_tree_reduce_backlog(), while pfifo_head_drop, sfq and
fq_codel do call it. In all of these, RED's backlog now moves by exactly the
child's growth, so it no longer stays put.

The comment seems to describe an earlier revision. Does the child check need
a different justification, or is it now redundant?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-BA27.v9.r0.20261108195325%40mojatatu.com