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 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. 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.
--
Thanks,
Petr
