回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race
From: 刘术高
Date: Fri Aug 28 2026 - 05:10:49 EST
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 — but this
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
-----邮件原件-----
发件人: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
发送时间: 2024年8月12日 22:20
收件人: 朱恺乾 <zhukaiqian@xxxxxxxxxx>; Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>; 张嘉伟 <zhangjiawei8@xxxxxxxxxx>
抄送: linux-kernel@xxxxxxxxxxxxxxx; 王韬 <lingyue@xxxxxxxxxx>; 熊亮 <xiongliang@xxxxxxxxxx>; isaacmanjarres@xxxxxxxxxx; Frederic Weisbecker <frederic@xxxxxxxxxx>; Anna-Maria Behnsen <anna-maria@xxxxxxxxxxxxx>; 梁伟鹏 <weipengliang@xxxxxxxxxx>; 翁金飞 <wengjinfei@xxxxxxxxxx>
主题: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race
[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xxxxxxxxxx进行反馈
朱恺乾 reported and decoded the following race condition when a broadcast device is replaced:
CPUA CPUB
__tick_broadcast_oneshot_control()
bc = tick_broadcast_device.evtdev;
tick_install_broadcast_device(dev)
clockevents_exchange_device(cur, dev)
shutdown(cur);
detach(cur);
cur->handler = noop;
tick_broadcast_device.evtdev = dev;
tick_broadcast_set_event(bc, next_event); <- FAIL: arms a detached device.
If the original broadcast device has a restricted interrupt affinity mask and the last CPU in that mask goes offline then the BUG() in
tick_cleanup_dead_cpu() triggers because the clockevent device is not in detached state.
The reason for this is that tick_install_broadcast_device() is not serialized 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 which does the exchange, shutdown and detach operation and the other which drops the module reference count. This is required because the module reference 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 exchange under tick_broadcast_lock and drop the module reference count after releasing it.
Fixes: f8381cba04ba ("[PATCH] tick-management: broadcast functionality")
Reported-by: 朱恺乾 <zhukaiqian@xxxxxxxxxx>
Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
---
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) == 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 = 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 = 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 = 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 = clockevents_handle_noop;
- tick_broadcast_device.evtdev = 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 = tick_broadcast_device.evtdev;
+ struct clock_event_device *bc =
+ READ_ONCE(tick_broadcast_device.evtdev);
return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false; } @@ -1217,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 = tick_broadcast_device.evtdev;
+ struct clock_event_device *bc =
+ 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
+*new);
extern void clockevents_exchange_device(struct clock_event_device *old,
struct clock_event_device *new); extern void clockevents_switch_state(struct clock_event_device *dev,