[PATCH 5/8] module: Add module_for_each_mod() function

[PATCH 5/8] module: Add module_for_each_mod() function

[Steven Rostedt]

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;
 }
 
+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

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Masami Hiramatsu]

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


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Steven Rostedt]

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

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Petr Pavlu]

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/

-- 
Thanks,
Petr

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

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Sebastian Andrzej Siewior]

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

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Steven Rostedt]

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

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Luis Chamberlain]

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

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Steven Rostedt]

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

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Steven Rostedt]

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.

Re: [PATCH 5/8] module: Add module_for_each_mod() function

[Luis Chamberlain]

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