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

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

[Jamal Hadi Salim]

fq_codel, cake, codel, pie, fq_pie and dualpi2 account every enqueued
packet's stab-adjusted length into a 32-bit sch->qstats.backlog.
fq_codel/codel uses it do decide if they should drop a packet at deq;
cake uses it to prune the longest-flow heap from per-flow backlogs;
pie and fq_pie use it to make early drop decisions and, dualpi2 decides
must_drop() on it. RED can can decide on a child's backlog based on it.
A crafted tc size table (TC_STAB) inflates qdisc_pkt_len() up to
QDISC_PKT_LEN_MAX (1 MiB), so a few thousand packets wrap the counter
mod 2^32. The AQM algo then reads a small backlog and makes the wrong
drop decision, and the dequeue-side subtractions keep the counter corrupt.

Fix:
Drop at enqueue once the accounted backlog would cross QDISC_MAX_BACKLOG
(U32_MAX - QDISC_PKT_LEN_MAX), the largest backlog one more maximum-size
packet cannot wrap.  This follows the existing bfifo/gred approach
(safe because its limit is checked against the accounted packet length);
the fixed qdiscs' limits are packet counts or otherwise do not bound
the aggregate bytes, so they need the byte bound here.

Conditions to recreate the bug: CAP_NET_ADMIN in a user namespace;
CONFIG_NET_SCH_FQ_CODEL=y.

  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 (IFF_BACKPRESSURE) so the qdisc
  # backlog persists, then send 6000 packets. At 4096 resident 1 MiB
  # packets the counter wraps; the unfixed kernel then reports a wrapped
  # backlog for 5969 resident packets, the fixed one drops at the ceiling.

  # RED with a grafted packet-limited child reads the child's backlog the
  # same way; the default bfifo child is byte-limited and safe, pfifo is
  # packet-limited:
  tc qdisc add dev tun0 root handle 1: stab overhead 2000000000 \
      red limit 1000000000 min 5000 max 10000 avpkt 1000 burst 32 ecn
  tc qdisc add dev tun0 parent 1:1 handle 30: pfifo limit 100000

Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v2 (2026-09-25) — approach rewrite per list discussion of v1:

  v1 approached the wrap by widening the per-flow backlog counters to
  u64. Eric Dumazet said "Storing 4GB in a qdisc is absolutely insane" ;-)
  and asked for a drop at enqueue once the backlog approaches the wrap point.
  Sebastian Moeller noted tc-stab is the generic overhead-accounting mechanism,
  so the fix must not penalize normal stab use.

  v2 replaces the u64 widening with an enqueue-side pre-check against
  QDISC_MAX_BACKLOG (U32_MAX - QDISC_PKT_LEN_MAX) and drops with
  QDISC_DROP_OVERLIMIT. No counter is widened, so the 32-bit
  sch->qstats.backlog the AQM qdisc reads can no longer wrap. Same pattern
  in other qdiscs fixed in one patch: fq_codel, cake, codel, pie, fq_pie,
  dualpi2, RED.
  Note: v1 touched fq_codel and cake only.

 include/net/pkt_sched.h  |  5 +++++
 net/sched/sch_cake.c     | 28 ++++++++++++++++++++++++++--
 net/sched/sch_codel.c    | 17 ++++++++++-------
 net/sched/sch_dualpi2.c  |  2 ++
 net/sched/sch_fq_codel.c |  7 +++++++
 net/sched/sch_fq_pie.c   |  4 +++-
 net/sched/sch_pie.c      |  4 +++-
 net/sched/sch_red.c      |  8 +++++++-
 8 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 90d3e7943b19..351b92f956ef 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -13,6 +13,11 @@
 #define DEFAULT_TX_QUEUE_LEN	1000
 #define STAB_SIZE_LOG_MAX	30
 #define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */
+/*
+ * Largest accounted backlog for which enqueuing one more maximum-size
+ * packet cannot wrap the 32-bit sch->qstats.backlog.
+ */
+#define QDISC_MAX_BACKLOG	(U32_MAX - QDISC_PKT_LEN_MAX)
 
 struct qdisc_walker {
 	int	stop;
diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..6a16546aa5b0 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1776,6 +1776,14 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	idx--;
 	flow = &b->flows[idx];
 
+	if (unlikely((u64)sch->qstats.backlog + len > QDISC_MAX_BACKLOG)) {
+		WRITE_ONCE(flow->dropped, flow->dropped + 1);
+		WRITE_ONCE(b->tin_dropped, b->tin_dropped + 1);
+		qdisc_qstats_overlimit(sch);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
 	/* ensure shaper state isn't stale */
 	if (!b->tin_backlog) {
 		if (ktime_before(b->time_next_packet, now))
@@ -1801,7 +1809,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		WRITE_ONCE(b->max_skblen, len);
 
 	if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
-		struct sk_buff *segs, *nskb;
+		struct sk_buff *segs, *nskb, *seg;
 		netdev_features_t features = netif_skb_features(skb);
 		unsigned int slen = 0, numsegs = 0;
 
@@ -1809,6 +1817,23 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		if (IS_ERR_OR_NULL(segs))
 			return qdisc_drop(skb, sch, to_free);
 
+		/* The segment list is accounted by the sum of its lengths,
+		 * which can exceed the original packet's accounted length, so
+		 * sum it before linking any segment and drop the whole list if
+		 * the post-split total would cross the ceiling.
+		 */
+		skb_list_walk_safe(segs, seg, nskb)
+			slen += seg->len;
+
+		if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
+			kfree_skb_list_reason(segs, SKB_DROP_REASON_QDISC_DROP);
+			WRITE_ONCE(flow->dropped, flow->dropped + 1);
+			WRITE_ONCE(b->tin_dropped, b->tin_dropped + 1);
+			qdisc_qstats_overlimit(sch);
+			return qdisc_drop_reason(skb, sch, to_free,
+						 QDISC_DROP_OVERLIMIT);
+		}
+
 		skb_list_walk_safe(segs, segs, nskb) {
 			skb_mark_not_on_list(segs);
 			qdisc_skb_cb(segs)->pkt_len = segs->len;
@@ -1820,7 +1845,6 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
 			qdisc_qlen_inc(sch);
 			numsegs++;
-			slen += segs->len;
 			q->buffer_used += segs->truesize;
 			WRITE_ONCE(b->packets, b->packets + 1);
 		}
diff --git a/net/sched/sch_codel.c b/net/sched/sch_codel.c
index 6aa5829d6961..f2770a438070 100644
--- a/net/sched/sch_codel.c
+++ b/net/sched/sch_codel.c
@@ -116,15 +116,18 @@ static struct sk_buff *codel_peek(struct Qdisc *sch)
 static int codel_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			       struct sk_buff **to_free)
 {
-	struct codel_sched_data *q;
+	struct codel_sched_data *q = qdisc_priv(sch);
 
-	if (likely(qdisc_qlen(sch) < sch->limit)) {
-		codel_set_enqueue_time(skb);
-		return qdisc_enqueue_tail(skb, sch);
+	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) >
+		     QDISC_MAX_BACKLOG)) {
+		WRITE_ONCE(q->drop_overlimit, q->drop_overlimit + 1);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
 	}
