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

[Qi Xi]

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@xxxxxxxxxx>
> > Signed-off-by: Qi Xi <xiqi2@xxxxxxxxxx>
> > ---
> > 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);        \