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

[Jamal Hadi Salim <jhs@mojatatu.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.

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