| Message ID | 20250205225103.760856859@goodmis.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Wed, 05 Feb 2025 17:50:36 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > The tracing system needs a way to save all the currently loaded modules > and their addresses into persistent memory so that it can evaluate the > addresses on a reboot from a crash. When the persistent memory trace > starts, it will load the module addresses and names into the persistent > memory. To do so, it will call the module_for_each_mod() function and pass > it a function and data structure to get called on each loaded module. Then > it can record the memory. > > This only implements that function. > > Cc: Luis Chamberlain <mcgrof@kernel.org> > Cc: Petr Pavlu <petr.pavlu@suse.com> > Cc: Sami Tolvanen <samitolvanen@google.com> > Cc: Daniel Gomez <da.gomez@samsung.com> > Cc: linux-modules@vger.kernel.org > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > include/linux/module.h | 6 ++++++ > kernel/module/main.c | 14 ++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/include/linux/module.h b/include/linux/module.h > index 23792d5d7b74..9e1f42f03511 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -779,6 +779,8 @@ static inline void *module_writable_address(struct module *mod, void *loc) > return __module_writable_address(mod, loc); > } > > +void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data); > + > #else /* !CONFIG_MODULES... */ > > static inline struct module *__module_address(unsigned long addr) > @@ -891,6 +893,10 @@ static inline void *module_writable_address(struct module *mod, void *loc) > { > return loc; > } > + > +static inline void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data) > +{ > +} > #endif /* CONFIG_MODULES */ > > #ifdef CONFIG_SYSFS > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 1fb9ad289a6f..ea1fe313fb89 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -3809,6 +3809,20 @@ bool is_module_text_address(unsigned long addr) > return ret; > } > It is better to add a kerneldoc for this API. /** * module_for_each_mod() - iterate all modules * @func: Callback function * @data: User data * * Call the @func with each module in the system. If @func returns !0, this * stops itrating. Note that @func must not sleep since it is called under * the preemption disabled. */ BTW, do we really need to disable preempt or is it enough to call rcu_read_lock()? Thank you, > +void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data) > +{ > + struct module *mod; > + > + preempt_disable(); > + list_for_each_entry_rcu(mod, &modules, list) { > + if (mod->state == MODULE_STATE_UNFORMED) > + continue; > + if (func(mod, data)) > + break; > + } > + preempt_enable(); > +} > + > /** > * __module_text_address() - get the module whose code contains an address. > * @addr: the address. > -- > 2.45.2 > >
On Thu, 6 Feb 2025 14:28:17 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > --- a/kernel/module/main.c > > +++ b/kernel/module/main.c > > @@ -3809,6 +3809,20 @@ bool is_module_text_address(unsigned long addr) > > return ret; > > } > > > > It is better to add a kerneldoc for this API. Agreed, but I was planning on this changing. Waiting to hear from the module maintainers. > > /** > * module_for_each_mod() - iterate all modules > * @func: Callback function > * @data: User data > * > * Call the @func with each module in the system. If @func returns !0, this > * stops itrating. Note that @func must not sleep since it is called under > * the preemption disabled. > */ > > BTW, do we really need to disable preempt or is it enough to call > rcu_read_lock()? Bah, as I expected this function to be changed, I didn't spend too much time on looking at its implementation. I just cut and pasted how the other loops worked. But yes, it should not be disabling preemption. In fact, I think the module code itself should not be disabling preemption! I'll have to go and look into that. Thanks! -- Steve > > Thank you, > > > +void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data) > > +{ > > + struct module *mod; > > + > > + preempt_disable(); > > + list_for_each_entry_rcu(mod, &modules, list) { > > + if (mod->state == MODULE_STATE_UNFORMED) > > + continue; > > + if (func(mod, data)) > > + break; > > + } > > + preempt_enable(); > > +} > > + > > /** > > * __module_text_address() - get the module whose code contains an address. > > * @addr: the address. > > --
On 2/6/25 16:27, Steven Rostedt wrote: > On Thu, 6 Feb 2025 14:28:17 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > >>> --- a/kernel/module/main.c >>> +++ b/kernel/module/main.c >>> @@ -3809,6 +3809,20 @@ bool is_module_text_address(unsigned long addr) >>> return ret; >>> } >>> >> >> It is better to add a kerneldoc for this API. > > Agreed, but I was planning on this changing. Waiting to hear from the > module maintainers. > >> >> /** >> * module_for_each_mod() - iterate all modules >> * @func: Callback function >> * @data: User data >> * >> * Call the @func with each module in the system. If @func returns !0, this >> * stops itrating. Note that @func must not sleep since it is called under >> * the preemption disabled. >> */ >> >> BTW, do we really need to disable preempt or is it enough to call >> rcu_read_lock()? > > Bah, as I expected this function to be changed, I didn't spend too much > time on looking at its implementation. I just cut and pasted how the other > loops worked. But yes, it should not be disabling preemption. In fact, I > think the module code itself should not be disabling preemption! > > I'll have to go and look into that. The series "module: Use RCU instead of RCU-sched" from Sebastian Andrzej Siewior cleans this up [1]. It is currently queued on modules-next (for 6.15-rc1). The new function module_for_each_mod() should then use "guard(rcu)();". [1] https://lore.kernel.org/linux-modules/20250108090457.512198-1-bigeasy@linutronix.de/
On 2025-02-10 14:04:35 [+0100], Petr Pavlu wrote: > >> BTW, do we really need to disable preempt or is it enough to call > >> rcu_read_lock()? > > > > Bah, as I expected this function to be changed, I didn't spend too much > > time on looking at its implementation. I just cut and pasted how the other > > loops worked. But yes, it should not be disabling preemption. In fact, I > > think the module code itself should not be disabling preemption! > > > > I'll have to go and look into that. > > The series "module: Use RCU instead of RCU-sched" from Sebastian Andrzej > Siewior cleans this up [1]. It is currently queued on modules-next (for > 6.15-rc1). > > The new function module_for_each_mod() should then use "guard(rcu)();". So the removal of the preempt-disable statements here already pays off. Nice. Sebastian
On Thu, 6 Feb 2025 10:27:20 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > > BTW, do we really need to disable preempt or is it enough to call > > rcu_read_lock()? > > Bah, as I expected this function to be changed, I didn't spend too much > time on looking at its implementation. I just cut and pasted how the other > loops worked. But yes, it should not be disabling preemption. In fact, I > think the module code itself should not be disabling preemption! > > I'll have to go and look into that. It really looks like it requires preempt_disable(), as the code in kernel/module/main.c has in several places: preempt_disable(); list_for_each_entry_rcu(mod, &modules, list) { [..] } preempt_enable(); Or module_assert_mutex_or_preempt(); [..] list_for_each_entry_rcu(mod, &modules, list, lockdep_is_held(&module_mutex)) { So it looks like it either requires preempt_disable or holding the module_mutex. As I need to call this with trace_types_lock held, and there's a place where trace_types_lock is within a module callback, I don't think it's safe to take that lock in that loop, otherwise we have the ABBA deadlock. Luis, Is this patch OK, and also is there any plan to move the module code to using just rcu_read_lock instead of preempt_disable? -- Steve
On Fri, Feb 14, 2025 at 05:30:17PM -0500, Steven Rostedt wrote: > On Thu, 6 Feb 2025 10:27:20 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > BTW, do we really need to disable preempt or is it enough to call > > > rcu_read_lock()? > > > > Bah, as I expected this function to be changed, I didn't spend too much > > time on looking at its implementation. I just cut and pasted how the other > > loops worked. But yes, it should not be disabling preemption. In fact, I > > think the module code itself should not be disabling preemption! > > > > I'll have to go and look into that. > > It really looks like it requires preempt_disable(), as the code in > kernel/module/main.c has in several places: > > preempt_disable(); > > list_for_each_entry_rcu(mod, &modules, list) { > [..] > } > > preempt_enable(); > > Or > > module_assert_mutex_or_preempt(); > > [..] > > list_for_each_entry_rcu(mod, &modules, list, > lockdep_is_held(&module_mutex)) { > > > So it looks like it either requires preempt_disable or holding the > module_mutex. > > As I need to call this with trace_types_lock held, and there's a place > where trace_types_lock is within a module callback, I don't think it's safe > to take that lock in that loop, otherwise we have the ABBA deadlock. > > Luis, > > Is this patch OK, and also is there any plan to move the module code to > using just rcu_read_lock instead of preempt_disable? The patch is not OK, you're looking at old code, look at modules-next and as Petr suggested look at Sebastian's recently merged work. git remote add korg-modules git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git Luis
On Tue, 18 Feb 2025 13:21:20 -0800 Luis Chamberlain <mcgrof@kernel.org> wrote: > > Luis, > > > > Is this patch OK, and also is there any plan to move the module code to > > using just rcu_read_lock instead of preempt_disable? > > The patch is not OK, you're looking at old code, look at > modules-next and as Petr suggested look at Sebastian's recently > merged work. > > git remote add korg-modules git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git Ha! Some how I missed seeing the two replies from Petr and Sebastian! Not sure how that happened. Something may be going wacky with claws-mail. :-/ Thanks for pointing them out. I'll definitely take a look. -- Steve
On Tue, 18 Feb 2025 13:21:20 -0800 Luis Chamberlain <mcgrof@kernel.org> wrote: > The patch is not OK, you're looking at old code, look at > modules-next and as Petr suggested look at Sebastian's recently > merged work. > > git remote add korg-modules git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git Would this be OK? I have this on v6.14-rc3, and it doesn't cause any conflicts when I merge it with modules-next, and it builds fine. -- Steve diff --git a/include/linux/module.h b/include/linux/module.h index 30e5b19bafa9..9a71dd2cb11f 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -782,6 +782,8 @@ static inline void *module_writable_address(struct module *mod, void *loc) return __module_writable_address(mod, loc); } +void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data); + #else /* !CONFIG_MODULES... */ static inline struct module *__module_address(unsigned long addr) @@ -894,6 +896,10 @@ static inline void *module_writable_address(struct module *mod, void *loc) { return loc; } + +static inline void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data) +{ +} #endif /* CONFIG_MODULES */ #ifdef CONFIG_SYSFS diff --git a/kernel/module/main.c b/kernel/module/main.c index 1fb9ad289a6f..1bd4e3b7098a 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -3809,6 +3809,19 @@ bool is_module_text_address(unsigned long addr) return ret; } +void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data) +{ + struct module *mod; + + guard(rcu)(); + list_for_each_entry_rcu(mod, &modules, list) { + if (mod->state == MODULE_STATE_UNFORMED) + continue; + if (func(mod, data)) + break; + } +} + /** * __module_text_address() - get the module whose code contains an address. * @addr: the address.
On Tue, Feb 18, 2025 at 07:24:46PM -0500, Steven Rostedt wrote: > On Tue, 18 Feb 2025 13:21:20 -0800 > Luis Chamberlain <mcgrof@kernel.org> wrote: > > > The patch is not OK, you're looking at old code, look at > > modules-next and as Petr suggested look at Sebastian's recently > > merged work. > > > > git remote add korg-modules git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git > > Would this be OK? > > I have this on v6.14-rc3, and it doesn't cause any conflicts when I merge > it with modules-next, and it builds fine. Seems more in line with what is used now. Luis
diff --git a/include/linux/module.h b/include/linux/module.h index 23792d5d7b74..9e1f42f03511 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -779,6 +779,8 @@ static inline void *module_writable_address(struct module *mod, void *loc) return __module_writable_address(mod, loc); } +void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data); + #else /* !CONFIG_MODULES... */ static inline struct module *__module_address(unsigned long addr) @@ -891,6 +893,10 @@ static inline void *module_writable_address(struct module *mod, void *loc) { return loc; } + +static inline void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data) +{ +} #endif /* CONFIG_MODULES */ #ifdef CONFIG_SYSFS diff --git a/kernel/module/main.c b/kernel/module/main.c index 1fb9ad289a6f..ea1fe313fb89 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -3809,6 +3809,20 @@ bool is_module_text_address(unsigned long addr) return ret; } +void module_for_each_mod(int(*func)(struct module *mod, void *data), void *data) +{ + struct module *mod; + + preempt_disable(); + list_for_each_entry_rcu(mod, &modules, list) { + if (mod->state == MODULE_STATE_UNFORMED) + continue; + if (func(mod, data)) + break; + } + preempt_enable(); +} + /** * __module_text_address() - get the module whose code contains an address. * @addr: the address.