All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] module: Fixes to tracking taint flags for unloaded modules
@ 2026-10-20 14:05 Petr Pavlu
  2026-10-20 14:05 ` [PATCH 1/3] module: Fix missing " Petr Pavlu
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Petr Pavlu @ 2026-10-20 14:05 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen
  Cc: Aaron Tomlin, linux-modules, linux-kernel

Fix missing taint flags for unloaded modules and a data race on
mod_unload_taint::count, plus move mod_unload_taint into tracking.c.

---
Petr Pavlu (3):
      module: Fix missing taint flags for unloaded modules
      module: Fix data race on mod_unload_taint::count
      module: Move mod_unload_taint to tracking.c

 kernel/module/internal.h |  7 -------
 kernel/module/tracking.c | 28 ++++++++++++++++++++++------
 2 files changed, 22 insertions(+), 13 deletions(-)
---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261020-taint-unload-f96d25823757

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] module: Fix missing taint flags for unloaded modules
  2026-10-20 14:05 [PATCH 0/3] module: Fixes to tracking taint flags for unloaded modules Petr Pavlu
@ 2026-10-20 14:05 ` Petr Pavlu
  2026-10-20 15:10   ` Aaron Tomlin
  2026-10-20 14:05 ` [PATCH 2/3] module: Fix data race on mod_unload_taint::count Petr Pavlu
  2026-10-20 14:05 ` [PATCH 3/3] module: Move mod_unload_taint to tracking.c Petr Pavlu
  2 siblings, 1 reply; 7+ messages in thread
From: Petr Pavlu @ 2026-10-20 14:05 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen
  Cc: Aaron Tomlin, linux-modules, linux-kernel

When recording taint flags for an unloaded module, try_add_tainted_module()
can fail to record some of the module's flags if a record for a module with
the same name already exists and the two have at least one flag in common.
This happens because the function uses 'mod_taint->taints & mod->taints' to
determine whether to merge the new module with the existing record. As
a result, the final report may not provide accurate information about the
taint flags of unloaded modules.

Fix the duplicate check by using the stricter equality comparison.

Fixes: 99bd9956551b ("module: Introduce module unload taint tracking")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/tracking.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
index 9033ff54c4e2..5bcd1c2701f0 100644
--- a/kernel/module/tracking.c
+++ b/kernel/module/tracking.c
@@ -27,7 +27,7 @@ int try_add_tainted_module(struct module *mod)
 	list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
 				lockdep_is_held(&module_mutex)) {
 		if (!strcmp(mod_taint->name, mod->name) &&
-		    mod_taint->taints & mod->taints) {
+		    mod_taint->taints == mod->taints) {
 			mod_taint->count++;
 			goto out;
 		}

-- 
2.55.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] module: Fix data race on mod_unload_taint::count
  2026-10-20 14:05 [PATCH 0/3] module: Fixes to tracking taint flags for unloaded modules Petr Pavlu
  2026-10-20 14:05 ` [PATCH 1/3] module: Fix missing " Petr Pavlu
