Re: Race condition when replacing the broadcast timer

Re: Race condition when replacing the broadcast timer

[Thomas Gleixner]

On Wed, Jun 26 2024 at 02:17, 朱恺乾 wrote:
> We find a possible race condition when replacing the broadcast
> timer. Here is how the race happend,

> 1. In thread 0, ___tick_broadcast_oneshot_control, timer 0 as a
> broadcast timer is updating the next_event.

> 2. In thread 1, tick_install_broadcast_device, timer 0 is going to be
> replaced by a new timer 1.

> 3. If thread 0 gets the broadcast timer first, it would have the old
> timer returned (timer 0). When thread 1 shuts the old timer down and
> marks it as detached, Thread 0 still have the chance to re-enable the
> old timer with a noop handler if it executes slower than thread 1.

> 4. As the old timer is binded to a CPU, when plug out that CPU, kernel
> fails at clockevents.c:653

Clearly tick_install_broadcast_device() lacks serialization.

The untested patch below should cure that.

Thanks,

        tglx
---
 kernel/time/clockevents.c    |   31 +++++++++++++++++++------------
 kernel/time/tick-broadcast.c |   36 ++++++++++++++++++++++--------------
 kernel/time/tick-internal.h  |    2 ++
 3 files changed, 43 insertions(+), 26 deletions(-)

--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -557,23 +557,14 @@ 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);
 	}
@@ -585,6 +576,22 @@ void clockevents_exchange_device(struct
 }
 
 /**
+ * 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)
+{
+	if (old)
+		module_put(old->owner);
+	__clockevents_exchange_device(old, new);
+}
+
+/**
  * 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(old->owner);
 
 	if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
 		return;
@@ -1185,7 +1193,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;
 }
@@ -1193,7 +1201,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,

RE: [External Mail]Re: Race condition when replacing the broadcast timer

[朱恺乾]

Thanks for the fast reply.
May I know when there'll be a formal patch on the mainline?

-----Original Message-----
From: Thomas Gleixner <tglx@linutronix.de>
Sent: Thursday, June 27, 2024 7:27 PM
To: 朱恺乾 <zhukaiqian@xiaomi.com>; Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮 <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen <anna-maria@linutronix.de>
Subject: [External Mail]Re: Race condition when replacing the broadcast timer

[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈

On Wed, Jun 26 2024 at 02:17, 朱恺乾 wrote:
> We find a possible race condition when replacing the broadcast timer.
> Here is how the race happend,

> 1. In thread 0, ___tick_broadcast_oneshot_control, timer 0 as a
> broadcast timer is updating the next_event.

> 2. In thread 1, tick_install_broadcast_device, timer 0 is going to be
> replaced by a new timer 1.

> 3. If thread 0 gets the broadcast timer first, it would have the old
> timer returned (timer 0). When thread 1 shuts the old timer down and
> marks it as detached, Thread 0 still have the chance to re-enable the
> old timer with a noop handler if it executes slower than thread 1.

> 4. As the old timer is binded to a CPU, when plug out that CPU, kernel
> fails at clockevents.c:653

Clearly tick_install_broadcast_device() lacks serialization.

The untested patch below should cure that.

Thanks,

        tglx
---
 kernel/time/clockevents.c    |   31 +++++++++++++++++++------------
 kernel/time/tick-broadcast.c |   36 ++++++++++++++++++++++--------------
 kernel/time/tick-internal.h  |    2 ++
 3 files changed, 43 insertions(+), 26 deletions(-)

--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -557,23 +557,14 @@ 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);
        }
@@ -585,6 +576,22 @@ void clockevents_exchange_device(struct
 }

 /**
+ * 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) {
+       if (old)
+               module_put(old->owner);
+       __clockevents_exchange_device(old, new); }
+
+/**
  * 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(old->owner);

        if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
                return;
@@ -1185,7 +1193,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;  } @@ -1193,7 +1201,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,
#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

Re: [External Mail]Re: Race condition when replacing the broadcast timer

[Daniel Lezcano]

On 28/06/2024 03:59, 朱恺乾 wrote:
> Thanks for the fast reply.
> May I know when there'll be a formal patch on the mainline?

Do you confirm the patch fixes the issue ?


> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, June 27, 2024 7:27 PM
> To: 朱恺乾 <zhukaiqian@xiaomi.com>; Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮 <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen <anna-maria@linutronix.de>
> Subject: [External Mail]Re: Race condition when replacing the broadcast timer
> 
> [外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈
> 
> On Wed, Jun 26 2024 at 02:17, 朱恺乾 wrote:
>> We find a possible race condition when replacing the broadcast timer.
>> Here is how the race happend,
> 
>> 1. In thread 0, ___tick_broadcast_oneshot_control, timer 0 as a
>> broadcast timer is updating the next_event.
> 
>> 2. In thread 1, tick_install_broadcast_device, timer 0 is going to be
>> replaced by a new timer 1.
> 
>> 3. If thread 0 gets the broadcast timer first, it would have the old
>> timer returned (timer 0). When thread 1 shuts the old timer down and
>> marks it as detached, Thread 0 still have the chance to re-enable the
>> old timer with a noop handler if it executes slower than thread 1.
> 
>> 4. As the old timer is binded to a CPU, when plug out that CPU, kernel
>> fails at clockevents.c:653
> 
> Clearly tick_install_broadcast_device() lacks serialization.
> 
> The untested patch below should cure that.
> 
> Thanks,
> 
>          tglx
> ---
>   kernel/time/clockevents.c    |   31 +++++++++++++++++++------------
>   kernel/time/tick-broadcast.c |   36 ++++++++++++++++++++++--------------
>   kernel/time/tick-internal.h  |    2 ++
>   3 files changed, 43 insertions(+), 26 deletions(-)
> 
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -557,23 +557,14 @@ 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);
>          }
> @@ -585,6 +576,22 @@ void clockevents_exchange_device(struct
>   }
> 
>   /**
> + * 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) {
> +       if (old)
> +               module_put(old->owner);
> +       __clockevents_exchange_device(old, new); }
> +
> +/**
>    * 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(old->owner);
> 
>          if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
>                  return;
> @@ -1185,7 +1193,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;  } @@ -1193,7 +1201,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,
> #/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [External Mail]Re: Race condition when replacing the broadcast timer

[朱恺乾]

+Jiawei

Jiawei,
Please update here when you have the test result


-----Original Message-----
From: Daniel Lezcano <daniel.lezcano@linaro.org>
Sent: Friday, June 28, 2024 3:23 PM
To: 朱恺乾 <zhukaiqian@xiaomi.com>; Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮 <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen <anna-maria@linutronix.de>
Subject: Re: [External Mail]Re: Race condition when replacing the broadcast timer

[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈

On 28/06/2024 03:59, 朱恺乾 wrote:
> Thanks for the fast reply.
> May I know when there'll be a formal patch on the mainline?

Do you confirm the patch fixes the issue ?


> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, June 27, 2024 7:27 PM
> To: 朱恺乾 <zhukaiqian@xiaomi.com>; Daniel Lezcano
> <daniel.lezcano@linaro.org>
> Cc: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮
> <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic
> Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen
> <anna-maria@linutronix.de>
> Subject: [External Mail]Re: Race condition when replacing the
> broadcast timer
>
> [外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈
>
> On Wed, Jun 26 2024 at 02:17, 朱恺乾 wrote:
>> We find a possible race condition when replacing the broadcast timer.
>> Here is how the race happend,
>
>> 1. In thread 0, ___tick_broadcast_oneshot_control, timer 0 as a
>> broadcast timer is updating the next_event.
>
>> 2. In thread 1, tick_install_broadcast_device, timer 0 is going to be
>> replaced by a new timer 1.
>
>> 3. If thread 0 gets the broadcast timer first, it would have the old
>> timer returned (timer 0). When thread 1 shuts the old timer down and
>> marks it as detached, Thread 0 still have the chance to re-enable the
>> old timer with a noop handler if it executes slower than thread 1.
>
>> 4. As the old timer is binded to a CPU, when plug out that CPU,
>> kernel fails at clockevents.c:653
>
> Clearly tick_install_broadcast_device() lacks serialization.
>
> The untested patch below should cure that.
>
> Thanks,
>
>          tglx
> ---
>   kernel/time/clockevents.c    |   31 +++++++++++++++++++------------
>   kernel/time/tick-broadcast.c |   36 ++++++++++++++++++++++--------------
>   kernel/time/tick-internal.h  |    2 ++
>   3 files changed, 43 insertions(+), 26 deletions(-)
>
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -557,23 +557,14 @@ 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);
>          }
> @@ -585,6 +576,22 @@ void clockevents_exchange_device(struct
>   }
>
>   /**
> + * 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) {
> +       if (old)
> +               module_put(old->owner);
> +       __clockevents_exchange_device(old, new); }
> +
> +/**
>    * 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(old->owner);
>
>          if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
>                  return;
> @@ -1185,7 +1193,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;  } @@ -1193,7 +1201,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,
> #/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部
> 或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and
> its attachments contain confidential information from XIAOMI, which is
> intended only for the person or entity whose address is listed above.
> Any use of the information contained herein in any way (including, but
> not limited to, total or partial disclosure, reproduction, or
> dissemination) by persons other than the intended recipient(s) is
> prohibited. If you receive this e-mail in error, please notify the
> sender by phone or email immediately and delete it!******/#

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog

#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#

RE: [External Mail]Re: Race condition when replacing the broadcast timer

[Thomas Gleixner]

On Mon, Jul 01 2024 at 02:11, 朱恺乾 wrote:
> Jiawei,
> Please update here when you have the test result

Any update on this?

RE: [External Mail]Re: Race condition when replacing the broadcast timer

[Thomas Gleixner]

On Wed, Jul 10 2024 at 22:30, Thomas Gleixner wrote:
> On Mon, Jul 01 2024 at 02:11, 朱恺乾 wrote:
>> Jiawei,
>> Please update here when you have the test result
>
> Any update on this?

Gentle reminder.

[PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

朱恺乾 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@xiaomi.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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,

Re: [PATCH] tick/broadcast: Plug clockevents replacement race

[Anna-Maria Behnsen]

Thomas Gleixner <tglx@linutronix.de> writes:

> 朱恺乾 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@xiaomi.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  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.

can you please transform the comment into a lockdep annotation?

Thanks

Re: [PATCH] tick/broadcast: Plug clockevents replacement race

[Frederic Weisbecker]

Le Mon, Aug 12, 2024 at 04:19:48PM +0200, Thomas Gleixner a écrit :
> 朱恺乾 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.

The reason why the module reference can not be dropped while holding
tick_broadcast_lock is not obvious though. What can go wrong?

Thanks.

回复: [External Mail][PATCH] tick/broadcast: 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 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@linutronix.de> 
发送时间: 2024年8月12日 22:20
收件人: 朱恺乾 <zhukaiqian@xiaomi.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; 张嘉伟 <zhangjiawei8@xiaomi.com>
抄送: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮 <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen <anna-maria@linutronix.de>; 梁伟鹏 <weipengliang@xiaomi.com>; 翁金飞 <wengjinfei@xiaomi.com>
主题: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈

朱恺乾 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@xiaomi.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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,

回复: [External Mail][PATCH] tick/broadcast: 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 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

-----邮件原件-----
发件人: 刘术高 
发送时间: 2026年8月28日 17:09
收件人: 'Thomas Gleixner' <tglx@linutronix.de>; 朱恺乾 <zhukaiqian@xiaomi.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; 张嘉伟 <zhangjiawei8@xiaomi.com>
抄送: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮 <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen <anna-maria@linutronix.de>; 梁伟鹏 <weipengliang@xiaomi.com>; 翁金飞 <wengjinfei@xiaomi.com>
主题: 回复: [External Mail][PATCH] tick/broadcast: 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 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@linutronix.de>
发送时间: 2024年8月12日 22:20
收件人: 朱恺乾 <zhukaiqian@xiaomi.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; 张嘉伟 <zhangjiawei8@xiaomi.com>
抄送: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮 <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen <anna-maria@linutronix.de>; 梁伟鹏 <weipengliang@xiaomi.com>; 翁金飞 <wengjinfei@xiaomi.com>
主题: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈

朱恺乾 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@xiaomi.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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,

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Bradley Morgan]

Hi, I have a pixel 7, if I could be asked to backport this, then I'll be
happy to, for the fix, add:

Reviewed-by: Bradley Morgan <brads@mainlining.org>

--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

