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

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

v2 -> v3:
 - Patch 1/4: reuse the rate snapshot in cake_dequeue() and
   cake_reconfigure() instead of a second READ_ONCE() per use, order the
   declarations longest-first (Björn Töpel), and scope the annotation
   claim: READ_ONCE()/WRITE_ONCE() do not make a 64-bit access
   single-copy atomic on 32-bit, so the lockless cake_config_dump() and
   cake_mq readers remain best-effort (Sashiko nipa).
 - Patch 2/4: no change; carries Toke's v1 Acked-by.
 - Patch 3/4: publish buffer_max_used as u64_stats_t under the qdisc lock
   and snapshot it with u64_stats_fetch_begin()/retry(), so the lockless
   stat read is coherent on 32-bit (Sashiko gemini + nipa).
 - Patch 4/4: no change; carries its Reviewed-by and Acked-by tags.

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 | 69 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH net-next v3 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
are enough for the accesses that run outside the lock.  The lockless
paths read the rate once into a local and reuse that value, so a single
coherent snapshot is used per operation.

The annotations do not make a 64-bit access single-copy atomic on 32-bit:
READ_ONCE()/WRITE_ONCE() are still two 32-bit accesses there.  The lockless
readers -- cake_config_dump() and the cake_mq children, which observe the
shared config under a different child's lock -- can therefore still see a
torn rate on 32-bit.  That rate is an advisory shaper value and is accepted
as best-effort; the state that must be consistent (the configured rate and
the autorate flag) is written under the qdisc lock.

Conditions to recreate the bug:
The lost update (a) needs no sanitizer. Put cake in autorate-ingress on a
device, drive bursty traffic with gaps so the 250 ms autorate reconfiguration
fires, and concurrently loop 'tc qdisc change ... cake bandwidth <N>kbit'
without autorate-ingress (specifying a bandwidth clears the flag).
If the autorate store lands after the change wrote its rate but before the
change reconfigures, the qdisc ends up with autorate disabled and the estimate
installed instead of <N>; reading the rate back then shows the estimate, which
an unfixed kernel can do and a fixed one cannot.
(b) The unsynchronized access is reported by KCSAN: build with CONFIG_KCSAN=y
and run the same trigger with the change loop keeping autorate-ingress set;
KCSAN reports a data race in cake_config_change / cake_enqueue.
The non-atomic 64-bit store only shows up as tearing on a 32-bit machine;
on 64-bit the store is atomic and KCSAN flags the access discipline instead.

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/875wzctsky.fsf@toke.dk/
Link: https://lore.kernel.org/netdev/87ld7t3kl3.fsf@all.your.base.are.belong.to.us/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---

v2 -> v3:
- Reuse the rate snapshot in cake_dequeue() and cake_reconfigure() instead
  of a second READ_ONCE() per use, and order the declarations longest-first
  (Björn Töpel).
- Commit log: scope the annotation claim.  READ_ONCE()/WRITE_ONCE() do not
  make a 64-bit access single-copy atomic on 32-bit, so the lockless
  cake_config_dump()/cake_mq readers remain best-effort (Sashiko nipa).
- Conditions: make recipe (a) distinguish fixed from unfixed (the change
  must clear autorate) and separate the KCSAN-only effect (b).

v1 -> v2:
- Drop atomic64_t for rate_bps.  Per Toke's v1 review, rate_bps takes only
  non-RMW ops, so per atomic_t.txt SEMANTICS a plain u64 with READ_ONCE()/
  WRITE_ONCE() annotations is the right shape (no atomic needed).  Reverts
  the struct field to u64 and every atomic64_* back to READ_ONCE()/
  WRITE_ONCE().  The cake_change() commit-and-reconfigure-under-sch_tree_lock
  fix (the real race fix) is unchanged.

 net/sched/sch_cake.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..d71617aa56d9 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);
 			}
