On Fri, Mar 6, 2026 at 6:10 PM Petr Pavlu <[email protected]> wrote: > > On 3/6/26 12:43 AM, Sami Tolvanen wrote: > > On Wed, Dec 31, 2025 at 05:40:04PM +0800, Yafang Shao wrote: > >> --- a/kernel/module/main.c > >> +++ b/kernel/module/main.c > >> @@ -3901,7 +3901,11 @@ 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, true)); > >> + pr_cont(" %s", mod->name); > >> + /* Only append version for out-of-tree modules */ > >> + if (mod->version && test_bit(TAINT_OOT_MODULE, &mod->taints)) > >> + pr_cont("-%s", mod->version); > >> + pr_cont("%s", module_flags(mod, buf, true)); > > > > On second thought, is using mod->version here safe? We release the > > memory for mod->version in: > > > > free_module > > -> mod_sysfs_teardown > > -> module_remove_modinfo_attrs > > -> attr->free = free_modinfo_version > > > > And this happens before the module is removed from the > > list. Couldn't there be a race condition where we read a non-NULL > > mod->version here, but the buffer is being concurrently released > > by another core that's unloading the module, resulting in a > > use-after-free in the pr_cont call? > > > > In order to do this safely, we should presumably drop the attr->free > > call from module_remove_modinfo_attrs and release the attributes > > only after the synchronize_rcu call in free_module (there's already > > free_modinfo we can use), so mod->version is valid for the entire > > time the module is on the list. > > This looks reasonable to me as a simple fix.
I will send a fix for it first. > I also noticed that > setup_modinfo() with its attr->setup() calls is invoked unconditionally > in kernel/module/main.c, while module_remove_modinfo_attrs() with > attr->free() is present in kernel/module/sysfs.c, which is conditional > on CONFIG_SYSFS. In the unlikely configuration where CONFIG_SYSFS=n and > CONFIG_MODULES=y, this can result in a memory leak of module::version > when a module is unloaded. > > In general, I think this could benefit from more cleanup in the future. > Most of the code related to modinfo_attrs should be moved into > kernel/module/sysfs.c. Following the change suggested by Sami, the modinfo_attrs definition, along with attr->free() and attr->setup(), remains in kernel/module/main.c. Should any of these components be relocated to kernel/module/sysfs.c? > Since module::version is now used from > print_modules(), which is part of the general module loader code, the > initialization of the variable should be independent of all sysfs logic. > Ideally, the sysfs code should only read module::version and no longer > manage it. After the above change, both the allocation and freeing of module::version now reside in kernel/module/main.c. Is this sufficient? -- Regards Yafang
