* [Cake] [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
@ 2026-09-25 8:53 Jamal Hadi Salim
2026-09-25 9:12 ` [Cake] " Eric Dumazet
2026-09-28 8:54 ` netdev-bot+sashiko
0 siblings, 2 replies; 12+ messages in thread
From: Jamal Hadi Salim @ 2026-09-25 8:53 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Toke Høiland-Jørgensen, cake,
Jiri Pirko, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Victor Nogueira, hybris, Sashiko
This is a follow-up to commit 8f735d64382d ("net/sched: bound
qdisc_pkt_len to prevent qdisc soft lockup"), which capped
qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
amplifier but leaves the per-flow backlog counter u32:
fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
and drops from an almost-empty flow, and the dequeue-side subtractions
corrupt the counter further.
Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
the drop threshold to u64. fq_codel is not lockless: every writer runs
under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
publish / READ_ONCE-consume pattern. The dump path
(fq_codel_dump_class_stats) stays a lockless stat-only read.
CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
b->backlogs[] and per-tin b->tin_backlog and consumes the values for
longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
staleness check, so it shares the bug. Widen those counters and the heap
comparison locals to u64; the class/tin stats keep exporting the low 32
bits through the unchanged uAPI fields.
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 4200 ecn drop_batch 4096
# hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
# backlog persists, then send at least 4300 packets (the wrap starts
# at 4096 resident; the over-limit drop that reads the wrapped
# threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
# 1 MiB and the fat-flow threshold reads the wrapped value.
With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
drop burst).
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>
---
net/sched/sch_cake.c | 23 ++++++++++++-----------
net/sched/sch_fq_codel.c | 22 ++++++++++++----------
2 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..8e99c85dab17 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -150,7 +150,7 @@ struct cake_heap_entry {
struct cake_tin_data {
struct cake_flow flows[CAKE_QUEUES];
- u32 backlogs[CAKE_QUEUES];
+ u64 backlogs[CAKE_QUEUES];
u32 tags[CAKE_QUEUES]; /* for set association */
u16 overflow_idx[CAKE_QUEUES];
struct cake_host hosts[CAKE_QUEUES]; /* for triple isolation */
@@ -177,7 +177,7 @@ struct cake_tin_data {
u16 tin_quantum;
s32 tin_deficit;
- u32 tin_backlog;
+ u64 tin_backlog;
u32 tin_dropped;
u32 tin_ecn_mark;
@@ -1466,17 +1466,17 @@ static void cake_heap_swap(struct cake_sched_data *q, u16 i, u16 j)
q->tins[jj.t].overflow_idx[jj.b] = i;
}
-static u32 cake_heap_get_backlog(const struct cake_sched_data *q, u16 i)
+static u64 cake_heap_get_backlog(const struct cake_sched_data *q, u16 i)
{
struct cake_heap_entry ii = q->overflow_heap[i];
- return q->tins[ii.t].backlogs[ii.b];
+ return READ_ONCE(q->tins[ii.t].backlogs[ii.b]);
}
static void cake_heapify(struct cake_sched_data *q, u16 i)
{
static const u32 a = CAKE_MAX_TINS * CAKE_QUEUES;
- u32 mb = cake_heap_get_backlog(q, i);
+ u64 mb = cake_heap_get_backlog(q, i);
u32 m = i;
while (m < a) {
@@ -1484,7 +1484,7 @@ static void cake_heapify(struct cake_sched_data *q, u16 i)
u32 r = l + 1;
if (l < a) {
- u32 lb = cake_heap_get_backlog(q, l);
+ u64 lb = cake_heap_get_backlog(q, l);
if (lb > mb) {
m = l;
@@ -1493,7 +1493,7 @@ static void cake_heapify(struct cake_sched_data *q, u16 i)
}
if (r < a) {
- u32 rb = cake_heap_get_backlog(q, r);
+ u64 rb = cake_heap_get_backlog(q, r);
if (rb > mb) {
m = r;
@@ -1514,8 +1514,8 @@ static void cake_heapify_up(struct cake_sched_data *q, u16 i)
{
while (i > 0 && i < CAKE_MAX_TINS * CAKE_QUEUES) {
u16 p = (i - 1) >> 1;
- u32 ib = cake_heap_get_backlog(q, i);
- u32 pb = cake_heap_get_backlog(q, p);
+ u64 ib = cake_heap_get_backlog(q, i);
+ u64 pb = cake_heap_get_backlog(q, p);
if (ib > pb) {
cake_heap_swap(q, i, p);
@@ -3046,7 +3046,8 @@ static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
PUT_TSTAT_U64(THRESHOLD_RATE64, READ_ONCE(b->tin_rate_bps));
PUT_TSTAT_U64(SENT_BYTES64, READ_ONCE(b->bytes));
- PUT_TSTAT_U32(BACKLOG_BYTES, READ_ONCE(b->tin_backlog));
+ PUT_TSTAT_U32(BACKLOG_BYTES,
+ (u32)READ_ONCE(b->tin_backlog));
PUT_TSTAT_U32(TARGET_US,
ktime_to_us(ns_to_ktime(READ_ONCE(b->cparams.target))));
@@ -3152,7 +3153,7 @@ static int cake_dump_class_stats(struct Qdisc *sch, unsigned long cl,
}
sch_tree_unlock(sch);
}
- qs.backlog = READ_ONCE(b->backlogs[idx % CAKE_QUEUES]);
+ qs.backlog = (u32)READ_ONCE(b->backlogs[idx % CAKE_QUEUES]);
qs.drops = READ_ONCE(flow->dropped);
}
if (gnet_stats_copy_queue(d, NULL, &qs, qs.qlen) < 0)
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 969b2510b0b8..3c20297cef07 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -51,7 +51,7 @@ struct fq_codel_sched_data {
struct tcf_proto __rcu *filter_list; /* optional external classifier */
struct tcf_block *block;
struct fq_codel_flow *flows; /* Flows table [flows_cnt] */
- u32 *backlogs; /* backlog table [flows_cnt] */
+ u64 *backlogs; /* backlog table [flows_cnt] */
u32 flows_cnt; /* number of flows */
u32 quantum; /* psched_mtu(qdisc_dev(sch)); */
u32 drop_batch_size;
@@ -138,22 +138,25 @@ static unsigned int fq_codel_drop(struct Qdisc *sch, unsigned int max_packets,
struct sk_buff **to_free)
{
struct fq_codel_sched_data *q = qdisc_priv(sch);
+ u64 maxbacklog = 0, len = 0;
struct sk_buff *skb;
- unsigned int maxbacklog = 0, idx = 0, i, len;
struct fq_codel_flow *flow;
- unsigned int threshold;
+ unsigned int idx = 0, i;
unsigned int mem = 0;
+ u64 threshold;
/* Queue is full! Find the fat flow and drop packet(s) from it.
* This might sound expensive, but with 1024 flows, we scan
- * 4KB of memory, and we dont need to handle a complex tree
+ * 8KB of memory, and we dont need to handle a complex tree
* in fast path (packet queue/enqueue) with many cache misses.
* In stress mode, we'll try to drop 64 packets from the flow,
* amortizing this linear lookup to one cache line per drop.
*/
for (i = 0; i < q->flows_cnt; i++) {
- if (q->backlogs[i] > maxbacklog) {
- maxbacklog = q->backlogs[i];
+ u64 backlog = READ_ONCE(q->backlogs[i]);
+
+ if (backlog > maxbacklog) {
+ maxbacklog = backlog;
idx = i;
}
}
@@ -162,7 +165,6 @@ static unsigned int fq_codel_drop(struct Qdisc *sch, unsigned int max_packets,
threshold = maxbacklog >> 1;
flow = &q->flows[idx];
- len = 0;
i = 0;
do {
skb = dequeue_head(flow);
@@ -384,7 +386,7 @@ static void fq_codel_reset(struct Qdisc *sch)
INIT_LIST_HEAD(&flow->flowchain);
codel_vars_init(&flow->cvars);
}
- memset(q->backlogs, 0, q->flows_cnt * sizeof(u32));
+ memset(q->backlogs, 0, q->flows_cnt * sizeof(u64));
q->memory_usage = 0;
}
@@ -542,7 +544,7 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
err = -ENOMEM;
goto init_failure;
}
- q->backlogs = kvcalloc(q->flows_cnt, sizeof(u32), GFP_KERNEL);
+ q->backlogs = kvcalloc(q->flows_cnt, sizeof(u64), GFP_KERNEL);
if (!q->backlogs) {
err = -ENOMEM;
goto alloc_failure;
@@ -720,7 +722,7 @@ static int fq_codel_dump_class_stats(struct Qdisc *sch, unsigned long cl,
}
sch_tree_unlock(sch);
}
- qs.backlog = READ_ONCE(q->backlogs[idx]);
+ qs.backlog = (u32)READ_ONCE(q->backlogs[idx]);
qs.drops = 0;
}
if (gnet_stats_copy_queue(d, NULL, &qs, qs.qlen) < 0)
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 8:53 [Cake] [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64 Jamal Hadi Salim
@ 2026-09-25 9:12 ` Eric Dumazet
2026-09-25 11:27 ` Jamal Hadi Salim
2026-09-28 8:54 ` netdev-bot+sashiko
1 sibling, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2026-09-25 9:12 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, Toke Høiland-Jørgensen, cake, Jiri Pirko,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Victor Nogueira, hybris, Sashiko
On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> This is a follow-up to commit 8f735d64382d ("net/sched: bound
> qdisc_pkt_len to prevent qdisc soft lockup"), which capped
> qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
> amplifier but leaves the per-flow backlog counter u32:
> fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
> so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
> counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
> and drops from an almost-empty flow, and the dequeue-side subtractions
> corrupt the counter further.
>
> Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
> the drop threshold to u64. fq_codel is not lockless: every writer runs
> under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
> publish / READ_ONCE-consume pattern. The dump path
> (fq_codel_dump_class_stats) stays a lockless stat-only read.
>
> CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
> b->backlogs[] and per-tin b->tin_backlog and consumes the values for
> longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
> staleness check, so it shares the bug. Widen those counters and the heap
> comparison locals to u64; the class/tin stats keep exporting the low 32
> bits through the unchanged uAPI fields.
>
> 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 4200 ecn drop_batch 4096
> # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
> # backlog persists, then send at least 4300 packets (the wrap starts
> # at 4096 resident; the over-limit drop that reads the wrapped
> # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
> # 1 MiB and the fat-flow threshold reads the wrapped value.
>
> With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
> packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
> wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
> where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
> drop burst).
>
> 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>
> ---
This makes no sense.
These qdisc have been developped to address bufferbloat issues.
Storing 4GB in a qdisc is absolutely insane.
Let's drop at enqueue if the current backlog is approaching 4GB (this
can later be a new config/attribute in net-next)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 9:12 ` [Cake] " Eric Dumazet
@ 2026-09-25 11:27 ` Jamal Hadi Salim
2026-09-25 12:22 ` Jamal Hadi Salim
2026-09-25 13:03 ` Eric Dumazet
0 siblings, 2 replies; 12+ messages in thread
From: Jamal Hadi Salim @ 2026-09-25 11:27 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Toke Høiland-Jørgensen, cake, Jiri Pirko,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Victor Nogueira, hybris, Sashiko
On Fri, Sep 25, 2026 at 5:13 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > This is a follow-up to commit 8f735d64382d ("net/sched: bound
> > qdisc_pkt_len to prevent qdisc soft lockup"), which capped
> > qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
> > amplifier but leaves the per-flow backlog counter u32:
> > fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
> > so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
> > counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
> > and drops from an almost-empty flow, and the dequeue-side subtractions
> > corrupt the counter further.
> >
> > Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
> > the drop threshold to u64. fq_codel is not lockless: every writer runs
> > under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
> > publish / READ_ONCE-consume pattern. The dump path
> > (fq_codel_dump_class_stats) stays a lockless stat-only read.
> >
> > CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
> > b->backlogs[] and per-tin b->tin_backlog and consumes the values for
> > longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
> > staleness check, so it shares the bug. Widen those counters and the heap
> > comparison locals to u64; the class/tin stats keep exporting the low 32
> > bits through the unchanged uAPI fields.
> >
> > 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 4200 ecn drop_batch 4096
> > # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
> > # backlog persists, then send at least 4300 packets (the wrap starts
> > # at 4096 resident; the over-limit drop that reads the wrapped
> > # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
> > # 1 MiB and the fat-flow threshold reads the wrapped value.
> >
> > With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
> > packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
> > wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
> > where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
> > drop burst).
> >
> > 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>
> > ---
>
> This makes no sense.
>
As absurd as it looks that code is reachable ;->
> These qdisc have been developped to address bufferbloat issues.
>
> Storing 4GB in a qdisc is absolutely insane.
>
The counter wrap is not because we stored 4GB, rather it is because
"tc .. stab overhead ..." inflates qdisc_pkt_len() for a 64B pkt to
1MB. So ~4K packets (put in other words a few "real" KB) makes that
backlog[0] cross 2^32.
Result is pruning the wrong flow..
> Let's drop at enqueue if the current backlog is approaching 4GB (this
> can later be a new config/attribute in net-next)
Note, this is net-next already. Enqueue is fast path - are you ok with that?
cheers,
jamal
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 11:27 ` Jamal Hadi Salim
@ 2026-09-25 12:22 ` Jamal Hadi Salim
2026-09-25 13:03 ` Eric Dumazet
1 sibling, 0 replies; 12+ messages in thread
From: Jamal Hadi Salim @ 2026-09-25 12:22 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Toke Høiland-Jørgensen, cake, Jiri Pirko,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Victor Nogueira, hybris, Sashiko
On Fri, Sep 25, 2026 at 7:27 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Fri, Sep 25, 2026 at 5:13 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > This is a follow-up to commit 8f735d64382d ("net/sched: bound
> > > qdisc_pkt_len to prevent qdisc soft lockup"), which capped
> > > qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
> > > amplifier but leaves the per-flow backlog counter u32:
> > > fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
> > > so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
> > > counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
> > > and drops from an almost-empty flow, and the dequeue-side subtractions
> > > corrupt the counter further.
> > >
> > > Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
> > > the drop threshold to u64. fq_codel is not lockless: every writer runs
> > > under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
> > > publish / READ_ONCE-consume pattern. The dump path
> > > (fq_codel_dump_class_stats) stays a lockless stat-only read.
> > >
> > > CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
> > > b->backlogs[] and per-tin b->tin_backlog and consumes the values for
> > > longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
> > > staleness check, so it shares the bug. Widen those counters and the heap
> > > comparison locals to u64; the class/tin stats keep exporting the low 32
> > > bits through the unchanged uAPI fields.
> > >
> > > 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 4200 ecn drop_batch 4096
> > > # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
> > > # backlog persists, then send at least 4300 packets (the wrap starts
> > > # at 4096 resident; the over-limit drop that reads the wrapped
> > > # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
> > > # 1 MiB and the fat-flow threshold reads the wrapped value.
> > >
> > > With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
> > > packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
> > > wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
> > > where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
> > > drop burst).
> > >
> > > 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>
> > > ---
> >
> > This makes no sense.
> >
>
> As absurd as it looks that code is reachable ;->
>
> > These qdisc have been developped to address bufferbloat issues.
> >
> > Storing 4GB in a qdisc is absolutely insane.
> >
>
> The counter wrap is not because we stored 4GB, rather it is because
> "tc .. stab overhead ..." inflates qdisc_pkt_len() for a 64B pkt to
> 1MB. So ~4K packets (put in other words a few "real" KB) makes that
> backlog[0] cross 2^32.
> Result is pruning the wrong flow..
>
> > Let's drop at enqueue if the current backlog is approaching 4GB (this
> > can later be a new config/attribute in net-next)
>
> Note, this is net-next already. Enqueue is fast path - are you ok with that?
>
And the least ugly approach (not tested) is attached.
cheers,
jamal
> cheers,
> jamal
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 11:27 ` Jamal Hadi Salim
2026-09-25 12:22 ` Jamal Hadi Salim
@ 2026-09-25 13:03 ` Eric Dumazet
2026-09-25 13:30 ` Sebastian Moeller
1 sibling, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2026-09-25 13:03 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, Toke Høiland-Jørgensen, cake, Jiri Pirko,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Victor Nogueira, hybris, Sashiko
On Fri, Sep 25, 2026 at 1:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Fri, Sep 25, 2026 at 5:13 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > This is a follow-up to commit 8f735d64382d ("net/sched: bound
> > > qdisc_pkt_len to prevent qdisc soft lockup"), which capped
> > > qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
> > > amplifier but leaves the per-flow backlog counter u32:
> > > fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
> > > so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
> > > counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
> > > and drops from an almost-empty flow, and the dequeue-side subtractions
> > > corrupt the counter further.
> > >
> > > Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
> > > the drop threshold to u64. fq_codel is not lockless: every writer runs
> > > under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
> > > publish / READ_ONCE-consume pattern. The dump path
> > > (fq_codel_dump_class_stats) stays a lockless stat-only read.
> > >
> > > CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
> > > b->backlogs[] and per-tin b->tin_backlog and consumes the values for
> > > longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
> > > staleness check, so it shares the bug. Widen those counters and the heap
> > > comparison locals to u64; the class/tin stats keep exporting the low 32
> > > bits through the unchanged uAPI fields.
> > >
> > > 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 4200 ecn drop_batch 4096
> > > # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
> > > # backlog persists, then send at least 4300 packets (the wrap starts
> > > # at 4096 resident; the over-limit drop that reads the wrapped
> > > # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
> > > # 1 MiB and the fat-flow threshold reads the wrapped value.
> > >
> > > With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
> > > packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
> > > wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
> > > where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
> > > drop burst).
> > >
> > > 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>
> > > ---
> >
> > This makes no sense.
> >
>
> As absurd as it looks that code is reachable ;->
>
> > These qdisc have been developped to address bufferbloat issues.
> >
> > Storing 4GB in a qdisc is absolutely insane.
> >
>
> The counter wrap is not because we stored 4GB, rather it is because
> "tc .. stab overhead ..." inflates qdisc_pkt_len() for a 64B pkt to
> 1MB. So ~4K packets (put in other words a few "real" KB) makes that
> backlog[0] cross 2^32.
Kill this stuff ? Who is still using this, for what reason ?
Really, it is time we stop adding code only for fuzzers.
> Result is pruning the wrong flow..
>
> > Let's drop at enqueue if the current backlog is approaching 4GB (this
> > can later be a new config/attribute in net-next)
>
> Note, this is net-next already. Enqueue is fast path - are you ok with that?
>
> cheers,
> jamal
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 13:03 ` Eric Dumazet
@ 2026-09-25 13:30 ` Sebastian Moeller
2026-09-25 13:43 ` Eric Dumazet
0 siblings, 1 reply; 12+ messages in thread
From: Sebastian Moeller @ 2026-09-25 13:30 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jamal Hadi Salim, netdev, Toke Høiland-Jørgensen, cake,
Jiri Pirko, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, Victor Nogueira, hybris, Sashiko
Hi Eric,
> On Sep 25, 2026, at 15:03, Eric Dumazet via Cake <cake@lists.bufferbloat.net> wrote:
>
> On Fri, Sep 25, 2026 at 1:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> On Fri, Sep 25, 2026 at 5:13 AM Eric Dumazet <edumazet@google.com> wrote:
>>>
>>> On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>
>>>> This is a follow-up to commit 8f735d64382d ("net/sched: bound
>>>> qdisc_pkt_len to prevent qdisc soft lockup"), which capped
>>>> qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
>>>> amplifier but leaves the per-flow backlog counter u32:
>>>> fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
>>>> so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
>>>> counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
>>>> and drops from an almost-empty flow, and the dequeue-side subtractions
>>>> corrupt the counter further.
>>>>
>>>> Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
>>>> the drop threshold to u64. fq_codel is not lockless: every writer runs
>>>> under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
>>>> publish / READ_ONCE-consume pattern. The dump path
>>>> (fq_codel_dump_class_stats) stays a lockless stat-only read.
>>>>
>>>> CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
>>>> b->backlogs[] and per-tin b->tin_backlog and consumes the values for
>>>> longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
>>>> staleness check, so it shares the bug. Widen those counters and the heap
>>>> comparison locals to u64; the class/tin stats keep exporting the low 32
>>>> bits through the unchanged uAPI fields.
>>>>
>>>> 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 4200 ecn drop_batch 4096
>>>> # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
>>>> # backlog persists, then send at least 4300 packets (the wrap starts
>>>> # at 4096 resident; the over-limit drop that reads the wrapped
>>>> # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
>>>> # 1 MiB and the fat-flow threshold reads the wrapped value.
>>>>
>>>> With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
>>>> packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
>>>> wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
>>>> where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
>>>> drop burst).
>>>>
>>>> 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>
>>>> ---
>>>
>>> This makes no sense.
>>>
>>
>> As absurd as it looks that code is reachable ;->
>>
>>> These qdisc have been developped to address bufferbloat issues.
>>>
>>> Storing 4GB in a qdisc is absolutely insane.
>>>
>>
>> The counter wrap is not because we stored 4GB, rather it is because
>> "tc .. stab overhead ..." inflates qdisc_pkt_len() for a 64B pkt to
>> 1MB. So ~4K packets (put in other words a few "real" KB) makes that
>> backlog[0] cross 2^32.
>
> Kill this stuff ? Who is still using this, for what reason ?
tc-stab? Same as before, if you need want to model a remote bottleneck with a local traffic shaper tc-stab is the generic solution (off the top of my head I only can enumerate cake as having its own traffic shaper that handles overheads).
One could argue that the "virtual" length should be accounted against cake's memlimit or fq-codel's memory_limit somehow...
In sane?/typical configurations overhead is expected to stay relatively small, so this would not limit the actual queue size too much, while potentially silencing this issue.
I might be off my rocker, in which has ignore (or preferably enlighten me).
Regards
Sebastian
>
> Really, it is time we stop adding code only for fuzzers.
>
>> Result is pruning the wrong flow..
>>
>>> Let's drop at enqueue if the current backlog is approaching 4GB (this
>>> can later be a new config/attribute in net-next)
>>
>> Note, this is net-next already. Enqueue is fast path - are you ok with that?
>>
>> cheers,
>> jamal
> _______________________________________________
> Cake mailing list -- cake@lists.bufferbloat.net
> To unsubscribe send an email to cake-leave@lists.bufferbloat.net
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 13:30 ` Sebastian Moeller
@ 2026-09-25 13:43 ` Eric Dumazet
2026-09-25 14:02 ` Sebastian Moeller
0 siblings, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2026-09-25 13:43 UTC (permalink / raw)
To: Sebastian Moeller
Cc: Jamal Hadi Salim, netdev, Toke Høiland-Jørgensen, cake,
Jiri Pirko, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, Victor Nogueira, hybris, Sashiko
On Fri, Sep 25, 2026 at 3:30 PM Sebastian Moeller <moeller0@gmx.de> wrote:
>
> Hi Eric,
>
>
> > On Sep 25, 2026, at 15:03, Eric Dumazet via Cake <cake@lists.bufferbloat.net> wrote:
> >
> > On Fri, Sep 25, 2026 at 1:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >>
> >> On Fri, Sep 25, 2026 at 5:13 AM Eric Dumazet <edumazet@google.com> wrote:
> >>>
> >>> On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >>>>
> >>>> This is a follow-up to commit 8f735d64382d ("net/sched: bound
> >>>> qdisc_pkt_len to prevent qdisc soft lockup"), which capped
> >>>> qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
> >>>> amplifier but leaves the per-flow backlog counter u32:
> >>>> fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
> >>>> so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
> >>>> counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
> >>>> and drops from an almost-empty flow, and the dequeue-side subtractions
> >>>> corrupt the counter further.
> >>>>
> >>>> Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
> >>>> the drop threshold to u64. fq_codel is not lockless: every writer runs
> >>>> under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
> >>>> publish / READ_ONCE-consume pattern. The dump path
> >>>> (fq_codel_dump_class_stats) stays a lockless stat-only read.
> >>>>
> >>>> CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
> >>>> b->backlogs[] and per-tin b->tin_backlog and consumes the values for
> >>>> longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
> >>>> staleness check, so it shares the bug. Widen those counters and the heap
> >>>> comparison locals to u64; the class/tin stats keep exporting the low 32
> >>>> bits through the unchanged uAPI fields.
> >>>>
> >>>> 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 4200 ecn drop_batch 4096
> >>>> # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
> >>>> # backlog persists, then send at least 4300 packets (the wrap starts
> >>>> # at 4096 resident; the over-limit drop that reads the wrapped
> >>>> # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
> >>>> # 1 MiB and the fat-flow threshold reads the wrapped value.
> >>>>
> >>>> With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
> >>>> packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
> >>>> wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
> >>>> where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
> >>>> drop burst).
> >>>>
> >>>> 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>
> >>>> ---
> >>>
> >>> This makes no sense.
> >>>
> >>
> >> As absurd as it looks that code is reachable ;->
> >>
> >>> These qdisc have been developped to address bufferbloat issues.
> >>>
> >>> Storing 4GB in a qdisc is absolutely insane.
> >>>
> >>
> >> The counter wrap is not because we stored 4GB, rather it is because
> >> "tc .. stab overhead ..." inflates qdisc_pkt_len() for a 64B pkt to
> >> 1MB. So ~4K packets (put in other words a few "real" KB) makes that
> >> backlog[0] cross 2^32.
> >
> > Kill this stuff ? Who is still using this, for what reason ?
>
> tc-stab? Same as before, if you need want to model a remote bottleneck with a local traffic shaper tc-stab is the generic solution (off the top of my head I only can enumerate cake as having its own traffic shaper that handles overheads).
> One could argue that the "virtual" length should be accounted against cake's memlimit or fq-codel's memory_limit somehow...
> In sane?/typical configurations overhead is expected to stay relatively small, so this would not limit the actual queue size too much, while potentially silencing this issue.
linux qdisc are in the fast path, for nearly all packets sent over this planet.
They aleady consume GW of energy.
Modeling / network emulation should incur zero cost on these
production grade qdisc.
netem could be one answer, I do not know, or a special
CONFIG_NET_SCHED_EXPENSIVE_EMULATION
>
> I might be off my rocker, in which has ignore (or preferably enlighten me).
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 13:43 ` Eric Dumazet
@ 2026-09-25 14:02 ` Sebastian Moeller
2026-09-26 7:43 ` Jamal Hadi Salim
0 siblings, 1 reply; 12+ messages in thread
From: Sebastian Moeller @ 2026-09-25 14:02 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jamal Hadi Salim, netdev, Toke Høiland-Jørgensen, cake,
Jiri Pirko, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, Victor Nogueira, hybris, Sashiko
Hi Eric,
> On Sep 25, 2026, at 15:43, Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Sep 25, 2026 at 3:30 PM Sebastian Moeller <moeller0@gmx.de> wrote:
>>
>> Hi Eric,
>>
>>
>>> On Sep 25, 2026, at 15:03, Eric Dumazet via Cake <cake@lists.bufferbloat.net> wrote:
>>>
>>> On Fri, Sep 25, 2026 at 1:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>
>>>> On Fri, Sep 25, 2026 at 5:13 AM Eric Dumazet <edumazet@google.com> wrote:
>>>>>
>>>>> On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>>>
>>>>>> This is a follow-up to commit 8f735d64382d ("net/sched: bound
>>>>>> qdisc_pkt_len to prevent qdisc soft lockup"), which capped
>>>>>> qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
>>>>>> amplifier but leaves the per-flow backlog counter u32:
>>>>>> fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
>>>>>> so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
>>>>>> counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
>>>>>> and drops from an almost-empty flow, and the dequeue-side subtractions
>>>>>> corrupt the counter further.
>>>>>>
>>>>>> Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
>>>>>> the drop threshold to u64. fq_codel is not lockless: every writer runs
>>>>>> under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
>>>>>> publish / READ_ONCE-consume pattern. The dump path
>>>>>> (fq_codel_dump_class_stats) stays a lockless stat-only read.
>>>>>>
>>>>>> CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
>>>>>> b->backlogs[] and per-tin b->tin_backlog and consumes the values for
>>>>>> longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
>>>>>> staleness check, so it shares the bug. Widen those counters and the heap
>>>>>> comparison locals to u64; the class/tin stats keep exporting the low 32
>>>>>> bits through the unchanged uAPI fields.
>>>>>>
>>>>>> 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 4200 ecn drop_batch 4096
>>>>>> # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
>>>>>> # backlog persists, then send at least 4300 packets (the wrap starts
>>>>>> # at 4096 resident; the over-limit drop that reads the wrapped
>>>>>> # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
>>>>>> # 1 MiB and the fat-flow threshold reads the wrapped value.
>>>>>>
>>>>>> With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
>>>>>> packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
>>>>>> wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
>>>>>> where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
>>>>>> drop burst).
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>
>>>>> This makes no sense.
>>>>>
>>>>
>>>> As absurd as it looks that code is reachable ;->
>>>>
>>>>> These qdisc have been developped to address bufferbloat issues.
>>>>>
>>>>> Storing 4GB in a qdisc is absolutely insane.
>>>>>
>>>>
>>>> The counter wrap is not because we stored 4GB, rather it is because
>>>> "tc .. stab overhead ..." inflates qdisc_pkt_len() for a 64B pkt to
>>>> 1MB. So ~4K packets (put in other words a few "real" KB) makes that
>>>> backlog[0] cross 2^32.
>>>
>>> Kill this stuff ? Who is still using this, for what reason ?
>>
>> tc-stab? Same as before, if you need want to model a remote bottleneck with a local traffic shaper tc-stab is the generic solution (off the top of my head I only can enumerate cake as having its own traffic shaper that handles overheads).
>> One could argue that the "virtual" length should be accounted against cake's memlimit or fq-codel's memory_limit somehow...
>> In sane?/typical configurations overhead is expected to stay relatively small, so this would not limit the actual queue size too much, while potentially silencing this issue.
>
> linux qdisc are in the fast path, for nearly all packets sent over this planet.
Tip of the head to the linux network experts that enabled that and that take care of it being efficient!
> They aleady consume GW of energy.
>
> Modeling / network emulation should incur zero cost on these
> production grade qdisc.
It is not modeling in a scientific sense (sorry I used inappropriate terminology here), but simply the fact that to shape traffic to avoid filling remote queues one needs to be able to take the properties of that remote queue/interface/link-layer into account.
And that can be surprisingly close by. Last time I looked the kernel on say eth0 added the 14 bytes of ethernet related overhead it handled itself to the packet size on top of the payload (no complaints that makes sense) but that is insufficient to properly traffic shape e.g. the same ethernet interface... (as the relevant ethernet L1-frame overhead contains more bytes, like the FCS, preamble and SFD). And that gets worse with stuff like DSL, cable or fibre modems that are connected via ethernet to a linux router.
>
> netem could be one answer, I do not know, or a special
> CONFIG_NET_SCHED_EXPENSIVE_EMULATION
Well, tc-stab (or cake's overhead accounting) is already optional, so only those incur the cost that actually use it (and only those users are affected by the reported issue, it took tc-stab to cause the problem and arguably in a configuration that is "insane").
I might be trying to explain the internet here to people that make the internet work, so apologies in advance but I want to make this as explicit as I can.
TTraffic shaping without proper overhead accounting will not work as expected (at least if the expectation is that the traffic shaper will honor the set gross shaper rate). For a fixed packet size the lack over overhead accounting can be papered over with reducing the shaper rate, but the amount of the required "over-shaping" depends on packet size (smaller packets require more over-shaping), so generally that is sub optimal, because either the shaper's guarantee is brittle or the over-shaping quite extreme (think the header to payload ration of minimally- and MTU-sized packets).
All I am saying is, we do still have use cases for tc-stab and proper overhead accounting, at least on the leafs of the network like home internet links.
>
>>
>> I might be off my rocker, in which has ignore (or preferably enlighten me).
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 14:02 ` Sebastian Moeller
@ 2026-09-26 7:43 ` Jamal Hadi Salim
2026-09-26 9:48 ` Eric Dumazet
0 siblings, 1 reply; 12+ messages in thread
From: Jamal Hadi Salim @ 2026-09-26 7:43 UTC (permalink / raw)
To: Sebastian Moeller
Cc: Eric Dumazet, Jamal Hadi Salim, netdev,
Toke Høiland-Jørgensen, cake, Jiri Pirko,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Victor Nogueira, hybris, Sashiko
On Fri, Sep 25, 2026 at 10:03 AM 'Sebastian Moeller' via
Hyper-Yielding Back-end Review & Insight System <hybris@mojatatu.com>
wrote:
>
> Hi Eric,
>
>
> > On Sep 25, 2026, at 15:43, Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Sep 25, 2026 at 3:30 PM Sebastian Moeller <moeller0@gmx.de> wrote:
> >>
> >> Hi Eric,
> >>
> >>
> >>> On Sep 25, 2026, at 15:03, Eric Dumazet via Cake <cake@lists.bufferbloat.net> wrote:
> >>>
> >>> On Fri, Sep 25, 2026 at 1:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >>>>
> >>>> On Fri, Sep 25, 2026 at 5:13 AM Eric Dumazet <edumazet@google.com> wrote:
> >>>>>
> >>>>> On Fri, Sep 25, 2026 at 10:54 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >>>>>>
> >>>>>> This is a follow-up to commit 8f735d64382d ("net/sched: bound
> >>>>>> qdisc_pkt_len to prevent qdisc soft lockup"), which capped
> >>>>>> qdisc_pkt_len() at QDISC_PKT_LEN_MAX (1 MiB). That cap bounds the stab
> >>>>>> amplifier but leaves the per-flow backlog counter u32:
> >>>>>> fq_codel_enqueue() accumulates qdisc_pkt_len(skb) into q->backlogs[idx],
> >>>>>> so a flow can still accumulate 4096 packets of 1 MiB each and wrap the
> >>>>>> counter mod 2^32. After a wrap, fq_codel_drop() sees a tiny maxbacklog
> >>>>>> and drops from an almost-empty flow, and the dequeue-side subtractions
> >>>>>> corrupt the counter further.
> >>>>>>
> >>>>>> Widen the fq_codel backlogs table, the fat-flow scan (maxbacklog/len) and
> >>>>>> the drop threshold to u64. fq_codel is not lockless: every writer runs
> >>>>>> under the root qdisc lock, so plain u64 arithmetic keeps the WRITE_ONCE
> >>>>>> publish / READ_ONCE-consume pattern. The dump path
> >>>>>> (fq_codel_dump_class_stats) stays a lockless stat-only read.
> >>>>>>
> >>>>>> CAKE accumulates the same generic qdisc_pkt_len(skb) into its per-flow
> >>>>>> b->backlogs[] and per-tin b->tin_backlog and consumes the values for
> >>>>>> longest-flow pruning (cake_heapify/cake_heapify_up) and for the shaper
> >>>>>> staleness check, so it shares the bug. Widen those counters and the heap
> >>>>>> comparison locals to u64; the class/tin stats keep exporting the low 32
> >>>>>> bits through the unchanged uAPI fields.
> >>>>>>
> >>>>>> 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 4200 ecn drop_batch 4096
> >>>>>> # hold the tun fd open without reading (IFF_BACKPRESSURE) so the qdisc
> >>>>>> # backlog persists, then send at least 4300 packets (the wrap starts
> >>>>>> # at 4096 resident; the over-limit drop that reads the wrapped
> >>>>>> # threshold fires past the 4200 limit): backlogs[0] wraps at 4096 x
> >>>>>> # 1 MiB and the fat-flow threshold reads the wrapped value.
> >>>>>>
> >>>>>> With a 1 MiB qdisc_pkt_len cap the counter wraps at 4096 resident
> >>>>>> packets. At limit 4200 the first over-limit enqueue (the 4201st) sees a
> >>>>>> wrapped 105 MiB (half-backlog threshold 52 MiB, a ~52 packet drop burst),
> >>>>>> where the u64 counter sees 4201 MiB (threshold 2100 MiB, a ~2100 packet
> >>>>>> drop burst).
> >>>>>>
> >>>>>> 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>
> >>>>>> ---
> >>>>>
> >>>>> This makes no sense.
> >>>>>
> >>>>
> >>>> As absurd as it looks that code is reachable ;->
> >>>>
> >>>>> These qdisc have been developped to address bufferbloat issues.
> >>>>>
> >>>>> Storing 4GB in a qdisc is absolutely insane.
> >>>>>
> >>>>
> >>>> The counter wrap is not because we stored 4GB, rather it is because
> >>>> "tc .. stab overhead ..." inflates qdisc_pkt_len() for a 64B pkt to
> >>>> 1MB. So ~4K packets (put in other words a few "real" KB) makes that
> >>>> backlog[0] cross 2^32.
> >>>
> >>> Kill this stuff ? Who is still using this, for what reason ?
> >>
> >> tc-stab? Same as before, if you need want to model a remote bottleneck with a local traffic shaper tc-stab is the generic solution (off the top of my head I only can enumerate cake as having its own traffic shaper that handles overheads).
> >> One could argue that the "virtual" length should be accounted against cake's memlimit or fq-codel's memory_limit somehow...
> >> In sane?/typical configurations overhead is expected to stay relatively small, so this would not limit the actual queue size too much, while potentially silencing this issue.
> >
> > linux qdisc are in the fast path, for nearly all packets sent over this planet.
>
> Tip of the head to the linux network experts that enabled that and that take care of it being efficient!
>
> > They aleady consume GW of energy.
> >
> > Modeling / network emulation should incur zero cost on these
> > production grade qdisc.
>
> It is not modeling in a scientific sense (sorry I used inappropriate terminology here), but simply the fact that to shape traffic to avoid filling remote queues one needs to be able to take the properties of that remote queue/interface/link-layer into account.
> And that can be surprisingly close by. Last time I looked the kernel on say eth0 added the 14 bytes of ethernet related overhead it handled itself to the packet size on top of the payload (no complaints that makes sense) but that is insufficient to properly traffic shape e.g. the same ethernet interface... (as the relevant ethernet L1-frame overhead contains more bytes, like the FCS, preamble and SFD). And that gets worse with stuff like DSL, cable or fibre modems that are connected via ethernet to a linux router.
>
> >
> > netem could be one answer, I do not know, or a special
> > CONFIG_NET_SCHED_EXPENSIVE_EMULATION
>
> Well, tc-stab (or cake's overhead accounting) is already optional, so only those incur the cost that actually use it (and only those users are affected by the reported issue, it took tc-stab to cause the problem and arguably in a configuration that is "insane").
> I might be trying to explain the internet here to people that make the internet work, so apologies in advance but I want to make this as explicit as I can.
> TTraffic shaping without proper overhead accounting will not work as expected (at least if the expectation is that the traffic shaper will honor the set gross shaper rate). For a fixed packet size the lack over overhead accounting can be papered over with reducing the shaper rate, but the amount of the required "over-shaping" depends on packet size (smaller packets require more over-shaping), so generally that is sub optimal, because either the shaper's guarantee is brittle or the over-shaping quite extreme (think the header to payload ration of minimally- and MTU-sized packets).
>
> All I am saying is, we do still have use cases for tc-stab and proper overhead accounting, at least on the leafs of the network like home internet links.
I am working on a v2 that drops at enqueue once the accounted backlog
crosses (U32_MAX - QDISC_PKT_LEN_MAX), so no counter can wrap.
That is Eric's original suggestion which preserves the point you raised.
While looking at this closely for this update i noticed more qdiscs
which for different reasons also suffer from ((u64)backlog + len <=
limit idiom.
So far it's clear from bfifo, gred and plug are the only ones safe
because their limit is in bytes (as opposed to others which are packet
counting)
Eric: Unless i hear otherwise from you, the configurable byte ceiling
you mentioned (a good noun seems to be CONFIG_NET_SCH_BACKLOG_CEILING)
is a follow-up;
cheers,
jamal
> >
> >>
> >> I might be off my rocker, in which has ignore (or preferably enlighten me).
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-26 7:43 ` Jamal Hadi Salim
@ 2026-09-26 9:48 ` Eric Dumazet
0 siblings, 0 replies; 12+ messages in thread
From: Eric Dumazet @ 2026-09-26 9:48 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Sebastian Moeller, Jamal Hadi Salim, netdev,
Toke Høiland-Jørgensen, cake, Jiri Pirko,
David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Victor Nogueira, hybris, Sashiko
On Sat, Sep 26, 2026 at 11:43 AM Jamal Hadi Salim <hadi@mojatatu.com> wrote:
> I am working on a v2 that drops at enqueue once the accounted backlog
> crosses (U32_MAX - QDISC_PKT_LEN_MAX), so no counter can wrap.
> That is Eric's original suggestion which preserves the point you raised.
>
> While looking at this closely for this update i noticed more qdiscs
> which for different reasons also suffer from ((u64)backlog + len <=
> limit idiom.
> So far it's clear from bfifo, gred and plug are the only ones safe
> because their limit is in bytes (as opposed to others which are packet
> counting)
>
> Eric: Unless i hear otherwise from you, the configurable byte ceiling
> you mentioned (a good noun seems to be CONFIG_NET_SCH_BACKLOG_CEILING)
> is a follow-up;
SGTM, thanks Jamal.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-25 8:53 [Cake] [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64 Jamal Hadi Salim
2026-09-25 9:12 ` [Cake] " Eric Dumazet
@ 2026-09-28 8:54 ` netdev-bot+sashiko
2026-09-28 11:29 ` Jamal Hadi Salim
1 sibling, 1 reply; 12+ messages in thread
From: netdev-bot+sashiko @ 2026-09-28 8:54 UTC (permalink / raw)
To: jhs
Cc: netdev, toke, cake, jiri, davem, edumazet, kuba, pabeni, horms,
victor, hybris, sashiko-bot
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 (1) · Low: 1
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 commit message understates the bug it fixes.
- [Medium] The patch widens the per-flow counter q->backlogs[] to u64,
but fq_codel's CoDel drop decision still uses the qdisc-wide u32…
- [Low] The commit fixes a user-triggerable bug in fq_codel and CAKE:
wrong fat-flow drop choices and, before the patch, a possible NULL…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
cake_heapify() does not refresh mb after a swap, so each call sifts…
--
net/sched: fq_codel, cake: widen backlogs to u64
This patch widens three counters from u32 to u64: the fq_codel per-flow
backlogs table, and the CAKE per-flow backlogs[] and per-tin tin_backlog
counters. This stops 1 MiB qdisc_pkt_len() values from wrapping them. The
fat-flow scan and heap comparison locals are widened to match, and the dump
paths export the low 32 bits through the existing uAPI fields.
Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
> After a wrap, fq_codel_drop() sees a tiny maxbacklog
> and drops from an almost-empty flow, and the dequeue-side subtractions
> corrupt the counter further.
[Severity: Medium]
Is "almost-empty flow" the worst case here? qdisc_pkt_len() is capped at
QDISC_PKT_LEN_MAX. So with the old u32 counters, 4096 resident 1 MiB
packets in one flow X leave q->backlogs[X] at exactly 0.
Take the case where flows_cnt >= 2 (the default is 1024), X != 0, and all
other flows are empty. The strict comparison in the fq_codel_drop() scan
then never fires:
for (i = 0; i < q->flows_cnt; i++) {
if (q->backlogs[i] > maxbacklog) {
So idx stays 0, and flow = &q->flows[0] is an empty flow. dequeue_head()
then does this with no NULL check:
struct sk_buff *skb = flow->head;
WRITE_ONCE(flow->head, skb->next);
Wouldn't that be a NULL pointer dereference, reachable with CAP_NET_ADMIN
in a user namespace? The reproducer uses flows 1, which hides it, because
idx 0 is then the fat flow.
The u64 counters fix this, since they can't wrap. Could the commit message
describe the oops, so stable and CVE triage see the real impact?
Is the claim about the dequeue-side subtractions accurate?
fq_codel_enqueue(), dequeue_func() and fq_codel_drop() all add and
subtract the same qdisc_pkt_len() values in modular u32 arithmetic. The
counter therefore stays equal to the true backlog mod 2^32; it can't
represent values at or above 2^32, but it doesn't drift further.
> 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 carry a Fixes: tag, and possibly Cc: stable?
The u32 per-flow backlogs go back to the original fq_codel and CAKE code.
The only reference to history is the prose "follow-up to commit
8f735d64382d", which backport tooling won't parse.
Other net fixes around this baseline carry Fixes: tags, such as "tcp:
prevent collapsing skbs across boundary in rtx queue" and "llc: reserve
device headroom for allocated frames".
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..8e99c85dab17b 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
[ ... ]
> @@ -1466,17 +1466,17 @@ static void cake_heap_swap(struct cake_sched_data *q, u16 i, u16 j)
[ ... ]
> static void cake_heapify(struct cake_sched_data *q, u16 i)
> {
> static const u32 a = CAKE_MAX_TINS * CAKE_QUEUES;
> - u32 mb = cake_heap_get_backlog(q, i);
> + u64 mb = cake_heap_get_backlog(q, i);
> u32 m = i;
>
> while (m < a) {
[ ... ]
> @@ -1493,7 +1493,7 @@ static void cake_heapify(struct cake_sched_data *q, u16 i)
> }
>
> if (r < a) {
> - u32 rb = cake_heap_get_backlog(q, r);
> + u64 rb = cake_heap_get_backlog(q, r);
>
> if (rb > mb) {
> m = r;
[Severity: Medium]
This isn't a bug introduced by this patch, but does cake_heapify() ever
sift down more than one level? The rest of the loop is:
if (m != i) {
cake_heap_swap(q, i, m);
i = m;
} else {
break;
}
When a child wins, mb takes the child's backlog. cake_heap_swap() then
moves the smaller original entry down to m, and i = m. But mb is not
refreshed, so it still holds the promoted child's larger value.
In a heap that was valid below, the grandchildren are <= that value. So
lb > mb and rb > mb never fire again, and the loop breaks.
For example, take [1000, 900, 0, 700, 0], where the root flow holds a
900-byte packet followed by a 100-byte packet. cake_drop() drops the
900-byte head, and cake_heapify(q, 0) produces [900, 100, 0, 700, 0].
Node 1 (100) is now above its 700 child.
After the next drop from the 900 root, the next overflow prune in
cake_drop() can pick the 100-byte flow instead of the 700-byte flow. The
bottom-up rebuild in cake_drop() has the same one-level limit.
[ ... ]
> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index 969b2510b0b86..3c20297cef07e 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -51,7 +51,7 @@ struct fq_codel_sched_data {
> struct tcf_proto __rcu *filter_list; /* optional external classifier */
> struct tcf_block *block;
> struct fq_codel_flow *flows; /* Flows table [flows_cnt] */
> - u32 *backlogs; /* backlog table [flows_cnt] */
> + u64 *backlogs; /* backlog table [flows_cnt] */
[Severity: Medium]
The per-flow counters are widened here, but does the CoDel drop decision
still see a backlog that wraps? fq_codel_enqueue() adds the same
qdisc_pkt_len() to both counters:
WRITE_ONCE(q->backlogs[idx], q->backlogs[idx] + qdisc_pkt_len(skb));
qdisc_qstats_backlog_inc(sch, skb);
sch->qstats.backlog is the __u32 backlog field in struct gnet_stats_queue.
__fq_codel_dequeue() passes it to CoDel:
skb = codel_dequeue(sch, &sch->qstats.backlog, &q->cparams,
and codel_should_drop() then does:
if (codel_time_before(vars->ldelay, params->target) ||
*backlog <= params->mtu) {
/* went below - stay below for at least interval */
vars->first_above_time = 0;
return false;
}
The aggregate is summed over all flows, so it wraps at or before the
point where the old per-flow counter did. With 4096 x 1 MiB queued across
any mix of flows, the aggregate is 0. CoDel then stops dropping and
ECN-marking whatever the sojourn time, and leaves the dropping state if
it was in it.
stab inflates pkt_len but not truesize, so this can happen below both
sch->limit and memory_limit.
The uAPI field can't be widened. Could CoDel be given a private u64 sum,
or a saturated value, instead?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-BA27.v1.20260922092618%40mojatatu.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Cake] Re: [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64
2026-09-28 8:54 ` netdev-bot+sashiko
@ 2026-09-28 11:29 ` Jamal Hadi Salim
0 siblings, 0 replies; 12+ messages in thread
From: Jamal Hadi Salim @ 2026-09-28 11:29 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: netdev, toke, cake, jiri, davem, edumazet, kuba, pabeni, horms,
victor, hybris, sashiko-bot
On Mon, Sep 28, 2026 at 4:54 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 (1) · Low: 1
>
> 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 commit message understates the bug it fixes.
> - [Medium] The patch widens the per-flow counter q->backlogs[] to u64,
> but fq_codel's CoDel drop decision still uses the qdisc-wide u32…
> - [Low] The commit fixes a user-triggerable bug in fq_codel and CAKE:
> wrong fat-flow drop choices and, before the patch, a possible NULL…
>
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
> cake_heapify() does not refresh mb after a swap, so each call sifts…
>
This is confusing - from the subject line "[PATCH net-next] net/sched:
fq_codel, cake: widen backlogs to u64" it is definetely a review for
V1 (for the second time).
I already sent v2 after some discussions with Eric. Having said that,
the cake_heapify() pre-existing issue is a new discovery for a
followup.
V2 subject is: "[PATCH net-next v2] net/sched: cap the accounted
backlog before it can wrap"
I may have confused sashiko scripts by changing the subject line. May
be i should have made this (since it is a brand new approach) stay at
v1?
I realize there are overwhelming amount of messages on the list but i
hope someone (Paolo/Jakub) is reading this message and can check.
Will wait for the sashiko embargo to be lifted for v2 then check if it
is worth sending v3
cheers,
jamal
> --
>
> net/sched: fq_codel, cake: widen backlogs to u64
>
> This patch widens three counters from u32 to u64: the fq_codel per-flow
> backlogs table, and the CAKE per-flow backlogs[] and per-tin tin_backlog
> counters. This stops 1 MiB qdisc_pkt_len() values from wrapping them. The
> fat-flow scan and heap comparison locals are widened to match, and the dump
> paths export the low 32 bits through the existing uAPI fields.
>
> Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
>
> > After a wrap, fq_codel_drop() sees a tiny maxbacklog
> > and drops from an almost-empty flow, and the dequeue-side subtractions
> > corrupt the counter further.
>
> [Severity: Medium]
> Is "almost-empty flow" the worst case here? qdisc_pkt_len() is capped at
> QDISC_PKT_LEN_MAX. So with the old u32 counters, 4096 resident 1 MiB
> packets in one flow X leave q->backlogs[X] at exactly 0.
>
> Take the case where flows_cnt >= 2 (the default is 1024), X != 0, and all
> other flows are empty. The strict comparison in the fq_codel_drop() scan
> then never fires:
>
> for (i = 0; i < q->flows_cnt; i++) {
> if (q->backlogs[i] > maxbacklog) {
>
> So idx stays 0, and flow = &q->flows[0] is an empty flow. dequeue_head()
> then does this with no NULL check:
>
> struct sk_buff *skb = flow->head;
>
> WRITE_ONCE(flow->head, skb->next);
>
> Wouldn't that be a NULL pointer dereference, reachable with CAP_NET_ADMIN
> in a user namespace? The reproducer uses flows 1, which hides it, because
> idx 0 is then the fat flow.
>
> The u64 counters fix this, since they can't wrap. Could the commit message
> describe the oops, so stable and CVE triage see the real impact?
>
> Is the claim about the dequeue-side subtractions accurate?
> fq_codel_enqueue(), dequeue_func() and fq_codel_drop() all add and
> subtract the same qdisc_pkt_len() values in modular u32 arithmetic. The
> counter therefore stays equal to the true backlog mod 2^32; it can't
> represent values at or above 2^32, but it doesn't drift further.
>
> > 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 carry a Fixes: tag, and possibly Cc: stable?
>
> The u32 per-flow backlogs go back to the original fq_codel and CAKE code.
> The only reference to history is the prose "follow-up to commit
> 8f735d64382d", which backport tooling won't parse.
>
> Other net fixes around this baseline carry Fixes: tags, such as "tcp:
> prevent collapsing skbs across boundary in rtx queue" and "llc: reserve
> device headroom for allocated frames".
>
> > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > index dc93267029e70..8e99c85dab17b 100644
> > --- a/net/sched/sch_cake.c
> > +++ b/net/sched/sch_cake.c
>
> [ ... ]
>
> > @@ -1466,17 +1466,17 @@ static void cake_heap_swap(struct cake_sched_data *q, u16 i, u16 j)
>
> [ ... ]
>
> > static void cake_heapify(struct cake_sched_data *q, u16 i)
> > {
> > static const u32 a = CAKE_MAX_TINS * CAKE_QUEUES;
> > - u32 mb = cake_heap_get_backlog(q, i);
> > + u64 mb = cake_heap_get_backlog(q, i);
> > u32 m = i;
> >
> > while (m < a) {
>
> [ ... ]
>
> > @@ -1493,7 +1493,7 @@ static void cake_heapify(struct cake_sched_data *q, u16 i)
> > }
> >
> > if (r < a) {
> > - u32 rb = cake_heap_get_backlog(q, r);
> > + u64 rb = cake_heap_get_backlog(q, r);
> >
> > if (rb > mb) {
> > m = r;
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does cake_heapify() ever
> sift down more than one level? The rest of the loop is:
>
> if (m != i) {
> cake_heap_swap(q, i, m);
> i = m;
> } else {
> break;
> }
>
> When a child wins, mb takes the child's backlog. cake_heap_swap() then
> moves the smaller original entry down to m, and i = m. But mb is not
> refreshed, so it still holds the promoted child's larger value.
>
> In a heap that was valid below, the grandchildren are <= that value. So
> lb > mb and rb > mb never fire again, and the loop breaks.
>
> For example, take [1000, 900, 0, 700, 0], where the root flow holds a
> 900-byte packet followed by a 100-byte packet. cake_drop() drops the
> 900-byte head, and cake_heapify(q, 0) produces [900, 100, 0, 700, 0].
> Node 1 (100) is now above its 700 child.
>
> After the next drop from the 900 root, the next overflow prune in
> cake_drop() can pick the 100-byte flow instead of the 700-byte flow. The
> bottom-up rebuild in cake_drop() has the same one-level limit.
>
> [ ... ]
>
> > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> > index 969b2510b0b86..3c20297cef07e 100644
> > --- a/net/sched/sch_fq_codel.c
> > +++ b/net/sched/sch_fq_codel.c
> > @@ -51,7 +51,7 @@ struct fq_codel_sched_data {
> > struct tcf_proto __rcu *filter_list; /* optional external classifier */
> > struct tcf_block *block;
> > struct fq_codel_flow *flows; /* Flows table [flows_cnt] */
> > - u32 *backlogs; /* backlog table [flows_cnt] */
> > + u64 *backlogs; /* backlog table [flows_cnt] */
>
> [Severity: Medium]
> The per-flow counters are widened here, but does the CoDel drop decision
> still see a backlog that wraps? fq_codel_enqueue() adds the same
> qdisc_pkt_len() to both counters:
>
> WRITE_ONCE(q->backlogs[idx], q->backlogs[idx] + qdisc_pkt_len(skb));
> qdisc_qstats_backlog_inc(sch, skb);
>
> sch->qstats.backlog is the __u32 backlog field in struct gnet_stats_queue.
> __fq_codel_dequeue() passes it to CoDel:
>
> skb = codel_dequeue(sch, &sch->qstats.backlog, &q->cparams,
>
> and codel_should_drop() then does:
>
> if (codel_time_before(vars->ldelay, params->target) ||
> *backlog <= params->mtu) {
> /* went below - stay below for at least interval */
> vars->first_above_time = 0;
> return false;
> }
>
> The aggregate is summed over all flows, so it wraps at or before the
> point where the old per-flow counter did. With 4096 x 1 MiB queued across
> any mix of flows, the aggregate is 0. CoDel then stops dropping and
> ECN-marking whatever the sojourn time, and leaves the dropping state if
> it was in it.
>
> stab inflates pkt_len but not truesize, so this can happen below both
> sch->limit and memory_limit.
>
> The uAPI field can't be widened. Could CoDel be given a private u64 sum,
> or a saturated value, instead?
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-BA27.v1.20260922092618%40mojatatu.com
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2026-09-28 11:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-25 8:53 [Cake] [PATCH net-next] net/sched: fq_codel, cake: widen backlogs to u64 Jamal Hadi Salim
2026-09-25 9:12 ` [Cake] " Eric Dumazet
2026-09-25 11:27 ` Jamal Hadi Salim
2026-09-25 12:22 ` Jamal Hadi Salim
2026-09-25 13:03 ` Eric Dumazet
2026-09-25 13:30 ` Sebastian Moeller
2026-09-25 13:43 ` Eric Dumazet
2026-09-25 14:02 ` Sebastian Moeller
2026-09-26 7:43 ` Jamal Hadi Salim
2026-09-26 9:48 ` Eric Dumazet
2026-09-28 8:54 ` netdev-bot+sashiko
2026-09-28 11:29 ` Jamal Hadi Salim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox