Re: [PATCH v3] once: fix race by moving DO_ONCE to separate section

[Qi Xi <xiqi2@huawei.com>]

kindly ping

On 09/09/2025 20:01, Qi Xi wrote:
>
> On 09/09/2025 19:29, Qi Xi wrote:
>> The commit c2c60ea37e5b ("once: use __section(".data.once")") moved
>> DO_ONCE's ___done variable to .data.once section, which conflicts with
>> DO_ONCE_LITE() that also uses the same section.
>>
>> This creates a race condition when clear_warn_once is used:
>>
>> Thread 1 (DO_ONCE)             Thread 2 (DO_ONCE)
>> __do_once_start
>>      read ___done (false)
>>      acquire once_lock
>> execute func
>> __do_once_done
>>      write ___done (true)      __do_once_start
>>      release once_lock             // Thread 3 clear_warn_once reset 
>> ___done
>>                                    read ___done (false)
>>                                    acquire once_lock
>>                                execute func
>> schedule once_work            __do_once_done
>> once_deferred: OK             write ___done (true)
>> static_branch_disable         release once_lock
>>                                schedule once_work
>>                                once_deferred:
>>                                    BUG_ON(!static_key_enabled)
> When simulating concurrent execution between DO_ONCE() and
> clear_warn_once (clears the entire .data..once section in 
> __do_once_done()),
> BUG_ON() can be easily reproduced.
>
> +#include <asm/sections.h>
>  void __do_once_done(bool *done, struct static_key_true *once_key,
>                     unsigned long *flags, struct module *mod)
>         __releases(once_lock)
>  {
>         *done = true;
>         spin_unlock_irqrestore(&once_lock, *flags);
> +       memset(__start_once, 0, __end_once - __start_once);
> +       pr_info("__do_once_done done: %d\n", *done); // *done equals 0
>         once_disable_jump(once_key, mod);
>  }
>
>>
>> DO_ONCE_LITE() in once_lite.h is used by WARN_ON_ONCE() and other 
>> warning
>> macros. Keep its ___done flag in the .data..once section and allow 
>> resetting
>> by clear_warn_once, as originally intended.
>>
>> In contrast, DO_ONCE() is used for functions like get_random_once() and
>> relies on its ___done flag for internal synchronization. We should 
>> not reset
>> DO_ONCE() by clear_warn_once.
>>
>> Fix it by isolating DO_ONCE's ___done into a separate .data..do_once 
>> section,
>> shielding it from clear_warn_once.
>>
>> Fixes: c2c60ea37e5b ("once: use __section(".data.once")")
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Qi Xi <xiqi2@huawei.com>
>> ---
>> v3 -> v2: apply the same section change to DO_ONCE_SLEEPABLE().
>> v2 -> v1: add comments for DO_ONCE_LITE() and DO_ONCE().
>> ---
>>   include/asm-generic/vmlinux.lds.h | 1 +
>>   include/linux/once.h              | 4 ++--
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/asm-generic/vmlinux.lds.h 
>> b/include/asm-generic/vmlinux.lds.h
>> index 883dbac79da9..94850b52e5cc 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -384,6 +384,7 @@
>>       __start_once = .;                        \
>>       *(.data..once)                            \
>>       __end_once = .;                            \
>> +    *(.data..do_once)                        \
>>       STRUCT_ALIGN();                            \
>>       *(__tracepoints)                        \
>>       /* implement dynamic printk debug */                \
>> diff --git a/include/linux/once.h b/include/linux/once.h
>> index 30346fcdc799..449a0e34ad5a 100644
>> --- a/include/linux/once.h
>> +++ b/include/linux/once.h
>> @@ -46,7 +46,7 @@ void __do_once_sleepable_done(bool *done, struct 
>> static_key_true *once_key,
>>   #define DO_ONCE(func, ...)                             \
>>       ({                                     \
>>           bool ___ret = false;                         \
>> -        static bool __section(".data..once") ___done = false;         \
>> +        static bool __section(".data..do_once") ___done = false;     \
>>           static DEFINE_STATIC_KEY_TRUE(___once_key);             \
>>           if (static_branch_unlikely(&___once_key)) {             \
>>               unsigned long ___flags;                     \
>> @@ -64,7 +64,7 @@ void __do_once_sleepable_done(bool *done, struct 
>> static_key_true *once_key,
>>   #define DO_ONCE_SLEEPABLE(func, ...)                        \
>>       ({                                    \
>>           bool ___ret = false;                        \
>> -        static bool __section(".data..once") ___done = false;        \
>> +        static bool __section(".data..do_once") ___done = false;    \
>>           static DEFINE_STATIC_KEY_TRUE(___once_key); \
>>           if (static_branch_unlikely(&___once_key)) {            \
>>               ___ret = __do_once_sleepable_start(&___done);        \