@ 2026-10-20 14:05 ` Petr Pavlu
  2026-10-20 14:13   ` sashiko-bot
  2026-10-20 14:05 ` [PATCH 3/3] module: Move mod_unload_taint to tracking.c Petr Pavlu
  2 siblings, 1 reply; 7+ messages in thread
From: Petr Pavlu @ 2026-10-20 14:05 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen
  Cc: Aaron Tomlin, linux-modules, linux-kernel

Updates to mod_unload_taint::count are serialized by module_mutex. However,
readers may access this value concurrently with these updates. Since the
counter is a u64, this can result in torn reads on 32-bit architectures.

Fix the issue by changing mod_unload_taint::count to 'unsigned int' and
using WRITE_ONCE()/READ_ONCE() to access it.

Fixes: 99bd9956551b ("module: Introduce module unload taint tracking")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/internal.h |  8 +++++++-
 kernel/module/tracking.c | 12 ++++++++----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 061161cc79d9..70ef20b97c84 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -257,7 +257,13 @@ struct mod_unload_taint {
 	struct list_head list;
 	char name[MODULE_NAME_LEN];
 	unsigned long taints;
-	u64 count;
+
+	/*
+	 * Number of times a module with this name and taint combination has
+	 * been unloaded. Updates are serialized by module_mutex and use
+	 * WRITE_ONCE(). Readers that may run concurrently use READ_ONCE().
+	 */
+	unsigned int count;
 };
 
 int try_add_tainted_module(struct module *mod);
diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
index 5bcd1c2701f0..873fceb30526 100644
--- a/kernel/module/tracking.c
+++ b/kernel/module/tracking.c
@@ -28,7 +28,10 @@ int try_add_tainted_module(struct module *mod)
 				lockdep_is_held(&module_mutex)) {
 		if (!strcmp(mod_taint->name, mod->name) &&
 		    mod_taint->taints == mod->taints) {
-			mod_taint->count++;
+			unsigned int count = mod_taint->count;
+
+			if (count != UINT_MAX)
+				WRITE_ONCE(mod_taint->count, count + 1);
 			goto out;
 		}
 	}
@@ -57,8 +60,8 @@ void print_unloaded_tainted_modules(void)
 
 			l = module_flags_taint(mod_taint->taints, buf);
 			buf[l++] = '\0';
-			pr_cont(" %s(%s):%llu", mod_taint->name, buf,
-				mod_taint->count);
+			pr_cont(" %s(%s):%u", mod_taint->name, buf,
+				READ_ONCE(mod_taint->count));
 		}
 	}
 }
@@ -92,7 +95,8 @@ static int unloaded_tainted_modules_seq_show(struct seq_file *m, void *p)
 	l = module_flags_taint(mod_taint->taints, buf);
 	buf[l++] = '\0';
 
-	seq_printf(m, "%s (%s) %llu", mod_taint->name, buf, mod_taint->count);
+	seq_printf(m, "%s (%s) %u", mod_taint->name, buf,
+		   READ_ONCE(mod_taint->count));
 	seq_puts(m, "\n");
 
 	return 0;

-- 
2.55.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] module: Move mod_unload_taint to tracking.c
  2026-10-20 14:05 [PATCH 0/3] module: Fixes to tracking taint flags for unloaded modules Petr Pavlu
  2026-10-20 14:05 ` [PATCH 1/3] module: Fix missing " Petr Pavlu
  2026-10-20 14:05 ` [PATCH 2/3] module: Fix data race on mod_unload_taint::count Petr Pavlu
@ 2026-10-20 14:05 ` Petr Pavlu
  2026-10-20 15:09   ` Aaron Tomlin
  2 siblings, 1 reply; 7+ messages in thread
From: Petr Pavlu @ 2026-10-20 14:05 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen
  Cc: Aaron Tomlin, linux-modules, linux-kernel

Move the definition of mod_unload_taint from internal.h to tracking.c, as
it doesn't need to be exposed to other module code, and remove the
redundant declaration of mod_debugfs_root from tracking.c, as it is already
declared in internal.h.

This is a non-functional change.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/internal.h | 13 -------------
 kernel/module/tracking.c | 14 +++++++++++++-
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 70ef20b97c84..3aca22253296 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -253,19 +253,6 @@ static inline void kmod_dup_request_announce(char *module_name, int ret)
 #endif
 
 #ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
-struct mod_unload_taint {
-	struct list_head list;
-	char name[MODULE_NAME_LEN];
-	unsigned long taints;
-
-	/*
-	 * Number of times a module with this name and taint combination has
-	 * been unloaded. Updates are serialized by module_mutex and use
-	 * WRITE_ONCE(). Readers that may run concurrently use READ_ONCE().
-	 */
-	unsigned int count;
-};
-
 int try_add_tainted_module(struct module *mod);
 void print_unloaded_tainted_modules(void);
 #else /* !CONFIG_MODULE_UNLOAD_TAINT_TRACKING */
diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
index 873fceb30526..46840dc22dcb 100644
--- a/kernel/module/tracking.c
+++ b/kernel/module/tracking.c
@@ -14,8 +14,20 @@
 #include <linux/rculist.h>
 #include "internal.h"
 
+struct mod_unload_taint {
+	struct list_head list;
+	char name[MODULE_NAME_LEN];
+	unsigned long taints;
+
+	/*
+	 * Number of times a module with this name and taint combination has
+	 * been unloaded. Updates are serialized by module_mutex and use
+	 * WRITE_ONCE(). Readers that may run concurrently use READ_ONCE().
+	 */
+	unsigned int count;
+};
+
 static LIST_HEAD(unloaded_tainted_modules);
-extern struct dentry *mod_debugfs_root;
 
 int try_add_tainted_module(struct module *mod)
 {

-- 
2.55.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] module: Fix data race on mod_unload_taint::count
  2026-10-20 14:05 ` [PATCH 2/3] module: Fix data race on mod_unload_taint::count Petr Pavlu
