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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message lists a torn 64-bit store of rate_bps on
  32-bit machines as effect (b) of the bug.
- [Medium] sch_cake: buffer_limit truncation wraps queue capacity at high
  bandwidths
- [Low] The commit fixes a real, user-visible lost update but has no
  Fixes: tag, and it describes itself as a 'follow-up'.
- [Low] Reproducer (a) in the commit message cannot show the bug.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  cake_mq_change() still commits the shared config without serializing…

--

net/sched/sch_cake: serialize reconfiguration with the datapath

This commit takes sch_tree_lock() in cake_change() before calling
cake_config_change(). The netlink configuration commit and
cake_reconfigure() are then serialized with the autorate-ingress rate
update in cake_enqueue(). The commit also annotates rate_bps accesses with
READ_ONCE()/WRITE_ONCE() for readers that run outside the qdisc lock.

Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmail.com
Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmail.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.

[Severity: Low]
Should this have a Fixes: tag?

Before this patch, cake_change() ran cake_config_change() with only RTNL
held. It took sch_tree_lock() only around cake_reconfigure(). The autorate
store in cake_enqueue() runs under the qdisc lock.

That second writer was added by commit 7298de9cd725 ("sch_cake: Add
ingress mode"). Commit bc0ce2bad36c ("net/sched: sch_cake: Factor out
config variables into separate struct") only moved the field into
q->config.

The referenced commit 7cbfb180945c carries "Fixes: 7298de9cd725", but
this patch has no Fixes: trailer. Without one, stable selection cannot
tell which trees need this fix.

Would Fixes: 7298de9cd725 ("sch_cake: Add ingress mode") be appropriate?

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

[Severity: Low]
Can recipe (a) tell a fixed kernel from an unfixed one?

The looped command ends with autorate-ingress. That sends
TCA_CAKE_AUTORATE=1, so cake_config_change() keeps the flag set:

	if (tb[TCA_CAKE_AUTORATE]) {
		if (!!nla_get_u32(tb[TCA_CAKE_AUTORATE])) {
			...
			rate_flags |= CAKE_FLAG_AUTORATE_INGRESS;

While the flag is set, cake_enqueue() is designed to overwrite rate_bps
with (avg_peak_bandwidth * 15) >> 4 once 250ms have passed since
last_reconfig_time. cake_change() never resets last_reconfig_time.

So reading the rate back through cake_config_dump() would show the
estimate both with and without this patch.

The race diagram above needs a change that clears autorate ("clears
autorate in its local rate_flags"). Should the recipe use a change that
turns autorate-ingress off instead?

The same paragraph starts with "build with CONFIG_KCSAN=y" but then says
(a) can be reproduced without a sanitizer. Could that be reworded?

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..a05da73b5f848 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);

[Severity: Medium]
On 32-bit kernels, isn't this WRITE_ONCE() of a u64 still two 32-bit
stores?

The commit message lists the torn 64-bit store of rate_bps on 32-bit as
effect (b), and then says:

  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)

compiletime_assert_rwonce_type() accepts 64-bit types on 32-bit, but the
volatile access is still split into two halves. KCSAN treats accesses up
to sizeof(long long) as atomic. The annotation therefore silences the
report without making the access single-copy atomic.

cake_config_dump() reads rate_bps without the qdisc lock, while this
store runs under that lock:

	if (nla_put_u64_64bit(skb, TCA_CAKE_BASE_RATE64,
			      READ_ONCE(q->rate_bps), TCA_CAKE_PAD))

For cake_mq, cake_mq_change() writes the shared rate_bps through
cake_config_change() with no child lock held:

	ret = cake_config_change(&priv->cake_config, opt, extack, &overhead_changed);

Meanwhile each child reads READ_ONCE(q->config->rate_bps) in
cake_dequeue() under its own root lock. It passes the value straight to
cake_configure_rates(sch, new_rate, true). A torn value there becomes the
shaper rate until the next sync or the later cake_reconfigure().

Can both of these readers still see a torn rate_bps on 32-bit? If so,
should the commit message limit the claim to readers under the same qdisc
lock?

>  				q->last_reconfig_time = now;
>  				cake_reconfigure(sch);
>  			}

[ ... ]

> @@ -2784,9 +2786,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;

[Severity: Medium]
This isn't a bug introduced by this patch, but cake_mq_change() still
commits the shared config the same way cake_change() did before this
patch:

	ret = cake_config_change(&priv->cake_config, opt, extack, &overhead_changed);
	...
		if (qd->tins) {
			sch_tree_lock(chld);
			cake_reconfigure(chld);
			sch_tree_unlock(chld);
		}

cake_config_change() publishes tin_mode, rate_bps, rate_flags, flow_mode
and the other fields with only RTNL held.

Until cake_mq_change() reaches a given child, that child's cake_dequeue()
can still run the is_shared sync block under its own root lock once
sync_time has elapsed:

cake_dequeue()
  cake_configure_rates(sch, new_rate, true)
    cake_config_besteffort()    /* new tin_mode, qd->tin_cnt = 1 */

Because rate_adjust is true, cake_configure_rates() skips the tin purge:

	if (!rate_adjust) {
		...
		for (c = qd->tin_cnt; c < CAKE_MAX_TINS; c++) {
			cake_clear_tin(sch, c);

Take a diffserv8 -> besteffort change. Could packets still queued in tins
1..7 then fall outside the shaped-mode scan? cake_dequeue() would return
NULL at the "No point in going further" check with sch->q.qlen > 0 and no
watchdog scheduled. That would last until cake_reconfigure() runs for
that child and purges those tins.

The child datapaths also read q->config->rate_flags, flow_mode and
tin_mode with plain loads. The matching WRITE_ONCE() stores run under a
different lock, so the children can see a mix of old and new config.

Should cake_mq_change() get similar treatment in a follow-up?

[ ... ]

> @@ -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);
>  
>  	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)) {
> +		u64 t = READ_ONCE(q->rate_bps) * q->interval;
>  
>  		do_div(t, USEC_PER_SEC / 4);
>  		buffer_limit = max_t(u32, t, 4U << 20);

[Severity: Medium]
This isn't a bug introduced by this patch, but this line is being
touched, so: t is computed as a u64 here and then handed to
max_t(u32, t, 4U << 20) two lines below. max_t() casts both operands
to u32 before it compares them. The upper 32 bits of t are therefore
thrown away rather than clamped.

After do_div(t, USEC_PER_SEC / 4), t is rate_bps * interval / 250000.
With the default 100ms rtt that is 0.4 * rate_bps. It passes U32_MAX
once rate_bps exceeds about 10.7e9 bytes/s, roughly 85.9 Gbit/s. A
larger rtt lowers the threshold in proportion; with rtt 1s it is
already reached at about 8.6 Gbit/s.

Above that point buffer_limit becomes t modulo 4GB. Right at the
threshold it drops to the 4MB floor, and it keeps cycling as the rate
grows. The final WRITE_ONCE(qd->buffer_limit, ...) takes the min()
against max(sch->limit * psched_mtu(), buffer_config_limit), which is
around 15MB with the default limit. So a higher configured rate can
end up with a smaller memory limit than a lower one. cake_enqueue()
then starts calling cake_drop() much earlier than the configuration
implies.

buffer_used is already a u64 in cake_sched_data, while buffer_limit
is still a u32. Should t be clamped in u64 before it is narrowed, for
example with min_t(u64, t, U32_MAX) ahead of the max_t()? Or should
buffer_limit be widened to match buffer_used?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-5RTM.v2.r0.20261015051735%40mojatatu.com