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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87v7ykf4no.fsf@somnus>
Date: Wed, 25 Sep 2024 11:45:15 +0200
From: Anna-Maria Behnsen <anna-maria@...utronix.de>
To: Thomas Gleixner <tglx@...utronix.de>, 朱恺乾
 <zhukaiqian@...omi.com>,
 Daniel Lezcano <daniel.lezcano@...aro.org>, 张嘉伟
 <zhangjiawei8@...omi.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 王韬
 <lingyue@...omi.com>, 熊亮 <xiongliang@...omi.com>,
 "isaacmanjarres@...gle.com" <isaacmanjarres@...gle.com>, Frederic
 Weisbecker <frederic@...nel.org>, 梁伟鹏
 <weipengliang@...omi.com>, 翁金飞
 <wengjinfei@...omi.com>
Subject: Re: [PATCH] tick/broadcast: Plug clockevents replacement race

Thomas Gleixner <tglx@...utronix.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@...omi.com>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.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