-	q = qdisc_priv(sch);
-	WRITE_ONCE(q->drop_overlimit, q->drop_overlimit + 1);
-	return qdisc_drop_reason(skb, sch, to_free, QDISC_DROP_OVERLIMIT);
+
+	codel_set_enqueue_time(skb);
+	return qdisc_enqueue_tail(skb, sch);
 }
 
 static const struct nla_policy codel_policy[TCA_CODEL_MAX + 1] = {
diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
index 4947def7c49e..ff5d55d502f2 100644
--- a/net/sched/sch_dualpi2.c
+++ b/net/sched/sch_dualpi2.c
@@ -392,6 +392,8 @@ static int dualpi2_enqueue_skb(struct sk_buff *skb, struct Qdisc *sch,
 	struct dualpi2_skb_cb *cb;
 
 	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) >
+		     QDISC_MAX_BACKLOG) ||
 	    unlikely((u64)q->memory_used + skb->truesize > q->memory_limit)) {
 		qdisc_qstats_overlimit(sch);
 		if (skb_in_l_queue(skb))
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 969b2510b0b8..e98bafa47da3 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -201,6 +201,13 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 	idx--;
 
+	if (unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) >
+		     QDISC_MAX_BACKLOG)) {
+		q->drop_overlimit++;
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
 	codel_set_enqueue_time(skb);
 	flow = &q->flows[idx];
 	flow_queue_add(flow, skb);
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index 5982847df8f8..6e62ce991c1c 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -155,7 +155,9 @@ static int fq_pie_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	memory_limited = q->memory_usage > q->memory_limit + skb->truesize;
 
 	/* Checks if the qdisc is full */
-	if (unlikely(qdisc_qlen(sch) >= sch->limit)) {
+	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) >
+		     QDISC_MAX_BACKLOG)) {
 		q->stats.overlimit++;
 		goto out;
 	} else if (unlikely(memory_limited)) {
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 3b7863ffd284..64095d27decc 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -89,7 +89,9 @@ static int pie_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct pie_sched_data *q = qdisc_priv(sch);
 	bool enqueue = false;
 
-	if (unlikely(qdisc_qlen(sch) >= sch->limit)) {
+	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) >
+		     QDISC_MAX_BACKLOG)) {
 		WRITE_ONCE(q->stats.overlimit, q->stats.overlimit + 1);
 		goto out;
 	}
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index d7598214270b..dff3d8b0556b 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -76,6 +76,13 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	unsigned int len;
 	int ret;
 
+	len = qdisc_pkt_len(skb);
+	if (unlikely((u64)sch->qstats.backlog + len > QDISC_MAX_BACKLOG)) {
+		qdisc_qstats_overlimit(sch);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
 	q->vars.qavg = red_calc_qavg(&q->parms,
 				     &q->vars,
 				     child->qstats.backlog);
@@ -135,7 +142,6 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		break;
 	}
 
-	len = qdisc_pkt_len(skb);
 	ret = qdisc_enqueue(skb, child, to_free);
 	if (likely(ret == NET_XMIT_SUCCESS)) {
 		qstats_backlog_add(sch, len);
-- 
2.43.0

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

[Eric Dumazet]

On Sat, Sep 26, 2026 at 7:49 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> fq_codel, cake, codel, pie, fq_pie and dualpi2 account every enqueued
> packet's stab-adjusted length into a 32-bit sch->qstats.backlog.
> fq_codel/codel uses it do decide if they should drop a packet at deq;
> cake uses it to prune the longest-flow heap from per-flow backlogs;
> pie and fq_pie use it to make early drop decisions and, dualpi2 decides
> must_drop() on it. RED can can decide on a child's backlog based on it.
> A crafted tc size table (TC_STAB) inflates qdisc_pkt_len() up to
> QDISC_PKT_LEN_MAX (1 MiB), so a few thousand packets wrap the counter
> mod 2^32. The AQM algo then reads a small backlog and makes the wrong
> drop decision, and the dequeue-side subtractions keep the counter corrupt.
>
> Fix:
> Drop at enqueue once the accounted backlog would cross QDISC_MAX_BACKLOG
> (U32_MAX - QDISC_PKT_LEN_MAX), the largest backlog one more maximum-size
> packet cannot wrap.  This follows the existing bfifo/gred approach
> (safe because its limit is checked against the accounted packet length);
> the fixed qdiscs' limits are packet counts or otherwise do not bound
> the aggregate bytes, so they need the byte bound here.

Hi Jamal,

Thanks for reworking this for v2. A few comments on the implementation:
1. QDISC_MAX_BACKLOG check
Since QDISC_MAX_BACKLOG is already defined as
(U32_MAX - QDISC_PKT_LEN_MAX), it already reserves QDISC_PKT_LEN_MAX
of headroom below U32_MAX for the incoming packet. Doing:
    if (unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) >
                 QDISC_MAX_BACKLOG))