@@ -2019,9 +2020,9 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
 
 	if (q->config->is_shared && q->rate_ns &&
 	    now - q->last_checked_active >= q->config->sync_time) {
+		u64 new_rate = READ_ONCE(q->config->rate_bps);
 		struct net_device *dev = qdisc_dev(sch);
 		struct cake_sched_data *other_priv;
-		u64 new_rate = 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(new_rate, num_active_qs);
 
 		cake_configure_rates(sch, new_rate, true);
 		q->last_checked_active = now;
@@ -2625,13 +2626,15 @@ static void cake_reconfigure(struct Qdisc *sch)
 	struct cake_sched_data *qd = qdisc_priv(sch);
 	struct cake_sched_config *q = qd->config;
 	u32 buffer_limit;
+	u64 rate_bps;
 
-	cake_configure_rates(sch, qd->config->rate_bps, false);
+	rate_bps = READ_ONCE(qd->config->rate_bps);
+	cake_configure_rates(sch, 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 (rate_bps) {
+		u64 t = rate_bps * q->interval;
 
 		do_div(t, USEC_PER_SEC / 4);
 		buffer_limit = max_t(u32, t, 4U << 20);
@@ -2784,9 +2787,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 +2805,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)
-- 
2.43.0

[PATCH net-next v3 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>
---
v2 -> v3:
- No changes to this patch. Resent as part of the series v3; keeps its v1 Reviewed-by and Acked-by tags.

v1 -> v2:
- No changes to this patch. Resent as part of the series v2.
  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 5223efae5a28..f97240b1ee9c 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -2704,10 +2704,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 v3 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.

The dump path runs without the qdisc lock: tc_fill_qdisc() passes NULL to
gnet_stats_start_copy_compat(), so the reader races with the enqueue-side
update.  READ_ONCE() is not an atomic 64-bit load on 32-bit CPUs, so a
reader can observe a torn high-water mark and report a bogus MEMORY_USED.
Publish the counter as u64_stats_t under the qdisc lock, which the enqueue
path already holds, and take a seqcount-protected snapshot in the dump.
The counter stays u64; only the exported value is bounded.

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)).

A TUN device is needed so the root qdisc can park packets instead of
dropping them.  Open /dev/net/tun, request TUNSETIFF with
IFF_TUN | IFF_NO_PI | IFF_BACKPRESSURE and keep the fd open without
reading it (run it in the background and stop it at teardown): with
IFF_BACKPRESSURE the full tx ring calls netif_tx_stop_queue() rather than
dropping, so the queue behind the qdisc can grow past 4 GiB of resident
truesize.  A plain 'ip tuntap add' attaches no fd and does not request
IFF_BACKPRESSURE, so it cannot park the queue this way.

  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

  # Then push more than 4 GiB of resident truesize through the qdisc: six
  # UDP senders using large SO_SNDBUFFORCE buffers send 1400-byte
  # datagrams (the reported OOM witness used 6 workers for 100 s on a
  # 12 GiB guest).  On the unfixed kernel buffer_used wraps, the admission
  # test passes, the drop loop is bypassed and the guest runs out of
  # memory; with the fix the counter passes the limit without wrapping and
  # the excess is dropped.

  # The ACK-filter over-count is a separate corner: it needs ack-filter
  # enabled and a queued pure ACK whose truesize is larger than the
  # replacement ACK.  Use a fresh interface (or 'tc qdisc change') rather
  # than a second root 'tc qdisc add' on an interface that already has a
  # qdisc:
  tc qdisc change dev tun0 root cake ack-filter

CONFIG_NET_SCH_CAKE=y and CONFIG_TUN=y are required, and the
memory-residency recipe needs a root-run sender envelope with enough
guest RAM to hold the queued data.

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

v2 -> v3:
- Make the widened buffer_max_used reader coherent on 32-bit: publish the
  high-water mark as u64_stats_t under the qdisc lock (the enqueue path
  already holds it) and snapshot it in cake_dump_stats() with
  u64_stats_fetch_begin()/u64_stats_fetch_retry().  cake_dump_stats() runs
  without the qdisc lock: tc_fill_qdisc() passes NULL to
  gnet_stats_start_copy_compat(), so the former lockless READ_ONCE() of the
  u64 raced with the enqueue-side update.  READ_ONCE() is not an atomic
  64-bit load on 32-bit CPUs, so MEMORY_USED could report a torn high-water
  mark (Sashiko gemini + nipa v2 review, Low).  No behavioural change on
  64-bit; the U32_MAX clamp of the exported stat is retained.

v1 -> v2:
- Clamp the MEMORY_USED export to U32_MAX: cake_dump_stats() still exported
  the widened buffer_max_used through PUT_STAT_U32(), so once the monotonic
  counter passed 2^32 the u32 stat truncated to peak - 2^32 (Sashiko nipa
  Medium "buffer_max_used export truncation").  The counter itself is
  unaffected; only the reported value is bounded.

 net/sched/sch_cake.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..290bbc77df16 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -63,6 +63,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/reciprocal_div.h>
+#include <linux/u64_stats_sync.h>
 #include <net/netlink.h>
 #include <linux/if_vlan.h>
 #include <net/gso.h>
@@ -234,8 +235,10 @@ struct cake_sched_data {
 	u16		tin_cnt;
 
 	/* resource tracking */
-	u32		buffer_used;
-	u32		buffer_max_used;
+
+	struct u64_stats_sync buffer_max_used_sync;
+	u64_stats_t	buffer_max_used;
+	u64		buffer_used;
 	u32		buffer_limit;
 
 	/* indices for dequeue */
@@ -1850,7 +1853,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);
 
@@ -1940,8 +1944,11 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		cake_inc_dsthost_bulk_flow_count(b, flow, q->config->flow_mode);
 	}
 
