[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

[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

[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

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

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