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

[PATCH net-next v2 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 needs no atomic
(it is only loaded and stored), so it stays a plain u64 with
READ_ONCE()/WRITE_ONCE() annotations.

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; the
u32 MEMORY_USED export is clamped so widening the counter does not
truncate the reported value.

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.

v1 -> v2:
 - Patch 1/4: drop atomic64_t; rate_bps stays a plain u64 with
   READ_ONCE()/WRITE_ONCE() annotations (per Toke's review: the accesses
   are plain loads/stores, so no atomic is needed).
 - Patch 2/4: no change; carries Toke's v1 Acked-by.
 - Patch 3/4: clamp the MEMORY_USED export to U32_MAX, so widening the
   counter does not truncate the u32 stat (Sashiko nipa Medium).
 - Patch 4/4: no change; carries Toke's v1 Acked-by.

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 | 52 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH net-next v2 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.

The issue:

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.

The fix:

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.  rate_bps needs no atomic: it is
only ever loaded and stored, so plain READ_ONCE()/WRITE_ONCE() annotations
cover the readers that run outside the lock (cake_config_dump(), which is
not serialized, and the cake_mq shared config, observed under different
child locks).

Conditions to recreate the bug: build with CONFIG_KCSAN=y.  Two distinct
effects are reachable.  (a) The lost update of the installed rate is
reproducible without a sanitizer: 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', then read the installed rate
back and observe the estimate instead of the configured rate.  (b) The
non-atomic 64-bit store that KCSAN reports needs a 32-bit machine, where
the torn store of rate_bps is observable; on 64-bit the value is written
atomically and KCSAN flags only the unsynchronized access.

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 | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..a05da73b5f84 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -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;
+				WRITE_ONCE(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 = READ_ONCE(q->config->rate_bps);
 		u64 other_qlen, other_last_active;
 		struct Qdisc *other_sch;
 		u32 num_active_qs = 1;
@@ -2042,7 +2043,8 @@ 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(READ_ONCE(q->config->rate_bps),
+					     num_active_qs);
 
 		cake_configure_rates(sch, new_rate, true);
 		q->last_checked_active = now;
@@ -2626,12 +2628,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, READ_ONCE(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 (READ_ONCE(q->rate_bps)) {
+		u64 t = READ_ONCE(q->rate_bps) * q->interval;
 
 		do_div(t, USEC_PER_SEC / 4);
 		buffer_limit = max_t(u32, t, 4U << 20);
@@ -2784,9 +2786,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 +2804,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)

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 2

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 lists a torn 64-bit store of rate_bps on
  32-bit machines as effect (b) of the bug.
- [Medium] sch_cake: buffer_limit truncation wraps queue capacity at high
  bandwidths
- [Low] The commit fixes a real, user-visible lost update but has no
  Fixes: tag, and it describes itself as a 'follow-up'.
- [Low] Reproducer (a) in the commit message cannot show the bug.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  cake_mq_change() still commits the shared config without serializing…

--

net/sched/sch_cake: serialize reconfiguration with the datapath

This commit takes sch_tree_lock() in cake_change() before calling
cake_config_change(). The netlink configuration commit and
cake_reconfigure() are then serialized with the autorate-ingress rate
update in cake_enqueue(). The commit also annotates rate_bps accesses with
READ_ONCE()/WRITE_ONCE() for readers that run outside the qdisc lock.

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.

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

Before this patch, cake_change() ran cake_config_change() with only RTNL
held. It took sch_tree_lock() only around cake_reconfigure(). The autorate
store in cake_enqueue() runs under the qdisc lock.

That second writer was added by commit 7298de9cd725 ("sch_cake: Add
ingress mode"). Commit bc0ce2bad36c ("net/sched: sch_cake: Factor out
config variables into separate struct") only moved the field into
q->config.

The referenced commit 7cbfb180945c carries "Fixes: 7298de9cd725", but
this patch has no Fixes: trailer. Without one, stable selection cannot
tell which trees need this fix.

Would Fixes: 7298de9cd725 ("sch_cake: Add ingress mode") be appropriate?

> Conditions to recreate the bug: build with CONFIG_KCSAN=y.  Two distinct
> effects are reachable.  (a) The lost update of the installed rate is
> reproducible without a sanitizer: 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', then read the installed rate
> back and observe the estimate instead of the configured rate.

[Severity: Low]
Can recipe (a) tell a fixed kernel from an unfixed one?

The looped command ends with autorate-ingress. That sends
TCA_CAKE_AUTORATE=1, so cake_config_change() keeps the flag set:

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

While the flag is set, cake_enqueue() is designed to overwrite rate_bps
with (avg_peak_bandwidth * 15) >> 4 once 250ms have passed since
last_reconfig_time. cake_change() never resets last_reconfig_time.

So reading the rate back through cake_config_dump() would show the
estimate both with and without this patch.

The race diagram above needs a change that clears autorate ("clears
autorate in its local rate_flags"). Should the recipe use a change that
turns autorate-ingress off instead?

The same paragraph starts with "build with CONFIG_KCSAN=y" but then says
(a) can be reproduced without a sanitizer. Could that be reworded?

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..a05da73b5f848 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -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;
> +				WRITE_ONCE(q->config->rate_bps,
> +					   (q->avg_peak_bandwidth * 15) >> 4);

[Severity: Medium]
On 32-bit kernels, isn't this WRITE_ONCE() of a u64 still two 32-bit
stores?

The commit message lists the torn 64-bit store of rate_bps on 32-bit as
effect (b), and then says:

  plain READ_ONCE()/WRITE_ONCE() annotations cover the readers that run
  outside the lock (cake_config_dump(), which is not serialized, and the
  cake_mq shared config, observed under different child locks)

compiletime_assert_rwonce_type() accepts 64-bit types on 32-bit, but the
volatile access is still split into two halves. KCSAN treats accesses up
to sizeof(long long) as atomic. The annotation therefore silences the
report without making the access single-copy atomic.

cake_config_dump() reads rate_bps without the qdisc lock, while this
store runs under that lock:

	if (nla_put_u64_64bit(skb, TCA_CAKE_BASE_RATE64,
			      READ_ONCE(q->rate_bps), TCA_CAKE_PAD))

For cake_mq, cake_mq_change() writes the shared rate_bps through
cake_config_change() with no child lock held:

	ret = cake_config_change(&priv->cake_config, opt, extack, &overhead_changed);

Meanwhile each child reads READ_ONCE(q->config->rate_bps) in
cake_dequeue() under its own root lock. It passes the value straight to
cake_configure_rates(sch, new_rate, true). A torn value there becomes the
shaper rate until the next sync or the later cake_reconfigure().

Can both of these readers still see a torn rate_bps on 32-bit? If so,
should the commit message limit the claim to readers under the same qdisc
lock?

>  				q->last_reconfig_time = now;
>  				cake_reconfigure(sch);
>  			}

[ ... ]

> @@ -2784,9 +2786,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;

[Severity: Medium]
This isn't a bug introduced by this patch, but cake_mq_change() still
commits the shared config the same way cake_change() did before this
patch:

	ret = cake_config_change(&priv->cake_config, opt, extack, &overhead_changed);
	...
		if (qd->tins) {
			sch_tree_lock(chld);
			cake_reconfigure(chld);
			sch_tree_unlock(chld);
		}

cake_config_change() publishes tin_mode, rate_bps, rate_flags, flow_mode
and the other fields with only RTNL held.

Until cake_mq_change() reaches a given child, that child's cake_dequeue()
can still run the is_shared sync block under its own root lock once
sync_time has elapsed:

cake_dequeue()
  cake_configure_rates(sch, new_rate, true)
    cake_config_besteffort()    /* new tin_mode, qd->tin_cnt = 1 */

Because rate_adjust is true, cake_configure_rates() skips the tin purge:

	if (!rate_adjust) {
		...
		for (c = qd->tin_cnt; c < CAKE_MAX_TINS; c++) {
			cake_clear_tin(sch, c);

Take a diffserv8 -> besteffort change. Could packets still queued in tins
1..7 then fall outside the shaped-mode scan? cake_dequeue() would return
NULL at the "No point in going further" check with sch->q.qlen > 0 and no
watchdog scheduled. That would last until cake_reconfigure() runs for
that child and purges those tins.

The child datapaths also read q->config->rate_flags, flow_mode and
tin_mode with plain loads. The matching WRITE_ONCE() stores run under a
different lock, so the children can see a mix of old and new config.

Should cake_mq_change() get similar treatment in a follow-up?

[ ... ]

> @@ -2626,12 +2628,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, READ_ONCE(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 (READ_ONCE(q->rate_bps)) {
> +		u64 t = READ_ONCE(q->rate_bps) * q->interval;
>  
>  		do_div(t, USEC_PER_SEC / 4);
>  		buffer_limit = max_t(u32, t, 4U << 20);

[Severity: Medium]
This isn't a bug introduced by this patch, but this line is being
touched, so: t is computed as a u64 here and then handed to
max_t(u32, t, 4U << 20) two lines below. max_t() casts both operands
to u32 before it compares them. The upper 32 bits of t are therefore
thrown away rather than clamped.

After do_div(t, USEC_PER_SEC / 4), t is rate_bps * interval / 250000.
With the default 100ms rtt that is 0.4 * rate_bps. It passes U32_MAX
once rate_bps exceeds about 10.7e9 bytes/s, roughly 85.9 Gbit/s. A
larger rtt lowers the threshold in proportion; with rtt 1s it is
already reached at about 8.6 Gbit/s.

Above that point buffer_limit becomes t modulo 4GB. Right at the
threshold it drops to the 4MB floor, and it keeps cycling as the rate
grows. The final WRITE_ONCE(qd->buffer_limit, ...) takes the min()
against max(sch->limit * psched_mtu(), buffer_config_limit), which is
around 15MB with the default limit. So a higher configured rate can
end up with a smaller memory limit than a lower one. cake_enqueue()
then starts calling cake_drop() much earlier than the configuration
implies.

buffer_used is already a u64 in cake_sched_data, while buffer_limit
is still a u32. Should t be clamped in u64 before it is narrowed, for
example with min_t(u64, t, U32_MAX) ahead of the max_t()? Or should
buffer_limit be widened to match buffer_used?

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

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

[Björn Töpel]

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

> 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.
>
> The issue:
>
> 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.
>
> The fix:
>
> 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.  rate_bps needs no atomic: it is
> only ever loaded and stored, so plain READ_ONCE()/WRITE_ONCE() annotations
> cover the readers that run outside the lock (cake_config_dump(), which is
> not serialized, and the cake_mq shared config, observed under different
> child locks).
>
> Conditions to recreate the bug: build with CONFIG_KCSAN=y.  Two distinct
> effects are reachable.  (a) The lost update of the installed rate is
> reproducible without a sanitizer: 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', then read the installed rate
> back and observe the estimate instead of the configured rate.  (b) The
> non-atomic 64-bit store that KCSAN reports needs a 32-bit machine, where
> the torn store of rate_bps is observable; on 64-bit the value is written
> atomically and KCSAN flags only the unsynchronized access.
>
> 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 | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e7..a05da73b5f84 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -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;
> +				WRITE_ONCE(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 = READ_ONCE(q->config->rate_bps);

Messing up reverse xmas tree.

>  		u64 other_qlen, other_last_active;
>  		struct Qdisc *other_sch;
>  		u32 num_active_qs = 1;
> @@ -2042,7 +2043,8 @@ 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(READ_ONCE(q->config->rate_bps),
> +					     num_active_qs);

Just use new_rate, instead of a new READ_ONCE().

>  
>  		cake_configure_rates(sch, new_rate, true);
>  		q->last_checked_active = now;
> @@ -2626,12 +2628,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, READ_ONCE(qd->config->rate_bps),
> false);

Maybe just introduce a u64 rate_bps = READ_ONCE(...), and use here... 
>  
>  	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 (READ_ONCE(q->rate_bps)) {

...and here...

> +		u64 t = READ_ONCE(q->rate_bps) * q->interval;

and here.


Björn

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

[Jamal Hadi Salim]

Hey Björn,

On Mon, Oct 19, 2026 at 9:07 AM Björn Töpel <bjorn@kernel.org> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > 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.
> >

You are reviewing a very old version - the last one was a v6[1], most
of these fixes are in the "no good deed goes unpunished" from sashiko
;->
I am about to post a v7. Maybe review that one?

cheers,
jamal

[1] https://lore.kernel.org/netdev/QDISC-BA27.v6.r0.20261014054826@mojatatu.com/

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

[Björn Töpel]

Jamal!

On Mon, 19 Oct 2026 at 15:47, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> Hey Björn,
>
> On Mon, Oct 19, 2026 at 9:07 AM Björn Töpel <bjorn@kernel.org> wrote:
> >
> > Jamal Hadi Salim <jhs@mojatatu.com> writes:
> >
> > > 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.
> > >
>
> You are reviewing a very old version - the last one was a v6[1], most
> of these fixes are in the "no good deed goes unpunished" from sashiko
> ;->
> I am about to post a v7. Maybe review that one?

OMG... sorry for the noise.


Björn

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

[Jamal Hadi Salim]

On Mon, Oct 19, 2026 at 10:53 AM Björn Töpel <bjorn@kernel.org> wrote:
>
> Jamal!
>
> On Mon, 19 Oct 2026 at 15:47, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > Hey Björn,
> >
> > On Mon, Oct 19, 2026 at 9:07 AM Björn Töpel <bjorn@kernel.org> wrote:
> > >
> > > Jamal Hadi Salim <jhs@mojatatu.com> writes:
> > >
> > > > 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.
> > > >
> >
> > You are reviewing a very old version - the last one was a v6[1], most
> > of these fixes are in the "no good deed goes unpunished" from sashiko
> > ;->
> > I am about to post a v7. Maybe review that one?
>
> OMG... sorry for the noise.
>

Not noise at all - with all these AI dump it is overwhelming. Always
great to have someone review. Sashiko will always find something to
whine about - the bikeshedding it engages in is shameless sometimes;->
I just sent an update.

cheers,
jamal


> Björn

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

[Toke Høiland-Jørgensen]

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

> On Mon, Oct 19, 2026 at 10:53 AM Björn Töpel <bjorn@kernel.org> wrote:
>>
>> Jamal!
>>
>> On Mon, 19 Oct 2026 at 15:47, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> >
>> > Hey Björn,
>> >
>> > On Mon, Oct 19, 2026 at 9:07 AM Björn Töpel <bjorn@kernel.org> wrote:
>> > >
>> > > Jamal Hadi Salim <jhs@mojatatu.com> writes:
>> > >
>> > > > 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.
>> > > >
>> >
>> > You are reviewing a very old version - the last one was a v6[1], most
>> > of these fixes are in the "no good deed goes unpunished" from sashiko
>> > ;->
>> > I am about to post a v7. Maybe review that one?
>>
>> OMG... sorry for the noise.
>>
>
> Not noise at all - with all these AI dump it is overwhelming. Always
> great to have someone review. Sashiko will always find something to
> whine about - the bikeshedding it engages in is shameless sometimes;->
> I just sent an update.

Erm, I think you're confusing patch series? This one is not the one
you're at v7 on, Björn did review the latest version AFAICT (and I agree
with his comments FWIW) :)

-Toke

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

[Jamal Hadi Salim]

On Mon, Oct 19, 2026 at 2:39 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > On Mon, Oct 19, 2026 at 10:53 AM Björn Töpel <bjorn@kernel.org> wrote:
> >>
> >> Jamal!
> >>
> >> On Mon, 19 Oct 2026 at 15:47, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >> >
> >> > Hey Björn,
> >> >
> >> > On Mon, Oct 19, 2026 at 9:07 AM Björn Töpel <bjorn@kernel.org> wrote:
> >> > >
> >> > > Jamal Hadi Salim <jhs@mojatatu.com> writes:
> >> > >
> >> > > > 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.
> >> > > >
> >> >
> >> > You are reviewing a very old version - the last one was a v6[1], most
> >> > of these fixes are in the "no good deed goes unpunished" from sashiko
> >> > ;->
> >> > I am about to post a v7. Maybe review that one?
> >>
> >> OMG... sorry for the noise.
> >>
> >
> > Not noise at all - with all these AI dump it is overwhelming. Always
> > great to have someone review. Sashiko will always find something to
> > whine about - the bikeshedding it engages in is shameless sometimes;->
> > I just sent an update.
>
> Erm, I think you're confusing patch series? This one is not the one
> you're at v7 on, Björn did review the latest version AFAICT (and I agree
> with his comments FWIW) :)

Sigh. You are correct - too many patches in flight (in this case both
were 4 patch series and had "cake" in the subject).
Sorry Björn - i will send with your suggestions in place (I notice
that both sashikos have responded and bizare with all their
bikeshedding skills they both missed reverse xmas;->)

cheers,
jamal

> -Toke

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

[Jamal Hadi Salim]

The issue:
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.

The fix:
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/
Link: https://lore.kernel.org/netdev/8733ugtry8.fsf@toke.dk/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v1 -> v2:
- No changes to this patch. Resent as part of the
  series_cake_clear_tin_backlog_underflow v2, which revises patches 1/4
  and 3/4. It is unchanged and carries Toke Høiland-Jørgensen's Acked-by
  from the v1 posting.

 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 v2 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 "net/sched/sch_cake: reduce parent backlog when
  clearing a tin" fixes a real, user-visible accounting bug but has no…

--

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

The patch changes cake_configure_rates() so that, on a reconfigure
(!rate_adjust), it snapshots CAKE's qlen and backlog before purging the
unused tins with cake_clear_tin(). Afterwards it calls
qdisc_tree_reduce_backlog() with the delta, so classful parents like HTB
no longer keep counting the purged packets. The reset path is left
alone because qdisc_purge_queue() already reduces the tree there.

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

> The issue:
> 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/
> Link: https://lore.kernel.org/netdev/8733ugtry8.fsf@toke.dk/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
This isn't a bug, but should this have a Fixes: trailer?

The changelog already names the commit that introduced the missing
parent accounting in cake_configure_rates()->cake_clear_tin(). The
trailer block has Reported-by, Link, Reviewed-by, Acked-by and
Signed-off-by, but no Fixes: tag.

Without the tag, stable and backport tooling may not match this fix to
83f8fd69af4f. Stable kernels would then keep the wrong HTB parent
qlen/backlog after a diffserv8 to diffserv3 change.

Would something like this work?

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

The code change itself looks correct to me. It reduces the parent only
for the reconfigure purge, and the reset path is already covered by
qdisc_purge_queue().

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

[PATCH net-next v2 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.

The issue:

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.

The fix:

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.

Widening the counters alone is incomplete because cake_dump_stats() still
exports buffer_max_used through PUT_STAT_U32(MEMORY_USED, ...), and
tc.yaml declares memory-used as u32.  buffer_max_used only ever rises, so
once the widened counter passes 2^32 the export reports peak - 2^32 for
the rest of the qdisc's life.  Clamp the exported value to U32_MAX; the
counter itself is unaffected.

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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..79a07fa80b35 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);
 
@@ -3012,7 +3014,8 @@ static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 
 	PUT_STAT_U64(CAPACITY_ESTIMATE64, READ_ONCE(q->avg_peak_bandwidth));
 	PUT_STAT_U32(MEMORY_LIMIT, READ_ONCE(q->buffer_limit));
-	PUT_STAT_U32(MEMORY_USED, READ_ONCE(q->buffer_max_used));
+	PUT_STAT_U32(MEMORY_USED,
+		     min_t(u64, READ_ONCE(q->buffer_max_used), U32_MAX));
 	PUT_STAT_U32(AVG_NETOFF, ((READ_ONCE(q->avg_netoff) + 0x8000) >> 16));
 	PUT_STAT_U32(MAX_NETLEN, READ_ONCE(q->max_netlen));
 	PUT_STAT_U32(MAX_ADJLEN, READ_ONCE(q->max_adjlen));

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

[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 user-reachable unbounded-queue / OOM bug but
  has no Fixes: tag (and no stable Cc).
- [Low] Widening `q->buffer_max_used` from u32 to u64 makes the lockless
  access from the dump path non-atomic on 32-bit architectures.

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

--

net/sched/sch_cake: widen buffer_used to u64

This commit widens cake_sched_data.buffer_used and buffer_max_used from
u32 to u64, so a 32-bit wrap can no longer bypass the memory limit
admission test in cake_enqueue(). It also splits the ACK-filter truesize
delta into two separate operations, and clamps the exported MEMORY_USED
statistic to U32_MAX.

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/

> 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 were added
by the original CAKE commit. That commit also added the admission test
and the drop loop in cake_enqueue().

With memlimit 4294967295, cake_reconfigure() sets buffer_limit to
U32_MAX. On those kernels the u32 test "q->buffer_used > q->buffer_limit"
can never be true.

The gred/bfifo/plug fix referenced in the commit message, 4c660ee8c809,
carries a Fixes: tag. Would this trailer work here?

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 445977a235725..602c4e913c0ce 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c

[ ... ]

> @@ -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);
>  

[Severity: High]
This is a pre-existing issue, but the per-flow backlogs[] counter is
still u32 and seems to have the same kind of 32-bit wrap. Can that make
the drop loop in cake_enqueue() spin forever?

A size table can raise qdisc_pkt_len() up to QDISC_PKT_LEN_MAX (1 MiB) in
__qdisc_calculate_pkt_len() without changing skb->truesize. The non-split
path in cake_enqueue() then does:

    WRITE_ONCE(b->backlogs[idx], b->backlogs[idx] + len - ack_pkt_len);

So 4096 such packets queued in one flow wrap backlogs[idx] to 0.

Suppose memlimit is chosen so that the 4096th enqueue is the first one
over buffer_limit. At that point overflow_timeout is still 0, so the heap
has never been maintained. overflow_heap[0] still has its cake_init()
layout and points at tin 0/flow 0, which can be empty.

The loop then runs:

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

cake_drop() rebuilds the heap. Every key from cake_heap_get_backlog() is
0, and cake_heapify() only swaps when "lb > mb" or "rb > mb", so the
empty root stays at index 0. Then:

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

buffer_used is not reduced, so the loop condition stays true. Every
later rebuild picks the same empty root.

This would spin with the qdisc root lock held and BH disabled. The
preconditions are CAP_NET_ADMIN (a user namespace is enough), a stab, a
chosen memlimit, and a dequeue that is held back.

Would it make sense to also widen backlogs[], or to have the loop stop
when cake_drop() dequeues nothing?

[ ... ]

> @@ -3028,7 +3030,8 @@ static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
>  
>  	PUT_STAT_U64(CAPACITY_ESTIMATE64, READ_ONCE(q->avg_peak_bandwidth));
>  	PUT_STAT_U32(MEMORY_LIMIT, READ_ONCE(q->buffer_limit));
> -	PUT_STAT_U32(MEMORY_USED, READ_ONCE(q->buffer_max_used));
> +	PUT_STAT_U32(MEMORY_USED,
> +		     min_t(u64, READ_ONCE(q->buffer_max_used), U32_MAX));

[Severity: Low]
On 32-bit architectures, can this READ_ONCE() of the now 64-bit
buffer_max_used tear?

cake_dump_stats() runs without the qdisc root lock, because
tc_fill_qdisc() passes a NULL lock to gnet_stats_start_copy_compat().
Meanwhile cake_enqueue() updates the field:

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

On 32-bit, a u64 READ_ONCE()/WRITE_ONCE() is usually two 32-bit accesses.
When the peak crosses 2^32, a dump could see the new low word with the
old high word of 0. It would then export a small MEMORY_USED value
instead of the U32_MAX clamp.

The window opens at most once per qdisc lifetime. Other u64 stats in
this function, such as avg_peak_bandwidth and b->bytes, are already read
the same way.

>  	PUT_STAT_U32(AVG_NETOFF, ((READ_ONCE(q->avg_netoff) + 0x8000) >> 16));
>  	PUT_STAT_U32(MAX_NETLEN, READ_ONCE(q->max_netlen));
>  	PUT_STAT_U32(MAX_ADJLEN, READ_ONCE(q->max_adjlen));

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

[PATCH net-next v2 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>
Link: https://lore.kernel.org/netdev/87v77csd5q.fsf@toke.dk/
Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v1 -> v2:
 - No changes to this patch. Re-sent as part of the
   series_cake_clear_tin_backlog_underflow v2; it keeps Toke's v1
   Acked-by, carried forward.
 - Subject version tag bumped to v2; Message-Id re-baked.

 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