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

["Toke Høiland-Jørgensen" <toke@toke.dk>]

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>