accounts for the incoming packet size twice and forces a 64-bit addition
at every call site.
Why not just check unlikely(sch->qstats.backlog > QDISC_MAX_BACKLOG)
(or provide a small helper in include/net/sch_generic.h)?


2. sch_cake.c (cake_enqueue)
There are a few issues with how cake_enqueue() is handled:
- The first check is placed after cake_classify(), which has already
  modified the packet's DSCP (cake_handle_diffserv()) and updated
  set-associative hash state and host bulk-flow counters in cake_hash()
  (srchost_bulk_flow_count / dsthost_bulk_flow_count) assuming the
  packet will be enqueued into flow.
- In the CAKE_FLAG_SPLIT_GSO branch, the second check runs after lines
  1780-1801 have already updated b->max_skblen, shaper timestamps
  (time_next_packet), qstats.overlimits, and scheduled &q->watchdog.
- Walking segs a second time on every GSO packet just to sum slen is
  unnecessary, and calling both kfree_skb_list_reason(segs, ...) (under
  the qdisc lock instead of via to_free) and qdisc_drop_reason(skb, ...)
  triggers duplicate drop tracepoints for both the segments and the
  parent GSO skb.
Since QDISC_MAX_BACKLOG already leaves QDISC_PKT_LEN_MAX (1 MiB) of
headroom, a single check at the very beginning of cake_enqueue() before
cake_classify() is sufficient and avoids touching the GSO split path
altogether.

3. sch_fq_codel.c (fq_codel_enqueue)
Can we move the backlog check before fq_codel_classify() (or in the
!q->filter_list fast path before fq_codel_hash()) so we do not compute
the flow hash for packets we are about to drop?
Also, please mention in the commit message that wrapping q->backlogs[i]
to 0 in fq_codel causes fq_codel_drop() to leave idx = 0 and dereference
a NULL flow->head if flow 0 is empty.

4. sch_red.c and sch_fifo.c (pfifo)
In red_enqueue(), red_calc_qavg() reads child->qstats.backlog. In your
commit message example, child->qstats.backlog wrapped because the child
was a packet-limited pfifo (limit 100000).
Shouldn't pfifo_enqueue() and pfifo_tail_enqueue() in
net/sched/sch_fifo.c also guard against qstats.backlog wrapping when
used standalone or under other classful qdiscs?

Thanks!

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 2 · 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).

- [High] RED's new ceiling check (red_enqueue(), sch_red.c:79-84) only
  tests sch->qstats.backlog + qdisc_pkt_len(skb) before the skb goes to…
- [High] The patch moves `len = qdisc_pkt_len(skb)` in red_enqueue() from
  after the red_action() switch to the top of the function…
- [Medium] The commit message leaves GRED untouched, saying it follows a
  safe approach ("safe because its limit is checked against the…
- [Medium] In the new split-GSO pre-check in cake_enqueue(), segment
  lengths are summed into `unsigned int slen` (declared at…
- [Low] The commit fixes a user-visible bug.
- [Low] In the split-GSO path of cake_enqueue(), when the post-split
  total would cross the ceiling, the patch frees every temporary segment…
- [Low] The comment on QDISC_MAX_BACKLOG (include/net/pkt_sched.h:16-19)
  and the commit message treat QDISC_PKT_LEN_MAX (1 MiB) as the maximum…
- [Low] The new RED ceiling rejection in red_enqueue() (sch_red.c:80-84)
  only calls qdisc_qstats_overlimit() and qdisc_drop_reason().

--

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

This patch adds an enqueue-side check to fq_codel, cake, codel, pie,
fq_pie, dualpi2 and RED. The check drops a packet when the 32-bit
sch->qstats.backlog would cross QDISC_MAX_BACKLOG
(U32_MAX - QDISC_PKT_LEN_MAX). The goal is to stop a crafted stab from
wrapping the backlog counter and corrupting AQM drop decisions.

Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/

> This follows the existing bfifo/gred approach
> (safe because its limit is checked against the accounted packet length);

[Severity: Medium]
Is this accurate for GRED? In non-WRED mode (plain or RIO),
gred_backlog() returns the per-VQ q->backlog. That means gred_enqueue()
only checks each virtual queue against its own q->limit:

net/sched/sch_gred.c:gred_enqueue() {
    ...
	if ((u64)gred_backlog(t, q, sch) + qdisc_pkt_len(skb) <= q->limit) {
		q->backlog += qdisc_pkt_len(skb);
		return qdisc_enqueue_tail(skb, sch);
	}
    ...
}

Every accepted packet is still added to the shared 32-bit
sch->qstats.backlog through qdisc_enqueue_tail(). Each q->limit is capped
at sch->limit, but there can be up to 16 VQs.

With two or more VQs whose limits add up to more than 4 GiB, plus the
same 1 MiB stab, can the aggregate wrap?

Once it wraps, the pass-through admission compares against a value that
is too small:

	if (likely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) <=
			sch->limit))
		return qdisc_enqueue_tail(skb, sch);

gred_backlog() returns this same aggregate in WRED mode. The WRED idle
check in gred_dequeue() also reads it if the table is switched to WRED
while packets are still queued. So a wrapped value could reach
red_calc_qavg().

Should GRED get the same QDISC_MAX_BACKLOG check, or should the commit
message be changed?

> Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
> Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
> Tested-by: hybris <hybris@mojatatu.ai>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
Should this have a Fixes: tag? The commit message describes a bug that
can be reached from a user namespace and includes a reproducer, but
there is no Fixes: tag, so the stable tooling will not pick it up
automatically.

QDISC_MAX_BACKLOG also depends on QDISC_PKT_LEN_MAX and on the pkt_len
clamp in __qdisc_calculate_pkt_len(). Both come from commit 8f735d64382d
("net/sched: bound qdisc_pkt_len to prevent qdisc soft lockup").

Could the commit message mention that dependency? A backport without it
will not build. 8f735d64382d has its own Fixes: 1da177e4c3f4, so it will
probably reach stable anyway.

> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> index 90d3e7943b19a..351b92f956efc 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -13,6 +13,11 @@
>  #define DEFAULT_TX_QUEUE_LEN	1000
>  #define STAB_SIZE_LOG_MAX	30
>  #define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */
> +/*
> + * Largest accounted backlog for which enqueuing one more maximum-size
> + * packet cannot wrap the 32-bit sch->qstats.backlog.
> + */
> +#define QDISC_MAX_BACKLOG	(U32_MAX - QDISC_PKT_LEN_MAX)

[Severity: Low]
Is QDISC_PKT_LEN_MAX really the largest possible accounted packet length?
The clamp is only applied in __qdisc_calculate_pkt_len(), which runs only
when a size table is attached:

	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);

Without a stab, qdisc_pkt_len_segs_init() sets the length with no clamp:

	qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len;

packet_snd() skips the MTU check for GSO packets and can build skbs
larger than 500 KB. virtio_net_hdr_to_skb() only forces
gso_size >= TCP_MIN_GSO_SIZE. So a GSO skb can carry an accounted length
well above 1 MiB.

The new checks compare in u64 against the real length, so they do not
wrap on their own. Still, the "one more maximum-size packet cannot wrap"
invariant in this comment and in the commit message does not hold. The
RED child segmentation question below relies on the same headroom
assumption.

