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

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

On Wed, 05 Feb 2025 17:50:36 -0500
Steven Rostedt [off-list ref] wrote:

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) [off-list ref]

On Thu, 6 Feb 2025 14:28:17 +0900
Masami Hiramatsu (Google) [off-list ref] wrote:

quoted

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

quoted

+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) [off-list ref] wrote:

quoted

quoted

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

quoted

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

quoted

quoted

+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 2025-02-10 14:04:35 [+0100], Petr Pavlu wrote:

quoted

quoted

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 [off-list ref] wrote:

quoted

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 [off-list ref] wrote:

quoted

quoted

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 [off-list ref] wrote:

quoted

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 [off-list ref] 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 [off-list ref] wrote:

quoted

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