[PATCH v2 00/28] module: Use RCU instead of RCU-sched.

[PATCH v2 00/28] module: Use RCU instead of RCU-sched.

[Sebastian Andrzej Siewior]

Hi,

This is an updated version of the inital 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 splitted it into smaller chunks which can be applied/ reviewed
independently. I'm just not sure about the cfi patch (28/28) so I added
just a comment instead.

v1…v2: https://lore.kernel.org/all/20241205215102.hRywUW2A@linutronix.de/
  - Splitted into smaller patches.
  - Converted all users.

Sebastian

[PATCH v2 01/28] module: Extend the preempt disabled section in dereference_symbol_descriptor().

[Sebastian Andrzej Siewior]

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 <christophe.leroy@csgroup.eu>
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 <sergey.senozhatsky.work@gmail.com>
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/kallsyms.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index c3f075e8f60cb..1c6a6c1704d8d 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -57,10 +57,10 @@ static inline void *dereference_symbol_descriptor(void *ptr)
 
 	preempt_disable();
 	mod = __module_address((unsigned long)ptr);
-	preempt_enable();
 
 	if (mod)
 		ptr = dereference_module_function_descriptor(mod, ptr);
+	preempt_enable();
 #endif
 	return ptr;
 }
-- 
2.45.2

Re: [PATCH v2 01/28] module: Extend the preempt disabled section in dereference_symbol_descriptor().

[Sergey Senozhatsky]

On (24/12/20 18:41), Sebastian Andrzej Siewior wrote:
> 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 <christophe.leroy@csgroup.eu>
> 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 <sergey.senozhatsky.work@gmail.com>
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

FWIW
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

[PATCH v2 02/28] module: Begin to move from RCU-sched to RCU.

[Sebastian Andrzej Siewior]

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.

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(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 5399c182b3cbe..5cce4a92d7ba3 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -67,7 +67,7 @@
 
 /*
  * Mutex protects:
- * 1) List of modules (also safely readable with preempt_disable),
+ * 1) List of modules (also safely readable within RCU read section),
  * 2) module_use links,
  * 3) mod_tree.addr_min/mod_tree.addr_max.
  * (delete and add uses RCU list operations).
@@ -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
 	/*
 	 * We want to free module_init, but be aware that kallsyms may be
-	 * walking this with preempt disabled.  In all the failure paths, we
+	 * walking this within an RCU read section. In all the failure paths, we
 	 * call synchronize_rcu(), but we don't want to slow down the success
 	 * path. execmem_free() cannot be called in an interrupt, so do the
 	 * work and call synchronize_rcu() in a work queue.
@@ -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;
@@ -3762,7 +3762,6 @@ void print_modules(void)
 	}
 
 	print_unloaded_tainted_modules();
-	preempt_enable();
 	if (last_unloaded_module.name[0])
 		pr_cont(" [last unloaded: %s%s]", last_unloaded_module.name,
 			last_unloaded_module.taints);
diff --git a/kernel/module/tree_lookup.c b/kernel/module/tree_lookup.c
index 277197977d438..d3204c5c74eb7 100644
--- a/kernel/module/tree_lookup.c
+++ b/kernel/module/tree_lookup.c
@@ -12,11 +12,11 @@
 
 /*
  * Use a latched RB-tree for __module_address(); this allows us to use
- * RCU-sched lookups of the address from any context.
+ * RCU lookups of the address from any context.
  *
- * This is conditional on PERF_EVENTS || TRACING because those can really hit
- * __module_address() hard by doing a lot of stack unwinding; potentially from
- * NMI context.
+ * This is conditional on PERF_EVENTS || TRACING || CFI_CLANG because those can
+ * really hit __module_address() hard by doing a lot of stack unwinding;
+ * potentially from NMI context.
  */
 
 static __always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
-- 
2.45.2

[PATCH v2 03/28] module: Use proper RCU assignment in add_kallsyms().

[Sebastian Andrzej Siewior]

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

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/kallsyms.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index bf65e0c3c86fc..45846ae4042d1 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -177,19 +177,15 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
 	unsigned long strtab_size;
 	void *data_base = mod->mem[MOD_DATA].base;
 	void *init_data_base = mod->mem[MOD_INIT_DATA].base;
+	struct mod_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;
 
 	/*
 	 * Now populate the cut down core kallsyms for after init
@@ -199,20 +195,19 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
 	mod->core_kallsyms.strtab = s = data_base + info->stroffs;
 	mod->core_kallsyms.typetab = data_base + info->core_typeoffs;
 	strtab_size = info->core_typeoffs - info->stroffs;
-	src = rcu_dereference(mod->kallsyms)->symtab;
-	for (ndst = i = 0; i < rcu_dereference(mod->kallsyms)->num_symtab; i++) {
-		rcu_dereference(mod->kallsyms)->typetab[i] = elf_type(src + i, info);
+	src = kallsyms->symtab;
+	for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
+		kallsyms->typetab[i] = elf_type(src + i, info);
 		if (i == 0 || is_livepatch_module(mod) ||
 		    is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
 				   info->index.pcpu)) {
 			ssize_t ret;
 
 			mod->core_kallsyms.typetab[ndst] =
-			    rcu_dereference(mod->kallsyms)->typetab[i];
+				kallsyms->typetab[i];
 			dst[ndst] = src[i];
 			dst[ndst++].st_name = s - mod->core_kallsyms.strtab;
-			ret = strscpy(s,
-				      &rcu_dereference(mod->kallsyms)->strtab[src[i].st_name],
+			ret = strscpy(s, &kallsyms->strtab[src[i].st_name],
 				      strtab_size);
 			if (ret < 0)
 				break;
@@ -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;
 }
 
-- 
2.45.2

[PATCH v2 04/28] module: Use RCU in find_kallsyms_symbol().

[Sebastian Andrzej Siewior]

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

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/kallsyms.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 45846ae4042d1..3f59d04795572 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -257,7 +257,7 @@ static const char *find_kallsyms_symbol(struct module *mod,
 {
 	unsigned int i, best = 0;
 	unsigned long nextval, bestval;
-	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
+	struct mod_kallsyms *kallsyms = rcu_dereference(mod->kallsyms);
 	struct module_memory *mod_mem;
 
 	/* At worse, next value is at end of module */
@@ -329,6 +329,7 @@ int module_address_lookup(unsigned long addr,
 	int ret = 0;
 	struct module *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)
 {
 	struct module *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)
 				goto out;
 
 			strscpy(symname, sym, KSYM_NAME_LEN);
-			preempt_enable();
 			return 0;
 		}
 	}
 out:
