| Message ID | 20210128181421.2279-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] powerpc/powernv: remove get_cxl_module | expand |
Hi Christoph, Christoph Hellwig <hch@lst.de> writes: > diff --git a/kernel/module.c b/kernel/module.c > index 981302f616b411..6772fb2680eb3e 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -668,7 +668,6 @@ static struct module *find_module_all(const char *name, size_t len, > > struct module *find_module(const char *name) > { > - module_assert_mutex(); Does it make sense to replace the assert above with the warn below (untested)? RCU_LOCKDEP_WARN(rcu_read_lock_sched_held()); > return find_module_all(name, strlen(name), false); > }
On Thu, Jan 28, 2021 at 05:50:56PM -0300, Thiago Jung Bauermann wrote: > > struct module *find_module(const char *name) > > { > > - module_assert_mutex(); > > Does it make sense to replace the assert above with the warn below (untested)? > > RCU_LOCKDEP_WARN(rcu_read_lock_sched_held()); One caller actually holds module_mutex still. And find_module_all, which implements the actual logic already asserts that either module_mutex is held or rcu_read_lock, so I don't tink we need an extra one here.
Christoph Hellwig <hch@lst.de> writes: > On Thu, Jan 28, 2021 at 05:50:56PM -0300, Thiago Jung Bauermann wrote: >> > struct module *find_module(const char *name) >> > { >> > - module_assert_mutex(); >> >> Does it make sense to replace the assert above with the warn below (untested)? >> >> RCU_LOCKDEP_WARN(rcu_read_lock_sched_held()); > > One caller actually holds module_mutex still. And find_module_all, > which implements the actual logic already asserts that either > module_mutex is held or rcu_read_lock, so I don't tink we need an > extra one here. Ok, thanks for the clarification.
On Fri, Jan 29, 2021 at 04:29:02PM +0100, Miroslav Benes wrote: > > > > - mutex_lock(&module_mutex); > > + rcu_read_lock_sched(); > > /* > > * We do not want to block removal of patched modules and therefore > > * we do not take a reference here. The patches are removed by > > @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object *obj) > > if (mod && mod->klp_alive) > > RCU always baffles me a bit, so I'll ask. Don't we need > rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I > wonder. rcu_dereference* is only used for dereferencing points where that reference itself is RCU protected, that is the lookup of mod itself down in find_module_all in this case.
+++ Miroslav Benes [29/01/21 16:29 +0100]: >On Thu, 28 Jan 2021, Christoph Hellwig wrote: > >> Allow for a RCU-sched critical section around find_module, following >> the lower level find_module_all helper, and switch the two callers >> outside of module.c to use such a RCU-sched critical section instead >> of module_mutex. > >That's a nice idea. > >> @@ -57,7 +58,7 @@ static void klp_find_object_module(struct klp_object *obj) >> if (!klp_is_module(obj)) >> return; >> >> - mutex_lock(&module_mutex); >> + rcu_read_lock_sched(); >> /* >> * We do not want to block removal of patched modules and therefore >> * we do not take a reference here. The patches are removed by >> @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object *obj) >> if (mod && mod->klp_alive) > >RCU always baffles me a bit, so I'll ask. Don't we need >rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I >wonder. Same here :-) I had to double check the RCU documentation. For our modules list case I believe the rcu list API should take care of that for us. Worth noting is this snippet from Documentation/RCU/whatisRCU.txt: rcu_dereference() is typically used indirectly, via the _rcu list-manipulation primitives, such as list_for_each_entry_rcu()
On Mon, 1 Feb 2021, Jessica Yu wrote: > +++ Miroslav Benes [29/01/21 16:29 +0100]: > >On Thu, 28 Jan 2021, Christoph Hellwig wrote: > > > >> Allow for a RCU-sched critical section around find_module, following > >> the lower level find_module_all helper, and switch the two callers > >> outside of module.c to use such a RCU-sched critical section instead > >> of module_mutex. > > > >That's a nice idea. > > > >> @@ -57,7 +58,7 @@ static void klp_find_object_module(struct klp_object > >> *obj) > >> if (!klp_is_module(obj)) > >> return; > >> > >> - mutex_lock(&module_mutex); > >> + rcu_read_lock_sched(); > >> /* > >> * We do not want to block removal of patched modules and therefore > >> * we do not take a reference here. The patches are removed by > >> @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object > >> *obj) > >> if (mod && mod->klp_alive) > > > >RCU always baffles me a bit, so I'll ask. Don't we need > >rcu_dereference_sched() here? "mod" comes from a RCU-protected list, so I > >wonder. > > Same here :-) I had to double check the RCU documentation. For our > modules list case I believe the rcu list API should take care of that > for us. Worth noting is this snippet from Documentation/RCU/whatisRCU.txt: > > rcu_dereference() is typically used indirectly, via the _rcu > list-manipulation primitives, such as list_for_each_entry_rcu() Ok, thanks to both for checking and explanation. Ack to the patch then. Miroslav
diff --git a/include/linux/module.h b/include/linux/module.h index 7a0bcb5b1ffccd..a64aa84d1b182c 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -586,7 +586,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 hold module_mutex. */ +/* Search for module by name: must be in a RCU-sched critical section. */ struct module *find_module(const char *name); struct symsearch { diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index f76fdb9255323d..262cd9b003b9f0 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -19,6 +19,7 @@ #include <linux/moduleloader.h> #include <linux/completion.h> #include <linux/memory.h> +#include <linux/rcupdate.h> #include <asm/cacheflush.h> #include "core.h" #include "patch.h" @@ -57,7 +58,7 @@ static void klp_find_object_module(struct klp_object *obj) if (!klp_is_module(obj)) return; - mutex_lock(&module_mutex); + rcu_read_lock_sched(); /* * We do not want to block removal of patched modules and therefore * we do not take a reference here. The patches are removed by @@ -74,7 +75,7 @@ static void klp_find_object_module(struct klp_object *obj) if (mod && mod->klp_alive) obj->mod = mod; - mutex_unlock(&module_mutex); + rcu_read_unlock_sched(); } static bool klp_initialized(void) diff --git a/kernel/module.c b/kernel/module.c index 981302f616b411..6772fb2680eb3e 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -668,7 +668,6 @@ static struct module *find_module_all(const char *name, size_t len, struct module *find_module(const char *name) { - module_assert_mutex(); return find_module_all(name, strlen(name), false); } diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index e6fba1798771b4..3137992baa5e7a 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -124,9 +124,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk) if (!p) return true; *p = '\0'; - mutex_lock(&module_mutex); + rcu_read_lock_sched(); ret = !!find_module(tk->symbol); - mutex_unlock(&module_mutex); + rcu_read_unlock_sched(); *p = ':'; return ret;
Allow for a RCU-sched critical section around find_module, following the lower level find_module_all helper, and switch the two callers outside of module.c to use such a RCU-sched critical section instead of module_mutex. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/module.h | 2 +- kernel/livepatch/core.c | 5 +++-- kernel/module.c | 1 - kernel/trace/trace_kprobe.c | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-)