[PATCH v2 0/3] module: Show the last unloaded module's taint flag(s)

[PATCH v2 0/3] module: Show the last unloaded module's taint flag(s)

[Aaron Tomlin]

Hi Luis, Christophe,

In addition to the previous iteration, since this particular series does
indeed modify last_unloaded_module, I decided to use strscpy() as a
replacement for the now deprecated strlcpy().

Changes since v1 [1][2]:

  - Replaced the deprecated strlcpy() for strscpy()
  - Replaced last_unloaded_module[] with an anonymous structure
    i.e. last_unloaded_module.name and last_unloaded_module.taints
  - Ensured we modify last_unloaded_module.taints only when required

[1]: https://lore.kernel.org/lkml/20220627164052.2416485-1-atomlin@redhat.com/
[2]: https://lore.kernel.org/lkml/20220627164052.2416485-2-atomlin@redhat.com/


Aaron Tomlin (3):
  module: Modify module_flags() to accept show_state argument
  module: Use strscpy() for last_unloaded_module
  module: Show the last unloaded module's taint flag(s)

 kernel/module/internal.h |  2 +-
 kernel/module/main.c     | 27 ++++++++++++++++++---------
 kernel/module/procfs.c   |  2 +-
 3 files changed, 20 insertions(+), 11 deletions(-)

-- 
2.34.3

[PATCH v2 1/3] module: Modify module_flags() to accept show_state argument

[Aaron Tomlin]

No functional change.

With this patch a given module's state information (i.e. 'mod->state')
can be omitted from the specified buffer. Please note that this is in
preparation to include the last unloaded module's taint flag(s),
if available.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/internal.h |  2 +-
 kernel/module/main.c     | 11 +++++++----
 kernel/module/procfs.c   |  2 +-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index bc5507ab8450..60312f23c7d0 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -100,7 +100,7 @@ struct module *find_module_all(const char *name, size_t len, bool even_unformed)
 int cmp_name(const void *name, const void *sym);
 long module_get_offset(struct module *mod, unsigned int *size, Elf_Shdr *sechdr,
 		       unsigned int section);
-char *module_flags(struct module *mod, char *buf);
+char *module_flags(struct module *mod, char *buf, bool show_state);
 size_t module_flags_taint(unsigned long taints, char *buf);
 
 static inline void module_assert_mutex_or_preempt(void)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index fed58d30725d..a4d23b0ebbc0 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2970,24 +2970,27 @@ static void cfi_cleanup(struct module *mod)
 }
 
 /* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
-char *module_flags(struct module *mod, char *buf)
+char *module_flags(struct module *mod, char *buf, bool show_state)
 {
 	int bx = 0;
 
 	BUG_ON(mod->state == MODULE_STATE_UNFORMED);
+	if (!mod->taints && !show_state)
+		goto out;
 	if (mod->taints ||
 	    mod->state == MODULE_STATE_GOING ||
 	    mod->state == MODULE_STATE_COMING) {
 		buf[bx++] = '(';
 		bx += module_flags_taint(mod->taints, buf + bx);
 		/* Show a - for module-is-being-unloaded */
-		if (mod->state == MODULE_STATE_GOING)
+		if (mod->state == MODULE_STATE_GOING && show_state)
 			buf[bx++] = '-';
 		/* Show a + for module-is-being-loaded */
-		if (mod->state == MODULE_STATE_COMING)
+		if (mod->state == MODULE_STATE_COMING && show_state)
 			buf[bx++] = '+';
 		buf[bx++] = ')';
 	}
+out:
 	buf[bx] = '\0';
 
 	return buf;
@@ -3120,7 +3123,7 @@ void print_modules(void)
 	list_for_each_entry_rcu(mod, &modules, list) {
 		if (mod->state == MODULE_STATE_UNFORMED)
 			continue;
-		pr_cont(" %s%s", mod->name, module_flags(mod, buf));
+		pr_cont(" %s%s", mod->name, module_flags(mod, buf, true));
 	}
 
 	print_unloaded_tainted_modules();