-	if (q->buffer_used > q->buffer_max_used)
-		WRITE_ONCE(q->buffer_max_used, q->buffer_used);
+	if (q->buffer_used > u64_stats_read(&q->buffer_max_used)) {
+		u64_stats_update_begin(&q->buffer_max_used_sync);
+		u64_stats_set(&q->buffer_max_used, q->buffer_used);
+		u64_stats_update_end(&q->buffer_max_used_sync);
+	}
 
 	if (q->buffer_used <= q->buffer_limit)
 		return NET_XMIT_SUCCESS;
@@ -2845,6 +2852,7 @@ static int cake_init(struct Qdisc *sch, struct nlattr *opt,
 	qd->cur_flow  = 0;
 	qd->config = q;
 
+	u64_stats_init(&qd->buffer_max_used_sync);
 	qdisc_watchdog_init(&qd->watchdog, sch);
 
 	if (opt) {
@@ -2995,6 +3003,8 @@ static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 	struct nlattr *stats = nla_nest_start_noflag(d->skb, TCA_STATS_APP);
 	struct cake_sched_data *q = qdisc_priv(sch);
 	struct nlattr *tstats, *ts;
+	u64 buffer_max_used;
+	unsigned int start;
 	int i;
 
 	if (!stats)
@@ -3010,9 +3020,14 @@ static int cake_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 			goto nla_put_failure;			       \
 	} while (0)
 
+	do {
+		start = u64_stats_fetch_begin(&q->buffer_max_used_sync);
+		buffer_max_used = u64_stats_read(&q->buffer_max_used);
+	} while (u64_stats_fetch_retry(&q->buffer_max_used_sync, start));
+
 	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, 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));
-- 
2.43.0

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

Link: https://lore.kernel.org/netdev/87v77csd5q.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>
---
v2 -> v3:
 - No changes to this patch. Re-sent as part of the series v3; keeps its Reviewed-by and Acked-by tags.

v1 -> v2:
 - No changes to this patch. Re-sent as part of the
   series v2; it keeps Toke's v1 Acked-by, carried forward.

 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 v3 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.
