[PATCH net-next 0/4] net/sched: sch_cake: fixes for accounting and reconfiguration

[PATCH net-next 0/4] net/sched: sch_cake: fixes for accounting and reconfiguration

[Jamal Hadi Salim]

Four independent fixes in the CAKE qdisc.  Three are follow-ups to the
autorate reconfiguration work around commit 7cbfb180945c ("net/sched:
sch_cake: fix autorate reconfiguration throttling") and the 32-bit
admission-counter wraps fixed in gred/bfifo/plug by commit 4c660ee8c809
("net: sched: fix 32-bit backlog wrap in gred, bfifo and plug enqueue");
the fourth is a dead-code cleanup found while auditing the dump path.

Patch 1 serializes a 'tc qdisc change' with the datapath's autorate
update under the qdisc lock, so the netlink writer and the autorate
writer can no longer lose the configured rate; rate_bps becomes
atomic64_t so the 64-bit store is atomic on 32-bit machines too.

Patch 2 propagates the backlog reduction of the reconfigure-time tin
purge to classful parents, so a CAKE leaf under HTB no longer leaves the
purged packets in the parent's qlen/backlog forever.

Patch 3 widens buffer_used and buffer_max_used to u64.  The u32
accumulator wraps once the resident truesize passes 2^32, bypassing the
over-limit drop loop while the configured limit is a u32 attribute.

Patch 4 drops an unreachable 'nla_nest_end() < 0' error check: the
helper never returns a negative value.

All four are hardening for net-next (no Fixes:, no Cc: stable): the
counter wrap, the missing parent-backlog propagation and the
reconfiguration race all predate any contract we could anchor a
regression on.  Patches 1-2 were reported by Sashiko (gemini) and patch
3 comes from the gred 32-bit-wrap family; patch 4 is a static cleanup.

Jamal Hadi Salim (4):
  net/sched: sch_cake: serialize reconfiguration with the datapath
  net/sched: sch_cake: reduce parent backlog when clearing a tin
  net/sched: sch_cake: widen buffer_used to u64
  net/sched: sch_cake: drop dead nla_nest_end() error check

 net/sched/sch_cake.c | 60 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH net-next 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath

[Jamal Hadi Salim]

This is a follow-up to commit 7cbfb180945c ("net/sched: sch_cake: fix
autorate reconfiguration throttling").  That change only stored
last_reconfig_time; it did not address the concurrency between the
autorate rate update in cake_enqueue() and the netlink reconfiguration
path, which the original review flagged.

cake_change() -> cake_config_change() updates the fields of the shared
struct cake_sched_config under RTNL but NOT under the qdisc lock: it
only took sch_tree_lock() later, around cake_reconfigure().  The datapath
in turn serializes cake_enqueue() with itself under the qdisc lock, and
its autorate-ingress path stores the estimated rate into
q->config->rate_bps.  Two writers therefore update the same rate with no
common lock:

  CPU0: cake_enqueue() sees CAKE_FLAG_AUTORATE_INGRESS, computes an
        estimate, and is about to store it.
  CPU1: 'tc qdisc change ... bandwidth X' clears autorate in its local
        rate_flags, writes rate_bps = X, publishes the cleared flag, and
        waits in sch_tree_lock().
  CPU0: stores the estimate, calls cake_reconfigure(), and unlocks.
  CPU1: reconfigures from the estimate and returns.

The qdisc is left with autorate disabled but with the estimate installed
instead of X.  On 32-bit machines the unlocked 64-bit rate_bps store is
also not atomic, so a reader can observe a torn rate.  iproute2 reaches
this because parsing 'bandwidth X' sets autorate = 0 and emits both
TCA_CAKE_BASE_RATE64 and TCA_CAKE_AUTORATE.

Commit the parsed configuration and reconfigure while holding the same
qdisc lock that serializes the datapath, so the netlink writer and the
autorate writer can no longer interleave.  Make rate_bps an atomic64_t
so a 64-bit rate update is atomic on 32-bit machines as well, and keep
the READ_ONCE()/WRITE_ONCE() annotations for the remaining lockless
readers (cake_config_dump(), which runs without the lock, and the
cake_mq shared config, which readers observe under different child
locks).

Conditions to recreate the bug: build with CONFIG_KCSAN=y (the race is
otherwise not observable on 64-bit), put cake in autorate-ingress on a
device, drive bursty traffic with gaps so the 250ms autorate
reconfiguration fires, and concurrently loop 'tc qdisc change dev ...
cake bandwidth <N>kbit autorate-ingress'.  The lost update of the
installed rate is reproducible without a sanitizer by running bursty
senders while repeatedly changing an autorate qdisc to a fixed
'bandwidth X' and reading the installed rate back.

Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.com/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_cake.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..1c29695dc928 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -199,7 +199,7 @@ struct cake_tin_data {
 }; /* number of tins is small, so size of this struct doesn't matter much */
 
 struct cake_sched_config {
-	u64		rate_bps;
+	atomic64_t	rate_bps;
 	u64		interval;
 	u64		target;
 	u64		sync_time;
@@ -1906,7 +1906,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			if (ktime_after(now,
 					ktime_add_ms(q->last_reconfig_time,
 						     250))) {
-				q->config->rate_bps = (q->avg_peak_bandwidth * 15) >> 4;
+				atomic64_set(&q->config->rate_bps,
+					     (q->avg_peak_bandwidth * 15) >> 4);
 				q->last_reconfig_time = now;
 				cake_reconfigure(sch);
 			}
@@ -2021,7 +2022,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
 	    now - q->last_checked_active >= q->config->sync_time) {
 		struct net_device *dev = qdisc_dev(sch);
 		struct cake_sched_data *other_priv;
-		u64 new_rate = q->config->rate_bps;
+		u64 new_rate = atomic64_read(&q->config->rate_bps);
 		u64 other_qlen, other_last_active;
 		struct Qdisc *other_sch;
 		u32 num_active_qs = 1;
@@ -2042,7 +2043,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
 		}
 
 		if (num_active_qs > 1)
-			new_rate = div64_u64(q->config->rate_bps, num_active_qs);
+			new_rate = div64_u64(atomic64_read(&q->config->rate_bps), num_active_qs);
 
 		cake_configure_rates(sch, new_rate, true);
 		q->last_checked_active = now;
@@ -2626,12 +2627,12 @@ static void cake_reconfigure(struct Qdisc *sch)
 	struct cake_sched_config *q = qd->config;
 	u32 buffer_limit;
 
-	cake_configure_rates(sch, qd->config->rate_bps, false);
+	cake_configure_rates(sch, atomic64_read(&qd->config->rate_bps), false);
 
 	if (q->buffer_config_limit) {
 		buffer_limit = q->buffer_config_limit;
-	} else if (q->rate_bps) {
-		u64 t = q->rate_bps * q->interval;
+	} else if (atomic64_read(&q->rate_bps)) {
+		u64 t = atomic64_read(&q->rate_bps) * q->interval;
 
 		do_div(t, USEC_PER_SEC / 4);
 		buffer_limit = max_t(u32, t, 4U << 20);
@@ -2686,8 +2687,8 @@ static int cake_config_change(struct cake_sched_config *q, struct nlattr *opt,
 	}
 
 	if (tb[TCA_CAKE_BASE_RATE64])
-		WRITE_ONCE(q->rate_bps,
-			   nla_get_u64(tb[TCA_CAKE_BASE_RATE64]));
+		atomic64_set(&q->rate_bps,
+			     nla_get_u64(tb[TCA_CAKE_BASE_RATE64]));
 
 	if (tb[TCA_CAKE_DIFFSERV_MODE])
 		WRITE_ONCE(q->tin_mode,
@@ -2784,9 +2785,16 @@ static int cake_change(struct Qdisc *sch, struct nlattr *opt,
 		return -EOPNOTSUPP;
 	}
 
+	/* Once the qdisc is live, commit and reconfigure under the lock that
+	 * serializes the datapath, so a concurrent 'tc qdisc change' and the
+	 * autorate update cannot lose the configured rate.
+	 */
+	if (qd->tins)
+		sch_tree_lock(sch);
+
 	ret = cake_config_change(q, opt, extack, &overhead_changed);
 	if (ret)
-		return ret;
+		goto unlock;
 
 	if (overhead_changed) {
 		WRITE_ONCE(qd->max_netlen, 0);
@@ -2795,13 +2803,13 @@ static int cake_change(struct Qdisc *sch, struct nlattr *opt,
 		WRITE_ONCE(qd->min_adjlen, ~0);
 	}
 
-	if (qd->tins) {
-		sch_tree_lock(sch);
+	if (qd->tins)
 		cake_reconfigure(sch);
+unlock:
+	if (qd->tins)
 		sch_tree_unlock(sch);
-	}
 
-	return 0;
+	return ret;
 }
 
 static void cake_destroy(struct Qdisc *sch)
@@ -2818,7 +2826,7 @@ static void cake_config_init(struct cake_sched_config *q, bool is_shared)
 	q->tin_mode = CAKE_DIFFSERV_DIFFSERV3;
 	q->flow_mode  = CAKE_FLOW_TRIPLE;
 
-	q->rate_bps = 0; /* unlimited by default */
+	atomic64_set(&q->rate_bps, 0); /* unlimited by default */
 
 	q->interval = 100000; /* 100ms default */
 	q->target   =   5000; /* 5ms: codel RFC argues
@@ -2889,7 +2897,7 @@ static int cake_init(struct Qdisc *sch, struct nlattr *opt,
 	}
 
 	cake_reconfigure(sch);
-	qd->avg_peak_bandwidth = q->rate_bps;
+	qd->avg_peak_bandwidth = atomic64_read(&q->rate_bps);
 	qd->min_netlen = ~0;
 	qd->min_adjlen = ~0;
 	qd->active_queues = 0;
@@ -2917,7 +2925,7 @@ static int cake_config_dump(struct cake_sched_config *q, struct sk_buff *skb)
 		goto nla_put_failure;
 
 	if (nla_put_u64_64bit(skb, TCA_CAKE_BASE_RATE64,
-			      READ_ONCE(q->rate_bps), TCA_CAKE_PAD))
+			      atomic64_read(&q->rate_bps), TCA_CAKE_PAD))
 		goto nla_put_failure;
 
 	flow_mode = READ_ONCE(q->flow_mode);
-- 
2.43.0

Re: [PATCH net-next 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath

[Toke Høiland-Jørgensen]

> The qdisc is left with autorate disabled but with the estimate installed
> instead of X.  On 32-bit machines the unlocked 64-bit rate_bps store is
> also not atomic, so a reader can observe a torn rate.  iproute2 reaches
> this because parsing 'bandwidth X' sets autorate = 0 and emits both
> TCA_CAKE_BASE_RATE64 and TCA_CAKE_AUTORATE.
>
> Commit the parsed configuration and reconfigure while holding the same
> qdisc lock that serializes the datapath, so the netlink writer and the
> autorate writer can no longer interleave.  Make rate_bps an atomic64_t
> so a 64-bit rate update is atomic on 32-bit machines as well, and keep
> the READ_ONCE()/WRITE_ONCE() annotations for the remaining lockless
> readers (cake_config_dump(), which runs without the lock, and the
> cake_mq shared config, which readers observe under different child
> locks).

So atomic_t.txt has this section:

"SEMANTICS
---------

Non-RMW ops:

The non-RMW ops are (typically) regular LOADs and STOREs and are canonically
implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and
smp_store_release() respectively. Therefore, if you find yourself only using
the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all
and are doing it wrong."


So AFAICT, we don't need atomic_t, we just need READ/WRITE_ONCE()
annotations?

-Toke

Re: [PATCH net-next 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath

[Jamal Hadi Salim]

On Thu, Oct 8, 2026 at 6:29 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> > The qdisc is left with autorate disabled but with the estimate installed
> > instead of X.  On 32-bit machines the unlocked 64-bit rate_bps store is
> > also not atomic, so a reader can observe a torn rate.  iproute2 reaches
> > this because parsing 'bandwidth X' sets autorate = 0 and emits both
> > TCA_CAKE_BASE_RATE64 and TCA_CAKE_AUTORATE.
> >
> > Commit the parsed configuration and reconfigure while holding the same
> > qdisc lock that serializes the datapath, so the netlink writer and the
> > autorate writer can no longer interleave.  Make rate_bps an atomic64_t
> > so a 64-bit rate update is atomic on 32-bit machines as well, and keep
> > the READ_ONCE()/WRITE_ONCE() annotations for the remaining lockless
> > readers (cake_config_dump(), which runs without the lock, and the
> > cake_mq shared config, which readers observe under different child
> > locks).
>
> So atomic_t.txt has this section:
>
> "SEMANTICS
> ---------
>
> Non-RMW ops:
>
> The non-RMW ops are (typically) regular LOADs and STOREs and are canonically
> implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and
> smp_store_release() respectively. Therefore, if you find yourself only using
> the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all
> and are doing it wrong."
>
>
> So AFAICT, we don't need atomic_t, we just need READ/WRITE_ONCE()
> annotations?
>

hrm. actually, the sch_tree_lock() already protects cake_change(): it
commits the parsed config and reconfigures together with the datapath
under the same qdisc lock, so the netlink writer and the autorate
store can no longer interleave. So the atomic64_t was never needed for
the lost update.

on the atomic_t.txt point you pointed out: rate_bps takes only non-RMW
ops, so per SEMANTICS it should be a plain u64 with
READ_ONCE()/WRITE_ONCE(). I'll drop the atomic64_t.

The only lockless readers left are cake_config_dump() (cosmetic) and
the cake_mq shared config, which a child reads while holding a
different child's lock than the parent writes it under - on 32-bit
READ_ONCE() on a u64 can tear there. It's an advisory rate that
self-corrects on the next sync interval, so best-effort is fine.

I will resend a v2 with rate_bps as u64 + READ/WRITE_ONCE. Let's wait
for Sashiko first - it will always find something to complain about.

cheers,
jamal

Re: [PATCH net-next 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath

[Toke Høiland-Jørgensen]

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

> On Thu, Oct 8, 2026 at 6:29 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>> > The qdisc is left with autorate disabled but with the estimate installed
>> > instead of X.  On 32-bit machines the unlocked 64-bit rate_bps store is
>> > also not atomic, so a reader can observe a torn rate.  iproute2 reaches
>> > this because parsing 'bandwidth X' sets autorate = 0 and emits both
>> > TCA_CAKE_BASE_RATE64 and TCA_CAKE_AUTORATE.
>> >
>> > Commit the parsed configuration and reconfigure while holding the same
>> > qdisc lock that serializes the datapath, so the netlink writer and the
>> > autorate writer can no longer interleave.  Make rate_bps an atomic64_t
>> > so a 64-bit rate update is atomic on 32-bit machines as well, and keep
>> > the READ_ONCE()/WRITE_ONCE() annotations for the remaining lockless
>> > readers (cake_config_dump(), which runs without the lock, and the
>> > cake_mq shared config, which readers observe under different child
>> > locks).
>>
>> So atomic_t.txt has this section:
>>
>> "SEMANTICS
>> ---------
>>
>> Non-RMW ops:
>>
>> The non-RMW ops are (typically) regular LOADs and STOREs and are canonically
>> implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and
>> smp_store_release() respectively. Therefore, if you find yourself only using
>> the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all
>> and are doing it wrong."
>>
>>
>> So AFAICT, we don't need atomic_t, we just need READ/WRITE_ONCE()
>> annotations?
>>
>
> hrm. actually, the sch_tree_lock() already protects cake_change(): it
> commits the parsed config and reconfigures together with the datapath
> under the same qdisc lock, so the netlink writer and the autorate
> store can no longer interleave. So the atomic64_t was never needed for
> the lost update.
>
> on the atomic_t.txt point you pointed out: rate_bps takes only non-RMW
> ops, so per SEMANTICS it should be a plain u64 with
> READ_ONCE()/WRITE_ONCE(). I'll drop the atomic64_t.
>
> The only lockless readers left are cake_config_dump() (cosmetic) and
> the cake_mq shared config, which a child reads while holding a
> different child's lock than the parent writes it under - on 32-bit
> READ_ONCE() on a u64 can tear there. It's an advisory rate that
> self-corrects on the next sync interval, so best-effort is fine.
>
> I will resend a v2 with rate_bps as u64 + READ/WRITE_ONCE. Let's wait
> for Sashiko first - it will always find something to complain about.

SGTM.

-Toke

Re: [PATCH net-next 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a real, user-visible lost update but has no
  Fixes: tag.
- [Low] The reproduction section contradicts itself, and its KCSAN recipe
  does not trigger the lost update the message describes.

--

net/sched/sch_cake: serialize reconfiguration with the datapath

This patch moves the cake_config_change() commit in cake_change() under
sch_tree_lock(), so it no longer races with the autorate-ingress rate
update in cake_enqueue(). It also converts cake_sched_config.rate_bps to
atomic64_t so that 64-bit rate stores are atomic on 32-bit machines.

Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.com/

> This is a follow-up to commit 7cbfb180945c ("net/sched: sch_cake: fix
> autorate reconfiguration throttling").  That change only stored
> last_reconfig_time; it did not address the concurrency between the
> autorate rate update in cake_enqueue() and the netlink reconfiguration
> path, which the original review flagged.

[ ... ]

> Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
> Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
Should this have a Fixes: tag?

Before this patch, cake_change() called cake_config_change() with only
RTNL held. The sch_tree_lock() covered only cake_reconfigure(). The
autorate store in cake_enqueue() runs under the qdisc root lock:

    q->config->rate_bps = (q->avg_peak_bandwidth * 15) >> 4;

Commit 7cbfb180945c only added the last_reconfig_time store and did not
introduce the race. Both racing writers go back to 7298de9cd725, and
7cbfb180945c lists that same commit in its own Fixes: tag.

Without a tag, stable selection may miss this fix. Would something like
this be appropriate?

    Fixes: 7298de9cd725 ("sch_cake: Add ingress mode")

> Conditions to recreate the bug: build with CONFIG_KCSAN=y (the race is
> otherwise not observable on 64-bit), put cake in autorate-ingress on a
> device, drive bursty traffic with gaps so the 250ms autorate
> reconfiguration fires, and concurrently loop 'tc qdisc change dev ...
> cake bandwidth <N>kbit autorate-ingress'.  The lost update of the
> installed rate is reproducible without a sanitizer by running bursty
> senders while repeatedly changing an autorate qdisc to a fixed
> 'bandwidth X' and reading the installed rate back.

[Severity: Low]
Do these two recipes describe the same bug?

The KCSAN loop passes autorate-ingress, so cake_config_change() keeps
CAKE_FLAG_AUTORATE_INGRESS set:

    if (tb[TCA_CAKE_AUTORATE]) {
        if (!!nla_get_u32(tb[TCA_CAKE_AUTORATE])) {
            ...
            rate_flags |= CAKE_FLAG_AUTORATE_INGRESS;
        } else {
            rate_flags &= ~CAKE_FLAG_AUTORATE_INGRESS;
        }
    }

With autorate still enabled, the next estimate is supposed to replace N.
That loop would produce a KCSAN data-race report on rate_bps. It would
not show the lost configured rate from the race diagram, which needs a
change that clears autorate ('bandwidth X' without autorate-ingress).

Also, "the race is otherwise not observable on 64-bit" seems to
contradict the next sentence. The lost update does not depend on a torn
store, so it happens on 64-bit too. Only the torn 64-bit store is
specific to 32-bit.

Could the reproduction section keep the two effects separate and give a
recipe that matches the race diagram?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-5RTM.v1.20261007130704%40mojatatu.com

[PATCH net-next 2/4] net/sched/sch_cake: reduce parent backlog when clearing a tin

[Jamal Hadi Salim]

CAKE has purged unused tins on DiffServ reconfiguration since commit
83f8fd69af4f ("sch_cake: Add DiffServ handling") without propagating the
resulting backlog reduction to classful parents. The issue was noticed
while reviewing 7cbfb180945c, but predates that change.

cake_clear_tin() drops every packet from a tin with
kfree_skb_reason() but never calls qdisc_tree_reduce_backlog(), so a
CAKE qdisc under a classful parent (e.g. HTB) leaves the dropped packets
in the parent qdisc's and class's qlen/backlog accounting forever. The
normal dequeue path does reduce the parent; only the live reconfigure
purge misses it.

Tins are cleared from cake_reset() and, on reconfigure, from
cake_configure_rates(). The qdisc core already owns the parent delta for
the reset path: qdisc_purge_queue() snapshots qlen/backlog, runs
qdisc_reset(), then reduces the tree, so propagating the reduction from
cake_clear_tin() itself would subtract the same backlog twice. Snapshot
CAKE's qlen/backlog around only the reconfiguration purge in
cake_configure_rates() and reduce the parent once afterwards, matching
the delta pattern already used by cake_enqueue().

Conditions to recreate the bug: put CAKE (diffserv8) under an HTB class,
flood DSCP-classified traffic into a high tin so it backlogs, then change
CAKE to diffserv3. The HTB parent keeps the purged packets in its qlen
and backlog; a kernel with the fix shows the parent at the leaf's backlog
(426b/5p) instead of retaining the purged packets.

Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.com/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_cake.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 1c29695dc928..2bad0b6eb13f 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -2611,10 +2611,17 @@ static void cake_configure_rates(struct Qdisc *sch, u64 rate, bool rate_adjust)
 	}
 
 	if (!rate_adjust) {
+		u32 backlog = sch->qstats.backlog, qlen = sch->q.qlen;
+
 		for (c = qd->tin_cnt; c < CAKE_MAX_TINS; c++) {
 			cake_clear_tin(sch, c);
 			qd->tins[c].cparams.mtu_time = qd->tins[ft].cparams.mtu_time;
 		}
+
+		backlog -= sch->qstats.backlog;
+		qlen -= sch->q.qlen;
+		if (qlen)
+			qdisc_tree_reduce_backlog(sch, qlen, backlog);
 	}
 
 	qd->rate_ns   = qd->tins[ft].tin_rate_ns;
-- 
2.43.0

Re: [PATCH net-next 2/4] net/sched/sch_cake: reduce parent backlog when clearing a tin

[Toke Høiland-Jørgensen]

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

> CAKE has purged unused tins on DiffServ reconfiguration since commit
> 83f8fd69af4f ("sch_cake: Add DiffServ handling") without propagating the
> resulting backlog reduction to classful parents. The issue was noticed
> while reviewing 7cbfb180945c, but predates that change.
>
> cake_clear_tin() drops every packet from a tin with
> kfree_skb_reason() but never calls qdisc_tree_reduce_backlog(), so a
> CAKE qdisc under a classful parent (e.g. HTB) leaves the dropped packets
> in the parent qdisc's and class's qlen/backlog accounting forever. The
> normal dequeue path does reduce the parent; only the live reconfigure
> purge misses it.
>
> Tins are cleared from cake_reset() and, on reconfigure, from
> cake_configure_rates(). The qdisc core already owns the parent delta for
> the reset path: qdisc_purge_queue() snapshots qlen/backlog, runs
> qdisc_reset(), then reduces the tree, so propagating the reduction from
> cake_clear_tin() itself would subtract the same backlog twice. Snapshot
> CAKE's qlen/backlog around only the reconfiguration purge in
> cake_configure_rates() and reduce the parent once afterwards, matching
> the delta pattern already used by cake_enqueue().
>
> Conditions to recreate the bug: put CAKE (diffserv8) under an HTB class,
> flood DSCP-classified traffic into a high tin so it backlogs, then change
> CAKE to diffserv3. The HTB parent keeps the purged packets in its qlen
> and backlog; a kernel with the fix shows the parent at the leaf's backlog
> (426b/5p) instead of retaining the purged packets.
>
> Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
> Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

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

Re: [PATCH net-next 2/4] net/sched/sch_cake: reduce parent backlog when clearing a tin

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit is missing a Fixes: tag.

--

net/sched/sch_cake: reduce parent backlog when clearing a tin

This commit changes cake_configure_rates() to record CAKE's qlen and
backlog before and after the purge of unused tins during DiffServ
reconfiguration. It then calls qdisc_tree_reduce_backlog() once with the
difference. Before this, cake_clear_tin() dropped those packets without
updating classful parents such as HTB, so their qlen/backlog accounting
was left wrong.

Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.com/

> CAKE has purged unused tins on DiffServ reconfiguration since commit
> 83f8fd69af4f ("sch_cake: Add DiffServ handling") without propagating the
> resulting backlog reduction to classful parents. The issue was noticed
> while reviewing 7cbfb180945c, but predates that change.

[ ... ]

> Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
> Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
Should this patch have a Fixes: tag? The commit message names 83f8fd69af4f
as the commit that added the cake_clear_tin() purge in
cake_configure_rates() without a parent backlog reduction. However, the
only trailers are Reported-by, Link, Reviewed-by and Signed-off-by.

Would it make sense to add something like this:

Fixes: 83f8fd69af4f ("sch_cake: Add DiffServ handling")

so that stable tooling can pick up the fix for older kernels?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-5RTM.v1.20261007130704%40mojatatu.com

[PATCH net-next 3/4] net/sched/sch_cake: widen buffer_used to u64

[Jamal Hadi Salim]

This is a follow-up to commit 4c660ee8c809 ("net: sched: fix 32-bit
backlog wrap in gred, bfifo and plug enqueue"), which promoted the
backlog + length admission sums in gred, bfifo and plug to u64.  CAKE's
own memory accounting has the same 32-bit wrap.

cake_sched_data.buffer_used accumulates skb->truesize and is compared
against memory_limit:

	q->buffer_used += skb->truesize;
	if (q->buffer_used <= q->buffer_limit)
		return NET_XMIT_SUCCESS;
	while (q->buffer_used > q->buffer_limit)
		drop...

buffer_used is u32, so once the resident truesize passes 2^32 it wraps to
a small value, the <= test passes and the drop loop is skipped even
though far more than the configured limit is queued.  memory_limit is a
u32 attribute (TCA_CAKE_MEMORY), so buffer_limit can be as large as
U32_MAX and never caps buffer_used below the wrap point; with a large
memory_limit the queue then grows until the machine is out of memory.

Widen buffer_used and buffer_max_used to u64, the same shape as the
gred/bfifo/plug fix.  buffer_limit stays u32; the comparison promotes it
to u64, so the bounded queue stops at the configured limit below 2^32 and
the counter can no longer wrap.  buffer_used is subtracted on dequeue and
drop, so the widened type flows through unchanged.

The ACK-filter replacement applied the net delta as one expression,
q->buffer_used += skb->truesize - ack->truesize.  Both operands are
unsigned int, so the subtraction is evaluated at u32 and a larger removed
ACK wraps the delta; the old u32 accumulator folded that back to the
correct net value, but the widened u64 accumulator would persist it as a
roughly 4 GiB over-count.  Apply the replacement as two exact operations
on the widened counter instead; the queue already accounts ack->truesize,
so the subtraction cannot underflow.

Conditions to recreate the bug: CAP_NET_ADMIN (root or a user namespace,
the qdisc attach is gated by netlink_net_capable(CAP_NET_ADMIN)).

  # hold a tun device's tx ring so the root qdisc parks packets
  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: cake memlimit 4294967295

  # drive >4 GiB of resident truesize through the qdisc (e.g. several UDP
  # sockets with SO_SNDBUFFORCE). On the unfixed kernel buffer_used wraps,
  # the drop loop is bypassed and the guest OOMs; with the fix the counter
  # passes the limit without wrapping and drops the excess.

  # the ACK-filter over-count is reached with ack-filter enabled and a
  # queued pure ACK whose truesize is larger than the replacement ACK:
  tc qdisc add dev tun0 root handle 1: cake ack-filter

Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
Link: https://lore.kernel.org/netdev/20260818095927.15901-1-jhs@mojatatu.com/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_cake.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 2bad0b6eb13f..4caf9718c9bd 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -234,8 +234,9 @@ struct cake_sched_data {
 	u16		tin_cnt;
 
 	/* resource tracking */
-	u32		buffer_used;
-	u32		buffer_max_used;
+
+	u64		buffer_max_used;
+	u64		buffer_used;
 	u32		buffer_limit;
 
 	/* indices for dequeue */
@@ -1850,7 +1851,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			qdisc_qstats_drop(sch);
 			ack_pkt_len = qdisc_pkt_len(ack);
 			WRITE_ONCE(b->bytes, b->bytes + ack_pkt_len);
-			q->buffer_used += skb->truesize - ack->truesize;
+			q->buffer_used += skb->truesize;
+			q->buffer_used -= ack->truesize;
 			if (q->config->rate_flags & CAKE_FLAG_INGRESS)
 				cake_advance_shaper(q, b, ack, now, true);
 
-- 
2.43.0

Re: [PATCH net-next 3/4] net/sched/sch_cake: widen buffer_used to u64

[Toke Høiland-Jørgensen]

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

> This is a follow-up to commit 4c660ee8c809 ("net: sched: fix 32-bit
> backlog wrap in gred, bfifo and plug enqueue"), which promoted the
> backlog + length admission sums in gred, bfifo and plug to u64.  CAKE's
> own memory accounting has the same 32-bit wrap.
>
> cake_sched_data.buffer_used accumulates skb->truesize and is compared
> against memory_limit:
>
> 	q->buffer_used += skb->truesize;
> 	if (q->buffer_used <= q->buffer_limit)
> 		return NET_XMIT_SUCCESS;
> 	while (q->buffer_used > q->buffer_limit)
> 		drop...
>
> buffer_used is u32, so once the resident truesize passes 2^32 it wraps to
> a small value, the <= test passes and the drop loop is skipped even
> though far more than the configured limit is queued.  memory_limit is a
> u32 attribute (TCA_CAKE_MEMORY), so buffer_limit can be as large as
> U32_MAX and never caps buffer_used below the wrap point; with a large
> memory_limit the queue then grows until the machine is out of memory.
>
> Widen buffer_used and buffer_max_used to u64, the same shape as the
> gred/bfifo/plug fix.  buffer_limit stays u32; the comparison promotes it
> to u64, so the bounded queue stops at the configured limit below 2^32 and
> the counter can no longer wrap.  buffer_used is subtracted on dequeue and
> drop, so the widened type flows through unchanged.
>
> The ACK-filter replacement applied the net delta as one expression,
> q->buffer_used += skb->truesize - ack->truesize.  Both operands are
> unsigned int, so the subtraction is evaluated at u32 and a larger removed
> ACK wraps the delta; the old u32 accumulator folded that back to the
> correct net value, but the widened u64 accumulator would persist it as a
> roughly 4 GiB over-count.  Apply the replacement as two exact operations
> on the widened counter instead; the queue already accounts ack->truesize,
> so the subtraction cannot underflow.
>
> Conditions to recreate the bug: CAP_NET_ADMIN (root or a user namespace,
> the qdisc attach is gated by netlink_net_capable(CAP_NET_ADMIN)).
>
>   # hold a tun device's tx ring so the root qdisc parks packets
>   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: cake memlimit 4294967295
>
>   # drive >4 GiB of resident truesize through the qdisc (e.g. several UDP
>   # sockets with SO_SNDBUFFORCE). On the unfixed kernel buffer_used wraps,
>   # the drop loop is bypassed and the guest OOMs; with the fix the counter
>   # passes the limit without wrapping and drops the excess.
>
>   # the ACK-filter over-count is reached with ack-filter enabled and a
>   # queued pure ACK whose truesize is larger than the replacement ACK:
>   tc qdisc add dev tun0 root handle 1: cake ack-filter
>
> Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
> Link: https://lore.kernel.org/netdev/20260818095927.15901-1-jhs@mojatatu.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

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

Re: [PATCH net-next 3/4] net/sched/sch_cake: widen buffer_used to u64

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 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 patch widens cake_sched_data.buffer_max_used to u64, but
  cake_dump_stats() still exports it with PUT_STAT_U32(MEMORY_USED,…
- [Low] The commit fixes a user-visible bug: with a large memlimit, the
  u32 cake_sched_data.buffer_used wraps, the memory-limit drop loop in…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the memory-
  limit drop loop in cake_enqueue() can spin forever when a populated…

--

net/sched/sch_cake: widen buffer_used to u64

This patch widens cake_sched_data.buffer_used and buffer_max_used from u32
to u64. CAKE's truesize accounting can then no longer wrap past 2^32 and
skip the memory-limit drop loop in cake_enqueue(). The patch also splits
the ACK-filter replacement accounting into a separate add and subtract, so
a wrapped u32 delta does not stay in the wider counter.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
Link: https://lore.kernel.org/netdev/20260818095927.15901-1-jhs@mojatatu.com/

> Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818095927.15901-1-jhs@mojatatu.com
> Link: https://lore.kernel.org/netdev/20260818095927.15901-1-jhs@mojatatu.com/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
Should this patch have a Fixes: tag?

The u32 buffer_used, buffer_max_used and buffer_limit fields all seem to
come from the original CAKE commit. The related fix this patch follows,
4c660ee8c809, has a Fixes: tag. Without one, stable backporters may miss
this OOM fix. Perhaps:

Fixes: 046f6fd5daef ("sched: Add Common Applications Kept Enhanced (cake) qdisc")

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index 2bad0b6eb13fd..4caf9718c9bda 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -234,8 +234,9 @@ struct cake_sched_data {
>  	u16		tin_cnt;
>  
>  	/* resource tracking */
> -	u32		buffer_used;
> -	u32		buffer_max_used;
> +
> +	u64		buffer_max_used;

[Severity: Medium]
With buffer_max_used now u64, can the memory-used stat get truncated?
cake_dump_stats() still exports it as a u32:

net/sched/sch_cake.c:cake_dump_stats() {
    ...
	PUT_STAT_U32(MEMORY_LIMIT, READ_ONCE(q->buffer_limit));
	PUT_STAT_U32(MEMORY_USED, READ_ONCE(q->buffer_max_used));
    ...
}

Documentation/netlink/specs/tc.yaml also declares memory-used in
cake-stats-attrs as u32.

cake_enqueue() records the high-water mark before the drop loop runs:

	if (q->buffer_used > q->buffer_max_used)
		WRITE_ONCE(q->buffer_max_used, q->buffer_used);

	if (q->buffer_used <= q->buffer_limit)
		return NET_XMIT_SUCCESS;
	...
	while (q->buffer_used > q->buffer_limit) {

With memlimit 4294967295, buffer_limit is U32_MAX. Once about 4 GiB of
truesize is queued, the next enqueue pushes buffer_used past 2^32. It
overshoots by up to that skb's truesize, or by the whole segmented GSO
aggregate with split-gso. The commit message says this case can happen:
"the counter passes the limit without wrapping".

Nothing ever lowers buffer_max_used. After that, nla_put_u32() reports
memory-used as peak - 2^32, which is a few KB, for the rest of the qdisc's
life. The final patch in the series does not change this.

Could the value be clamped, e.g. with min_t(u64, ..., U32_MAX)? Or could
it be exported through a new 64-bit attribute, the way CAPACITY_ESTIMATE64
uses PUT_STAT_U64 with TCA_CAKE_STATS_PAD?

> +	u64		buffer_used;
>  	u32		buffer_limit;

[Severity: High]
This issue is pre-existing and was not introduced by this patch. Can the
memory-limit drop loop in cake_enqueue() spin forever when a flow's u32
backlog wraps to 0?

The heap uses per-flow byte counters as keys to pick drop victims, and
those counters are still u32:

struct cake_tin_data {
    ...
	u32	backlogs[CAKE_QUEUES];
    ...
};

A size table can push qdisc_pkt_len() as high as QDISC_PKT_LEN_MAX
(1 << 20). For example, with tsize=0 and overhead >= 1 MiB,
__qdisc_calculate_pkt_len() clamps the length to that value.

If 4096 such non-GSO packets sit in one flow, backlogs[idx] wraps to
exactly 0. Those packets use only a few MB of real truesize. If that
truesize is above a small memlimit, every heap key is now 0.

cake_drop() rebuilds the heap, but cake_heapify() only swaps when it finds
a strictly larger backlog:

		if (lb > mb) {
			m  = l;
			mb = lb;
		}

So the root stays at the initial tin0/flow0 entry, or at a flow that was
already drained. The root flow is empty, so cake_drop() returns without
lowering buffer_used:

	skb = dequeue_head(flow);
	if (unlikely(!skb)) {
		/* heap has gone wrong, rebuild it next time */
		q->overflow_timeout = 0;
		return idx + (tin << 16);
	}

The caller in cake_enqueue() has no other way out of the loop:

	while (q->buffer_used > q->buffer_limit) {
		drop_id = cake_drop(sch, to_free);
		...
	}

This loop runs in the xmit path with the qdisc root lock held and BH
disabled. A CAP_NET_ADMIN user in a user namespace can set it up with a
stopped tun TX ring, as in the commit message's reproducer.

This patch widens buffer_used but leaves backlogs[], tin_backlog and the
heap keys at u32. The final patch in the series does not change this
either.

Should the drop loop also stop when cake_drop() makes no progress?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-5RTM.v1.20261007130704%40mojatatu.com

[PATCH net-next 4/4] net/sched/sch_cake: drop dead nla_nest_end() error check

[Jamal Hadi Salim]

cake_dump_class_stats() closes the TCA_STATS_APP nest with

	if (nla_nest_end(d->skb, stats) < 0)
		return -1;

nla_nest_end() never returns a negative value: it has a single
`return skb->len;` and no error path (that is why the overflow-aware
nla_nest_end_safe() helper was later added). The '< 0' test can never
be true, so the 'return -1' is unreachable dead code.

Drop the check and close the nest with the bare statement, matching the
sibling cake nests in the same file.

This issue was discovered while fixing a bug on the gate action, see:
https://lore.kernel.org/netdev/QDISC-H19Z.v1.20261001053234@mojatatu.com/

Conditions to recreate the bug:
- not runtime observable: the issue is found by inspection.

Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_cake.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 4caf9718c9bd..124f552023f8 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -3211,8 +3211,7 @@ static int cake_dump_class_stats(struct Qdisc *sch, unsigned long cl,
 						       READ_ONCE(flow->cvars.drop_next))));
 		}
 
-		if (nla_nest_end(d->skb, stats) < 0)
-			return -1;
+		nla_nest_end(d->skb, stats);
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH net-next 4/4] net/sched/sch_cake: drop dead nla_nest_end() error check

[Toke Høiland-Jørgensen]

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

> cake_dump_class_stats() closes the TCA_STATS_APP nest with
>
> 	if (nla_nest_end(d->skb, stats) < 0)
> 		return -1;
>
> nla_nest_end() never returns a negative value: it has a single
> `return skb->len;` and no error path (that is why the overflow-aware
> nla_nest_end_safe() helper was later added). The '< 0' test can never
> be true, so the 'return -1' is unreachable dead code.
>
> Drop the check and close the nest with the bare statement, matching the
> sibling cake nests in the same file.
>
> This issue was discovered while fixing a bug on the gate action, see:
> https://lore.kernel.org/netdev/QDISC-H19Z.v1.20261001053234@mojatatu.com/
>
> Conditions to recreate the bug:
> - not runtime observable: the issue is found by inspection.
>
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

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