From nobody Sat Sep 26 23:51:23 2026 Received: from outboundhk.mxmail.xiaomi.com (outboundhk.mxmail.xiaomi.com [207.226.244.123]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 2A2B52DCC13 for ; Fri, 28 Aug 2026 09:10:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=207.226.244.123 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787908227; cv=none; b=QoOLUDMazUdKXodzSHu9q+3gCj08scmYZYj7NQaXdBVJIAmWW5JjXwWLIU9CO8sWX4tlKFdEE1iIbtat3cwEg3a15VEpxpWbo2JILCGcLclvDngnDZBKa27r+s4OdVbi3wjACOzdOsM0lZ7IJi7YkQIIwPiYjyM7WgW8S4kZpAc= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787908227; c=relaxed/simple; bh=EsusHY5ARJBCusxK9C2SnL8H2xMAUfEH91AMSnyM0Ic=; h=From:To:CC:Subject:Date:Message-ID:References:In-Reply-To: Content-Type:MIME-Version; b=m5qSIs32zsoGGeHrOewgQWJ5z6vhflidgDE7z+JcQiS3XT6epGJMaHzGfPPEE1o8DHfylC2kF3Q7rRDizNMLaHV0I5PJf6gufBDl8kMMQ+ApV9oo9wo3kY0H9SFAyBoECJnGw5vBYb8nahXzFYDplylhw9xNDPej82fbaemf4LE= 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=207.226.244.123 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: b5MUztG/SpiJQGbaat2Y+A== X-CSE-MsgGUID: f/DQpWVgQJGw2fEJD5hMww== X-IronPort-AV: E=Sophos;i="6.25,248,1779120000"; d="scan'208";a="187088479" From: =?utf-8?B?5YiY5pyv6auY?= To: Thomas Gleixner , =?utf-8?B?5pyx5oG65Lm+?= , Daniel Lezcano , =?utf-8?B?5byg5ZiJ5Lyf?= 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: AQHa7MK0Vqp0kbQNSkKwEnDO0Z5mtLa3v96g Date: Fri, 28 Aug 2026 09:08:53 +0000 Message-ID: <6eb4fac303f4457f9686510664e79bff@xiaomi.com> References: <042520850d394f0bb0004a226db63d0d@xiaomi.com> <87o77m1v9r.ffs@tglx> <835d5847-1aa0-4852-89c7-6a6996b3eb65@linaro.org> <87o775uh0y.ffs@tglx> <87frrs8lsg.ffs@tglx> <87cymdsu0r.ffs@tglx> In-Reply-To: <87cymdsu0r.ffs@tglx> 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 updates tick_broadcast_device.evtdev to the new device =E2=80=94 but t= his sequence is not protected by tick_broadcast_lock. The cpuidle/hrtimer paths can 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 concurrent 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 would 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 Tested-by: .... -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- =E5=8F=91=E4=BB=B6=E4=BA=BA: Thomas Gleixner =20 =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,