* [PATCH v3] once: fix race by moving DO_ONCE to separate section
@ 2025-09-09 11:29 Qi Xi
2025-09-09 12:01 ` Qi Xi
2025-09-25 6:14 ` Arnd Bergmann
0 siblings, 2 replies; 5+ messages in thread
From: Qi Xi @ 2025-09-09 11:29 UTC (permalink / raw)
To: bobo.shaobowang, xiqi2, xiexiuqi, arnd, masahiroy, kuba, edumazet,
linux-arch
Cc: linux-kernel
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)
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); \
--
2.33.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] once: fix race by moving DO_ONCE to separate section
2025-09-09 11:29 [PATCH v3] once: fix race by moving DO_ONCE to separate section Qi Xi
@ 2025-09-09 12:01 ` Qi Xi
2025-09-25 1:49 ` Qi Xi
2025-09-25 6:14 ` Arnd Bergmann
1 sibling, 1 reply; 5+ messages in thread
From: Qi Xi @ 2025-09-09 12:01 UTC (permalink / raw)
To: bobo.shaobowang, xiexiuqi, arnd, masahiroy, kuba, edumazet,
linux-arch
Cc: linux-kernel
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); \
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] once: fix race by moving DO_ONCE to separate section
2025-09-09 12:01 ` Qi Xi
@ 2025-09-25 1:49 ` Qi Xi
0 siblings, 0 replies; 5+ messages in thread
From: Qi Xi @ 2025-09-25 1:49 UTC (permalink / raw)
To: bobo.shaobowang, xiexiuqi, arnd, masahiroy, kuba, edumazet,
linux-arch
Cc: linux-kernel
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); \
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] once: fix race by moving DO_ONCE to separate section
2025-09-09 11:29 [PATCH v3] once: fix race by moving DO_ONCE to separate section Qi Xi
2025-09-09 12:01 ` Qi Xi
@ 2025-09-25 6:14 ` Arnd Bergmann
2025-09-26 0:19 ` Nathan Chancellor
1 sibling, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2025-09-25 6:14 UTC (permalink / raw)
To: Qi Xi, bobo.shaobowang, xiexiuqi
Cc: linux-kernel, Masahiro Yamada, Jakub Kicinski, Eric Dumazet,
Linux-Arch, linux-kbuild, Nathan Chancellor, Nicolas Schier,
Andrew Morton
On Tue, Sep 9, 2025, at 13: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)
>
> 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(-)
Hi,
I hadn't seen this until your ping and had a look since it
touches asm-generic. The patch looks correct to me, thanks for
addressing this and for the detailed patch description.
I think what happened is that nobody felt responsible for
applying it, between the networking (which originally
added the infrastructure) and kbuild maintainers (Masahiro
did the last changes to this bit, but recently handed
over maintenance to Nathan).
I've applied the fix to the asm-generic tree for 6.18 now
to be sure that it makes it in and gets linux-next testing,
but it's not my area either really.
It would be best to have another review though. If Nathan
or Andrew want to instead pick it up through one of their
trees, please add
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] once: fix race by moving DO_ONCE to separate section
2025-09-25 6:14 ` Arnd Bergmann
@ 2025-09-26 0:19 ` Nathan Chancellor
0 siblings, 0 replies; 5+ messages in thread
From: Nathan Chancellor @ 2025-09-26 0:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Qi Xi, bobo.shaobowang, xiexiuqi, linux-kernel, Masahiro Yamada,
Jakub Kicinski, Eric Dumazet, Linux-Arch, linux-kbuild,
Nicolas Schier, Andrew Morton
On Thu, Sep 25, 2025 at 08:14:35AM +0200, Arnd Bergmann wrote:
> I hadn't seen this until your ping and had a look since it
> touches asm-generic. The patch looks correct to me, thanks for
> addressing this and for the detailed patch description.
Agreed, I am not super familiar with this infrastructure but this seems
correct from my basic understanding.
> I think what happened is that nobody felt responsible for
> applying it, between the networking (which originally
> added the infrastructure) and kbuild maintainers (Masahiro
> did the last changes to this bit, but recently handed
> over maintenance to Nathan).
Yeah, that seems likely. For the record, I would not have felt
responsible for this code even if it was Cc'd to me since this does not
read as Kbuild material to me.
> I've applied the fix to the asm-generic tree for 6.18 now
> to be sure that it makes it in and gets linux-next testing,
> but it's not my area either really.
>
> It would be best to have another review though. If Nathan
> or Andrew want to instead pick it up through one of their
> trees, please add
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
I do not have a strong opinion on whether you or Andrew take it (I think
it makes sense either way) but I do not think it makes sense for me to
take it via Kbuild.
Cheers,
Nathan
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-09-26 0:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-09 11:29 [PATCH v3] once: fix race by moving DO_ONCE to separate section Qi Xi
2025-09-09 12:01 ` Qi Xi
2025-09-25 1:49 ` Qi Xi
2025-09-25 6:14 ` Arnd Bergmann
2025-09-26 0:19 ` Nathan Chancellor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).