Re: [PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

On Thu, Oct 17 2024 at 18:16, Frederic Weisbecker wrote:
> Le Mon, Aug 12, 2024 at 04:19:48PM +0200, Thomas Gleixner a écrit :
>> 朱恺乾 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.
>
> The reason why the module reference can not be dropped while holding
> tick_broadcast_lock is not obvious though. What can go wrong?

tick_broadcast_lock() is a raw_spinlock and deeply nested ...

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

On Fri, Aug 28 2026 at 09:08, 刘术高 wrote:

Can you please adhere to the kernel mailing list netiquette:

  https://people.kernel.org/tglx/notes-about-netiquette

> 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

and to the documented process in Documentation/process/* which clearly
states that the way to confirm that a bug is fixed is expressed by a

Tested-by: ....

tag, which allows tools to pick it up automatically.

Thanks,

        tglx

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

On Thu, Sep 03 2026 at 06:33, 刘术高 wrote:
> 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.

Just for the record. Here is the timeline:

2024-06-26: Issue reported
2024-08-12: Patch available after some discussion
2026-08-28: Confirmation that the patch works
2026-09-05: Request for time line

It took you more than fricking TWO YEARS to test and validate that patch
and now that you finally came around to take care of it without
following the documented process you expect me to merge it within a
couple of days and preferrably expedite it and provide you the expected
timeline because you have a business case.

IOW, this could have been fixed TWO YEARS ago if you wold have paid
attention.

Seriously?

Thanks,

        tglx

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

On Thu, Sep 03 2026 at 16:12, Bradley Morgan wrote:
> Hi, I have a pixel 7, if I could be asked to backport this, then I'll be
> happy to, for the fix, add:
>
> Reviewed-by: Bradley Morgan <brads@mainlining.org>

It would be helpful to respond to the actual patch and not to some
arbitrary reply. That way you might avoid your tags to be overseen by
tools.

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Bradley Morgan]

On 5 September 2026 22:21:24 BST, Thomas Gleixner <tglx@linutronix.de>
wrote:
>On Thu, Sep 03 2026 at 16:12, Bradley Morgan wrote:
>> Hi, I have a pixel 7, if I could be asked to backport this, then I'll be
>> happy to, for the fix, add:
>>
>> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>
>It would be helpful to respond to the actual patch and not to some
>arbitrary reply. That way you might avoid your tags to be overseen by
>tools.
>
Sorry my mail client sometimes glitches,

+Remove anna-maria, she's on leave, and she sends OOO email.

Thanks,
--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

On Sat, Sep 05 2026 at 22:24, Bradley Morgan wrote:
> On 5 September 2026 22:21:24 BST, Thomas Gleixner <tglx@linutronix.de> wrote:
>>On Thu, Sep 03 2026 at 16:12, Bradley Morgan wrote:
>>> Hi, I have a pixel 7, if I could be asked to backport this, then I'll be
>>> happy to, for the fix, add:
>>>
>>> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>>
>>It would be helpful to respond to the actual patch and not to some
>>arbitrary reply. That way you might avoid your tags to be overseen by
>>tools.
>>
> Sorry my mail client sometimes glitches,

I fail to see how your mail client is responsible for you to hit reply
on the wrong mail.

But what do I know about all these new fangled mail clients who decide
on their own what to do. When this happens to me I'm 100% confident that
_I_ got it wrong and not my mail client. Maybe I'm just too old-school.

Thanks,

        tglx

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

On Sat, Sep 05 2026 at 22:24, Bradley Morgan wrote:

> On 5 September 2026 22:21:24 BST, Thomas Gleixner <tglx@linutronix.de>
> wrote:
>>On Thu, Sep 03 2026 at 16:12, Bradley Morgan wrote:
>>> Hi, I have a pixel 7, if I could be asked to backport this, then I'll be
>>> happy to, for the fix, add:
>>>
>>> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>>
>>It would be helpful to respond to the actual patch and not to some
>>arbitrary reply. That way you might avoid your tags to be overseen by
>>tools.
>>
> Sorry my mail client sometimes glitches,
>
> +Remove anna-maria, she's on leave, and she sends OOO email.

You missed to change Daniel's address which bounces...

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Bradley Morgan]

On 5 September 2026 23:00:05 BST, Thomas Gleixner <tglx@linutronix.de>
wrote:
>On Sat, Sep 05 2026 at 22:24, Bradley Morgan wrote:
>> On 5 September 2026 22:21:24 BST, Thomas Gleixner <tglx@linutronix.de>
>wrote:
>>>On Thu, Sep 03 2026 at 16:12, Bradley Morgan wrote:
>>>> Hi, I have a pixel 7, if I could be asked to backport this, then I'll
>be
>>>> happy to, for the fix, add:
>>>>
>>>> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>>>
>>>It would be helpful to respond to the actual patch and not to some
>>>arbitrary reply. That way you might avoid your tags to be overseen by
>>>tools.
>>>
>> Sorry my mail client sometimes glitches,
>
>I fail to see how your mail client is responsible for you to hit reply
>on the wrong mail.
>
>But what do I know about all these new fangled mail clients who decide
>on their own what to do. When this happens to me I'm 100% confident that
>_I_ got it wrong and not my mail client. Maybe I'm just too old-school.
>


Remove Daniel too. Bounces.


My bad. 

>Thanks,
>
>        tglx
>

--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Thomas Gleixner]

On Sat, Sep 05 2026 at 23:06, Bradley Morgan wrote:
> On 5 September 2026 23:00:05 BST, Thomas Gleixner <tglx@linutronix.de>
> wrote:
>>On Sat, Sep 05 2026 at 22:24, Bradley Morgan wrote:
>>> On 5 September 2026 22:21:24 BST, Thomas Gleixner <tglx@linutronix.de>
>>wrote:
>>>>On Thu, Sep 03 2026 at 16:12, Bradley Morgan wrote:
>>>>> Hi, I have a pixel 7, if I could be asked to backport this, then I'll
>>be
>>>>> happy to, for the fix, add:
>>>>>
>>>>> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>>>>
>>>>It would be helpful to respond to the actual patch and not to some
>>>>arbitrary reply. That way you might avoid your tags to be overseen by
>>>>tools.
>>>>
>>> Sorry my mail client sometimes glitches,
>>
>>I fail to see how your mail client is responsible for you to hit reply
>>on the wrong mail.
>>
>>But what do I know about all these new fangled mail clients who decide
>>on their own what to do. When this happens to me I'm 100% confident that
>>_I_ got it wrong and not my mail client. Maybe I'm just too old-school.
>>
> Remove Daniel too. Bounces.

Sorry no. That's not how this works

 #1 An OOO response is purely informational and does not justify to
    remove the recipient from the CC list just because

 #2 A bouncing email address is of course a valid reason to remove
    someone from CC, but it is a matter of courtesy in the context of
    the Linux kernel lists to lookup whether the person vansished from
    the planet or just changed the mail address. That's what the
    MAINTAINERS file is for.

    The thread you responded to was from TWO years ago...

Aside of that you did not respond to my actual complaint about
responding to a random reply instead of responding to the actual patch.

If you fail to review what the AI agent of your choice hallucinates
then so be it, but don't be surprised that I'm going to put you on my
ignore list as a consequence and recommend to all other maintainers to
do so as well.

   Thomas

Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[Bradley Morgan]

On 6 September 2026 00:15:28 BST, Thomas Gleixner <tglx@linutronix.de>
wrote:
>On Sat, Sep 05 2026 at 23:06, Bradley Morgan wrote:
>> On 5 September 2026 23:00:05 BST, Thomas Gleixner <tglx@linutronix.de>
>> wrote:
>>>On Sat, Sep 05 2026 at 22:24, Bradley Morgan wrote:
>>>> On 5 September 2026 22:21:24 BST, Thomas Gleixner <tglx@linutronix.de>
>>>wrote:
>>>>>On Thu, Sep 03 2026 at 16:12, Bradley Morgan wrote:
>>>>>> Hi, I have a pixel 7, if I could be asked to backport this, then
>I'll
>>>be
>>>>>> happy to, for the fix, add:
>>>>>>
>>>>>> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>>>>>
>>>>>It would be helpful to respond to the actual patch and not to some
>>>>>arbitrary reply. That way you might avoid your tags to be overseen by
>>>>>tools.
>>>>>
>>>> Sorry my mail client sometimes glitches,
>>>
>>>I fail to see how your mail client is responsible for you to hit reply
>>>on the wrong mail.
>>>
>>>But what do I know about all these new fangled mail clients who decide
>>>on their own what to do. When this happens to me I'm 100% confident that
>>>_I_ got it wrong and not my mail client. Maybe I'm just too old-school.
>>>
>> Remove Daniel too. Bounces.
>
>Sorry no. That's not how this works
>
> #1 An OOO response is purely informational and does not justify to
>    remove the recipient from the CC list just because
>
> #2 A bouncing email address is of course a valid reason to remove
>    someone from CC, but it is a matter of courtesy in the context of
>    the Linux kernel lists to lookup whether the person vansished from
>    the planet or just changed the mail address. That's what the
>    MAINTAINERS file is for.
>
>    The thread you responded to was from TWO years ago...
>
>Aside of that you did not respond to my actual complaint about
>responding to a random reply instead of responding to the actual patch.
>
>If you fail to review what the AI agent of your choice hallucinates
>then so be it, but don't be surprised that I'm going to put you on my
>ignore list as a consequence and recommend to all other maintainers to
>do so as well.
>
>   Thomas
>
>
>
oh crap, I see it now. I thought it was a recent thread cause of my lore
search!


Probably:

https://lore.kernel.org/all/87qzj74dxi.ffs@fw13/ ?
--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

回复: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[刘术高]

On Thu, Sep 03 2026 at 06:33, Thomas Gleixner wrote:
> It took you more than fricking TWO YEARS to test and validate that
> patch and now that you finally came around to take care of it without
> following the documented process you expect me to merge it within a
> couple of days and preferrably expedite it and provide you the
> expected timeline because you have a business case.

You are right. The delay is entirely on our side, and I should not
have requested expediting or a timeline. I apologize for that.

Tested-by: 刘术高 <liushugao@xiaomi.com>

I will make sure to follow the documented process and respond more
promptly in the future.

Thanks,

	刘术高

-----邮件原件-----
发件人: Thomas Gleixner <tglx@linutronix.de> 
发送时间: 2026年9月6日 5:19
收件人: 刘术高 <liushugao@xiaomi.com>; 朱恺乾 <zhukaiqian@xiaomi.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; '张嘉伟' <zhangjiawei8@xiaomi.com>
抄送: linux-kernel@vger.kernel.org; 王韬 <lingyue@xiaomi.com>; 熊亮 <xiongliang@xiaomi.com>; isaacmanjarres@google.com; Frederic Weisbecker <frederic@kernel.org>; Anna-Maria Behnsen <anna-maria@linutronix.de>; 梁伟鹏 <weipengliang@xiaomi.com>; 翁金飞 <wengjinfei@xiaomi.com>
主题: Re: 回复: [External Mail][PATCH] tick/broadcast: Plug clockevents replacement race

[外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈

On Thu, Sep 03 2026 at 06:33, 刘术高 wrote:
> 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.

Just for the record. Here is the timeline:

2024-06-26: Issue reported
2024-08-12: Patch available after some discussion
2026-08-28: Confirmation that the patch works
2026-09-05: Request for time line

It took you more than fricking TWO YEARS to test and validate that patch and now that you finally came around to take care of it without following the documented process you expect me to merge it within a couple of days and preferrably expedite it and provide you the expected timeline because you have a business case.

IOW, this could have been fixed TWO YEARS ago if you wold have paid attention.

Seriously?

Thanks,

        tglx

[tip: timers/urgent] tick/broadcast: Plug clockevents replacement race

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     113a9796effe3376d2ec5aabcca1fef4fef4cd62
Gitweb:        https://git.kernel.org/tip/113a9796effe3376d2ec5aabcca1fef4fef4cd62
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Mon, 12 Aug 2024 16:19:48 +02:00
Committer:     Thomas Gleixner <tglx@kernel.org>
CommitterDate: Mon, 07 Sep 2026 11:10:20 +02:00

tick/broadcast: Plug clockevents replacement race

朱恺乾 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@xiaomi.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
Reviewed-by: Bradley Morgan <brads@mainlining.org>
Tested-by: 刘术高 <liushugao@xiaomi.com>
Cc: stable@vger.kernel.org
Link: https://patch.msgid.link/87cymdsu0r.ffs@tglx
---
 kernel/time/clockevents.c    | 33 +++++++++++++++++++-------------
 kernel/time/tick-broadcast.c | 36 +++++++++++++++++++++--------------
 kernel/time/tick-internal.h  |  2 ++-
 3 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 0014d16..62ad7c0 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -615,34 +615,41 @@ void clockevents_handle_noop(struct clock_event_device *dev)
 {
 }
 
-/**
- * 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)
@@ -699,7 +706,7 @@ void tick_offline_cpu(unsigned 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);
 		}
 	}
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 115e0bf..bda3d23 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -165,23 +165,31 @@ static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev,
  */
 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;
@@ -1218,7 +1226,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;
 }
@@ -1226,7 +1234,7 @@ bool tick_broadcast_oneshot_available(void)
 #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;
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 182974c..65680db 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -55,6 +55,8 @@ static inline void clockevent_set_state(struct clock_event_device *dev,
 }
 
 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,