-	preempt_enable();
 	return -ERANGE;
 }
 
-- 
2.45.2

[PATCH v2 05/28] module: Use RCU in module_get_kallsym().

[Sebastian Andrzej Siewior]

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.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/kallsyms.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 3f59d04795572..4eef518204eb5 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -381,13 +381,13 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 {
 	struct module *mod;
 
-	preempt_disable();
+	guard(rcu)();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		struct mod_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) {
 			const Elf_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();
 			return 0;
 		}
 		symnum -= kallsyms->num_symtab;
 	}
-	preempt_enable();
 	return -ERANGE;
 }
 
-- 
2.45.2

[PATCH v2 06/28] module: Use RCU in find_module_all().

[Sebastian Andrzej Siewior]

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
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(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 94acbacdcdf18..5c1f7ea76c8cb 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -663,7 +663,7 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
 	return within_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. */
 struct module *find_module(const char *name);
 
 extern void __noreturn __module_put_and_kthread_exit(struct module *mod,
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3c21c31796db0..f8932c63b08e3 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -59,7 +59,7 @@ static void klp_find_object_module(struct klp_object *obj)
 	if (!klp_is_module(obj))
 		return;
 
-	rcu_read_lock_sched();
+	guard(rcu)();
 	/*
 	 * We do not want to block removal of patched modules and therefore
 	 * we do not take a reference here. The patches are removed by
@@ -75,8 +75,6 @@ static void klp_find_object_module(struct klp_object *obj)
 	 */
 	if (mod && mod->klp_alive)
 		obj->mod = mod;
-
-	rcu_read_unlock_sched();
 }
 
 static bool klp_initialized(void)
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 4eef518204eb5..3cba9f933b24f 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -450,6 +450,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
 	unsigned long ret;
 
 	/* Don't lock: we're in enough trouble already. */
+	guard(rcu)();
 	preempt_disable();
 	ret = __module_kallsyms_lookup_name(name);
 	preempt_enable();
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 5cce4a92d7ba3..5aa56ec8e203e 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -374,16 +374,14 @@ bool find_symbol(struct find_symbol_arg *fsa)
 }
 
 /*
- * Search for module by name: must hold module_mutex (or preempt disabled
- * for read-only access).
+ * Search for module by name: must hold module_mutex (or RCU for read-only
+ * access).
  */
 struct module *find_module_all(const char *name, size_t len,
 			       bool even_unformed)
 {
 	struct module *mod;
 
-	module_assert_mutex_or_preempt();
-
 	list_for_each_entry_rcu(mod, &modules, list,
 				lockdep_is_held(&module_mutex)) {
 		if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 263fac44d3ca3..c7db326f4e88e 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -123,9 +123,8 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 	if (!p)
 		return true;
 	*p = '\0';
-	rcu_read_lock_sched();
-	ret = !!find_module(tk->symbol);
-	rcu_read_unlock_sched();
+	scoped_guard(rcu)
+		ret = !!find_module(tk->symbol);
 	*p = ':';
 
 	return ret;
@@ -800,12 +799,10 @@ static struct module *try_module_get_by_name(const char *name)
 {
 	struct module *mod;
 
-	rcu_read_lock_sched();
+	guard(rcu)();
 	mod = find_module(name);
 	if (mod && !try_module_get(mod))
 		mod = NULL;
-	rcu_read_unlock_sched();
-
 	return mod;
 }
 #else
-- 
2.45.2

Re: [PATCH v2 06/28] module: Use RCU in find_module_all().

[Petr Mladek]

On Fri 2024-12-20 18:41:20, Sebastian Andrzej Siewior wrote:
> 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
> 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(-)

I looked primary on the changes in the livepatch code. But the entire
patch looks good to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH v2 07/28] module: Use RCU in __find_kallsyms_symbol_value().

[Sebastian Andrzej Siewior]

module::kallsyms can be accessed under RCU assumption.

Use rcu_dereference() to access module::kallsyms.
Update callers.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/kallsyms.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 3cba9f933b24f..e3c55bc879c11 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -407,7 +407,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 static unsigned long __find_kallsyms_symbol_value(struct module *mod, const char *name)
 {
 	unsigned int i;
-	struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);
+	struct mod_kallsyms *kallsyms = rcu_dereference(mod->kallsyms);
 
 	for (i = 0; i < kallsyms->num_symtab; i++) {
 		const Elf_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. */
 unsigned long module_kallsyms_lookup_name(const char *name)
 {
-	unsigned long ret;
-
 	/* Don't lock: we're in enough trouble already. */
 	guard(rcu)();
-	preempt_disable();
-	ret = __module_kallsyms_lookup_name(name);
-	preempt_enable();
-	return ret;
+	return __module_kallsyms_lookup_name(name);
 }
 
 unsigned long find_kallsyms_symbol_value(struct module *mod, const char *name)
 {
-	unsigned long ret;
-
-	preempt_disable();
-	ret = __find_kallsyms_symbol_value(mod, name);
-	preempt_enable();
-	return ret;
+	guard(rcu)();
+	return __find_kallsyms_symbol_value(mod, name);
 }
 
 int module_kallsyms_on_each_symbol(const char *modname,
-- 
2.45.2

[PATCH v2 08/28] module: Use RCU in module_kallsyms_on_each_symbol().

[Sebastian Andrzej Siewior]

module::kallsyms can be accessed under RCU assumption.

Use rcu_dereference() to access module::kallsyms.
Update callers.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/kallsyms.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index e3c55bc879c11..0e8ec6486d95c 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -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++) {
 			const Elf_Sym *sym = &kallsyms->symtab[i];
-- 
2.45.2

[PATCH v2 09/28] module: Remove module_assert_mutex_or_preempt() from try_add_tainted_module().

[Sebastian Andrzej Siewior]

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

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/tracking.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
index 16742d1c630c6..4fefec5b683c6 100644
--- a/kernel/module/tracking.c
+++ b/kernel/module/tracking.c
@@ -21,8 +21,6 @@ int try_add_tainted_module(struct module *mod)
 {
 	struct mod_unload_taint *mod_taint;
 
-	module_assert_mutex_or_preempt();
-
 	if (!mod->taints)
 		goto out;
 
-- 
2.45.2

[PATCH v2 10/28] module: Use RCU in find_symbol().

[Sebastian Andrzej Siewior]

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.

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(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 5aa56ec8e203e..71e73deed76c0 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -331,7 +331,7 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms,
 
 /*
  * Find an exported symbol and return it, along with, (optional) crc and
- * (optional) module which owns it.  Needs preempt disabled or module_mutex.
+ * (optional) module which owns it. Needs RCU or module_mutex.
  */
 bool find_symbol(struct find_symbol_arg *fsa)
 {
@@ -345,8 +345,6 @@ bool find_symbol(struct find_symbol_arg *fsa)
 	struct module *mod;
 	unsigned int i;
 
-	module_assert_mutex_or_preempt();
-
 	for (i = 0; i < ARRAY_SIZE(arr); i++)
 		if (find_exported_symbol_in_section(&arr[i], NULL, fsa))
 			return true;
@@ -812,10 +810,9 @@ void __symbol_put(const char *symbol)
 		.gplok	= true,
 	};
 
-	preempt_disable();
+	guard(rcu)();
 	BUG_ON(!find_symbol(&fsa));
 	module_put(fsa.owner);
-	preempt_enable();
 }
 EXPORT_SYMBOL(__symbol_put);
 
@@ -1369,21 +1366,18 @@ void *__symbol_get(const char *symbol)
 		.warn	= true,
 	};
 
-	preempt_disable();
-	if (!find_symbol(&fsa))
-		goto fail;
-	if (fsa.license != GPL_ONLY) {
-		pr_warn("failing symbol_get of non-GPLONLY symbol %s.\n",
-			symbol);
-		goto fail;
+	scoped_guard(rcu) {
+		if (!find_symbol(&fsa))
+			return NULL;
+		if (fsa.license != GPL_ONLY) {
+			pr_warn("failing symbol_get of non-GPLONLY symbol %s.\n",
+				symbol);
+			return NULL;
+		}
+		if (strong_try_module_get(fsa.owner))
+			return NULL;
 	}
-	if (strong_try_module_get(fsa.owner))
-		goto fail;
-	preempt_enable();
 	return (void *)kernel_symbol_value(fsa.sym);
-fail:
-	preempt_enable();
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(__symbol_get);
 
diff --git a/kernel/module/version.c b/kernel/module/version.c
index 53f43ac5a73e9..65ef8f2a821da 100644
--- a/kernel/module/version.c
+++ b/kernel/module/version.c
@@ -62,17 +62,17 @@ int check_modstruct_version(const struct load_info *info,
 		.name	= "module_layout",
 		.gplok	= true,
 	};
+	bool have_symbol;
 
 	/*
 	 * Since this should be found in kernel (which can't be removed), no
-	 * locking is necessary -- use preempt_disable() to placate lockdep.
+	 * locking is necessary. Regardless use a RCU read section to keep
+	 * lockdep happy.
 	 */
-	preempt_disable();
-	if (!find_symbol(&fsa)) {
-		preempt_enable();
-		BUG();
-	}
-	preempt_enable();
+	scoped_guard(rcu)
+		have_symbol = find_symbol(&fsa);
+	BUG_ON(!have_symbol);
+
 	return check_version(info, "module_layout", mod, fsa.crc);
 }
 
-- 
2.45.2

[PATCH v2 11/28] module: Use RCU in __is_module_percpu_address().

[Sebastian Andrzej Siewior]

The modules list can be accessed under RCU assumption.

Use RCU protection instead preempt_disable().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/main.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 71e73deed76c0..126f7f05dedf8 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -450,8 +450,7 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr)
 	struct module *mod;
 	unsigned int cpu;
 
-	preempt_disable();
-
+	guard(rcu)();
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (mod->state == MODULE_STATE_UNFORMED)
 			continue;
@@ -468,13 +467,10 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr)
 						per_cpu_ptr(mod->percpu,
 							    get_boot_cpu_id());
 				}