>
> 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.
>
> The dump path runs without the qdisc lock: tc_fill_qdisc() passes NULL to
> gnet_stats_start_copy_compat(), so the reader races with the enqueue-side
> update.  READ_ONCE() is not an atomic 64-bit load on 32-bit CPUs, so a
> reader can observe a torn high-water mark and report a bogus MEMORY_USED.
> Publish the counter as u64_stats_t under the qdisc lock, which the enqueue
> path already holds, and take a seqcount-protected snapshot in the dump.
> The counter stays u64; only the exported value is bounded.
>
> 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)).
>
> A TUN device is needed so the root qdisc can park packets instead of
> dropping them.  Open /dev/net/tun, request TUNSETIFF with
> IFF_TUN | IFF_NO_PI | IFF_BACKPRESSURE and keep the fd open without
> reading it (run it in the background and stop it at teardown): with
> IFF_BACKPRESSURE the full tx ring calls netif_tx_stop_queue() rather than
> dropping, so the queue behind the qdisc can grow past 4 GiB of resident
> truesize.  A plain 'ip tuntap add' attaches no fd and does not request
> IFF_BACKPRESSURE, so it cannot park the queue this way.
>
>   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
>
>   # Then push more than 4 GiB of resident truesize through the qdisc: six
>   # UDP senders using large SO_SNDBUFFORCE buffers send 1400-byte
>   # datagrams (the reported OOM witness used 6 workers for 100 s on a
>   # 12 GiB guest).  On the unfixed kernel buffer_used wraps, the admission
>   # test passes, the drop loop is bypassed and the guest runs out of
>   # memory; with the fix the counter passes the limit without wrapping and
>   # the excess is dropped.
>
>   # The ACK-filter over-count is a separate corner: it needs ack-filter
>   # enabled and a queued pure ACK whose truesize is larger than the
>   # replacement ACK.  Use a fresh interface (or 'tc qdisc change') rather
>   # than a second root 'tc qdisc add' on an interface that already has a
>   # qdisc:
>   tc qdisc change dev tun0 root cake ack-filter
>
> CONFIG_NET_SCH_CAKE=y and CONFIG_TUN=y are required, and the
> memory-residency recipe needs a root-run sender envelope with enough
> guest RAM to hold the queued data.
>
> 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 v3 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath

[Toke Høiland-Jørgensen]

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
> are enough for the accesses that run outside the lock.  The lockless
> paths read the rate once into a local and reuse that value, so a single
> coherent snapshot is used per operation.
>
> The annotations do not make a 64-bit access single-copy atomic on 32-bit:
> READ_ONCE()/WRITE_ONCE() are still two 32-bit accesses there.  The lockless
> readers -- cake_config_dump() and the cake_mq children, which observe the
> shared config under a different child's lock -- can therefore still see a
> torn rate on 32-bit.  That rate is an advisory shaper value and is accepted
> as best-effort; the state that must be consistent (the configured rate and
> the autorate flag) is written under the qdisc lock.
>
> Conditions to recreate the bug:
> The lost update (a) needs no sanitizer. Put cake in autorate-ingress on a
> device, drive bursty traffic with gaps so the 250 ms autorate reconfiguration
> fires, and concurrently loop 'tc qdisc change ... cake bandwidth <N>kbit'
> without autorate-ingress (specifying a bandwidth clears the flag).
> If the autorate store lands after the change wrote its rate but before the
> change reconfigures, the qdisc ends up with autorate disabled and the estimate
> installed instead of <N>; reading the rate back then shows the estimate, which
> an unfixed kernel can do and a fixed one cannot.
> (b) The unsynchronized access is reported by KCSAN: build with CONFIG_KCSAN=y
> and run the same trigger with the change loop keeping autorate-ingress set;
> KCSAN reports a data race in cake_config_change / cake_enqueue.
> The non-atomic 64-bit store only shows up as tearing on a 32-bit machine;
> on 64-bit the store is atomic and KCSAN flags the access discipline instead.
>
> 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/875wzctsky.fsf@toke.dk/
> Link: https://lore.kernel.org/netdev/87ld7t3kl3.fsf@all.your.base.are.belong.to.us/
> 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>