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

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