From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail.toke.dk; dkim=none; arc=none (Message is not ARC signed); dmarc=fail (Used From Domain Record) header.from=toke.dk policy.dmarc=none From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Jamal Hadi Salim , netdev@vger.kernel.org Cc: Jamal Hadi Salim , =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= , Jiri Pirko , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , cake@lists.bufferbloat.net, Victor Nogueira , Sashiko In-Reply-To: References: Date: Tue, 20 Oct 2026 14:49:57 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87wlrctu2y.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: PNTY4QM46N4VYVEIHALA5HR5FPRLLHHD X-Message-ID-Hash: PNTY4QM46N4VYVEIHALA5HR5FPRLLHHD X-MailFrom: toke@toke.dk X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list Subject: [Cake] Re: [PATCH net-next v3 1/4] net/sched/sch_cake: serialize reconfiguration with the datapath List-Id: Cake - FQ_codel the next generation Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Jamal Hadi Salim 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 =3D 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 lockle= ss > 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 accept= ed > 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 reconfigura= tion > fires, and concurrently loop 'tc qdisc change ... cake bandwidth 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 est= imate > installed instead of ; 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_KCS= AN=3Dy > and run the same trigger with the change loop keeping autorate-ingress se= t; > 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 inste= ad. > > Reported-by: Sashiko (gemini) > Link: https://sashiko.dev/#/patchset/20260816012109.2865223-1-ooonea@gmai= l.com > Link: https://lore.kernel.org/netdev/20260816012109.2865223-1-ooonea@gmai= l.com/ > Link: https://lore.kernel.org/netdev/875wzctsky.fsf@toke.dk/ > Link: https://lore.kernel.org/netdev/87ld7t3kl3.fsf@all.your.base.are.bel= ong.to.us/ > Reviewed-by: Victor Nogueira > Signed-off-by: Jamal Hadi Salim Acked-by: Toke H=C3=B8iland-J=C3=B8rgensen