From nobody Sat Sep 26 08:37:17 2026 Received: from outboundhk.mxmail.xiaomi.com (outboundhk.mxmail.xiaomi.com [118.143.206.90]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 9B57E3F1ABE for ; Thu, 3 Sep 2026 06:33:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=118.143.206.90 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788417214; cv=none; b=TUwABsak8GtCCaLK78D2LUf8ujbnG9Wv6+aqjnD7FM+bt/T1UUJ3fAYSqTuZ8OCxWvpK4U8SppyBUBc8bZAny8Cltf0yVdrphoANKNE54hmIORGCzNgz7utd5CWJvi/4xk/8C3usLjsx34xdytidsh0+j3fqpuCgyLr6BhEonhY= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788417214; c=relaxed/simple; bh=Bg8Pyj40Qp7XNBb8ZsG+4JuPP0uWQJjSIpYmxH8ewEs=; h=From:To:CC:Subject:Date:Message-ID:References:In-Reply-To: Content-Type:MIME-Version; b=fZSuRIBN0Ef8fE1bf9iMbkf5m/2add9dRHB07uFTBLjnI2CjP57bij8acayazYO+N9hvdLgeZDn6AFhCZQauz7DgDmCQtvHsceibUeFkCXM1R/Vmu0nvbmKUwfFyD8N9X3fnopqXC2ToYkWQ5MqjLY3POA7YqYVAdlF6dd7UzrA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=xiaomi.com; spf=pass smtp.mailfrom=xiaomi.com; arc=none smtp.client-ip=118.143.206.90 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=xiaomi.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=xiaomi.com X-CSE-ConnectionGUID: Or8IsnrbT3iAGP4hNK/UQA== X-CSE-MsgGUID: KbHS2GKTQ7i6p51oxEoYnw== X-IronPort-AV: E=Sophos;i="6.25,258,1779120000"; d="scan'208";a="161397705" From: =?utf-8?B?5YiY5pyv6auY?= To: Thomas Gleixner , =?utf-8?B?5pyx5oG65Lm+?= , Daniel Lezcano , =?utf-8?B?J+W8oOWYieS8nyc=?= CC: "linux-kernel@vger.kernel.org" , =?utf-8?B?546L6Z+s?= , =?utf-8?B?54aK5Lqu?= , "isaacmanjarres@google.com" , Frederic Weisbecker , Anna-Maria Behnsen , =?utf-8?B?5qKB5Lyf6bmP?= , =?utf-8?B?57+B6YeR6aOe?= Subject: =?utf-8?B?5Zue5aSNOiBbRXh0ZXJuYWwgTWFpbF1bUEFUQ0hdIHRpY2svYnJvYWRjYXN0?= =?utf-8?Q?:_Plug_clockevents_replacement_race?= Thread-Topic: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race Thread-Index: AQHa7MK0Vqp0kbQNSkKwEnDO0Z5mtLa3v96ggAlCo9A= Date: Thu, 3 Sep 2026 06:33:21 +0000 Message-ID: References: <042520850d394f0bb0004a226db63d0d@xiaomi.com> <87o77m1v9r.ffs@tglx> <835d5847-1aa0-4852-89c7-6a6996b3eb65@linaro.org> <87o775uh0y.ffs@tglx> <87frrs8lsg.ffs@tglx> <87cymdsu0r.ffs@tglx> <6eb4fac303f4457f9686510664e79bff@xiaomi.com> In-Reply-To: <6eb4fac303f4457f9686510664e79bff@xiaomi.com> Accept-Language: zh-CN, en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Hi, We have verified this patch on our platform (kernel v6.18 / Android 17) and= can confirm it resolves the race condition issue. The problem is that tick_install_broadcast_device() calls clockevents_exchange_device() to move= the old device to the released list (setting it to DETACHED state), then u= pdates tick_broadcast_device.evtdev to the new device =E2=80=94 but this se= quence is not protected by tick_broadcast_lock. The cpuidle/hrtimer paths c= an observe the old device in DETACHED state during this window, leading to = unexpected behavior. After applying the patch, the issue no longer reproduces under our concurre= nt CPU idle + clockevent device registration testing. Could you please share the expected timeline for merging this fix into the = mainline kernel? We have downstream products depending on this fix and woul= d appreciate it if the merge could be expedited. Test environment: - Kernel: 6.18 - Platform: Android 17 - Test: Concurrent CPU idle + clockevent device registration Thanks, liushugao Reported-by: =E6=9C=B1=E6=81=BA=E4=B9=BE Reviewed-by: Bradley Morgan Tested-by: =E5=88=98=E6=9C=AF=E9=AB=98 -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- =E5=8F=91=E4=BB=B6=E4=BA=BA: =E5=88=98=E6=9C=AF=E9=AB=98=20 =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2026=E5=B9=B48=E6=9C=8828=E6=97=A5 17= :09 =E6=94=B6=E4=BB=B6=E4=BA=BA: 'Thomas Gleixner' ; =E6=9C= =B1=E6=81=BA=E4=B9=BE ; Daniel Lezcano ; =E5=BC=A0=E5=98=89=E4=BC=9F =E6=8A=84=E9=80=81: linux-kernel@vger.kernel.org; =E7=8E=8B=E9=9F=AC ; =E7=86=8A=E4=BA=AE ; isaacmanjarres@= google.com; Frederic Weisbecker ; Anna-Maria Behnsen <= anna-maria@linutronix.de>; =E6=A2=81=E4=BC=9F=E9=B9=8F ; =E7=BF=81=E9=87=91=E9=A3=9E =E4=B8=BB=E9=A2=98: =E5=9B=9E=E5=A4=8D: [External Mail][PATCH] tick/broadca= st: Plug clockevents replacement race Hi, We have verified this patch on our platform (kernel v6.18 / Android 17) and= can confirm it resolves the race condition issue. The problem is that tick_install_broadcast_device() calls clockevents_exchange_device() to move= the old device to the released list (setting it to DETACHED state), then u= pdates tick_broadcast_device.evtdev to the new device =E2=80=94 but this se= quence is not protected by tick_broadcast_lock. The cpuidle/hrtimer paths c= an observe the old device in DETACHED state during this window, leading to = unexpected behavior. After applying the patch, the issue no longer reproduces under our concurre= nt CPU idle + clockevent device registration testing. Could you please share the expected timeline for merging this fix into the = mainline kernel? We have downstream products depending on this fix and woul= d appreciate it if the merge could be expedited. Test environment: - Kernel: 6.18 - Platform: Android 17 - Test: Concurrent CPU idle + clockevent device registration Thanks, liushugao -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- =E5=8F=91=E4=BB=B6=E4=BA=BA: Thomas Gleixner =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2024=E5=B9=B48=E6=9C=8812=E6=97=A5 22= :20 =E6=94=B6=E4=BB=B6=E4=BA=BA: =E6=9C=B1=E6=81=BA=E4=B9=BE ; Daniel Lezcano ; =E5=BC=A0=E5=98=89=E4=BC= =9F =E6=8A=84=E9=80=81: linux-kernel@vger.kernel.org; =E7=8E=8B=E9=9F=AC ; =E7=86=8A=E4=BA=AE ; isaacmanjarres@= google.com; Frederic Weisbecker ; Anna-Maria Behnsen <= anna-maria@linutronix.de>; =E6=A2=81=E4=BC=9F=E9=B9=8F ; =E7=BF=81=E9=87=91=E9=A3=9E =E4=B8=BB=E9=A2=98: [External Mail][PATCH] tick/broadcast: Plug clockevents= replacement race [=E5=A4=96=E9=83=A8=E9=82=AE=E4=BB=B6] =E6=AD=A4=E9=82=AE=E4=BB=B6=E6=9D=A5= =E6=BA=90=E4=BA=8E=E5=B0=8F=E7=B1=B3=E5=85=AC=E5=8F=B8=E5=A4=96=E9=83=A8=EF= =BC=8C=E8=AF=B7=E8=B0=A8=E6=85=8E=E5=A4=84=E7=90=86=E3=80=82=E8=8B=A5=E5=AF= =B9=E9=82=AE=E4=BB=B6=E5=AE=89=E5=85=A8=E6=80=A7=E5=AD=98=E7=96=91=EF=BC=8C= =E8=AF=B7=E5=B0=86=E9=82=AE=E4=BB=B6=E8=BD=AC=E5=8F=91=E7=BB=99misec@xiaomi= .com=E8=BF=9B=E8=A1=8C=E5=8F=8D=E9=A6=88 =E6=9C=B1=E6=81=BA=E4=B9=BE reported and decoded the following race conditi= on when a broadcast device is replaced: CPUA CPUB __tick_broadcast_oneshot_control() bc =3D tick_broadcast_device.evtdev; tick_install_broadcast_device(dev) clockevents_exchange_device(cur, de= v) shutdown(cur); detach(cur); cur->handler =3D noop; tick_broadcast_device.evtdev =3D= dev; tick_broadcast_set_event(bc, next_event); <- FAIL: arms a detached device. If the original broadcast device has a restricted interrupt affinity mask a= nd the last CPU in that mask goes offline then the BUG() in tick_cleanup_dead_cpu() triggers because the clockevent device is not in de= tached state. The reason for this is that tick_install_broadcast_device() is not serializ= ed vs. tick broadcast operations. The obvious cure is to serialize tick_install_broadcast_device() with tick_= broadcast_lock against a concurrent tick broadcast operation. That requires to split clockevents_exchange_device() into two parts, one wh= ich does the exchange, shutdown and detach operation and the other which dr= ops the module reference count. This is required because the module referen= ce cannot be dropped while holding tick_broadcast_lock. Let clockevents_exchange_device() do both operations as before, but let the= broadcast device code take the two step approach and do the device exchang= e under tick_broadcast_lock and drop the module reference count after relea= sing it. Fixes: f8381cba04ba ("[PATCH] tick-management: broadcast functionality") Reported-by: =E6=9C=B1=E6=81=BA=E4=B9=BE Signed-off-by: Thomas Gleixner --- kernel/time/clockevents.c | 33 ++++++++++++++++++++------------- kernel/time/tick-broadcast.c | 36 ++++++++++++++++++++++-------------- kernel/time/tick-internal.h | 2 ++ 3 files changed, 44 insertions(+), 27 deletions(-) --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -557,34 +557,41 @@ void clockevents_handle_noop(struct cloc { } -/** - * clockevents_exchange_device - release and request clock devices - * @old: device to release (can be NULL) - * @new: device to request (can be NULL) - * - * Called from various tick functions with clockevents_lock held and - * interrupts disabled. - */ -void clockevents_exchange_device(struct clock_event_device *old, - struct clock_event_device *new) +void __clockevents_exchange_device(struct clock_event_device *old, + struct clock_event_device *new) { /* * Caller releases a clock event device. We queue it into the * released list and do a notify add later. */ if (old) { - module_put(old->owner); clockevents_switch_state(old, CLOCK_EVT_STATE_DETACHED); list_move(&old->list, &clockevents_released); } if (new) { - BUG_ON(!clockevent_state_detached(new)); + WARN_ON(!clockevent_state_detached(new)); clockevents_shutdown(new); } } /** + * clockevents_exchange_device - release and request clock devices + * @old: device to release (can be NULL) + * @new: device to request (can be NULL) + * + * Called from various tick functions with clockevents_lock held and + * interrupts disabled. + */ +void clockevents_exchange_device(struct clock_event_device *old, + struct clock_event_device *new) { + __clockevents_exchange_device(old, new); + if (old) + module_put(old->owner); +} + +/** * clockevents_suspend - suspend clock devices */ void clockevents_suspend(void) @@ -650,7 +657,7 @@ void tick_cleanup_dead_cpu(int cpu) if (cpumask_test_cpu(cpu, dev->cpumask) && cpumask_weight(dev->cpumask) =3D=3D 1 && !tick_is_broadcast_device(dev)) { - BUG_ON(!clockevent_state_detached(dev)); + WARN_ON(!clockevent_state_detached(dev)); list_del(&dev->list); } } --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -162,23 +162,31 @@ static bool tick_set_oneshot_wakeup_devi */ void tick_install_broadcast_device(struct clock_event_device *dev, int cpu= ) { - struct clock_event_device *cur =3D tick_broadcast_device.evtdev; + struct clock_event_device *cur; - if (tick_set_oneshot_wakeup_device(dev, cpu)) - return; + scoped_guard(raw_spinlock_irqsave, &tick_broadcast_lock) { - if (!tick_check_broadcast_device(cur, dev)) - return; + if (tick_set_oneshot_wakeup_device(dev, cpu)) + return; - if (!try_module_get(dev->owner)) - return; + cur =3D tick_broadcast_device.evtdev; + if (!tick_check_broadcast_device(cur, dev)) + return; - clockevents_exchange_device(cur, dev); + if (!try_module_get(dev->owner)) + return; + + __clockevents_exchange_device(cur, dev); + if (cur) + cur->event_handler =3D clockevents_handle_noop; + WRITE_ONCE(tick_broadcast_device.evtdev, dev); + if (!cpumask_empty(tick_broadcast_mask)) + tick_broadcast_start_periodic(dev); + } + + /* Module release must be outside of the lock */ if (cur) - cur->event_handler =3D clockevents_handle_noop; - tick_broadcast_device.evtdev =3D dev; - if (!cpumask_empty(tick_broadcast_mask)) - tick_broadcast_start_periodic(dev); + module_put(cur->owner); if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT)) return; @@ -1209,7 +1217,7 @@ int tick_broadcast_oneshot_active(void) */ bool tick_broadcast_oneshot_available(void) { - struct clock_event_device *bc =3D tick_broadcast_device.evtdev; + struct clock_event_device *bc =3D=20 + READ_ONCE(tick_broadcast_device.evtdev); return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false; } @@ -1= 217,7 +1225,7 @@ bool tick_broadcast_oneshot_available(vo #else int __tick_broadcast_oneshot_control(enum tick_broadcast_state state) { - struct clock_event_device *bc =3D tick_broadcast_device.evtdev; + struct clock_event_device *bc =3D=20 + READ_ONCE(tick_broadcast_device.evtdev); if (!bc || (bc->features & CLOCK_EVT_FEAT_HRTIMER)) return -EBUSY; --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -53,6 +53,8 @@ static inline void clockevent_set_state( } extern void clockevents_shutdown(struct clock_event_device *dev); +extern void __clockevents_exchange_device(struct clock_event_device *old, + struct clock_event_device=20 +*new); extern void clockevents_exchange_device(struct clock_event_device *old, struct clock_event_device *new); e= xtern void clockevents_switch_state(struct clock_event_device *dev,