>  
>  struct qdisc_walker {
>  	int	stop;
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..6a16546aa5b0e 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c

[ ... ]

> @@ -1801,7 +1809,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		WRITE_ONCE(b->max_skblen, len);
>  
>  	if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
> -		struct sk_buff *segs, *nskb;
> +		struct sk_buff *segs, *nskb, *seg;
>  		netdev_features_t features = netif_skb_features(skb);
>  		unsigned int slen = 0, numsegs = 0;
>  
> @@ -1809,6 +1817,23 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		if (IS_ERR_OR_NULL(segs))
>  			return qdisc_drop(skb, sch, to_free);
>  
> +		/* The segment list is accounted by the sum of its lengths,
> +		 * which can exceed the original packet's accounted length, so
> +		 * sum it before linking any segment and drop the whole list if
> +		 * the post-split total would cross the ceiling.
> +		 */
> +		skb_list_walk_safe(segs, seg, nskb)
> +			slen += seg->len;
> +
> +		if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {

[Severity: Medium]
Can slen wrap before the u64 comparison? slen is still an unsigned int.
If the segment list adds up to more than 4 GiB, the sum wraps and the
check passes with a small value.

cake would then add the wrapped slen to sch->qstats.backlog,
b->tin_backlog and b->backlogs[idx], while the queued segments keep
their real lengths. Later dequeue or cake_drop() subtractions would then
underflow those counters.

Here is how that size might be reached:

  - packet_snd() skips the MTU check for GSO packets and has no
    GSO_MAX_SIZE cap.
  - packet_alloc_skb() allows about 557 KB of order-3 frags plus a large
    linear part.
  - virtio_net_hdr_to_skb() only raises gso_size to TCP_MIN_GSO_SIZE (8).
  - ipv6_gso_pull_exthdrs() walks any number of GSO-capable extension
    headers.

A TCPv6 DODGY skb with about 60 KB of extension headers and 560 KB or
more of payload at gso_size 8 would split into more than 70k segments of
about 60 KB each. pkt_segs is a truncated u16 but is still > 1, so the
split path is taken.

This needs skb_segment() to succeed at more than 4 GiB of GFP_ATOMIC
allocations from a single packet. Not every check on the segmentation
path has been traced. Would a u64 accumulator for this pre-check be
simpler than working out whether it can happen?

> +			kfree_skb_list_reason(segs, SKB_DROP_REASON_QDISC_DROP);
> +			WRITE_ONCE(flow->dropped, flow->dropped + 1);
> +			WRITE_ONCE(b->tin_dropped, b->tin_dropped + 1);
> +			qdisc_qstats_overlimit(sch);
> +			return qdisc_drop_reason(skb, sch, to_free,
> +						 QDISC_DROP_OVERLIMIT);
> +		}

[Severity: Low]
When this ceiling is hit, the segments are freed with
SKB_DROP_REASON_QDISC_DROP. The original skb is then also dropped with
QDISC_DROP_OVERLIMIT.

Doesn't that produce N+1 kfree_skb tracepoint and drop-monitor events,
under two different reasons, for one rejected packet? flow->dropped,
tin_dropped and qstats.drops each go up by only one.

The segments were never queued. Could they be consumed instead, or
dropped with the same overlimit reason, so the event is reported once?
That would also match the changelog's statement that this "drops with
QDISC_DROP_OVERLIMIT".

[ ... ]

> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
> index d7598214270b8..dff3d8b0556bf 100644
> --- a/net/sched/sch_red.c
> +++ b/net/sched/sch_red.c
> @@ -76,6 +76,13 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	unsigned int len;
>  	int ret;
>  
> +	len = qdisc_pkt_len(skb);
> +	if (unlikely((u64)sch->qstats.backlog + len > QDISC_MAX_BACKLOG)) {
> +		qdisc_qstats_overlimit(sch);
> +		return qdisc_drop_reason(skb, sch, to_free,
> +					 QDISC_DROP_OVERLIMIT);
> +	}

[Severity: High]
Does this check still bound RED's backlog when the child segments GSO
skbs? It only looks at the length before segmentation, before the skb
is handed to the child.

With a TBF child, tbf_enqueue() calls tbf_segment() when
qdisc_pkt_len(skb) > q->max_size. tbf_segment() re-accounts every
segment at its raw length and pushes the difference up to RED:

net/sched/sch_tbf.c:tbf_segment() {
    ...
		seg_len = segs->len;
		qdisc_skb_cb(segs)->pkt_len = seg_len;
    ...
	qstats_backlog_add(sch, len);
	if (nb > 0) {
		qdisc_tree_reduce_backlog(sch, 1 - nb, prev_len - len);
    ...
}

qdisc_tree_reduce_backlog() calls qstats_backlog_sub() on each ancestor.
A negative adjustment therefore grows RED's backlog by (len - prev_len)
with no ceiling check. netem_enqueue() does the same through its
netem_segment() path:

	qdisc_tree_reduce_backlog(sch, -(nb - 1), -(len - prev_len));

A DODGY GSO skb from packet_snd() has no GSO size cap, and gso_size can
be as small as TCP_MIN_GSO_SIZE (8). With maximum IPv4/TCP headers
(134 bytes), about 60 KB of payload then yields more than 1 MiB of
segment bytes. A stab can map that skb to a length just above TBF
max_size, while filler packets map to 1 MiB.

Take the commit's own RED reproducer, with the packet-limited pfifo
placed under a TBF, and RED's backlog near QDISC_MAX_BACKLOG. Can both
RED's and TBF's 32-bit backlogs still wrap?

If so, red_enqueue() would feed the wrapped child->qstats.backlog into
red_calc_qavg(), which is what this patch is meant to prevent for RED.
cake and dualpi2 re-check after segmentation, but RED with a segmenting
child does not.

[Severity: Low]
This new drop path only increments qstats.overlimits and qstats.drops.
Should it also update one of RED's own counters? red_dump_stats()
exports pdrop from q->stats.pdrop, and the uAPI defines it as:

	__u32           pdrop;          /* Drops due to queue limits */

Every other drop path in red_enqueue() updates prob_drop, forced_drop or
pdrop, so a drop at the byte ceiling never shows up in RED's xstats.
Qevent TC_ACT_SHOT drops are already missing from xstats, so the
breakdown did not add up to qstats.drops before this patch either.

[ ... ]

> @@ -135,7 +142,6 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		break;
>  	}
>  
> -	len = qdisc_pkt_len(skb);
>  	ret = qdisc_enqueue(skb, child, to_free);
>  	if (likely(ret == NET_XMIT_SUCCESS)) {
>  		qstats_backlog_add(sch, len);

[Severity: High]
Since this read moved to the top of red_enqueue(), len is now taken
before the qe_mark tcf_qevent_handle() calls in the RED_PROB_MARK and
RED_HARD_MARK cases. Can a qevent action change qdisc_pkt_len(skb) in
between?

With RED in ecn mode and an act_ct action in the qe_mark block, act_ct
does this after a successful defragmentation:

net/sched/act_ct.c:tcf_ct_act() {
    ...
out_clear:
	if (defrag)
		qdisc_skb_cb(skb)->pkt_len = skb->len;
	return retval;
    ...
}

tcf_qevent_handle() returns the reassembled skb for any verdict other
than shot or stolen. RED then adds the stale len here, while
red_dequeue() subtracts the new length:

	qdisc_qstats_backlog_dec(sch, skb);

If the reassembled packet is larger, doesn't RED's u32 backlog underflow
to nearly 2^32, which is above QDISC_MAX_BACKLOG?

After that, every red_enqueue() would fail the new ceiling check even
with an empty child. RED would drop all traffic until the qdisc is
reset. If the stab-inflated fragment length is the larger one, the
backlog grows instead and eventually hits the ceiling too.

Before this patch, len was read after the qevents, so the two values
matched. Could the ceiling check use qdisc_pkt_len(skb) directly, and the
original read stay just before qdisc_enqueue()?

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

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

[Jamal Hadi Salim]

On Mon, Sep 28, 2026 at 8:34 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Sat, Sep 26, 2026 at 7:49 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > fq_codel, cake, codel, pie, fq_pie and dualpi2 account every enqueued
> > packet's stab-adjusted length into a 32-bit sch->qstats.backlog.
> > fq_codel/codel uses it do decide if they should drop a packet at deq;
> > cake uses it to prune the longest-flow heap from per-flow backlogs;
> > pie and fq_pie use it to make early drop decisions and, dualpi2 decides
> > must_drop() on it. RED can can decide on a child's backlog based on it.
> > A crafted tc size table (TC_STAB) inflates qdisc_pkt_len() up to
> > QDISC_PKT_LEN_MAX (1 MiB), so a few thousand packets wrap the counter
> > mod 2^32. The AQM algo then reads a small backlog and makes the wrong
> > drop decision, and the dequeue-side subtractions keep the counter corrupt.
> >
> > Fix:
> > Drop at enqueue once the accounted backlog would cross QDISC_MAX_BACKLOG
> > (U32_MAX - QDISC_PKT_LEN_MAX), the largest backlog one more maximum-size
> > packet cannot wrap.  This follows the existing bfifo/gred approach
> > (safe because its limit is checked against the accounted packet length);
> > the fixed qdiscs' limits are packet counts or otherwise do not bound
> > the aggregate bytes, so they need the byte bound here.
>
> Hi Jamal,
>


Most of these look reasonable.

pw-bot: cr

cheeers,
jamal
> Thanks for reworking this for v2. A few comments on the implementation:
> 1. QDISC_MAX_BACKLOG check
> Since QDISC_MAX_BACKLOG is already defined as
> (U32_MAX - QDISC_PKT_LEN_MAX), it already reserves QDISC_PKT_LEN_MAX
> of headroom below U32_MAX for the incoming packet. Doing:
>     if (unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) >
>                  QDISC_MAX_BACKLOG))
> accounts for the incoming packet size twice and forces a 64-bit addition
> at every call site.
> Why not just check unlikely(sch->qstats.backlog > QDISC_MAX_BACKLOG)
> (or provide a small helper in include/net/sch_generic.h)?
>
>
> 2. sch_cake.c (cake_enqueue)
> There are a few issues with how cake_enqueue() is handled:
> - The first check is placed after cake_classify(), which has already
>   modified the packet's DSCP (cake_handle_diffserv()) and updated
>   set-associative hash state and host bulk-flow counters in cake_hash()
>   (srchost_bulk_flow_count / dsthost_bulk_flow_count) assuming the
>   packet will be enqueued into flow.
> - In the CAKE_FLAG_SPLIT_GSO branch, the second check runs after lines
>   1780-1801 have already updated b->max_skblen, shaper timestamps
>   (time_next_packet), qstats.overlimits, and scheduled &q->watchdog.
> - Walking segs a second time on every GSO packet just to sum slen is
>   unnecessary, and calling both kfree_skb_list_reason(segs, ...) (under
>   the qdisc lock instead of via to_free) and qdisc_drop_reason(skb, ...)
>   triggers duplicate drop tracepoints for both the segments and the
>   parent GSO skb.
> Since QDISC_MAX_BACKLOG already leaves QDISC_PKT_LEN_MAX (1 MiB) of
> headroom, a single check at the very beginning of cake_enqueue() before
> cake_classify() is sufficient and avoids touching the GSO split path
> altogether.
>
> 3. sch_fq_codel.c (fq_codel_enqueue)
> Can we move the backlog check before fq_codel_classify() (or in the
> !q->filter_list fast path before fq_codel_hash()) so we do not compute
> the flow hash for packets we are about to drop?
> Also, please mention in the commit message that wrapping q->backlogs[i]
> to 0 in fq_codel causes fq_codel_drop() to leave idx = 0 and dereference
> a NULL flow->head if flow 0 is empty.
>
> 4. sch_red.c and sch_fifo.c (pfifo)
> In red_enqueue(), red_calc_qavg() reads child->qstats.backlog. In your
> commit message example, child->qstats.backlog wrapped because the child
> was a packet-limited pfifo (limit 100000).
> Shouldn't pfifo_enqueue() and pfifo_tail_enqueue() in
> net/sched/sch_fifo.c also guard against qstats.backlog wrapping when
> used standalone or under other classful qdiscs?
>
> Thanks!

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

