Re: [PATCH 2/3] module: Fix data race on mod_unload_taint::count

[sashiko-bot@kernel.org]

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