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 <[email protected]> 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/[email protected]?part=2