@ 2026-10-20 14:13   ` sashiko-bot
  0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-10-20 14:13 UTC (permalink / raw)
  To: Petr Pavlu; +Cc: linux-modules

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] module: data race and uninitialized read of count in try_add_tainted_module()
--

commit 742b3a34fee074246edb757213ccfc148f6e7300
Author: Petr Pavlu <petr.pavlu@suse.com>

module: Fix data race on mod_unload_taint::count

This commit fixes a data race on mod_unload_taint::count between updates
serialized by module_mutex and concurrent RCU readers by changing the
field to unsigned int and using WRITE_ONCE()/READ_ONCE() accesses.

> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 061161cc79d90..70ef20b97c84a 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -257,7 +257,13 @@ struct mod_unload_taint {
>  	struct list_head list;
>  	char name[MODULE_NAME_LEN];
>  	unsigned long taints;
> -	u64 count;
> +
> +	/*
> +	 * Number of times a module with this name and taint combination has
> +	 * been unloaded. Updates are serialized by module_mutex and use
> +	 * WRITE_ONCE(). Readers that may run concurrently use READ_ONCE().
> +	 */
> +	unsigned int count;
>  };
>  
>  int try_add_tainted_module(struct module *mod);
> diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
> index 5bcd1c2701f07..873fceb305268 100644
> --- a/kernel/module/tracking.c
> +++ b/kernel/module/tracking.c
> @@ -28,7 +28,10 @@ int try_add_tainted_module(struct module *mod)
>  				lockdep_is_held(&module_mutex)) {
>  		if (!strcmp(mod_taint->name, mod->name) &&
>  		    mod_taint->taints == mod->taints) {
> -			mod_taint->count++;
> +			unsigned int count = mod_taint->count;
> +
> +			if (count != UINT_MAX)
> +				WRITE_ONCE(mod_taint->count, count + 1);
>  			goto out;
>  		}
>  	}

[Severity: Medium]
Does try_add_tainted_module() in kernel/module/tracking.c still have a
data race and potential uninitialized read on mod_taint->count when
allocating a new entry?

Further down in try_add_tainted_module(), a newly allocated entry is
published via list_add_rcu() before mod_taint->count is initialized:

	mod_taint = kmalloc_obj(*mod_taint);
	if (unlikely(!mod_taint))
		return -ENOMEM;
	strscpy(mod_taint->name, mod->name, MODULE_NAME_LEN);
	mod_taint->taints = mod->taints;
	list_add_rcu(&mod_taint->list, &unloaded_tainted_modules);
	mod_taint->count = 1;

Because kmalloc_obj() does not zero-initialize the allocation and
list_add_rcu() publishes the entry to unloaded_tainted_modules first,
concurrent RCU readers in print_unloaded_tainted_modules() or
unloaded_tainted_modules_seq_show() can observe mod_taint and execute
READ_ONCE(mod_taint->count) while mod_taint->count still contains
uninitialized slab memory, or race with the plain store setting it to 1.

Should mod_taint->count = 1 be initialized before calling list_add_rcu()?

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-taint-unload-v1-0-d1c764889afc@suse.com?part=2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] module: Move mod_unload_taint to tracking.c
  2026-10-20 14:05 ` [PATCH 3/3] module: Move mod_unload_taint to tracking.c Petr Pavlu