diff --git a/kernel/module/procfs.c b/kernel/module/procfs.c
index 9a8f4f0f6329..cf5b9f1e6ec4 100644
--- a/kernel/module/procfs.c
+++ b/kernel/module/procfs.c
@@ -91,7 +91,7 @@ static int m_show(struct seq_file *m, void *p)
 
 	/* Taints info */
 	if (mod->taints)
-		seq_printf(m, " %s", module_flags(mod, buf));
+		seq_printf(m, " %s", module_flags(mod, buf, true));
 
 	seq_puts(m, "\n");
 	return 0;
-- 
2.34.3

[PATCH v2 2/3] module: Use strscpy() for last_unloaded_module

[Aaron Tomlin]

The use of strlcpy() is considered deprecated [1].
In this particular context, there is no need to remain with strlcpy().
Therefore we transition to strscpy().

[1]: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index a4d23b0ebbc0..c5db13d06995 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -754,7 +754,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 	async_synchronize_full();
 
 	/* Store the name of the last unloaded module for diagnostic purposes */
-	strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
+	strscpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
 
 	free_module(mod);
 	/* someone could wait for the module in add_unformed_module() */
-- 
2.34.3

[PATCH v2 3/3] module: Show the last unloaded module's taint flag(s)

[Aaron Tomlin]

For diagnostic purposes, this patch, in addition to keeping a record/or
track of the last known unloaded module, we now will include the
module's taint flag(s) too e.g: " [last unloaded: fpga_mgr_mod(OE)]"

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/main.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index c5db13d06995..96ec7f94228d 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -524,7 +524,10 @@ static struct module_attribute modinfo_##field = {                    \
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
 
-static char last_unloaded_module[MODULE_NAME_LEN+1];
+static struct {
+	char name[MODULE_NAME_LEN + 1];
+	char taints[MODULE_FLAGS_BUF_SIZE];
+} last_unloaded_module;
 
 #ifdef CONFIG_MODULE_UNLOAD
 
@@ -694,6 +697,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 {
 	struct module *mod;
 	char name[MODULE_NAME_LEN];
+	char buf[MODULE_FLAGS_BUF_SIZE];
 	int ret, forced = 0;
 
 	if (!capable(CAP_SYS_MODULE) || modules_disabled)
@@ -753,8 +757,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 
 	async_synchronize_full();
 
-	/* Store the name of the last unloaded module for diagnostic purposes */
-	strscpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
+	/* Store the name and taints of the last unloaded module for diagnostic purposes */
+	strscpy(last_unloaded_module.name, mod->name, sizeof(last_unloaded_module.name));
+	strscpy(last_unloaded_module.taints, module_flags(mod, buf, false), sizeof(last_unloaded_module.taints));
 
 	free_module(mod);
 	/* someone could wait for the module in add_unformed_module() */
@@ -3128,7 +3133,8 @@ void print_modules(void)
 
 	print_unloaded_tainted_modules();
 	preempt_enable();
-	if (last_unloaded_module[0])
-		pr_cont(" [last unloaded: %s]", last_unloaded_module);
+	if (last_unloaded_module.name[0])
+		pr_cont(" [last unloaded: %s%s]", last_unloaded_module.name,
+			last_unloaded_module.taints);
 	pr_cont("\n");
 }
-- 
2.34.3

Re: [PATCH v2 0/3] module: Show the last unloaded module's taint flag(s)

[Luis Chamberlain]

On Thu, Jul 14, 2022 at 04:39:30PM +0100, Aaron Tomlin wrote:
> Hi Luis, Christophe,
> 
> In addition to the previous iteration, since this particular series does
> indeed modify last_unloaded_module, I decided to use strscpy() as a
> replacement for the now deprecated strlcpy().

Nice, looks super clean now, applied and pushed to modules-next, thanks!
BTW since you and Christophe seem to contribute a lot to modules lately,
any chance for future stuff you can ask 0day folks to add your trees
and branches to get tested prior to posting patches? Then you can
suggest in your cover letter they've been blessed by 0-day or whatever.

Typically I push to modules-testing, wait and if no complaints come back
then I push to modules-next but in this case I just went to
modules-next directly as I couldn't see how this could break.

  Luis