From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:07
Hi,
This is an updated version of the initial post after PeterZ made me
aware that there are users outside of the module directory.
The goal is replace the mix auf rcu_read_lock(), rcu_read_lock_sched()
and preempt_disable() with just rcu_read_lock().
I've split it into smaller chunks which can be applied/ reviewed
independently.
v2…v3: https://lore.kernel.org/all/20241220174731.514432-1-bigeasy@linutronix.de/
- Converted cfi to use RCU.
- Use scoped_guard() in LoongArch's ftrace code after Steven suggested
it.
v1…v2: https://lore.kernel.org/all/20241205215102.hRywUW2A@linutronix.de/
- Split into smaller patches.
- Converted all users.
Sebastian Andrzej Siewior (28):
module: Extend the preempt disabled section in
dereference_symbol_descriptor().
module: Begin to move from RCU-sched to RCU.
module: Use proper RCU assignment in add_kallsyms().
module: Use RCU in find_kallsyms_symbol().
module: Use RCU in module_get_kallsym().
module: Use RCU in find_module_all().
module: Use RCU in __find_kallsyms_symbol_value().
module: Use RCU in module_kallsyms_on_each_symbol().
module: Remove module_assert_mutex_or_preempt() from
try_add_tainted_module().
module: Use RCU in find_symbol().
module: Use RCU in __is_module_percpu_address().
module: Allow __module_address() to be called from RCU section.
module: Use RCU in search_module_extables().
module: Use RCU in all users of __module_address().
module: Use RCU in all users of __module_text_address().
ARM: module: Use RCU in all users of __module_text_address().
arm64: module: Use RCU in all users of __module_text_address().
LoongArch/orc: Use RCU in all users of __module_address().
LoongArch: ftrace: Use RCU in all users of __module_text_address().
powerpc/ftrace: Use RCU in all users of __module_text_address().
cfi: Use RCU while invoking __module_address().
x86: Use RCU in all users of __module_address().
jump_label: Use RCU in all users of __module_address().
jump_label: Use RCU in all users of __module_text_address().
bpf: Use RCU in all users of __module_text_address().
kprobes: Use RCU in all users of __module_text_address().
static_call: Use RCU in all users of __module_text_address().
bug: Use RCU instead RCU-sched to protect module_bug_list.
arch/arm/kernel/module-plts.c | 4 +-
arch/arm64/kernel/ftrace.c | 7 +-
arch/loongarch/kernel/ftrace_dyn.c | 9 +-
arch/loongarch/kernel/unwind_orc.c | 4 +-
arch/powerpc/kernel/trace/ftrace.c | 6 +-
arch/powerpc/kernel/trace/ftrace_64_pg.c | 6 +-
arch/x86/kernel/callthunks.c | 3 +-
arch/x86/kernel/unwind_orc.c | 4 +-
include/linux/kallsyms.h | 3 +-
include/linux/module.h | 2 +-
kernel/cfi.c | 5 +-
kernel/jump_label.c | 31 ++++---
kernel/kprobes.c | 4 +-
kernel/livepatch/core.c | 4 +-
kernel/module/internal.h | 11 ---
kernel/module/kallsyms.c | 73 ++++++----------
kernel/module/main.c | 103 ++++++++---------------
kernel/module/tracking.c | 2 -
kernel/module/tree_lookup.c | 8 +-
kernel/module/version.c | 14 +--
kernel/static_call_inline.c | 13 ++-
kernel/trace/bpf_trace.c | 19 ++---
kernel/trace/trace_kprobe.c | 9 +-
lib/bug.c | 22 ++---
24 files changed, 136 insertions(+), 230 deletions(-)
Sebastian
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:15
The RCU usage in module was introduced in commit d72b37513cdfb ("Remove
stop_machine during module load v2") and it claimed not to be RCU but
similar. Then there was another improvement in commit e91defa26c527
("module: don't use stop_machine on module load"). It become a mix of
RCU and RCU-sched and was eventually fixed 0be964be0d450 ("module:
Sanitize RCU usage and locking"). Later RCU & RCU-sched was merged in
commit cb2f55369d3a9 ("modules: Replace synchronize_sched() and
call_rcu_sched()") so that was aligned.
Looking at it today, there is still leftovers. The preempt_disable() was
used instead rcu_read_lock_sched(). The RCU & RCU-sched merge was not
complete as there is still rcu_dereference_sched() for module::kallsyms.
The RCU-list modules and unloaded_tainted_modules are always accessed
under RCU protection or the module_mutex. The modules list iteration can
always happen safely because the module will not disappear.
Once the module is removed (free_module()) then after removing the
module from the list, there is a synchronize_rcu() which waits until
every RCU reader left the section. That means iterating over the list
within a RCU-read section is enough, there is no need to disable
preemption. module::kallsyms is first assigned in add_kallsyms() before
the module is added to the list. At this point, it points to init data.
This pointer is later updated and before the init code is removed there
is also synchronize_rcu() in do_free_init(). That means A RCU read lock
is enough for protection and rcu_dereference() can be safely used.
Convert module code and its users step by step. Update comments and
convert print_modules() to use RCU.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/main.c | 9 ++++-----
kernel/module/tree_lookup.c | 8 ++++----
2 files changed, 8 insertions(+), 9 deletions(-)
@@ -1348,7 +1348,7 @@ static void free_module(struct module *mod)mod_tree_remove(mod);/* Remove this module from bug list, this uses list_del_rcu */module_bug_cleanup(mod);-/* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */+/* Wait for RCU synchronizing before releasing mod->list and buglist. */synchronize_rcu();if(try_add_tainted_module(mod))pr_err("%s: adding tainted module to the unloaded tainted modules list failed.\n",
@@ -2965,7 +2965,7 @@ static noinline int do_init_module(struct module *mod)#endif/**Wewanttofreemodule_init,butbeawarethatkallsymsmaybe-*walkingthiswithpreemptdisabled.Inallthefailurepaths,we+*walkingthiswithinanRCUreadsection.Inallthefailurepaths,we*callsynchronize_rcu(),butwedon'twanttoslowdownthesuccess*path.execmem_free()cannotbecalledinaninterrupt,sodothe*workandcallsynchronize_rcu()inaworkqueue.
@@ -3754,7 +3754,7 @@ void print_modules(void)printk(KERN_DEFAULT"Modules linked in:");/* Most callers should already have preempt disabled, but make sure */-preempt_disable();+guard(rcu)();list_for_each_entry_rcu(mod,&modules,list){if(mod->state==MODULE_STATE_UNFORMED)continue;
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:15
dereference_symbol_descriptor() needs to obtain the module pointer
belonging to pointer in order to resolve that pointer.
The returned mod pointer is obtained under RCU-sched/ preempt_disable()
guarantees and needs to be used within this section to ensure that the
module is not removed in the meantime.
Extend the preempt_disable() section to also cover
dereference_module_function_descriptor().
Fixes: 04b8eb7a4ccd9 ("symbol lookup: introduce dereference_symbol_descriptor()")
Cc: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Christophe Leroy <redacted>
Cc: Helge Deller <deller@gmx.de>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Sergey Senozhatsky <redacted>
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/kallsyms.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:16
add_kallsyms() assigns the RCU pointer module::kallsyms and setups the
structures behind it which point to init-data. The module was not
published yet, nothing can see the kallsyms pointer and the data behind
it. Also module's init function was not yet invoked.
There is no need to use rcu_dereference() here, it is just to keep
checkers quiet. The whole RCU read section is also not needed.
Use a local kallsyms pointer and setup the data structures. Assign that
pointer to the data structure at the end via rcu_assign_pointer().
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/kallsyms.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
@@ -177,19 +177,15 @@ void add_kallsyms(struct module *mod, const struct load_info *info)unsignedlongstrtab_size;void*data_base=mod->mem[MOD_DATA].base;void*init_data_base=mod->mem[MOD_INIT_DATA].base;+structmod_kallsyms*kallsyms;-/* Set up to point into init section. */-mod->kallsyms=(void__rcu*)init_data_base+-info->mod_kallsyms_init_off;+kallsyms=init_data_base+info->mod_kallsyms_init_off;-rcu_read_lock();-/* The following is safe since this pointer cannot change */-rcu_dereference(mod->kallsyms)->symtab=(void*)symsec->sh_addr;-rcu_dereference(mod->kallsyms)->num_symtab=symsec->sh_size/sizeof(Elf_Sym);+kallsyms->symtab=(void*)symsec->sh_addr;+kallsyms->num_symtab=symsec->sh_size/sizeof(Elf_Sym);/* Make sure we get permanent strtab: don't use info->strtab. */-rcu_dereference(mod->kallsyms)->strtab=-(void*)info->sechdrs[info->index.str].sh_addr;-rcu_dereference(mod->kallsyms)->typetab=init_data_base+info->init_typeoffs;+kallsyms->strtab=(void*)info->sechdrs[info->index.str].sh_addr;+kallsyms->typetab=init_data_base+info->init_typeoffs;/**Nowpopulatethecutdowncorekallsymsforafterinit
@@ -220,7 +215,9 @@ void add_kallsyms(struct module *mod, const struct load_info *info)strtab_size-=ret+1;}}-rcu_read_unlock();++/* Set up to point into init section. */+rcu_assign_pointer(mod->kallsyms,kallsyms);mod->core_kallsyms.num_symtab=ndst;}
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:16
The modules list and module::kallsyms can be accessed under RCU
assumption.
Iterate the modules with RCU protection, use rcu_dereference() to access
the kallsyms pointer.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/kallsyms.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
@@ -381,13 +381,13 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,{structmodule*mod;-preempt_disable();+guard(rcu)();list_for_each_entry_rcu(mod,&modules,list){structmod_kallsyms*kallsyms;if(mod->state==MODULE_STATE_UNFORMED)continue;-kallsyms=rcu_dereference_sched(mod->kallsyms);+kallsyms=rcu_dereference(mod->kallsyms);if(symnum<kallsyms->num_symtab){constElf_Sym*sym=&kallsyms->symtab[symnum];
@@ -396,12 +396,10 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,strscpy(name,kallsyms_symbol_name(kallsyms,symnum),KSYM_NAME_LEN);strscpy(module_name,mod->name,MODULE_NAME_LEN);*exported=is_exported(name,*value,mod);-preempt_enable();return0;}symnum-=kallsyms->num_symtab;}-preempt_enable();return-ERANGE;}
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:16
The modules list and module::kallsyms can be accessed under RCU
assumption.
Use rcu_dereference() to reference the kallsyms pointer in
find_kallsyms_symbol(). Use a RCU section instead of preempt_disable in
callers of find_kallsyms_symbol(). Keep the preempt-disable in
module_address_lookup() due to __module_address().
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/kallsyms.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
@@ -257,7 +257,7 @@ static const char *find_kallsyms_symbol(struct module *mod,{unsignedinti,best=0;unsignedlongnextval,bestval;-structmod_kallsyms*kallsyms=rcu_dereference_sched(mod->kallsyms);+structmod_kallsyms*kallsyms=rcu_dereference(mod->kallsyms);structmodule_memory*mod_mem;/* At worse, next value is at end of module */
@@ -329,6 +329,7 @@ int module_address_lookup(unsigned long addr,intret=0;structmodule*mod;+guard(rcu)();preempt_disable();mod=__module_address(addr);if(mod){
@@ -356,7 +357,7 @@ int lookup_module_symbol_name(unsigned long addr, char *symname){structmodule*mod;-preempt_disable();+guard(rcu)();list_for_each_entry_rcu(mod,&modules,list){if(mod->state==MODULE_STATE_UNFORMED)continue;
@@ -368,12 +369,10 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)gotoout;strscpy(symname,sym,KSYM_NAME_LEN);-preempt_enable();return0;}}out:-preempt_enable();return-ERANGE;}
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:19
module_assert_mutex_or_preempt() is not needed in
try_add_tainted_module(). The function checks for RCU-sched or the
module_mutex to be acquired. The list_for_each_entry_rcu() below does
the same check.
Remove module_assert_mutex_or_preempt() from try_add_tainted_module().
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/tracking.c | 2 --
1 file changed, 2 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:19
module::kallsyms can be accessed under RCU assumption.
Use rcu_dereference() to access module::kallsyms.
Update callers.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/kallsyms.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
@@ -407,7 +407,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,staticunsignedlong__find_kallsyms_symbol_value(structmodule*mod,constchar*name){unsignedinti;-structmod_kallsyms*kallsyms=rcu_dereference_sched(mod->kallsyms);+structmod_kallsyms*kallsyms=rcu_dereference(mod->kallsyms);for(i=0;i<kallsyms->num_symtab;i++){constElf_Sym*sym=&kallsyms->symtab[i];
@@ -447,24 +447,15 @@ static unsigned long __module_kallsyms_lookup_name(const char *name)/* Look for this name: can be of form module:name. */unsignedlongmodule_kallsyms_lookup_name(constchar*name){-unsignedlongret;-/* Don't lock: we're in enough trouble already. */guard(rcu)();-preempt_disable();-ret=__module_kallsyms_lookup_name(name);-preempt_enable();-returnret;+return__module_kallsyms_lookup_name(name);}unsignedlongfind_kallsyms_symbol_value(structmodule*mod,constchar*name){-unsignedlongret;--preempt_disable();-ret=__find_kallsyms_symbol_value(mod,name);-preempt_enable();-returnret;+guard(rcu)();+return__find_kallsyms_symbol_value(mod,name);}intmodule_kallsyms_on_each_symbol(constchar*modname,
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:19
The modules list can be accessed under RCU assumption.
Use RCU protection instead preempt_disable().
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/main.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:19
mod_find() uses either the modules list to find a module or a tree
lookup (CONFIG_MODULES_TREE_LOOKUP). The list and the tree can both be
iterated under RCU assumption (as well as RCU-sched).
Remove module_assert_mutex_or_preempt() from __module_address() and
entirely since __module_address() is the last user.
Update comments.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/internal.h | 11 -----------
kernel/module/main.c | 4 +---
2 files changed, 1 insertion(+), 14 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:19
The modules list and module::kallsyms can be accessed under RCU
assumption.
Remove module_assert_mutex_or_preempt() from find_module_all() so it can
be used under RCU protection without warnings. Update its callers to use
RCU protection instead of preempt_disable().
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Joe Lawrence <joe.lawrence@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Miroslav Benes <mbenes@suse.cz>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-trace-kernel@vger.kernel.org
Cc: live-patching@vger.kernel.org
Reviewed-by: Petr Mladek <pmladek@suse.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/module.h | 2 +-
kernel/livepatch/core.c | 4 +---
kernel/module/kallsyms.c | 1 +
kernel/module/main.c | 6 ++----
kernel/trace/trace_kprobe.c | 9 +++------
5 files changed, 8 insertions(+), 14 deletions(-)
@@ -663,7 +663,7 @@ static inline bool within_module(unsigned long addr, const struct module *mod)returnwithin_module_init(addr,mod)||within_module_core(addr,mod);}-/* Search for module by name: must be in a RCU-sched critical section. */+/* Search for module by name: must be in a RCU critical section. */structmodule*find_module(constchar*name);externvoid__noreturn__module_put_and_kthread_exit(structmodule*mod,
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:19
module_assert_mutex_or_preempt() is not needed in find_symbol(). The
function checks for RCU-sched or the module_mutex to be acquired. The
list_for_each_entry_rcu() below does the same check.
Remove module_assert_mutex_or_preempt() from try_add_tainted_module().
Use RCU protection to invoke find_symbol() and update callers.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/main.c | 30 ++++++++++++------------------
kernel/module/version.c | 14 +++++++-------
2 files changed, 19 insertions(+), 25 deletions(-)
@@ -1369,21 +1366,18 @@ void *__symbol_get(const char *symbol).warn=true,};-preempt_disable();-if(!find_symbol(&fsa))-gotofail;-if(fsa.license!=GPL_ONLY){-pr_warn("failing symbol_get of non-GPLONLY symbol %s.\n",-symbol);-gotofail;+scoped_guard(rcu){+if(!find_symbol(&fsa))+returnNULL;+if(fsa.license!=GPL_ONLY){+pr_warn("failing symbol_get of non-GPLONLY symbol %s.\n",+symbol);+returnNULL;+}+if(strong_try_module_get(fsa.owner))+returnNULL;}-if(strong_try_module_get(fsa.owner))-gotofail;-preempt_enable();return(void*)kernel_symbol_value(fsa.sym);-fail:-preempt_enable();-returnNULL;}EXPORT_SYMBOL_GPL(__symbol_get);
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:19
module::kallsyms can be accessed under RCU assumption.
Use rcu_dereference() to access module::kallsyms.
Update callers.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/kallsyms.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
@@ -476,10 +476,8 @@ int module_kallsyms_on_each_symbol(const char *modname,if(modname&&strcmp(modname,mod->name))continue;-/* Use rcu_dereference_sched() to remain compliant with the sparse tool */-preempt_disable();-kallsyms=rcu_dereference_sched(mod->kallsyms);-preempt_enable();+kallsyms=rcu_dereference_check(mod->kallsyms,+lockdep_is_held(&module_mutex));for(i=0;i<kallsyms->num_symtab;i++){constElf_Sym*sym=&kallsyms->symtab[i];
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:20
search_module_extables() returns an exception_table_entry belonging to a
module. The lookup via __module_address() can be performed with RCU
protection.
The returned exception_table_entry remains valid because the passed
address usually belongs to a module that is currently executed. So the
module can not be removed because "something else" holds a reference to
it, ensuring that it can not be removed.
Exceptions here are:
- kprobe, acquires a reference on the module beforehand
- MCE, invokes the function from within a timer and the RCU lifetime
guarantees (of the timer) are sufficient.
Therefore it is safe to return the exception_table_entry outside the RCU
section which provided the module.
Use RCU for the lookup in search_module_extables() and update the
comment.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/main.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
@@ -3621,28 +3621,23 @@ char *module_flags(struct module *mod, char *buf, bool show_state)/* Given an address, look for it in the module exception tables. */conststructexception_table_entry*search_module_extables(unsignedlongaddr){-conststructexception_table_entry*e=NULL;structmodule*mod;-preempt_disable();+guard(rcu)();mod=__module_address(addr);if(!mod)-gotoout;+returnNULL;if(!mod->num_exentries)-gotoout;--e=search_extable(mod->extable,-mod->num_exentries,-addr);-out:-preempt_enable();-+returnNULL;/*-*Now,ifwefoundone,wearerunninginsideitnow,hence-*wecannotunloadthemodule,hencenorefcntneeded.+*Theaddresspassedherebelongstoamodulethatiscurrently+*invoked(wearerunninginsideit).Thereforeitsmodule::refcnt+*needsalreadybe>0toensurethatitisnotremovedatthisstage.+*AllotheruserneedtoinvokethisfunctionwithinaRCUread+*section.*/-returne;+returnsearch_extable(mod->extable,mod->num_exentries,addr);}/**
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:21
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/main.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:21
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with
RCU.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/kallsyms.h | 3 +--
kernel/module/kallsyms.c | 5 +----
kernel/module/main.c | 9 ++-------
3 files changed, 4 insertions(+), 13 deletions(-)
@@ -330,7 +330,6 @@ int module_address_lookup(unsigned long addr,structmodule*mod;guard(rcu)();-preempt_disable();mod=__module_address(addr);if(mod){if(modname)
@@ -348,8 +347,6 @@ int module_address_lookup(unsigned long addr,if(sym)ret=strscpy(namebuf,sym,KSYM_NAME_LEN);}-preempt_enable();-returnret;}
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:22
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-trace-kernel@vger.kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/arm64/kernel/ftrace.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:22
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/arm/kernel/module-plts.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:22
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with
RCU.
Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: WANG Xuerui <kernel@xen0n.name>
Cc: loongarch@lists.linux.dev
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/loongarch/kernel/unwind_orc.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:23
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: WANG Xuerui <kernel@xen0n.name>
Cc: linux-trace-kernel@vger.kernel.org
Cc: loongarch@lists.linux.dev
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/loongarch/kernel/ftrace_dyn.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:23
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: Christophe Leroy <redacted>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-trace-kernel@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/powerpc/kernel/trace/ftrace.c | 6 ++----
arch/powerpc/kernel/trace/ftrace_64_pg.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
@@ -115,10 +115,8 @@ static unsigned long ftrace_lookup_module_stub(unsigned long ip, unsigned long a{structmodule*mod=NULL;-preempt_disable();-mod=__module_text_address(ip);-preempt_enable();-+scoped_guard(rcu)+mod=__module_text_address(ip);if(!mod)pr_err("No module loaded at addr=%lx\n",ip);
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:24
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
The _notrace() variant was introduced in commit 14c4c8e41511a ("cfi: Use
rcu_read_{un}lock_sched_notrace"). The recursive case where
__cfi_slowpath_diag() could end up calling itself is no longer present,
as all that logic is gone since commit 89245600941e ("cfi: Switch to
-fsanitize=kcfi").
Sami Tolvanen said that KCFI checks don't perform function calls.
Elliot Berman verified it with
| modprobe -a dummy_stm stm_ftrace stm_p_basic
| mkdir -p /sys/kernel/config/stp-policy/dummy_stm.0.my-policy/default
| echo function > /sys/kernel/tracing/current_tracer
| echo 1 > /sys/kernel/tracing/tracing_on
| echo dummy_stm.0 > /sys/class/stm_source/ftrace/stm_source_link
Replace the rcu_read_lock_sched_notrace() section around
__module_address() with RCU.
Cc: Elliot Berman <redacted>
Cc: Kees Cook <kees@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: llvm@lists.linux.dev
Tested-by: Elliot Berman <redacted> # sm8650-qrd
Link: https://lore.kernel.org/all/20241230185812429-0800.eberman@hu-eberman-lv.qualcomm.com
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/cfi.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:24
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with RCU.
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Jason Baron <jbaron@akamai.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/jump_label.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
@@ -746,9 +746,9 @@ static int jump_label_add_module(struct module *mod)kfree(jlm);return-ENOMEM;}-preempt_disable();-jlm2->mod=__module_address((unsignedlong)key);-preempt_enable();+scoped_guard(rcu)+jlm2->mod=__module_address((unsignedlong)key);+jlm2->entries=static_key_entries(key);jlm2->next=NULL;static_key_set_mod(key,jlm2);
@@ -906,13 +906,13 @@ static void jump_label_update(struct static_key *key)return;}-preempt_disable();-mod=__module_address((unsignedlong)key);-if(mod){-stop=mod->jump_entries+mod->num_jump_entries;-init=mod->state==MODULE_STATE_COMING;+scoped_guard(rcu){+mod=__module_address((unsignedlong)key);+if(mod){+stop=mod->jump_entries+mod->num_jump_entries;+init=mod->state==MODULE_STATE_COMING;+}}-preempt_enable();#endifentry=static_key_entries(key);/* if there are no users, entry can be NULL */
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:24
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with
RCU.
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
Cc: x86@kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/x86/kernel/callthunks.c | 3 +--
arch/x86/kernel/unwind_orc.c | 4 +---
2 files changed, 2 insertions(+), 5 deletions(-)
@@ -476,7 +476,7 @@ bool unwind_next_frame(struct unwind_state *state)returnfalse;/* Don't let modules unload while we're reading their ORC data. */-preempt_disable();+guard(rcu)();/* End-of-stack check for user tasks: */if(state->regs&&user_mode(state->regs))
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:25
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Jason Baron <jbaron@akamai.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/jump_label.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:26
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/static_call_inline.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:26
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with
RCU.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <redacted>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Matt Bobrowski <redacted>
Cc: Song Liu <song@kernel.org>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: bpf@vger.kernel.org
Cc: linux-trace-kernel@vger.kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/trace/bpf_trace.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
@@ -2907,16 +2906,14 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3for(i=0;i<addrs_cnt;i++){structmodule*mod;-preempt_disable();-mod=__module_address(addrs[i]);-/* Either no module or we it's already stored */-if(!mod||has_module(&arr,mod)){-preempt_enable();-continue;+scoped_guard(rcu){+mod=__module_address(addrs[i]);+/* Either no module or we it's already stored */+if(!mod||has_module(&arr,mod))+continue;+if(!try_module_get(mod))+err=-EINVAL;}-if(!try_module_get(mod))-err=-EINVAL;-preempt_enable();if(err)break;err=add_module(&arr,mod);
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:26
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: David S. Miller <davem@davemloft.net>
Cc: Anil S Keshavamurthy <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: linux-trace-kernel@vger.kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/kprobes.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -1566,7 +1566,7 @@ static int check_kprobe_address_safe(struct kprobe *p,if(ret)returnret;jump_label_lock();-preempt_disable();+rcu_read_lock();/* Ensure the address is in a text area, and find a module if exists. */*probed_mod=NULL;
@@ -1612,7 +1612,7 @@ static int check_kprobe_address_safe(struct kprobe *p,}out:-preempt_enable();+rcu_read_unlock();jump_label_unlock();returnret;
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-08 09:05:27
The list module_bug_list relies on module_mutex for writer
synchronisation. The list is already RCU style.
The list removal is synchronized with modules' synchronize_rcu() in
free_module().
Use RCU read lock protection instead of RCU-sched.
Cc: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
lib/bug.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
dereference_symbol_descriptor() needs to obtain the module pointer
belonging to pointer in order to resolve that pointer.
The returned mod pointer is obtained under RCU-sched/ preempt_disable()
guarantees and needs to be used within this section to ensure that the
module is not removed in the meantime.
Extend the preempt_disable() section to also cover
dereference_module_function_descriptor().
Fixes: 04b8eb7a4ccd9 ("symbol lookup: introduce dereference_symbol_descriptor()")
Cc: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Christophe Leroy <redacted>
Cc: Helge Deller <deller@gmx.de>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Sergey Senozhatsky <redacted>
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Nice catch.
Acked-by: Helge Deller <deller@gmx.de>
This patch really should be backported.
Can you add a Cc: stable tag?
Helge
On Wed, Jan 8, 2025 at 1:05 AM Sebastian Andrzej Siewior
[off-list ref] wrote:
quoted hunk
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with
RCU.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <redacted>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Matt Bobrowski <redacted>
Cc: Song Liu <song@kernel.org>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: bpf@vger.kernel.org
Cc: linux-trace-kernel@vger.kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/trace/bpf_trace.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
@@ -2907,16 +2906,14 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3for(i=0;i<addrs_cnt;i++){structmodule*mod;-preempt_disable();-mod=__module_address(addrs[i]);-/* Either no module or we it's already stored */-if(!mod||has_module(&arr,mod)){-preempt_enable();-continue;+scoped_guard(rcu){+mod=__module_address(addrs[i]);+/* Either no module or we it's already stored */+if(!mod||has_module(&arr,mod))+continue;+if(!try_module_get(mod))+err=-EINVAL;
lgtm.
Should we take into bpf-next or the whole set is handled together
somewhere?
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-09 20:54:44
On 2025-01-09 10:38:03 [-0800], Alexei Starovoitov wrote:
lgtm.
Should we take into bpf-next or the whole set is handled together
somewhere?
If you don't mind, I would hope to route the whole series via the
modules tree. Some of the lower functions (__module_address()) check for
disabled preemption and will trigger warnings at runtime if this gets
applied before (earlier in the series) the check gets replaced.
Sebastian
On Thu, Jan 9, 2025 at 12:54 PM Sebastian Andrzej Siewior
[off-list ref] wrote:
On 2025-01-09 10:38:03 [-0800], Alexei Starovoitov wrote:
quoted
lgtm.
Should we take into bpf-next or the whole set is handled together
somewhere?
If you don't mind, I would hope to route the whole series via the
modules tree. Some of the lower functions (__module_address()) check for
disabled preemption and will trigger warnings at runtime if this gets
applied before (earlier in the series) the check gets replaced.
I see. Then
Acked-by: Alexei Starovoitov <ast@kernel.org>
From: Petr Pavlu <petr.pavlu@suse.com> Date: 2025-01-13 11:09:29
On 1/8/25 10:04, Sebastian Andrzej Siewior wrote:
This is an updated version of the initial post after PeterZ made me
aware that there are users outside of the module directory.
The goal is replace the mix auf rcu_read_lock(), rcu_read_lock_sched()
and preempt_disable() with just rcu_read_lock().
Thanks for this cleanup. I've queued the fix in patch #1 on
modules-fixes. For the rest, I plan to give folks more time to look at
the changes as this affects a number of subsystems. If there are no
other concerns, I'd then add the series on modules-next.
--
Cheers,
Petr
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-13 13:24:01
On 2025-01-13 12:09:27 [+0100], Petr Pavlu wrote:
Thanks for this cleanup. I've queued the fix in patch #1 on
modules-fixes. For the rest, I plan to give folks more time to look at
the changes as this affects a number of subsystems. If there are no
other concerns, I'd then add the series on modules-next.
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: Christophe Leroy <redacted>
Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-trace-kernel@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/powerpc/kernel/trace/ftrace.c | 6 ++----
arch/powerpc/kernel/trace/ftrace_64_pg.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
Ran ftrace (function graph) on preempt=full kernel with rcutorture while doing modprobe/rmmod.
rcutorture succeeded and didn't see any splats.
If there is any other method to test it out, please let me know.
So for powerpc bits:
Tested-by: Shrikanth Hegde <redacted>
@@ -115,10 +115,8 @@ static unsigned long ftrace_lookup_module_stub(unsigned long ip, unsigned long a{structmodule*mod=NULL;-preempt_disable();-mod=__module_text_address(ip);-preempt_enable();-+scoped_guard(rcu)+mod=__module_text_address(ip);if(!mod)pr_err("No module loaded at addr=%lx\n",ip);
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-24 17:49:52
On 2025-01-13 12:09:27 [+0100], Petr Pavlu wrote:
Thanks for this cleanup. I've queued the fix in patch #1 on
modules-fixes. For the rest, I plan to give folks more time to look at
the changes as this affects a number of subsystems. If there are no
other concerns, I'd then add the series on modules-next.
#26 (kprobes) clashes with the changes that have been merged upstream.
Do you want me to resend the whole series or just #26? The other patches
apply cleanly so far.
Sebastian
From: Petr Pavlu <petr.pavlu@suse.com> Date: 2025-01-27 12:22:19
On 1/24/25 18:49, Sebastian Andrzej Siewior wrote:
On 2025-01-13 12:09:27 [+0100], Petr Pavlu wrote:
quoted
Thanks for this cleanup. I've queued the fix in patch #1 on
modules-fixes. For the rest, I plan to give folks more time to look at
the changes as this affects a number of subsystems. If there are no
other concerns, I'd then add the series on modules-next.
#26 (kprobes) clashes with the changes that have been merged upstream.
Do you want me to resend the whole series or just #26? The other patches
apply cleanly so far.
I think sending only the updated patch #26 should be sufficient in this
case, it's only a small adjustment. Please preferably post it as a reply
to the email with that specific patch.
--
Thanks,
Petr
On Wed, 8 Jan 2025 10:04:55 +0100
Sebastian Andrzej Siewior [off-list ref] wrote:
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
@@ -1566,7 +1566,7 @@ static int check_kprobe_address_safe(struct kprobe *p,if(ret)returnret;jump_label_lock();-preempt_disable();+rcu_read_lock();/* Ensure the address is in a text area, and find a module if exists. */*probed_mod=NULL;
@@ -1612,7 +1612,7 @@ static int check_kprobe_address_safe(struct kprobe *p,}out:-preempt_enable();+rcu_read_unlock();jump_label_unlock();returnret;
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-29 08:47:54
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with
RCU.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <redacted>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Matt Bobrowski <redacted>
Cc: Song Liu <song@kernel.org>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: bpf@vger.kernel.org
Cc: linux-trace-kernel@vger.kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
The previous version was broken in terms that the break statement broke
out of the scoped_guard loop and added something to the list. This is
now fixed by adding the "skip_add" bool.
While at it, I updated the comment by removing the "we".
kernel/trace/bpf_trace.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -2932,18 +2931,21 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3u32i,err=0;for(i=0;i<addrs_cnt;i++){+boolskip_add=false;structmodule*mod;-preempt_disable();-mod=__module_address(addrs[i]);-/* Either no module or we it's already stored */-if(!mod||has_module(&arr,mod)){-preempt_enable();-continue;+scoped_guard(rcu){+mod=__module_address(addrs[i]);+/* Either no module or it's already stored */+if(!mod||has_module(&arr,mod)){+skip_add=true;+break;/* scoped_guard */+}+if(!try_module_get(mod))+err=-EINVAL;}-if(!try_module_get(mod))-err=-EINVAL;-preempt_enable();+if(skip_add)+continue;if(err)break;err=add_module(&arr,mod);
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-29 08:49:28
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Cc: David S. Miller <davem@davemloft.net>
Cc: Anil S Keshavamurthy <redacted>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: linux-trace-kernel@vger.kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Update due to the collision during the merge window.
kernel/kprobes.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -1547,7 +1547,7 @@ static int check_kprobe_address_safe(struct kprobe *p,/* Ensure the address is in a text area, and find a module if exists. */*probed_mod=NULL;if(!core_kernel_text((unsignedlong)p->addr)){-guard(preempt)();+guard(rcu)();*probed_mod=__module_text_address((unsignedlong)p->addr);if(!(*probed_mod))return-EINVAL;
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-01-29 08:52:23
On 2025-01-27 13:22:17 [+0100], Petr Pavlu wrote:
On 1/24/25 18:49, Sebastian Andrzej Siewior wrote:
quoted
On 2025-01-13 12:09:27 [+0100], Petr Pavlu wrote:
quoted
Thanks for this cleanup. I've queued the fix in patch #1 on
modules-fixes. For the rest, I plan to give folks more time to look at
the changes as this affects a number of subsystems. If there are no
other concerns, I'd then add the series on modules-next.
#26 (kprobes) clashes with the changes that have been merged upstream.
Do you want me to resend the whole series or just #26? The other patches
apply cleanly so far.
I think sending only the updated patch #26 should be sufficient in this
case, it's only a small adjustment. Please preferably post it as a reply
to the email with that specific patch.
I just sent two updates:
[PATCH v3.5 25/28] bpf: Use RCU in all users of __module_text_address().
[PATCH v3.5 26/28] kprobes: Use RCU in all users of __module_text_address().
Sebastian
From: Petr Pavlu <petr.pavlu@suse.com> Date: 2025-01-30 13:42:03
On 1/29/25 09:52, Sebastian Andrzej Siewior wrote:
On 2025-01-27 13:22:17 [+0100], Petr Pavlu wrote:
quoted
On 1/24/25 18:49, Sebastian Andrzej Siewior wrote:
quoted
On 2025-01-13 12:09:27 [+0100], Petr Pavlu wrote:
quoted
Thanks for this cleanup. I've queued the fix in patch #1 on
modules-fixes. For the rest, I plan to give folks more time to look at
the changes as this affects a number of subsystems. If there are no
other concerns, I'd then add the series on modules-next.
#26 (kprobes) clashes with the changes that have been merged upstream.
Do you want me to resend the whole series or just #26? The other patches
apply cleanly so far.
I think sending only the updated patch #26 should be sufficient in this
case, it's only a small adjustment. Please preferably post it as a reply
to the email with that specific patch.
I just sent two updates:
[PATCH v3.5 25/28] bpf: Use RCU in all users of __module_text_address().
[PATCH v3.5 26/28] kprobes: Use RCU in all users of __module_text_address().
I've now queued the series and its two updated patches #25 and #26 on
modules-next (for 6.15-rc1).
--
Thanks,
Petr
From: Benjamin Berg <hidden> Date: 2025-04-23 15:17:37
Hi,
On Wed, 2025-01-08 at 10:04 +0100, Sebastian Andrzej Siewior wrote:
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Unfortunately, this patch causes a performance regression for us. The
trouble is that we enable kmemleak and run trace-cmd so a lot of stack
traces need to be collected. Obviously, we also have lockdep enabled.
Now, combine this with the UML stack dumping code calling into
__kernel_text_address a lot[1] and it really has a relevant performance
impact. I saw the kernel spending 40% of its own CPU time just on the
lock in is_module_text_address.
Maybe kernel_text_address should leave the RCU handling to the caller
and assume that the RCU read lock is already taken?
Benjamin
[1] The UM arch dump_stack function reads every "unsigned long" on the
stack and tests it using __kernel_text_address.
quoted hunk
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/main.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
/*
* Even though we hold a reference on the module; we still
need to
- * disable preemption in order to safely traverse the data
structure.
+ * RCU read section in order to safely traverse the data
structure.
*/
- preempt_disable();
+ guard(rcu)();
modaddr = __module_text_address(a);
BUG_ON(!modaddr);
module_put(modaddr);
- preempt_enable();
}
EXPORT_SYMBOL_GPL(symbol_put_addr);
@@ -3694,20 +3693,15 @@ struct module *__module_address(unsigned long
addr)
*/
bool is_module_text_address(unsigned long addr)
{
- bool ret;
-
- preempt_disable();
- ret = __module_text_address(addr) != NULL;
- preempt_enable();
-
- return ret;
+ guard(rcu)();
+ return __module_text_address(addr) != NULL;
}
/**
* __module_text_address() - get the module whose code contains an
address.
* @addr: the address.
*
- * Must be called with preempt disabled or module mutex held so that
+ * Must be called within RCU read section or module mutex held so
that
* module doesn't get freed during this.
*/
struct module *__module_text_address(unsigned long addr)
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2025-04-23 18:16:50
On Wed, Apr 23, 2025 at 05:17:31PM +0200, Benjamin Berg wrote:
Hi,
On Wed, 2025-01-08 at 10:04 +0100, Sebastian Andrzej Siewior wrote:
quoted
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Unfortunately, this patch causes a performance regression for us. The
trouble is that we enable kmemleak and run trace-cmd so a lot of stack
traces need to be collected. Obviously, we also have lockdep enabled.
Now, combine this with the UML stack dumping code calling into
__kernel_text_address a lot[1] and it really has a relevant performance
impact. I saw the kernel spending 40% of its own CPU time just on the
lock in is_module_text_address.
Maybe kernel_text_address should leave the RCU handling to the caller
and assume that the RCU read lock is already taken?
Benjamin
[1] The UM arch dump_stack function reads every "unsigned long" on the
stack and tests it using __kernel_text_address.
Use of a single guard(rcu)() is regressing performance? Interesting and
quite unexpected. That said, tiven the amount of debug you have enabled,
I am not so sure that people are going to be all that excited about a
further performance regression.
But is this regression due to the cleanup hook that guard(rcu)()
registers? If so, please feel free to try using rcu_read_lock()
and rcu_read_unlock() instead. I would be surprised if this makes a
difference, but then again, your initial regression report also comes
as a surprise, so...
Another way to reduce guard(rcu)() overhead is to build your kernel
with CONFIG_PREEMPT_NONE=y. Not so good for real-time response, but
then again, neither are your debug options.
Thanx, Paul
quoted
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
kernel/module/main.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
/*
* Even though we hold a reference on the module; we still
need to
- * disable preemption in order to safely traverse the data
structure.
+ * RCU read section in order to safely traverse the data
structure.
*/
- preempt_disable();
+ guard(rcu)();
modaddr = __module_text_address(a);
BUG_ON(!modaddr);
module_put(modaddr);
- preempt_enable();
}
EXPORT_SYMBOL_GPL(symbol_put_addr);
@@ -3694,20 +3693,15 @@ struct module *__module_address(unsigned long
addr)
*/
bool is_module_text_address(unsigned long addr)
{
- bool ret;
-
- preempt_disable();
- ret = __module_text_address(addr) != NULL;
- preempt_enable();
-
- return ret;
+ guard(rcu)();
+ return __module_text_address(addr) != NULL;
}
/**
* __module_text_address() - get the module whose code contains an
address.
* @addr: the address.
*
- * Must be called with preempt disabled or module mutex held so that
+ * Must be called within RCU read section or module mutex held so
that
* module doesn't get freed during this.
*/
struct module *__module_text_address(unsigned long addr)
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-04-24 09:05:43
On 2025-04-23 11:16:49 [-0700], Paul E. McKenney wrote:
On Wed, Apr 23, 2025 at 05:17:31PM +0200, Benjamin Berg wrote:
quoted
Hi,
On Wed, 2025-01-08 at 10:04 +0100, Sebastian Andrzej Siewior wrote:
quoted
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Unfortunately, this patch causes a performance regression for us. The
trouble is that we enable kmemleak and run trace-cmd so a lot of stack
traces need to be collected. Obviously, we also have lockdep enabled.
Now, combine this with the UML stack dumping code calling into
__kernel_text_address a lot[1] and it really has a relevant performance
impact. I saw the kernel spending 40% of its own CPU time just on the
lock in is_module_text_address.
Maybe kernel_text_address should leave the RCU handling to the caller
and assume that the RCU read lock is already taken?
Benjamin
[1] The UM arch dump_stack function reads every "unsigned long" on the
stack and tests it using __kernel_text_address.
Use of a single guard(rcu)() is regressing performance? Interesting and
quite unexpected. That said, tiven the amount of debug you have enabled,
I am not so sure that people are going to be all that excited about a
further performance regression.
But is this regression due to the cleanup hook that guard(rcu)()
registers? If so, please feel free to try using rcu_read_lock()
and rcu_read_unlock() instead. I would be surprised if this makes a
difference, but then again, your initial regression report also comes
as a surprise, so...
Another way to reduce guard(rcu)() overhead is to build your kernel
with CONFIG_PREEMPT_NONE=y. Not so good for real-time response, but
then again, neither are your debug options.
The guard notation is not regression I guess it is just the plenty of
rcu_read_lock()/ unlock(). We had one regression which was "fixed" by
commit ee57ab5a32129 ("locking/lockdep: Disable KASAN instrumentation of lockdep.c").
My guess would be that this is a preemptible kernel and the preempt
disable/ enable is cheaper that the RCU version. So going back to a
non-preemtible kernel should "fix" it.
Looking at kernel_text_address(), is_bpf_text_address() has also a
RCU read section so probably subject to the same trouble. And
is_ftrace_trampoline() could be also converted to RCU which would
increase the trouble.
Improving the stack trace on UM or caching some of the most common one
might help. Not sure if disabling kmemleak for lockdep is possible/
makes a difference.
From: Benjamin Berg <hidden> Date: 2025-04-24 09:30:45
On Thu, 2025-04-24 at 11:05 +0200, Sebastian Andrzej Siewior wrote:
On 2025-04-23 11:16:49 [-0700], Paul E. McKenney wrote:
quoted
On Wed, Apr 23, 2025 at 05:17:31PM +0200, Benjamin Berg wrote:
quoted
Hi,
On Wed, 2025-01-08 at 10:04 +0100, Sebastian Andrzej Siewior wrote:
quoted
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Unfortunately, this patch causes a performance regression for us. The
trouble is that we enable kmemleak and run trace-cmd so a lot of stack
traces need to be collected. Obviously, we also have lockdep enabled.
Now, combine this with the UML stack dumping code calling into
__kernel_text_address a lot[1] and it really has a relevant performance
impact. I saw the kernel spending 40% of its own CPU time just on the
lock in is_module_text_address.
Maybe kernel_text_address should leave the RCU handling to the caller
and assume that the RCU read lock is already taken?
Benjamin
[1] The UM arch dump_stack function reads every "unsigned long" on the
stack and tests it using __kernel_text_address.
Use of a single guard(rcu)() is regressing performance? Interesting and
quite unexpected. That said, tiven the amount of debug you have enabled,
I am not so sure that people are going to be all that excited about a
further performance regression.
But is this regression due to the cleanup hook that guard(rcu)()
registers? If so, please feel free to try using rcu_read_lock()
and rcu_read_unlock() instead. I would be surprised if this makes a
difference, but then again, your initial regression report also comes
as a surprise, so...
Another way to reduce guard(rcu)() overhead is to build your kernel
with CONFIG_PREEMPT_NONE=y. Not so good for real-time response, but
then again, neither are your debug options.
The guard notation is not regression I guess it is just the plenty of
rcu_read_lock()/ unlock(). We had one regression which was "fixed" by
commit ee57ab5a32129 ("locking/lockdep: Disable KASAN instrumentation of lockdep.c").
Yup, we really pretty much created a micro-benchmark for grabbing stack
traces.
My guess would be that this is a preemptible kernel and the preempt
disable/ enable is cheaper that the RCU version. So going back to a
non-preemtible kernel should "fix" it.
Yes, preempt_disable() is extremely cheap.
Looking at kernel_text_address(), is_bpf_text_address() has also a
RCU read section so probably subject to the same trouble. And
is_ftrace_trampoline() could be also converted to RCU which would
increase the trouble.
Improving the stack trace on UM or caching some of the most common one
might help. Not sure if disabling kmemleak for lockdep is possible/
makes a difference.
What does seem to help is to simply disable lockdep inside dump_trace.
That should be good enough for us at least, bringing the overhead down
to a manageable amount when running these tests.
Some unscientific numbers:
config dump_trace locking
----
no locking (preempt_disable) 6 % -
guard(rcu)() + lockdep_off 15 % 58 % of that
rcu_read_lock + lockdep_off 17 % 60 % of that
guard(rcu)() 48 % 91 % of that
That confirms that guard(rcu)() really is not a problem. There might be
slight overhead, but it is probably within the margin of error. Turning
lockdep off/on inside the UML dump_trace() function brings down the
overhead a lot and I guess that should be an acceptable level for us.
Not sure if something like that would be desirable upstream. This is
happening for us when running the hostap "hwsim" tests inside UML (with
time-travel). At least internally, we could carry a custom patch to add
the lockdep_off()/lockdep_on() to dump_trace in order to work around
it[1].
Benjamin
[1] Actually, now I am reminded that we already have that for kmemleak
as lockdep was considerably slowing down the scanning.
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2025-04-24 14:47:39
On Thu, Apr 24, 2025 at 11:30:39AM +0200, Benjamin Berg wrote:
On Thu, 2025-04-24 at 11:05 +0200, Sebastian Andrzej Siewior wrote:
quoted
On 2025-04-23 11:16:49 [-0700], Paul E. McKenney wrote:
quoted
On Wed, Apr 23, 2025 at 05:17:31PM +0200, Benjamin Berg wrote:
quoted
Hi,
On Wed, 2025-01-08 at 10:04 +0100, Sebastian Andrzej Siewior wrote:
quoted
__module_text_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_text_address()
with RCU.
Unfortunately, this patch causes a performance regression for us. The
trouble is that we enable kmemleak and run trace-cmd so a lot of stack
traces need to be collected. Obviously, we also have lockdep enabled.
Now, combine this with the UML stack dumping code calling into
__kernel_text_address a lot[1] and it really has a relevant performance
impact. I saw the kernel spending 40% of its own CPU time just on the
lock in is_module_text_address.
Maybe kernel_text_address should leave the RCU handling to the caller
and assume that the RCU read lock is already taken?
Benjamin
[1] The UM arch dump_stack function reads every "unsigned long" on the
stack and tests it using __kernel_text_address.
Use of a single guard(rcu)() is regressing performance? Interesting and
quite unexpected. That said, tiven the amount of debug you have enabled,
I am not so sure that people are going to be all that excited about a
further performance regression.
But is this regression due to the cleanup hook that guard(rcu)()
registers? If so, please feel free to try using rcu_read_lock()
and rcu_read_unlock() instead. I would be surprised if this makes a
difference, but then again, your initial regression report also comes
as a surprise, so...
Another way to reduce guard(rcu)() overhead is to build your kernel
with CONFIG_PREEMPT_NONE=y. Not so good for real-time response, but
then again, neither are your debug options.
The guard notation is not regression I guess it is just the plenty of
rcu_read_lock()/ unlock(). We had one regression which was "fixed" by
commit ee57ab5a32129 ("locking/lockdep: Disable KASAN instrumentation of lockdep.c").
Yup, we really pretty much created a micro-benchmark for grabbing stack
traces.
quoted
My guess would be that this is a preemptible kernel and the preempt
disable/ enable is cheaper that the RCU version. So going back to a
non-preemtible kernel should "fix" it.
Yes, preempt_disable() is extremely cheap.
quoted
Looking at kernel_text_address(), is_bpf_text_address() has also a
RCU read section so probably subject to the same trouble. And
is_ftrace_trampoline() could be also converted to RCU which would
increase the trouble.
Improving the stack trace on UM or caching some of the most common one
might help. Not sure if disabling kmemleak for lockdep is possible/
makes a difference.
What does seem to help is to simply disable lockdep inside dump_trace.
That should be good enough for us at least, bringing the overhead down
to a manageable amount when running these tests.
Some unscientific numbers:
config dump_trace locking
----
no locking (preempt_disable) 6 % -
guard(rcu)() + lockdep_off 15 % 58 % of that
rcu_read_lock + lockdep_off 17 % 60 % of that
guard(rcu)() 48 % 91 % of that
That confirms that guard(rcu)() really is not a problem. There might be
slight overhead, but it is probably within the margin of error. Turning
lockdep off/on inside the UML dump_trace() function brings down the
overhead a lot and I guess that should be an acceptable level for us.
Whew!!! ;-)
Not sure if something like that would be desirable upstream. This is
happening for us when running the hostap "hwsim" tests inside UML (with
time-travel). At least internally, we could carry a custom patch to add
the lockdep_off()/lockdep_on() to dump_trace in order to work around
it[1].
That makes sense to me, but I am not the maintainer of that code. ;-)
Thanx, Paul
Benjamin
[1] Actually, now I am reminded that we already have that for kmemleak
as lockdep was considerably slowing down the scanning.
From: Peter Zijlstra <peterz@infradead.org> Date: 2025-04-24 15:17:21
On Thu, Apr 24, 2025 at 11:30:39AM +0200, Benjamin Berg wrote:
Not sure if something like that would be desirable upstream. This is
happening for us when running the hostap "hwsim" tests inside UML (with
time-travel). At least internally, we could carry a custom patch to add
the lockdep_off()/lockdep_on() to dump_trace in order to work around
it[1].
Urgh, so lockdep_off() usage is a really bad sign.
Having just done a git-grep, I see there's crap to clean out :-(
Some day I'll manage to remove that thing.
From: Michal Pecio <hidden> Date: 2025-11-03 10:08:47
quoted hunk
x86: Use RCU in all users of __module_address().
__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.
Replace the preempt_disable() section around __module_address() with
RCU.
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
Cc: x86@kernel.org
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
arch/x86/kernel/callthunks.c | 3 +--
arch/x86/kernel/unwind_orc.c | 4 +---
2 files changed, 2 insertions(+), 5 deletions(-)
@@ -476,7 +476,7 @@ bool unwind_next_frame(struct unwind_state *state)returnfalse;/* Don't let modules unload while we're reading their ORC data. */-preempt_disable();+guard(rcu)();/* End-of-stack check for user tasks: */if(state->regs&&user_mode(state->regs))
Hi,
There is a regression report on a distribution forum which involves
an out of tree module on a patched kernel (yes, I know) calling
stack_trace_save() in task context, which arrives here and apparently
calls the various deref_stack_xxx() functions with preemption enabled,
which in turn call stack_access_ok() leading to a BUG:
Nov 02 21:44:30 ArchBasement kernel: BUG: using smp_processor_id() in preemptible [00000000] code: Xorg/1183
Nov 02 21:44:30 ArchBasement kernel: caller is in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel: CPU: 0 UID: 1000 PID: 1183 Comm: Xorg Tainted: P OE 6.16.12-hardened1-1-hardened #1 PREEMPT(full) 6edb90a7a07fab33bbee72d6d5ef53ba6eec3b9c
Nov 02 21:44:30 ArchBasement kernel: Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Nov 02 21:44:30 ArchBasement kernel: Hardware name: ASUS All Series/Z97-E, BIOS 0803 02/23/2016
Nov 02 21:44:30 ArchBasement kernel: Call Trace:
Nov 02 21:44:30 ArchBasement kernel: <TASK>
Nov 02 21:44:30 ArchBasement kernel: dump_stack_lvl+0x5d/0x80
Nov 02 21:44:30 ArchBasement kernel: check_preemption_disabled+0xe5/0xf0
Nov 02 21:44:30 ArchBasement kernel: in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel: get_stack_info+0x2c/0x80
Nov 02 21:44:30 ArchBasement kernel: stack_access_ok+0x51/0xa0
Nov 02 21:44:30 ArchBasement kernel: unwind_next_frame+0x1cb/0x7b0
Nov 02 21:44:30 ArchBasement kernel: ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel: ? __pfx_stack_trace_consume_entry+0x10/0x10
Nov 02 21:44:30 ArchBasement kernel: arch_stack_walk+0xa6/0x110
Nov 02 21:44:30 ArchBasement kernel: ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel: stack_trace_save+0x4d/0x70
Is this nvidia doing something wrong, or a problem with this commit?
The removed code suggests that preemption is allowed here, and as far
as I see, this call trace is still possible on vanilla 6.18. Perhaps
preempt_disable() needs to be restored around this code?
Regards,
Michal
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: 2025-11-03 10:34:38
On 2025-11-03 11:08:35 [+0100], Michal Pecio wrote:
Hi,
Hi,
There is a regression report on a distribution forum which involves
an out of tree module on a patched kernel (yes, I know) calling
stack_trace_save() in task context, which arrives here and apparently
calls the various deref_stack_xxx() functions with preemption enabled,
which in turn call stack_access_ok() leading to a BUG:
Nov 02 21:44:30 ArchBasement kernel: BUG: using smp_processor_id() in preemptible [00000000] code: Xorg/1183
Nov 02 21:44:30 ArchBasement kernel: caller is in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel: CPU: 0 UID: 1000 PID: 1183 Comm: Xorg Tainted: P OE 6.16.12-hardened1-1-hardened #1 PREEMPT(full) 6edb90a7a07fab33bbee72d6d5ef53ba6eec3b9c
Nov 02 21:44:30 ArchBasement kernel: Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Nov 02 21:44:30 ArchBasement kernel: Hardware name: ASUS All Series/Z97-E, BIOS 0803 02/23/2016
Nov 02 21:44:30 ArchBasement kernel: Call Trace:
Nov 02 21:44:30 ArchBasement kernel: <TASK>
Nov 02 21:44:30 ArchBasement kernel: dump_stack_lvl+0x5d/0x80
Nov 02 21:44:30 ArchBasement kernel: check_preemption_disabled+0xe5/0xf0
Nov 02 21:44:30 ArchBasement kernel: in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel: get_stack_info+0x2c/0x80
Nov 02 21:44:30 ArchBasement kernel: stack_access_ok+0x51/0xa0
Nov 02 21:44:30 ArchBasement kernel: unwind_next_frame+0x1cb/0x7b0
Nov 02 21:44:30 ArchBasement kernel: ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel: ? __pfx_stack_trace_consume_entry+0x10/0x10
Nov 02 21:44:30 ArchBasement kernel: arch_stack_walk+0xa6/0x110
Nov 02 21:44:30 ArchBasement kernel: ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel: stack_trace_save+0x4d/0x70
Is this nvidia doing something wrong, or a problem with this commit?
The removed code suggests that preemption is allowed here, and as far
as I see, this call trace is still possible on vanilla 6.18. Perhaps
preempt_disable() needs to be restored around this code?
Do you have the complete backtrace? Is this SMP or UP build?
From: Michal Pecio <hidden> Date: 2025-11-03 10:39:15
On Mon, 3 Nov 2025 11:34:34 +0100, Sebastian Andrzej Siewior wrote:
On 2025-11-03 11:08:35 [+0100], Michal Pecio wrote:
quoted
Hi,
Hi,
quoted
There is a regression report on a distribution forum which involves
an out of tree module on a patched kernel (yes, I know) calling
stack_trace_save() in task context, which arrives here and apparently
calls the various deref_stack_xxx() functions with preemption enabled,
which in turn call stack_access_ok() leading to a BUG:
Nov 02 21:44:30 ArchBasement kernel: BUG: using smp_processor_id() in preemptible [00000000] code: Xorg/1183
Nov 02 21:44:30 ArchBasement kernel: caller is in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel: CPU: 0 UID: 1000 PID: 1183 Comm: Xorg Tainted: P OE 6.16.12-hardened1-1-hardened #1 PREEMPT(full) 6edb90a7a07fab33bbee72d6d5ef53ba6eec3b9c
Nov 02 21:44:30 ArchBasement kernel: Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Nov 02 21:44:30 ArchBasement kernel: Hardware name: ASUS All Series/Z97-E, BIOS 0803 02/23/2016
Nov 02 21:44:30 ArchBasement kernel: Call Trace:
Nov 02 21:44:30 ArchBasement kernel: <TASK>
Nov 02 21:44:30 ArchBasement kernel: dump_stack_lvl+0x5d/0x80
Nov 02 21:44:30 ArchBasement kernel: check_preemption_disabled+0xe5/0xf0
Nov 02 21:44:30 ArchBasement kernel: in_entry_stack+0x11/0x60
Nov 02 21:44:30 ArchBasement kernel: get_stack_info+0x2c/0x80
Nov 02 21:44:30 ArchBasement kernel: stack_access_ok+0x51/0xa0
Nov 02 21:44:30 ArchBasement kernel: unwind_next_frame+0x1cb/0x7b0
Nov 02 21:44:30 ArchBasement kernel: ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel: ? __pfx_stack_trace_consume_entry+0x10/0x10
Nov 02 21:44:30 ArchBasement kernel: arch_stack_walk+0xa6/0x110
Nov 02 21:44:30 ArchBasement kernel: ? _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
Nov 02 21:44:30 ArchBasement kernel: stack_trace_save+0x4d/0x70
Is this nvidia doing something wrong, or a problem with this commit?
The removed code suggests that preemption is allowed here, and as far
as I see, this call trace is still possible on vanilla 6.18. Perhaps
preempt_disable() needs to be restored around this code?
Do you have the complete backtrace? Is this SMP or UP build?
The stack trace is a bit odd. The compressed version is:
| BUG: using smp_processor_id() in preemptible [00000000] code: Xorg/1183
| caller is in_entry_stack+0x11/0x60
| CPU: 3 UID: 1000 PID: 1183 Comm: Xorg Tainted: P OE 6.16.12-hardened1-1-hardened #1 PREEMPT(full) 6edb90a7a07fab33bbee72d6d5ef53ba6eec3b9c
| Tainted: [P]=PROPRIETARY_MODULE, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
| Hardware name: ASUS All Series/Z97-E, BIOS 0803 02/23/2016
| Call Trace:
| <TASK>
| dump_stack_lvl+0x5d/0x80
| check_preemption_disabled+0xe5/0xf0
| in_entry_stack+0x11/0x60
| get_stack_info+0x2c/0x80
| stack_access_ok+0x51/0xa0
| unwind_next_frame+0x1cb/0x7b0
| arch_stack_walk+0xa6/0x110
| stack_trace_save+0x4d/0x70
| __kfence_alloc+0xb7/0x6f0
| __kmalloc_noprof+0x520/0x560
| os_alloc_mem+0x108/0x120 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv015295rm+0x34/0x50 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv015297rm+0x2b/0xd0 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv016352rm+0x1c/0x90 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv059298rm+0x65/0xb0 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv054041rm+0x20f/0x360 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv056165rm+0x54/0xd0 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv056096rm+0xa0/0x500 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv015919rm+0x424/0x680 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv054015rm+0x69/0xd0 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv014185rm+0x86/0xa0 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| _nv000652rm+0x5e/0x70 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| rm_kernel_rmapi_op+0x167/0x273 [nvidia 9746d397d5c5bffeb186e829669bb24c0846a4a7]
| nvkms_call_rm+0x4c/0x80 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
| _nv003168kms+0x42/0x50 [nvidia_modeset 90775ea8a26c5e58b97ef4b3f46eb45efa040eb2]
| ? do_syscall_64+0x82/0x8d0
| ? entry_SYSCALL_64_after_hwframe+0x76/0x7e
| </TASK>
The last two entries start with a '?' which means it did not originate
from the "stack unwind" but was laying around while passing through.
I would expect the last two entries to be there without the '?' because
userland (as in X here) enters the kernel via a proper syscall entry
which should be part of the stack strace.
Now, get_stack_info() where the warning originates: It starts with a
check to see if the stack pointer belongs to the current task's stack
frame which it does not. Then it checks if the task found is the
currently running task. That it does. So in that case, we must be
serving an exception (such as an IRQ) because the stack does not belong
to the current task. However preemption is not disabled which indicates
that we do not do this.
This in turn suggests that nvidia replaced the stack from while entering
the syscall probably in _nv003168kms() or the binary blob which invokes
the kernel function does not have a proper ORC entry which leads to a
wrong turn in the process.
So the warning is well deserved.
Sebastian
From: Michal Pecio <hidden> Date: 2025-11-03 17:37:25
On Mon, 3 Nov 2025 12:37:50 +0100, Sebastian Andrzej Siewior wrote:
Now, get_stack_info() where the warning originates: It starts with a
check to see if the stack pointer belongs to the current task's stack
frame which it does not. Then it checks if the task found is the
currently running task. That it does. So in that case, we must be
serving an exception (such as an IRQ) because the stack does not
belong to the current task. However preemption is not disabled which
indicates that we do not do this.
This in turn suggests that nvidia replaced the stack from while
entering the syscall probably in _nv003168kms() or the binary blob
which invokes the kernel function does not have a proper ORC entry
which leads to a wrong turn in the process.
OK, I see, preemption should only be enabled in the first case, so
others are free to assume it's disabled. No bug.
Thank you.