@ 2026-10-20 15:09   ` Aaron Tomlin
  0 siblings, 0 replies; 7+ messages in thread
From: Aaron Tomlin @ 2026-10-20 15:09 UTC (permalink / raw)
  To: Petr Pavlu
  Cc: Luis Chamberlain, Daniel Gomez, Sami Tolvanen, linux-modules,
	linux-kernel

On Tue, Oct 20, 2026 at 04:05:21PM +0200, Petr Pavlu wrote:
> Move the definition of mod_unload_taint from internal.h to tracking.c, as
> it doesn't need to be exposed to other module code, and remove the
> redundant declaration of mod_debugfs_root from tracking.c, as it is already
> declared in internal.h.
> 
> This is a non-functional change.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  kernel/module/internal.h | 13 -------------
>  kernel/module/tracking.c | 14 +++++++++++++-
>  2 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 70ef20b97c84..3aca22253296 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -253,19 +253,6 @@ static inline void kmod_dup_request_announce(char *module_name, int ret)
>  #endif
>  
>  #ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> -struct mod_unload_taint {
> -	struct list_head list;
> -	char name[MODULE_NAME_LEN];
> -	unsigned long taints;
> -
> -	/*
> -	 * Number of times a module with this name and taint combination has
> -	 * been unloaded. Updates are serialized by module_mutex and use
> -	 * WRITE_ONCE(). Readers that may run concurrently use READ_ONCE().
> -	 */
> -	unsigned int count;
> -};
> -
>  int try_add_tainted_module(struct module *mod);
>  void print_unloaded_tainted_modules(void);
>  #else /* !CONFIG_MODULE_UNLOAD_TAINT_TRACKING */
> diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
> index 873fceb30526..46840dc22dcb 100644
> --- a/kernel/module/tracking.c
> +++ b/kernel/module/tracking.c
> @@ -14,8 +14,20 @@
>  #include <linux/rculist.h>
>  #include "internal.h"
>  
> +struct mod_unload_taint {
> +	struct list_head list;
> +	char name[MODULE_NAME_LEN];
> +	unsigned long taints;
> +
> +	/*
> +	 * Number of times a module with this name and taint combination has
> +	 * been unloaded. Updates are serialized by module_mutex and use
> +	 * WRITE_ONCE(). Readers that may run concurrently use READ_ONCE().
> +	 */
> +	unsigned int count;
> +};
> +
>  static LIST_HEAD(unloaded_tainted_modules);
> -extern struct dentry *mod_debugfs_root;
>  
>  int try_add_tainted_module(struct module *mod)
>  {
> 
> -- 
> 2.55.0

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] module: Fix missing taint flags for unloaded modules
  2026-10-20 14:05 ` [PATCH 1/3] module: Fix missing " Petr Pavlu
@ 2026-10-20 15:10   ` Aaron Tomlin
  0 siblings, 0 replies; 7+ messages in thread
From: Aaron Tomlin @ 2026-10-20 15:10 UTC (permalink / raw)
  To: Petr Pavlu
  Cc: Luis Chamberlain, Daniel Gomez, Sami Tolvanen, linux-modules,
	linux-kernel

On Tue, Oct 20, 2026 at 04:05:19PM +0200, Petr Pavlu wrote:
> When recording taint flags for an unloaded module, try_add_tainted_module()
> can fail to record some of the module's flags if a record for a module with
> the same name already exists and the two have at least one flag in common.
> This happens because the function uses 'mod_taint->taints & mod->taints' to
> determine whether to merge the new module with the existing record. As
> a result, the final report may not provide accurate information about the
> taint flags of unloaded modules.
> 
> Fix the duplicate check by using the stricter equality comparison.
> 
> Fixes: 99bd9956551b ("module: Introduce module unload taint tracking")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  kernel/module/tracking.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
> index 9033ff54c4e2..5bcd1c2701f0 100644
> --- a/kernel/module/tracking.c
> +++ b/kernel/module/tracking.c
> @@ -27,7 +27,7 @@ int try_add_tainted_module(struct module *mod)
>  	list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
>  				lockdep_is_held(&module_mutex)) {
>  		if (!strcmp(mod_taint->name, mod->name) &&
> -		    mod_taint->taints & mod->taints) {
> +		    mod_taint->taints == mod->taints) {
>  			mod_taint->count++;
>  			goto out;
>  		}
> 
> -- 
> 2.55.0

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-10-20 15:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-20 14:05 [PATCH 0/3] module: Fixes to tracking taint flags for unloaded modules Petr Pavlu
2026-10-20 14:05 ` [PATCH 1/3] module: Fix missing " Petr Pavlu
2026-10-20 15:10   ` Aaron Tomlin
2026-10-20 14:05 ` [PATCH 2/3] module: Fix data race on mod_unload_taint::count Petr Pavlu
2026-10-20 14:13   ` sashiko-bot
2026-10-20 14:05 ` [PATCH 3/3] module: Move mod_unload_taint to tracking.c Petr Pavlu
2026-10-20 15:09   ` Aaron Tomlin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.