From nobody Fri Dec 19 04:58:41 2025 Received: from outboundhk.mxmail.xiaomi.com (outboundhk.mxmail.xiaomi.com [118.143.206.90]) by smtp.subspace.kernel.org (Postfix) with ESMTP id EFE361CD11 for ; Fri, 28 Jun 2024 01:59:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=118.143.206.90 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1719539954; cv=none; b=MVsjiPEWyPb/iQ9/9alVGN/60ljVNLBlo7rMct7iuPUn/cm3hDftuZ5o3cPHU5i2Xks9hm4W8uplxjJh0B9xGiZ4WY4ZO9N8HKaaE6cT+VIU+GHxERtl3Opf1/Cv9mu+bW7gcui57Z8QXVQVdUDh2fD0MaVCMyW8M2cNw0n7u38= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1719539954; c=relaxed/simple; bh=tHz0ETxGUjMmqs0QL9IU4UcusIwi4JFzZ4iA/q2R48o=; h=From:To:CC:Subject:Date:Message-ID:References:In-Reply-To: Content-Type:MIME-Version; b=q7GfGu70vKb/BMFuL7TcOtq7jPav/VoAqZ5b23VPZfLHHsHxPxlpmkGDiwxaK9Suj/Kx/h0YSC1KHchy4BXKUywL7MAbVuxJGeJ4AFzowF0HmnL1+OY1hGwdG8NgNPu6DZ3Yjvqzt0zXyK6gjUfMdYE6RCWYGTvXNFuov3LZO9U= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=xiaomi.com; spf=pass smtp.mailfrom=xiaomi.com; arc=none smtp.client-ip=118.143.206.90 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=xiaomi.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=xiaomi.com X-CSE-ConnectionGUID: LxzmufuZTRqWUSjIvwze1g== X-CSE-MsgGUID: 2ertYfZ9SPq/T6ePoiW7cg== X-IronPort-AV: E=Sophos;i="6.09,167,1716220800"; d="scan'208";a="89331574" From: =?utf-8?B?5pyx5oG65Lm+?= To: Thomas Gleixner , Daniel Lezcano CC: "linux-kernel@vger.kernel.org" , =?utf-8?B?546L6Z+s?= , =?utf-8?B?54aK5Lqu?= , "isaacmanjarres@google.com" , Frederic Weisbecker , Anna-Maria Behnsen Subject: RE: [External Mail]Re: Race condition when replacing the broadcast timer Thread-Topic: [External Mail]Re: Race condition when replacing the broadcast timer Thread-Index: AdrHa8Ctven+S08/SKWKg8CfgMneBAA1hJcAAC8tusA= Date: Fri, 28 Jun 2024 01:59:03 +0000 Message-ID: References: <042520850d394f0bb0004a226db63d0d@xiaomi.com> <87o77m1v9r.ffs@tglx> In-Reply-To: <87o77m1v9r.ffs@tglx> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Thanks for the fast reply. May I know when there'll be a formal patch on the mainline? -----Original Message----- From: Thomas Gleixner Sent: Thursday, June 27, 2024 7:27 PM To: =E6=9C=B1=E6=81=BA=E4=B9=BE ; Daniel Lezcano Cc: linux-kernel@vger.kernel.org; =E7=8E=8B=E9=9F=AC ; = =E7=86=8A=E4=BA=AE ; isaacmanjarres@google.com; Fred= eric Weisbecker ; Anna-Maria Behnsen Subject: [External Mail]Re: Race condition when replacing the broadcast tim= er [=E5=A4=96=E9=83=A8=E9=82=AE=E4=BB=B6] =E6=AD=A4=E9=82=AE=E4=BB=B6=E6=9D=A5= =E6=BA=90=E4=BA=8E=E5=B0=8F=E7=B1=B3=E5=85=AC=E5=8F=B8=E5=A4=96=E9=83=A8=EF= =BC=8C=E8=AF=B7=E8=B0=A8=E6=85=8E=E5=A4=84=E7=90=86=E3=80=82=E8=8B=A5=E5=AF= =B9=E9=82=AE=E4=BB=B6=E5=AE=89=E5=85=A8=E6=80=A7=E5=AD=98=E7=96=91=EF=BC=8C= =E8=AF=B7=E5=B0=86=E9=82=AE=E4=BB=B6=E8=BD=AC=E5=8F=91=E7=BB=99misec@xiaomi= .com=E8=BF=9B=E8=A1=8C=E5=8F=8D=E9=A6=88 On Wed, Jun 26 2024 at 02:17, =E6=9C=B1=E6=81=BA=E4=B9=BE 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) =3D=3D 1 && !tick_is_broadcast_device(dev)) { - BUG_ON(!clockevent_state_detached(dev)); + WARN_ON(!clockevent_state_detached(dev)); list_del(&dev->list); } } --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -162,23 +162,31 @@ static bool tick_set_oneshot_wakeup_devi */ void tick_install_broadcast_device(struct clock_event_device *dev, int cpu= ) { - struct clock_event_device *cur =3D tick_broadcast_device.evtdev; + struct clock_event_device *cur; - if (tick_set_oneshot_wakeup_device(dev, cpu)) - return; + scoped_guard(raw_spinlock_irqsave, &tick_broadcast_lock) { - if (!tick_check_broadcast_device(cur, dev)) - return; + if (tick_set_oneshot_wakeup_device(dev, cpu)) + return; - if (!try_module_get(dev->owner)) - return; + cur =3D tick_broadcast_device.evtdev; + if (!tick_check_broadcast_device(cur, dev)) + return; - clockevents_exchange_device(cur, dev); + if (!try_module_get(dev->owner)) + return; + + __clockevents_exchange_device(cur, dev); + if (cur) + cur->event_handler =3D clockevents_handle_noop; + WRITE_ONCE(tick_broadcast_device.evtdev, dev); + if (!cpumask_empty(tick_broadcast_mask)) + tick_broadcast_start_periodic(dev); + } + + /* Module release must be outside of the lock */ if (cur) - cur->event_handler =3D clockevents_handle_noop; - tick_broadcast_device.evtdev =3D dev; - if (!cpumask_empty(tick_broadcast_mask)) - tick_broadcast_start_periodic(dev); + module_put(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 =3D tick_broadcast_device.evtdev; + struct clock_event_device *bc =3D + READ_ONCE(tick_broadcast_device.evtdev); return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false; } @@ -1= 193,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 =3D tick_broadcast_device.evtdev; + struct clock_event_device *bc =3D + 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); e= xtern void clockevents_switch_state(struct clock_event_device *dev, #/******=E6=9C=AC=E9=82=AE=E4=BB=B6=E5=8F=8A=E5=85=B6=E9=99=84=E4=BB=B6=E5= =90=AB=E6=9C=89=E5=B0=8F=E7=B1=B3=E5=85=AC=E5=8F=B8=E7=9A=84=E4=BF=9D=E5=AF= =86=E4=BF=A1=E6=81=AF=EF=BC=8C=E4=BB=85=E9=99=90=E4=BA=8E=E5=8F=91=E9=80=81= =E7=BB=99=E4=B8=8A=E9=9D=A2=E5=9C=B0=E5=9D=80=E4=B8=AD=E5=88=97=E5=87=BA=E7= =9A=84=E4=B8=AA=E4=BA=BA=E6=88=96=E7=BE=A4=E7=BB=84=E3=80=82=E7=A6=81=E6=AD= =A2=E4=BB=BB=E4=BD=95=E5=85=B6=E4=BB=96=E4=BA=BA=E4=BB=A5=E4=BB=BB=E4=BD=95= =E5=BD=A2=E5=BC=8F=E4=BD=BF=E7=94=A8=EF=BC=88=E5=8C=85=E6=8B=AC=E4=BD=86=E4= =B8=8D=E9=99=90=E4=BA=8E=E5=85=A8=E9=83=A8=E6=88=96=E9=83=A8=E5=88=86=E5=9C= =B0=E6=B3=84=E9=9C=B2=E3=80=81=E5=A4=8D=E5=88=B6=E3=80=81=E6=88=96=E6=95=A3= =E5=8F=91=EF=BC=89=E6=9C=AC=E9=82=AE=E4=BB=B6=E4=B8=AD=E7=9A=84=E4=BF=A1=E6= =81=AF=E3=80=82=E5=A6=82=E6=9E=9C=E6=82=A8=E9=94=99=E6=94=B6=E4=BA=86=E6=9C= =AC=E9=82=AE=E4=BB=B6=EF=BC=8C=E8=AF=B7=E6=82=A8=E7=AB=8B=E5=8D=B3=E7=94=B5= =E8=AF=9D=E6=88=96=E9=82=AE=E4=BB=B6=E9=80=9A=E7=9F=A5=E5=8F=91=E4=BB=B6=E4= =BA=BA=E5=B9=B6=E5=88=A0=E9=99=A4=E6=9C=AC=E9=82=AE=E4=BB=B6=EF=BC=81 This = e-mail and its attachments contain confidential information from XIAOMI, wh= ich 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 no= t limited to, total or partial disclosure, reproduction, or dissemination) = by persons other than the intended recipient(s) is prohibited. If you recei= ve this e-mail in error, please notify the sender by phone or email immedia= tely and delete it!******/#