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

["Björn Töpel" <bjorn@kernel.org>]

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