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

[Cake] [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

[Cake] 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!

[Cake] 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!

[Cake] 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