[Jamal Hadi Salim]

On Mon, Sep 28, 2026 at 8:51 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 8 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · Medium: 2 · 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).
>
> - [High] RED's new ceiling check (red_enqueue(), sch_red.c:79-84) only
>   tests sch->qstats.backlog + qdisc_pkt_len(skb) before the skb goes to…
> - [High] The patch moves `len = qdisc_pkt_len(skb)` in red_enqueue() from
>   after the red_action() switch to the top of the function…
> - [Medium] The commit message leaves GRED untouched, saying it follows a
>   safe approach ("safe because its limit is checked against the…
> - [Medium] In the new split-GSO pre-check in cake_enqueue(), segment
>   lengths are summed into `unsigned int slen` (declared at…
> - [Low] The commit fixes a user-visible bug.
> - [Low] In the split-GSO path of cake_enqueue(), when the post-split
>   total would cross the ceiling, the patch frees every temporary segment…
> - [Low] The comment on QDISC_MAX_BACKLOG (include/net/pkt_sched.h:16-19)
>   and the commit message treat QDISC_PKT_LEN_MAX (1 MiB) as the maximum…
> - [Low] The new RED ceiling rejection in red_enqueue() (sch_red.c:80-84)
>   only calls qdisc_qstats_overlimit() and qdisc_drop_reason().
>

I will send a V3 to address some of these issues (also brought up by Eric)

cheers,
jamal

> --
>
> net/sched: cap the accounted backlog before it can wrap
>
> This patch adds an enqueue-side check to fq_codel, cake, codel, pie,
> fq_pie, dualpi2 and RED. The check drops a packet when the 32-bit
> sch->qstats.backlog would cross QDISC_MAX_BACKLOG
> (U32_MAX - QDISC_PKT_LEN_MAX). The goal is to stop a crafted stab from
> wrapping the backlog counter and corrupting AQM drop decisions.
>
> Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
>
> > This follows the existing bfifo/gred approach
> > (safe because its limit is checked against the accounted packet length);
>
> [Severity: Medium]
> Is this accurate for GRED? In non-WRED mode (plain or RIO),
> gred_backlog() returns the per-VQ q->backlog. That means gred_enqueue()
> only checks each virtual queue against its own q->limit:
>
> net/sched/sch_gred.c:gred_enqueue() {
>     ...
>         if ((u64)gred_backlog(t, q, sch) + qdisc_pkt_len(skb) <= q->limit) {
>                 q->backlog += qdisc_pkt_len(skb);
>                 return qdisc_enqueue_tail(skb, sch);
>         }
>     ...
> }
>
> Every accepted packet is still added to the shared 32-bit
> sch->qstats.backlog through qdisc_enqueue_tail(). Each q->limit is capped
> at sch->limit, but there can be up to 16 VQs.
>
> With two or more VQs whose limits add up to more than 4 GiB, plus the
> same 1 MiB stab, can the aggregate wrap?
>
> Once it wraps, the pass-through admission compares against a value that
> is too small:
>
>         if (likely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) <=
>                         sch->limit))
>                 return qdisc_enqueue_tail(skb, sch);
>
> gred_backlog() returns this same aggregate in WRED mode. The WRED idle
> check in gred_dequeue() also reads it if the table is switched to WRED
> while packets are still queued. So a wrapped value could reach
> red_calc_qavg().
>
> Should GRED get the same QDISC_MAX_BACKLOG check, or should the commit
> message be changed?
>
> > Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
> > Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
> > Tested-by: hybris <hybris@mojatatu.ai>
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> [Severity: Low]
> Should this have a Fixes: tag? The commit message describes a bug that
> can be reached from a user namespace and includes a reproducer, but
> there is no Fixes: tag, so the stable tooling will not pick it up
> automatically.
>
> QDISC_MAX_BACKLOG also depends on QDISC_PKT_LEN_MAX and on the pkt_len
> clamp in __qdisc_calculate_pkt_len(). Both come from commit 8f735d64382d
> ("net/sched: bound qdisc_pkt_len to prevent qdisc soft lockup").
>
> Could the commit message mention that dependency? A backport without it
> will not build. 8f735d64382d has its own Fixes: 1da177e4c3f4, so it will
> probably reach stable anyway.
>
> > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> > index 90d3e7943b19a..351b92f956efc 100644
> > --- a/include/net/pkt_sched.h
> > +++ b/include/net/pkt_sched.h
> > @@ -13,6 +13,11 @@
> >  #define DEFAULT_TX_QUEUE_LEN 1000
> >  #define STAB_SIZE_LOG_MAX    30
> >  #define QDISC_PKT_LEN_MAX    (1 << 20)       /* 1 MiB */
> > +/*
> > + * Largest accounted backlog for which enqueuing one more maximum-size
> > + * packet cannot wrap the 32-bit sch->qstats.backlog.
> > + */
> > +#define QDISC_MAX_BACKLOG    (U32_MAX - QDISC_PKT_LEN_MAX)
>
> [Severity: Low]
> Is QDISC_PKT_LEN_MAX really the largest possible accounted packet length?
> The clamp is only applied in __qdisc_calculate_pkt_len(), which runs only
> when a size table is attached:
>
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>
> Without a stab, qdisc_pkt_len_segs_init() sets the length with no clamp:
>
>         qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len;
>
> packet_snd() skips the MTU check for GSO packets and can build skbs
> larger than 500 KB. virtio_net_hdr_to_skb() only forces
> gso_size >= TCP_MIN_GSO_SIZE. So a GSO skb can carry an accounted length
> well above 1 MiB.
>
> The new checks compare in u64 against the real length, so they do not
> wrap on their own. Still, the "one more maximum-size packet cannot wrap"
> invariant in this comment and in the commit message does not hold. The
> RED child segmentation question below relies on the same headroom
> assumption.
>
> >
> >  struct qdisc_walker {
> >       int     stop;
> > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > index dc93267029e70..6a16546aa5b0e 100644
> > --- a/net/sched/sch_cake.c
> > +++ b/net/sched/sch_cake.c
>
> [ ... ]
>
> > @@ -1801,7 +1809,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >               WRITE_ONCE(b->max_skblen, len);
> >
> >       if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
> > -             struct sk_buff *segs, *nskb;
> > +             struct sk_buff *segs, *nskb, *seg;
> >               netdev_features_t features = netif_skb_features(skb);
> >               unsigned int slen = 0, numsegs = 0;
> >
> > @@ -1809,6 +1817,23 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >               if (IS_ERR_OR_NULL(segs))
> >                       return qdisc_drop(skb, sch, to_free);
> >
> > +             /* The segment list is accounted by the sum of its lengths,
> > +              * which can exceed the original packet's accounted length, so
> > +              * sum it before linking any segment and drop the whole list if
> > +              * the post-split total would cross the ceiling.
> > +              */
> > +             skb_list_walk_safe(segs, seg, nskb)
> > +                     slen += seg->len;
> > +
> > +             if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
>
> [Severity: Medium]
> Can slen wrap before the u64 comparison? slen is still an unsigned int.
> If the segment list adds up to more than 4 GiB, the sum wraps and the
> check passes with a small value.
>
> cake would then add the wrapped slen to sch->qstats.backlog,
> b->tin_backlog and b->backlogs[idx], while the queued segments keep
> their real lengths. Later dequeue or cake_drop() subtractions would then
> underflow those counters.
>
> Here is how that size might be reached:
>
>   - packet_snd() skips the MTU check for GSO packets and has no
>     GSO_MAX_SIZE cap.
>   - packet_alloc_skb() allows about 557 KB of order-3 frags plus a large
>     linear part.
>   - virtio_net_hdr_to_skb() only raises gso_size to TCP_MIN_GSO_SIZE (8).
>   - ipv6_gso_pull_exthdrs() walks any number of GSO-capable extension
>     headers.
>
> A TCPv6 DODGY skb with about 60 KB of extension headers and 560 KB or
> more of payload at gso_size 8 would split into more than 70k segments of
> about 60 KB each. pkt_segs is a truncated u16 but is still > 1, so the
> split path is taken.
>
> This needs skb_segment() to succeed at more than 4 GiB of GFP_ATOMIC
> allocations from a single packet. Not every check on the segmentation
> path has been traced. Would a u64 accumulator for this pre-check be
> simpler than working out whether it can happen?
>
> > +                     kfree_skb_list_reason(segs, SKB_DROP_REASON_QDISC_DROP);
> > +                     WRITE_ONCE(flow->dropped, flow->dropped + 1);
> > +                     WRITE_ONCE(b->tin_dropped, b->tin_dropped + 1);
> > +                     qdisc_qstats_overlimit(sch);
> > +                     return qdisc_drop_reason(skb, sch, to_free,
> > +                                              QDISC_DROP_OVERLIMIT);
> > +             }
>
> [Severity: Low]
> When this ceiling is hit, the segments are freed with
> SKB_DROP_REASON_QDISC_DROP. The original skb is then also dropped with
> QDISC_DROP_OVERLIMIT.
>
> Doesn't that produce N+1 kfree_skb tracepoint and drop-monitor events,
> under two different reasons, for one rejected packet? flow->dropped,
> tin_dropped and qstats.drops each go up by only one.
>
> The segments were never queued. Could they be consumed instead, or
> dropped with the same overlimit reason, so the event is reported once?
> That would also match the changelog's statement that this "drops with
> QDISC_DROP_OVERLIMIT".
>
> [ ... ]
>
> > diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
> > index d7598214270b8..dff3d8b0556bf 100644
> > --- a/net/sched/sch_red.c
> > +++ b/net/sched/sch_red.c
> > @@ -76,6 +76,13 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >       unsigned int len;
> >       int ret;
> >
> > +     len = qdisc_pkt_len(skb);
> > +     if (unlikely((u64)sch->qstats.backlog + len > QDISC_MAX_BACKLOG)) {
> > +             qdisc_qstats_overlimit(sch);
> > +             return qdisc_drop_reason(skb, sch, to_free,
> > +                                      QDISC_DROP_OVERLIMIT);
> > +     }
>
> [Severity: High]
> Does this check still bound RED's backlog when the child segments GSO
> skbs? It only looks at the length before segmentation, before the skb
> is handed to the child.
>
> With a TBF child, tbf_enqueue() calls tbf_segment() when
> qdisc_pkt_len(skb) > q->max_size. tbf_segment() re-accounts every
> segment at its raw length and pushes the difference up to RED:
>
> net/sched/sch_tbf.c:tbf_segment() {
>     ...
>                 seg_len = segs->len;
>                 qdisc_skb_cb(segs)->pkt_len = seg_len;
>     ...
>         qstats_backlog_add(sch, len);
>         if (nb > 0) {
>                 qdisc_tree_reduce_backlog(sch, 1 - nb, prev_len - len);
>     ...
> }
>
> qdisc_tree_reduce_backlog() calls qstats_backlog_sub() on each ancestor.
> A negative adjustment therefore grows RED's backlog by (len - prev_len)
> with no ceiling check. netem_enqueue() does the same through its
> netem_segment() path:
>
>         qdisc_tree_reduce_backlog(sch, -(nb - 1), -(len - prev_len));
>
> A DODGY GSO skb from packet_snd() has no GSO size cap, and gso_size can
> be as small as TCP_MIN_GSO_SIZE (8). With maximum IPv4/TCP headers
> (134 bytes), about 60 KB of payload then yields more than 1 MiB of
> segment bytes. A stab can map that skb to a length just above TBF
> max_size, while filler packets map to 1 MiB.
>
> Take the commit's own RED reproducer, with the packet-limited pfifo
> placed under a TBF, and RED's backlog near QDISC_MAX_BACKLOG. Can both
> RED's and TBF's 32-bit backlogs still wrap?
>
> If so, red_enqueue() would feed the wrapped child->qstats.backlog into
> red_calc_qavg(), which is what this patch is meant to prevent for RED.
> cake and dualpi2 re-check after segmentation, but RED with a segmenting
> child does not.
>
> [Severity: Low]
> This new drop path only increments qstats.overlimits and qstats.drops.
> Should it also update one of RED's own counters? red_dump_stats()
> exports pdrop from q->stats.pdrop, and the uAPI defines it as:
>
>         __u32           pdrop;          /* Drops due to queue limits */
>
> Every other drop path in red_enqueue() updates prob_drop, forced_drop or
> pdrop, so a drop at the byte ceiling never shows up in RED's xstats.
> Qevent TC_ACT_SHOT drops are already missing from xstats, so the
> breakdown did not add up to qstats.drops before this patch either.
>
> [ ... ]
>
> > @@ -135,7 +142,6 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >               break;
> >       }
> >
> > -     len = qdisc_pkt_len(skb);
> >       ret = qdisc_enqueue(skb, child, to_free);
> >       if (likely(ret == NET_XMIT_SUCCESS)) {
> >               qstats_backlog_add(sch, len);
>
> [Severity: High]
> Since this read moved to the top of red_enqueue(), len is now taken
> before the qe_mark tcf_qevent_handle() calls in the RED_PROB_MARK and
> RED_HARD_MARK cases. Can a qevent action change qdisc_pkt_len(skb) in
> between?
>
> With RED in ecn mode and an act_ct action in the qe_mark block, act_ct
> does this after a successful defragmentation:
>
> net/sched/act_ct.c:tcf_ct_act() {
>     ...
> out_clear:
>         if (defrag)
>                 qdisc_skb_cb(skb)->pkt_len = skb->len;
>         return retval;
>     ...
> }
>
> tcf_qevent_handle() returns the reassembled skb for any verdict other
> than shot or stolen. RED then adds the stale len here, while
> red_dequeue() subtracts the new length:
>
>         qdisc_qstats_backlog_dec(sch, skb);
>
> If the reassembled packet is larger, doesn't RED's u32 backlog underflow
> to nearly 2^32, which is above QDISC_MAX_BACKLOG?
>
> After that, every red_enqueue() would fail the new ceiling check even
> with an empty child. RED would drop all traffic until the qdisc is
> reset. If the stab-inflated fragment length is the larger one, the
> backlog grows instead and eventually hits the ceiling too.
>
> Before this patch, len was read after the qevents, so the two values
> matched. Could the ceiling check use qdisc_pkt_len(skb) directly, and the
> original read stay just before qdisc_enqueue()?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-BA27.v2.20260926123300%40mojatatu.com