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

[Thomas Gleixner <tglx@linutronix.de>]

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 ...