-				preempt_enable();
 				return true;
 			}
 		}
 	}
-
-	preempt_enable();
 	return false;
 }
 
-- 
2.45.2

[PATCH v2 12/28] module: Allow __module_address() to be called from RCU section.

[Sebastian Andrzej Siewior]

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.

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(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index daef2be839022..030d2ed175fa8 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -122,17 +122,6 @@ char *module_next_tag_pair(char *string, unsigned long *secsize);
 #define for_each_modinfo_entry(entry, info, name) \
 	for (entry = get_modinfo(info, name); entry; entry = get_next_modinfo(info, name, entry))
 
-static inline void module_assert_mutex_or_preempt(void)
-{
-#ifdef CONFIG_LOCKDEP
-	if (unlikely(!debug_locks))
-		return;
-
-	WARN_ON_ONCE(!rcu_read_lock_sched_held() &&
-		     !lockdep_is_held(&module_mutex));
-#endif
-}
-
 static inline unsigned long kernel_symbol_value(const struct kernel_symbol *sym)
 {
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 126f7f05dedf8..686b74c7c17f5 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3667,7 +3667,7 @@ bool is_module_address(unsigned long addr)
  * __module_address() - get the module which 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_address(unsigned long addr)
@@ -3685,8 +3685,6 @@ struct module *__module_address(unsigned long addr)
 	return NULL;
 
 lookup:
-	module_assert_mutex_or_preempt();
-
 	mod = mod_find(addr, &mod_tree);
 	if (mod) {
 		BUG_ON(!within_module(addr, mod));
-- 
2.45.2

[PATCH v2 13/28] module: Use RCU in search_module_extables().

[Sebastian Andrzej Siewior]

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.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/main.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 686b74c7c17f5..74b9e9ddb4b65 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -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. */
 const struct exception_table_entry *search_module_extables(unsigned long addr)
 {
-	const struct exception_table_entry *e = NULL;
 	struct module *mod;
 
-	preempt_disable();
+	guard(rcu)();
 	mod = __module_address(addr);
 	if (!mod)
-		goto out;
+		return NULL;
 
 	if (!mod->num_exentries)
-		goto out;
-
-	e = search_extable(mod->extable,
-			   mod->num_exentries,
-			   addr);
-out:
-	preempt_enable();
-
+		return NULL;
 	/*
-	 * Now, if we found one, we are running inside it now, hence
-	 * we cannot unload the module, hence no refcnt needed.
+	 * The address passed here belongs to a module that is currently
+	 * invoked (we are running inside it). Therefore its module::refcnt
+	 * needs already be >0 to ensure that it is not removed at this stage.
+	 * All other user need to invoke this function within a RCU read
+	 * section.
 	 */
-	return e;
+	return search_extable(mod->extable, mod->num_exentries, addr);
 }
 
 /**
-- 
2.45.2

[PATCH v2 14/28] module: Use RCU in all users of __module_address().

[Sebastian Andrzej Siewior]

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

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(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 1c6a6c1704d8d..d5dd54c53ace6 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -55,12 +55,11 @@ static inline void *dereference_symbol_descriptor(void *ptr)
 	if (is_ksym_addr((unsigned long)ptr))
 		return ptr;
 
-	preempt_disable();
+	guard(rcu)();
 	mod = __module_address((unsigned long)ptr);
 
 	if (mod)
 		ptr = dereference_module_function_descriptor(mod, ptr);
-	preempt_enable();
 #endif
 	return ptr;
 }
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 0e8ec6486d95c..00a60796327c0 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -316,7 +316,7 @@ void * __weak dereference_module_function_descriptor(struct module *mod,
 
 /*
  * For kallsyms to ask for address resolution.  NULL means not found.  Careful
- * not to lock to avoid deadlock on oopses, simply disable preemption.
+ * not to lock to avoid deadlock on oopses, RCU is enough.
  */
 int module_address_lookup(unsigned long addr,
 			  unsigned long *size,
@@ -330,7 +330,6 @@ int module_address_lookup(unsigned long addr,
 	struct module *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();
-
 	return ret;
 }
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 74b9e9ddb4b65..80877741ac7e5 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3649,13 +3649,8 @@ const struct exception_table_entry *search_module_extables(unsigned long addr)
  */
 bool is_module_address(unsigned long addr)
 {
-	bool ret;
-
-	preempt_disable();
-	ret = __module_address(addr) != NULL;
-	preempt_enable();
-
-	return ret;
+	guard(rcu)();
+	return __module_address(addr) != NULL;
 }
 
 /**
-- 
2.45.2

[PATCH v2 15/28] module: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

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

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/module/main.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 80877741ac7e5..6a99076146cbc 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -823,13 +823,12 @@ void symbol_put_addr(void *addr)
 
 	/*
 	 * 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)
-- 
2.45.2

[PATCH v2 16/28] ARM: module: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

__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
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/kernel/module-plts.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
index da2ee8d6ef1a7..354ce16d83cb5 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -285,11 +285,9 @@ bool in_module_plt(unsigned long loc)
 	struct module *mod;
 	bool ret;
 
-	preempt_disable();
+	guard(rcu)();
 	mod = __module_text_address(loc);
 	ret = mod && (loc - (u32)mod->arch.core.plt_ent < mod->arch.core.plt_count * PLT_ENT_SIZE ||
 		      loc - (u32)mod->arch.init.plt_ent < mod->arch.init.plt_count * PLT_ENT_SIZE);
-	preempt_enable();
-
 	return ret;
 }
-- 
2.45.2

[PATCH v2 17/28] arm64: module: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

__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
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm64/kernel/ftrace.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 245cb419ca24d..2b76939b6304f 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -257,14 +257,13 @@ static bool ftrace_find_callable_addr(struct dyn_ftrace *rec,
 	 * dealing with an out-of-range condition, we can assume it
 	 * is due to a module being loaded far away from the kernel.
 	 *
-	 * NOTE: __module_text_address() must be called with preemption
-	 * disabled, but we can rely on ftrace_lock to ensure that 'mod'
+	 * NOTE: __module_text_address() must be called within a RCU read
+	 * section, but we can rely on ftrace_lock to ensure that 'mod'
 	 * retains its validity throughout the remainder of this code.
 	 */
 	if (!mod) {
-		preempt_disable();
+		guard(rcu)();
 		mod = __module_text_address(pc);
-		preempt_enable();
 	}
 
 	if (WARN_ON(!mod))
-- 
2.45.2

[PATCH v2 18/28] LoongArch/orc: Use RCU in all users of __module_address().

[Sebastian Andrzej Siewior]

__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
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/loongarch/kernel/unwind_orc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/loongarch/kernel/unwind_orc.c b/arch/loongarch/kernel/unwind_orc.c
index b257228763317..d623935a75471 100644
--- a/arch/loongarch/kernel/unwind_orc.c
+++ b/arch/loongarch/kernel/unwind_orc.c
@@ -399,7 +399,7 @@ bool unwind_next_frame(struct unwind_state *state)
 		return false;
 
 	/* Don't let modules unload while we're reading their ORC data. */
-	preempt_disable();
+	guard(rcu)();
 
 	if (is_entry_func(state->pc))
 		goto end;
@@ -514,14 +514,12 @@ bool unwind_next_frame(struct unwind_state *state)
 	if (!__kernel_text_address(state->pc))
 		goto err;
 
-	preempt_enable();
 	return true;
 
 err:
 	state->error = true;
 
 end:
-	preempt_enable();
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;
 }
-- 
2.45.2

[PATCH v2 19/28] LoongArch: ftrace: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

__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
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/loongarch/kernel/ftrace_dyn.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/kernel/ftrace_dyn.c b/arch/loongarch/kernel/ftrace_dyn.c
index 18056229e22e4..5e0d870935542 100644
--- a/arch/loongarch/kernel/ftrace_dyn.c
+++ b/arch/loongarch/kernel/ftrace_dyn.c
@@ -85,14 +85,13 @@ static bool ftrace_find_callable_addr(struct dyn_ftrace *rec, struct module *mod
 	 * dealing with an out-of-range condition, we can assume it
 	 * is due to a module being loaded far away from the kernel.
 	 *
-	 * NOTE: __module_text_address() must be called with preemption
-	 * disabled, but we can rely on ftrace_lock to ensure that 'mod'
+	 * NOTE: __module_text_address() must be called within a RCU read
+	 * section, but we can rely on ftrace_lock to ensure that 'mod'
 	 * retains its validity throughout the remainder of this code.
 	 */
 	if (!mod) {
-		preempt_disable();
+		guard(rcu)();
 		mod = __module_text_address(pc);
-		preempt_enable();
 	}
 
 	if (WARN_ON(!mod))
-- 
2.45.2

Re: [PATCH v2 19/28] LoongArch: ftrace: Use RCU in all users of __module_text_address().

[Steven Rostedt]

On Fri, 20 Dec 2024 18:41:33 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> --- a/arch/loongarch/kernel/ftrace_dyn.c
> +++ b/arch/loongarch/kernel/ftrace_dyn.c
> @@ -85,14 +85,13 @@ static bool ftrace_find_callable_addr(struct dyn_ftrace *rec, struct module *mod
>  	 * dealing with an out-of-range condition, we can assume it
>  	 * is due to a module being loaded far away from the kernel.
>  	 *
> -	 * NOTE: __module_text_address() must be called with preemption
> -	 * disabled, but we can rely on ftrace_lock to ensure that 'mod'
> +	 * NOTE: __module_text_address() must be called within a RCU read
> +	 * section, but we can rely on ftrace_lock to ensure that 'mod'
>  	 * retains its validity throughout the remainder of this code.
>  	 */
>  	if (!mod) {
> -		preempt_disable();
> +		guard(rcu)();
>  		mod = __module_text_address(pc);
> -		preempt_enable();
>  	}
>  
>  	if (WARN_ON(!mod))
> -- 

I personally dislike swapping one line of protection for the guard() code.

Although, I do think scoped_guard() can work.

That is:

	if (!mod) {
		read_rcu_lock();
		mod = __module_text_address(pc);
		read_rcu_unlock();
	}

Is easier to understand than:

	if (!mod) {
		guard(rcu)();
		mod = __module_text_address(pc);
	}

Because it makes me wonder, why use a guard() for a one liner?

But, when I saw your other patch, if we had:

	if (!mod) {
		scoped_guard(rcu)()
			mod = __module_text_address(pc);
	}

To me, hat looks much better than the guard() as it is obvious to what the
code is protecting. Even though, I still prefer the explicit, lock/unlock.
Maybe, just because I'm more used to it.

IMHO, guard() is for complex functions that are error prone. A single line
is not something that is error prone (unless you don't match the lock and
unlock properly, but that's pretty obvious when that happens).

But this is just my own opinion.

-- Steve

Re: [PATCH v2 19/28] LoongArch: ftrace: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

On 2024-12-27 12:19:46 [-0500], Steven Rostedt wrote:
…
> Is easier to understand than:
> 
> 	if (!mod) {
> 		guard(rcu)();
> 		mod = __module_text_address(pc);
> 	}
> 
> Because it makes me wonder, why use a guard() for a one liner?

Why not? The context ends immediately after.

> But, when I saw your other patch, if we had:
> 
> 	if (!mod) {
> 		scoped_guard(rcu)()
> 			mod = __module_text_address(pc);
> 	}

Okay, if this looks better, let me update it. It just you already have a
scope (the {} after the if) and then we start yet another block. But if
this looks better so be it.

> To me, hat looks much better than the guard() as it is obvious to what the
> code is protecting. Even though, I still prefer the explicit, lock/unlock.
> Maybe, just because I'm more used to it.

I'm probably also used to the explicit part but numerous people said to
use this from now on. And it results in less lines and you don't have to
worry about each return statement. So it somehow looks/ feels line an
upgrade.

> IMHO, guard() is for complex functions that are error prone. A single line
> is not something that is error prone (unless you don't match the lock and
> unlock properly, but that's pretty obvious when that happens).

True but this is now a one liner which might be extended later on. Also
the context is one line.

> But this is just my own opinion.
> 
> -- Steve

Sebastian

[PATCH v2 20/28] powerpc/ftrace: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

__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 <christophe.leroy@csgroup.eu>
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
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(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 5ccd791761e8f..558d7f4e4bea6 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -115,10 +115,8 @@ static unsigned long ftrace_lookup_module_stub(unsigned long ip, unsigned long a
 {
 	struct module *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);
 
diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.c b/arch/powerpc/kernel/trace/ftrace_64_pg.c
index 98787376eb87c..531d40f10c8a1 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_pg.c
+++ b/arch/powerpc/kernel/trace/ftrace_64_pg.c
@@ -120,10 +120,8 @@ static struct module *ftrace_lookup_module(struct dyn_ftrace *rec)
 {
 	struct module *mod;
 
-	preempt_disable();
-	mod = __module_text_address(rec->ip);
-	preempt_enable();
-
+	scoped_guard(rcu)
+		mod = __module_text_address(rec->ip);
 	if (!mod)
 		pr_err("No module loaded at addr=%lx\n", rec->ip);
 
-- 
2.45.2

[PATCH v2 21/28] x86: Use RCU in all users of __module_address().

[Sebastian Andrzej Siewior]

__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 <tglx@linutronix.de>
Cc: x86@kernel.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(-)

diff --git a/arch/x86/kernel/callthunks.c b/arch/x86/kernel/callthunks.c
index 4656474567533..15bcec780881f 100644
--- a/arch/x86/kernel/callthunks.c
+++ b/arch/x86/kernel/callthunks.c
@@ -98,11 +98,10 @@ static inline bool within_module_coretext(void *addr)
 #ifdef CONFIG_MODULES
 	struct module *mod;
 
-	preempt_disable();
+	guard(rcu)();
 	mod = __module_address((unsigned long)addr);
 	if (mod && within_module_core((unsigned long)addr, mod))
 		ret = true;
-	preempt_enable();
 #endif
 	return ret;
 }
diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index d4705a348a804..977ee75e047c8 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -476,7 +476,7 @@ bool unwind_next_frame(struct unwind_state *state)
 		return false;
 
 	/* 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))
@@ -669,14 +669,12 @@ bool unwind_next_frame(struct unwind_state *state)
 		goto err;
 	}
 
-	preempt_enable();
 	return true;
 
 err:
 	state->error = true;
 
 the_end:
-	preempt_enable();
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;
 }
-- 
2.45.2

[PATCH v2 22/28] jump_label: Use RCU in all users of __module_address().

[Sebastian Andrzej Siewior]

__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>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/jump_label.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 93a822d3c468c..7fcf4017cb383 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -746,9 +746,9 @@ static int jump_label_add_module(struct module *mod)
 				kfree(jlm);
 				return -ENOMEM;
 			}
-			preempt_disable();
-			jlm2->mod = __module_address((unsigned long)key);
-			preempt_enable();
+			scoped_guard(rcu)
+				jlm2->mod = __module_address((unsigned long)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((unsigned long)key);
-	if (mod) {
-		stop = mod->jump_entries + mod->num_jump_entries;
-		init = mod->state == MODULE_STATE_COMING;
+	scoped_guard(rcu) {
+		mod = __module_address((unsigned long)key);
+		if (mod) {
+			stop = mod->jump_entries + mod->num_jump_entries;
+			init = mod->state == MODULE_STATE_COMING;
+		}
 	}
-	preempt_enable();
 #endif
 	entry = static_key_entries(key);
 	/* if there are no users, entry can be NULL */
-- 
2.45.2

[PATCH v2 23/28] jump_label: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

__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>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/jump_label.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 7fcf4017cb383..7cb19e6014266 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -653,13 +653,12 @@ static int __jump_label_mod_text_reserved(void *start, void *end)
 	struct module *mod;
 	int ret;
 
-	preempt_disable();
-	mod = __module_text_address((unsigned long)start);
-	WARN_ON_ONCE(__module_text_address((unsigned long)end) != mod);
-	if (!try_module_get(mod))
-		mod = NULL;
-	preempt_enable();
-
+	scoped_guard(rcu) {
+		mod = __module_text_address((unsigned long)start);
+		WARN_ON_ONCE(__module_text_address((unsigned long)end) != mod);
+		if (!try_module_get(mod))
+			mod = NULL;
+	}
 	if (!mod)
 		return 0;
 
-- 
2.45.2

[PATCH v2 24/28] bpf: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

__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 <haoluo@google.com>
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 <mattbobrowski@google.com>
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
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/trace/bpf_trace.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 1b8db5aee9d38..020df7b6ff90c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2336,10 +2336,9 @@ void bpf_put_raw_tracepoint(struct bpf_raw_event_map *btp)
 {
 	struct module *mod;
 
-	preempt_disable();
+	guard(rcu)();
 	mod = __module_address((unsigned long)btp);
 	module_put(mod);
-	preempt_enable();
 }
 
 static __always_inline
@@ -2907,16 +2906,14 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
 	for (i = 0; i < addrs_cnt; i++) {
 		struct module *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);
-- 
2.45.2

[PATCH v2 25/28] kprobes: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

__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 <anil.s.keshavamurthy@intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Naveen N Rao <naveen@kernel.org>
Cc: linux-trace-kernel@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/kprobes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b027a4030976a..22e47a27df4aa 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1566,7 +1566,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
 	if (ret)
 		return ret;
 	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();
 
 	return ret;
-- 
2.45.2

[PATCH v2 26/28] static_call: Use RCU in all users of __module_text_address().

[Sebastian Andrzej Siewior]

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

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/static_call_inline.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/static_call_inline.c b/kernel/static_call_inline.c
index 5259cda486d05..06b14d8362b9b 100644
--- a/kernel/static_call_inline.c
+++ b/kernel/static_call_inline.c
@@ -325,13 +325,12 @@ static int __static_call_mod_text_reserved(void *start, void *end)
 	struct module *mod;
 	int ret;
 
-	preempt_disable();
-	mod = __module_text_address((unsigned long)start);
-	WARN_ON_ONCE(__module_text_address((unsigned long)end) != mod);
-	if (!try_module_get(mod))
-		mod = NULL;
-	preempt_enable();
-
+	scoped_guard(rcu) {
+		mod = __module_text_address((unsigned long)start);
+		WARN_ON_ONCE(__module_text_address((unsigned long)end) != mod);
+		if (!try_module_get(mod))
+			mod = NULL;
+	}
 	if (!mod)
 		return 0;
 
-- 
2.45.2

[PATCH v2 27/28] bug: Use RCU instead RCU-sched to protect module_bug_list.

[Sebastian Andrzej Siewior]

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>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 lib/bug.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/lib/bug.c b/lib/bug.c
index e0ff219899902..b1f07459c2ee3 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -66,23 +66,19 @@ static LIST_HEAD(module_bug_list);
 
 static struct bug_entry *module_find_bug(unsigned long bugaddr)
 {
+	struct bug_entry *bug;
 	struct module *mod;
-	struct bug_entry *bug = NULL;
 
-	rcu_read_lock_sched();
+	guard(rcu)();
 	list_for_each_entry_rcu(mod, &module_bug_list, bug_list) {
 		unsigned i;
 
 		bug = mod->bug_table;
 		for (i = 0; i < mod->num_bugs; ++i, ++bug)
 			if (bugaddr == bug_addr(bug))
-				goto out;
+				return bug;
 	}
-	bug = NULL;
-out:
-	rcu_read_unlock_sched();
-
-	return bug;
+	return NULL;
 }
 
 void module_bug_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
@@ -235,11 +231,11 @@ void generic_bug_clear_once(void)
 #ifdef CONFIG_MODULES
 	struct module *mod;
 
-	rcu_read_lock_sched();
-	list_for_each_entry_rcu(mod, &module_bug_list, bug_list)
-		clear_once_table(mod->bug_table,
-				 mod->bug_table + mod->num_bugs);
-	rcu_read_unlock_sched();
+	scoped_guard(rcu) {
+		list_for_each_entry_rcu(mod, &module_bug_list, bug_list)
+			clear_once_table(mod->bug_table,
+					 mod->bug_table + mod->num_bugs);
+	}
 #endif
 
 	clear_once_table(__start___bug_table, __stop___bug_table);
-- 
2.45.2

[PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Sebastian Andrzej Siewior]

__module_address() can be invoked within a RCU section, there is no
requirement to have preemption disabled.

I'm not sure if using rcu_read_lock() will introduce the regression that
has been fixed in commit 14c4c8e41511a ("cfi: Use
rcu_read_{un}lock_sched_notrace").

Cc: Elliot Berman <quic_eberman@quicinc.com>
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
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/cfi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/cfi.c b/kernel/cfi.c
index 08caad7767176..c8f2b5a51b2e6 100644
--- a/kernel/cfi.c
+++ b/kernel/cfi.c
@@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr)
 	struct module *mod;
 	bool found = false;
 
+	/*
+	 * XXX this could be RCU protected but would it introcude the regression
+	 * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace")
+	 */
 	rcu_read_lock_sched_notrace();
 
 	mod = __module_address(addr);
-- 
2.45.2

Re: [PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Petr Pavlu]

On 12/20/24 18:41, Sebastian Andrzej Siewior wrote:
> __module_address() can be invoked within a RCU section, there is no
> requirement to have preemption disabled.
> 
> I'm not sure if using rcu_read_lock() will introduce the regression that
> has been fixed in commit 14c4c8e41511a ("cfi: Use
> rcu_read_{un}lock_sched_notrace").
> 
> Cc: Elliot Berman <quic_eberman@quicinc.com>
> 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
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/cfi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index 08caad7767176..c8f2b5a51b2e6 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr)
>  	struct module *mod;
>  	bool found = false;
>  
> +	/*
> +	 * XXX this could be RCU protected but would it introcude the regression
> +	 * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace")
> +	 */
>  	rcu_read_lock_sched_notrace();
>  
>  	mod = __module_address(addr);

I think that since 89245600941e ("cfi: Switch to -fsanitize=kcfi"), this
can be a call to rcu_read_lock_sched(), or in your case rcu_read_lock().
The recursive case where __cfi_slowpath_diag() could end up calling
itself is no longer present, as all that logic is gone. I then don't see
another reason this should use the notrace variant.

@Sami, could you please confirm this?

-- 
Thanks,
Petr

Re: [PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Sami Tolvanen]

Hi Petr,

On Mon, Dec 30, 2024 at 1:13 PM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> On 12/20/24 18:41, Sebastian Andrzej Siewior wrote:
> > __module_address() can be invoked within a RCU section, there is no
> > requirement to have preemption disabled.
> >
> > I'm not sure if using rcu_read_lock() will introduce the regression that
> > has been fixed in commit 14c4c8e41511a ("cfi: Use
> > rcu_read_{un}lock_sched_notrace").
> >
> > Cc: Elliot Berman <quic_eberman@quicinc.com>
> > 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
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >  kernel/cfi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel/cfi.c b/kernel/cfi.c
> > index 08caad7767176..c8f2b5a51b2e6 100644
> > --- a/kernel/cfi.c
> > +++ b/kernel/cfi.c
> > @@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr)
> >       struct module *mod;
> >       bool found = false;
> >
> > +     /*
> > +      * XXX this could be RCU protected but would it introcude the regression
> > +      * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace")
> > +      */
> >       rcu_read_lock_sched_notrace();
> >
> >       mod = __module_address(addr);
>
> I think that since 89245600941e ("cfi: Switch to -fsanitize=kcfi"), this
> can be a call to rcu_read_lock_sched(), or in your case rcu_read_lock().
> The recursive case where __cfi_slowpath_diag() could end up calling
> itself is no longer present, as all that logic is gone. I then don't see
> another reason this should use the notrace variant.
>
> @Sami, could you please confirm this?

Switching is_module_cfi_trap() to use rcu_read_lock() in this series
should be fine. KCFI checks don't perform function calls, so there's
no risk of recursion, and this function is only called during the
error handling path.

Sami

Re: [PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Elliot Berman]

On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote:
> __module_address() can be invoked within a RCU section, there is no
> requirement to have preemption disabled.
> 
> I'm not sure if using rcu_read_lock() will introduce the regression that
> has been fixed in commit 14c4c8e41511a ("cfi: Use
> rcu_read_{un}lock_sched_notrace").
> 

You can replace the rcu_read_lock_sched_notrace() with guard(rcu)().
Regular rcu lock doesn't generate function traces, so the recursive loop
isn't possible.

I've tested:
 - the current kernel (no recursive loop)
 - Revert back to rcu_read_lock_sched() (fails)
 - Your series as-is (no recurisve loop)
 - Replace with guard(rcu)() (no recursive loop)

Whether you'd like to stick with the current patch or replace with
guard(rcu)():

Tested-by: Elliot Berman <elliot.berman@oss.qualcomm.com>  # sm8650-qrd

-

I don't know why I didn't mention steps to reproduce, even for my own
benefit. Lesson learned :)

Here are the steps to reproduce; you'll need a system with support for
CFI: qemu arm64 probably does the trick and you'll need clang>=16. I'm
happy to help test future revisions of this series since I have the
setup all done.

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

The trace buffer should not be full of stm calls due to the recursive
loop as mentioned in my original commit.


Regards,
Elliot Berman

> Cc: Elliot Berman <quic_eberman@quicinc.com>
> 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
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/cfi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index 08caad7767176..c8f2b5a51b2e6 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -71,6 +71,10 @@ static bool is_module_cfi_trap(unsigned long addr)
>  	struct module *mod;
>  	bool found = false;
>  
> +	/*
> +	 * XXX this could be RCU protected but would it introcude the regression
> +	 * fixed in 14c4c8e41511a ("cfi: Use rcu_read_{un}lock_sched_notrace")
> +	 */
>  	rcu_read_lock_sched_notrace();
>  
>  	mod = __module_address(addr);
> -- 
> 2.45.2
> 

Re: [PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Sami Tolvanen]

Hi Elliot,

On Mon, Dec 30, 2024 at 7:33 PM Elliot Berman
<elliot.berman@oss.qualcomm.com> wrote:
>
> On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote:
> > __module_address() can be invoked within a RCU section, there is no
> > requirement to have preemption disabled.
> >
> > I'm not sure if using rcu_read_lock() will introduce the regression that
> > has been fixed in commit 14c4c8e41511a ("cfi: Use
> > rcu_read_{un}lock_sched_notrace").
> >
>
> You can replace the rcu_read_lock_sched_notrace() with guard(rcu)().
> Regular rcu lock doesn't generate function traces, so the recursive loop
> isn't possible.
>
> I've tested:
>  - the current kernel (no recursive loop)
>  - Revert back to rcu_read_lock_sched() (fails)

Which kernel version did you test? I assume something pre-KCFI as
arm64 doesn't use this code since v6.1.

>  - Your series as-is (no recurisve loop)

Note that this patch only adds a comment to is_module_cfi_trap(), so I
wouldn't expect a functional change.

Sami

Re: [PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Elliot Berman]

On Thu, Jan 02, 2025 at 04:24:22PM -0800, Sami Tolvanen wrote:
> Hi Elliot,
> 
> On Mon, Dec 30, 2024 at 7:33 PM Elliot Berman
> <elliot.berman@oss.qualcomm.com> wrote:
> >
> > On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote:
> > > __module_address() can be invoked within a RCU section, there is no
> > > requirement to have preemption disabled.
> > >
> > > I'm not sure if using rcu_read_lock() will introduce the regression that
> > > has been fixed in commit 14c4c8e41511a ("cfi: Use
> > > rcu_read_{un}lock_sched_notrace").
> > >
> >
> > You can replace the rcu_read_lock_sched_notrace() with guard(rcu)().
> > Regular rcu lock doesn't generate function traces, so the recursive loop
> > isn't possible.
> >
> > I've tested:
> >  - the current kernel (no recursive loop)
> >  - Revert back to rcu_read_lock_sched() (fails)
> 
> Which kernel version did you test? I assume something pre-KCFI as
> arm64 doesn't use this code since v6.1.
> 

Ah, thanks for calling me out. I dug a bit more, I thought I was looking
at a recursive loop in the ftrace buffers, but was actually the expected
behavior. When I tested on the other configurations, the stm dummy
driver hadn't kicked in yet by the time I looked at the ftrace. Indeed,
this function code is not used on arm64.

I experimented with an x86 build as well and I was able to get the hang
I remember seeing after some tweaks to force a CFI failure. Still,
guard(rcu)() is okay by me :)

> >  - Your series as-is (no recurisve loop)
> 
> Note that this patch only adds a comment to is_module_cfi_trap(), so I
> wouldn't expect a functional change.
> 

Agreed I wouldn't expect it to make any issues; I mentioned it for
completeness sake.

Regards,
Elliot

Re: [PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Sami Tolvanen]

Hi,

On Mon, Jan 6, 2025 at 10:00 AM Elliot Berman
<elliot.berman@oss.qualcomm.com> wrote:
>
> On Thu, Jan 02, 2025 at 04:24:22PM -0800, Sami Tolvanen wrote:
> > Hi Elliot,
> >
> > On Mon, Dec 30, 2024 at 7:33 PM Elliot Berman
> > <elliot.berman@oss.qualcomm.com> wrote:
> > >
> > > On Fri, Dec 20, 2024 at 06:41:42PM +0100, Sebastian Andrzej Siewior wrote:
> > > > __module_address() can be invoked within a RCU section, there is no
> > > > requirement to have preemption disabled.
> > > >
> > > > I'm not sure if using rcu_read_lock() will introduce the regression that
> > > > has been fixed in commit 14c4c8e41511a ("cfi: Use
> > > > rcu_read_{un}lock_sched_notrace").
> > > >
> > >
> > > You can replace the rcu_read_lock_sched_notrace() with guard(rcu)().
> > > Regular rcu lock doesn't generate function traces, so the recursive loop
> > > isn't possible.
> > >
> > > I've tested:
> > >  - the current kernel (no recursive loop)
> > >  - Revert back to rcu_read_lock_sched() (fails)
> >
> > Which kernel version did you test? I assume something pre-KCFI as
> > arm64 doesn't use this code since v6.1.
> >
>
> Ah, thanks for calling me out. I dug a bit more, I thought I was looking
> at a recursive loop in the ftrace buffers, but was actually the expected
> behavior. When I tested on the other configurations, the stm dummy
> driver hadn't kicked in yet by the time I looked at the ftrace. Indeed,
> this function code is not used on arm64.
>
> I experimented with an x86 build as well and I was able to get the hang
> I remember seeing after some tweaks to force a CFI failure. Still,
> guard(rcu)() is okay by me :)

OK, great. That makes sense. Thanks for taking the time to test this!

Sami

Re: [PATCH v2 28/28] cfi: Use RCU while invoking __module_address().

[Sebastian Andrzej Siewior]

On 2025-01-06 13:24:28 [-0800], Sami Tolvanen wrote:
> Hi,
Hi,

> OK, great. That makes sense. Thanks for taking the time to test this!

Thank you two for testing, confirming and adding more informations to
what and why. I take some of this for the patch description.

> Sami

Sebastian

Re: [PATCH v2 00/28] module: Use RCU instead of RCU-sched.

[Christophe Leroy]

Hi Sebastian,

Le 20/12/2024 à 18:41, Sebastian Andrzej Siewior a écrit :
> Hi,
> 
> This is an updated version of the inital 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 splitted it into smaller chunks which can be applied/ reviewed
> independently. I'm just not sure about the cfi patch (28/28) so I added
> just a comment instead.
> 
> v1…v2: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20241205215102.hRywUW2A%40linutronix.de%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cbad0e7f9344a47371a0808dd211fdefd%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638703142988931970%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=a%2FV5g5zRqceJWedAAZJ4zt6XJpZ0Yqm%2BqKPrsItuEXs%3D&reserved=0
>    - Splitted into smaller patches.
>    - Converted all users.
> 

How did you generate that cover letter ?

It should contain the full list of files modified by the series, so that 
I can see if any of them is of interest to me without going into each patch.

This is done automatically when you use 'git format-patch --cover-letter'.

Christophe

Re: [PATCH v2 00/28] module: Use RCU instead of RCU-sched.

[Sebastian Andrzej Siewior]

On 2025-01-02 13:47:06 [+0100], Christophe Leroy wrote:
> Hi Sebastian,
Hi Christophe,

> How did you generate that cover letter ?
> 
> It should contain the full list of files modified by the series, so that I
> can see if any of them is of interest to me without going into each patch.
> 
> This is done automatically when you use 'git format-patch --cover-letter'.

I could do so for v3. This was format-patch without --cover-letter.

> Christophe

Sebastian

Re: [PATCH v2 00/28] module: Use RCU instead of RCU-sched.

[Peter Zijlstra]

On Fri, Dec 20, 2024 at 06:41:14PM +0100, Sebastian Andrzej Siewior wrote:
> Hi,
> 
> This is an updated version of the inital 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 splitted it into smaller chunks which can be applied/ reviewed
> independently. I'm just not sure about the cfi patch (28/28) so I added
> just a comment instead.
> 
> v1…v2: https://lore.kernel.org/all/20241205215102.hRywUW2A@linutronix.de/
>   - Splitted into smaller patches.
>   - Converted all users.

Thanks for doing the cleanup!

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>