On 3/8/26 3:14 PM, Yafang Shao wrote:
> 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.
Ack.
>
>> 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?
I think we can further clean up this area by moving more items into
kernel/module/sysfs.c and untangling module::(src)version from the sysfs
code. A quick prototype is shown below. However, this doesn't need to be
done now and I can send this separately later.
--
Thanks,
Petr
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 791f9e444df8..dfff1b5fa463 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4296,7 +4296,7 @@ struct device *__root_device_register(const char *name,
struct module *owner)
return ERR_PTR(err);
}
-#ifdef CONFIG_MODULES /* gotta find a "cleaner" way to do this */
+#if defined(CONFIG_MODULES) && defined(CONFIG_SYSFS) /* gotta find a
"cleaner" way to do this */
if (owner) {
struct module_kobject *mk = &owner->mkobj;
diff --git a/include/linux/module.h b/include/linux/module.h
index 14f391b186c6..5959a9695d93 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -57,9 +57,7 @@ struct module_attribute {
char *);
ssize_t (*store)(const struct module_attribute *, struct module_kobject
*,
const char *, size_t count);
- void (*setup)(struct module *, const char *);
int (*test)(struct module *);
- void (*free)(struct module *);
};
struct module_version_attribute {
@@ -408,12 +406,15 @@ struct module {
unsigned char build_id[BUILD_ID_SIZE_MAX];
#endif
+ const char *version;
+ const char *srcversion;
+
+#ifdef CONFIG_SYSFS
/* Sysfs stuff. */
struct module_kobject mkobj;
struct module_attribute *modinfo_attrs;
- const char *version;
- const char *srcversion;
struct kobject *holders_dir;
+#endif
/* Exported symbols */
const struct kernel_symbol *syms;
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 618202578b42..cbaa14af9b5a 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -47,9 +47,6 @@ struct kernel_symbol {
extern struct mutex module_mutex;
extern struct list_head modules;
-extern const struct module_attribute *const modinfo_attrs[];
-extern const size_t modinfo_attrs_count;
-
/* Provided by the linker */
extern const struct kernel_symbol __start___ksymtab[];
extern const struct kernel_symbol __stop___ksymtab[];
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c3ce106c70af..c665c4144080 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -577,36 +577,6 @@ bool __is_module_percpu_address(unsigned long addr,
unsigned long *can_addr)
#endif /* CONFIG_SMP */
-#define MODINFO_ATTR(field) \
-static void setup_modinfo_##field(struct module *mod, const char *s) \
-{ \
- mod->field = kstrdup(s, GFP_KERNEL); \
-} \
-static ssize_t show_modinfo_##field(const struct module_attribute *mattr, \
- struct module_kobject *mk, char *buffer) \
-{ \
- return scnprintf(buffer, PAGE_SIZE, "%s\n", mk->mod->field); \
-} \
-static int modinfo_##field##_exists(struct module *mod) \
-{ \
- return mod->field != NULL; \
-} \
-static void free_modinfo_##field(struct module *mod) \
-{ \
- kfree(mod->field); \
- mod->field = NULL; \
-} \
-static const struct module_attribute modinfo_##field = { \
- .attr = { .name = __stringify(field), .mode = 0444 }, \
- .show = show_modinfo_##field, \
- .setup = setup_modinfo_##field, \
- .test = modinfo_##field##_exists, \
- .free = free_modinfo_##field, \
-};
-
-MODINFO_ATTR(version);
-MODINFO_ATTR(srcversion);
-
static struct {
char name[MODULE_NAME_LEN];
char taints[MODULE_FLAGS_BUF_SIZE];
@@ -886,15 +856,6 @@ void symbol_put_addr(void *addr)
}
EXPORT_SYMBOL_GPL(symbol_put_addr);
-static ssize_t show_refcnt(const struct module_attribute *mattr,
- struct module_kobject *mk, char *buffer)
-{
- return sprintf(buffer, "%i\n", module_refcount(mk->mod));
-}
-
-static const struct module_attribute modinfo_refcnt =
- __ATTR(refcnt, 0444, show_refcnt, NULL);
-
void __module_get(struct module *module)
{
if (module) {
@@ -961,118 +922,6 @@ size_t module_flags_taint(unsigned long taints, char *buf)
return l;
}
-static ssize_t show_initstate(const struct module_attribute *mattr,
- struct module_kobject *mk, char *buffer)
-{
- const char *state = "unknown";
-
- switch (mk->mod->state) {
- case MODULE_STATE_LIVE:
- state = "live";
- break;
- case MODULE_STATE_COMING:
- state = "coming";
- break;
- case MODULE_STATE_GOING:
- state = "going";
- break;
- default:
- BUG();
- }
- return sprintf(buffer, "%s\n", state);
-}
-
-static const struct module_attribute modinfo_initstate =
- __ATTR(initstate, 0444, show_initstate, NULL);
-
-static ssize_t store_uevent(const struct module_attribute *mattr,
- struct module_kobject *mk,
- const char *buffer, size_t count)
-{
- int rc;
-
- rc = kobject_synth_uevent(&mk->kobj, buffer, count);
- return rc ? rc : count;
-}
-
-const struct module_attribute module_uevent =
- __ATTR(uevent, 0200, NULL, store_uevent);
-
-static ssize_t show_coresize(const struct module_attribute *mattr,
- struct module_kobject *mk, char *buffer)
-{
- unsigned int size = mk->mod->mem[MOD_TEXT].size;
-
- if (!IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC)) {
- for_class_mod_mem_type(type, core_data)
- size += mk->mod->mem[type].size;
- }
- return sprintf(buffer, "%u\n", size);
-}
-
-static const struct module_attribute modinfo_coresize =
- __ATTR(coresize, 0444, show_coresize, NULL);
-
-#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
-static ssize_t show_datasize(const struct module_attribute *mattr,
- struct module_kobject *mk, char *buffer)
-{
- unsigned int size = 0;
-
- for_class_mod_mem_type(type, core_data)
- size += mk->mod->mem[type].size;
- return sprintf(buffer, "%u\n", size);
-}
-
-static const struct module_attribute modinfo_datasize =
- __ATTR(datasize, 0444, show_datasize, NULL);
-#endif
-
-static ssize_t show_initsize(const struct module_attribute *mattr,
- struct module_kobject *mk, char *buffer)
-{
- unsigned int size = 0;
-
- for_class_mod_mem_type(type, init)
- size += mk->mod->mem[type].size;
- return sprintf(buffer, "%u\n", size);
-}
-
-static const struct module_attribute modinfo_initsize =
- __ATTR(initsize, 0444, show_initsize, NULL);
-
-static ssize_t show_taint(const struct module_attribute *mattr,
- struct module_kobject *mk, char *buffer)
-{
- size_t l;
-
- l = module_flags_taint(mk->mod->taints, buffer);
- buffer[l++] = '\n';
- return l;
-}
-
-static const struct module_attribute modinfo_taint =
- __ATTR(taint, 0444, show_taint, NULL);
-
-const struct module_attribute *const modinfo_attrs[] = {
- &module_uevent,
- &modinfo_version,
- &modinfo_srcversion,
- &modinfo_initstate,
- &modinfo_coresize,
-#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
- &modinfo_datasize,
-#endif
- &modinfo_initsize,
- &modinfo_taint,
-#ifdef CONFIG_MODULE_UNLOAD
- &modinfo_refcnt,
-#endif
- NULL,
-};
-
-const size_t modinfo_attrs_count = ARRAY_SIZE(modinfo_attrs);
-
static const char vermagic[] = VERMAGIC_STRING;
int try_to_force_load(struct module *mod, const char *reason)
@@ -1762,14 +1611,11 @@ static void module_license_taint_check(struct module
*mod, const char *license)
static int setup_modinfo(struct module *mod, struct load_info *info)
{
- const struct module_attribute *attr;
char *imported_namespace;
- int i;
- for (i = 0; (attr = modinfo_attrs[i]); i++) {
- if (attr->setup)
- attr->setup(mod, get_modinfo(info, attr->attr.name));
- }
+ /* TODO Check for failed kstrdup(). */
+ mod->version = kstrdup(get_modinfo(info, "version"), GFP_KERNEL);
+ mod->srcversion = kstrdup(get_modinfo(info, "srcversion"), GFP_KERNEL);
for_each_modinfo_entry(imported_namespace, info, "import_ns") {
/*
@@ -1788,13 +1634,8 @@ static int setup_modinfo(struct module *mod, struct
load_info *info)
static void free_modinfo(struct module *mod)
{
- const struct module_attribute *attr;
- int i;
-
- for (i = 0; (attr = modinfo_attrs[i]); i++) {
- if (attr->free)
- attr->free(mod);
- }
+ kfree(mod->version);
+ kfree(mod->srcversion);
}
bool __weak module_init_section(const char *name)
@@ -3060,8 +2901,10 @@ static noinline int do_init_module(struct module *mod)
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_LIVE, mod);
+#ifdef CONFIG_SYSFS
/* Delay uevent until module has finished its init routine */
kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
+#endif
/*
* We need to finish all async code before the module init sequence
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index 01c65d608873..f2e3a625bcd5 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -14,6 +14,144 @@
#include <linux/mutex.h>
#include "internal.h"
+#define MODINFO_ATTR(field) \
+static ssize_t show_modinfo_##field(const struct module_attribute *mattr, \
+ struct module_kobject *mk, char *buffer) \
+{ \
+ return scnprintf(buffer, PAGE_SIZE, "%s\n", mk->mod->field); \
+} \
+static int modinfo_##field##_exists(struct module *mod) \
+{ \
+ return mod->field != NULL; \
+} \
+static const struct module_attribute modinfo_##field = { \
+ .attr = { .name = __stringify(field), .mode = 0444 }, \
+ .show = show_modinfo_##field, \
+ .test = modinfo_##field##_exists, \
+};
+
+MODINFO_ATTR(version);
+MODINFO_ATTR(srcversion);
+
+static ssize_t show_initstate(const struct module_attribute *mattr,
+ struct module_kobject *mk, char *buffer)
+{
+ const char *state = "unknown";
+
+ switch (mk->mod->state) {
+ case MODULE_STATE_LIVE:
+ state = "live";
+ break;
+ case MODULE_STATE_COMING:
+ state = "coming";
+ break;
+ case MODULE_STATE_GOING:
+ state = "going";
+ break;
+ default:
+ BUG();
+ }
+ return sprintf(buffer, "%s\n", state);
+}
+
+static const struct module_attribute modinfo_initstate =
+ __ATTR(initstate, 0444, show_initstate, NULL);
+
+static ssize_t store_uevent(const struct module_attribute *mattr,
+ struct module_kobject *mk,
+ const char *buffer, size_t count)
+{
+ int rc;
+
+ rc = kobject_synth_uevent(&mk->kobj, buffer, count);
+ return rc ? rc : count;
+}
+
+const struct module_attribute module_uevent =
+ __ATTR(uevent, 0200, NULL, store_uevent);
+
+static ssize_t show_coresize(const struct module_attribute *mattr,
+ struct module_kobject *mk, char *buffer)
+{
+ unsigned int size = mk->mod->mem[MOD_TEXT].size;
+
+ if (!IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC)) {
+ for_class_mod_mem_type(type, core_data)
+ size += mk->mod->mem[type].size;
+ }
+ return sprintf(buffer, "%u\n", size);
+}
+
+static const struct module_attribute modinfo_coresize =
+ __ATTR(coresize, 0444, show_coresize, NULL);
+
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+static ssize_t show_datasize(const struct module_attribute *mattr,
+ struct module_kobject *mk, char *buffer)
+{
+ unsigned int size = 0;
+
+ for_class_mod_mem_type(type, core_data)
+ size += mk->mod->mem[type].size;
+ return sprintf(buffer, "%u\n", size);
+}
+
+static const struct module_attribute modinfo_datasize =
+ __ATTR(datasize, 0444, show_datasize, NULL);
+#endif
+
+static ssize_t show_initsize(const struct module_attribute *mattr,
+ struct module_kobject *mk, char *buffer)
+{
+ unsigned int size = 0;
+
+ for_class_mod_mem_type(type, init)
+ size += mk->mod->mem[type].size;
+ return sprintf(buffer, "%u\n", size);
+}
+
+static const struct module_attribute modinfo_initsize =
+ __ATTR(initsize, 0444, show_initsize, NULL);
+
+static ssize_t show_taint(const struct module_attribute *mattr,
+ struct module_kobject *mk, char *buffer)
+{
+ size_t l;
+
+ l = module_flags_taint(mk->mod->taints, buffer);
+ buffer[l++] = '\n';
+ return l;
+}
+
+static const struct module_attribute modinfo_taint =
+ __ATTR(taint, 0444, show_taint, NULL);
+
+static ssize_t show_refcnt(const struct module_attribute *mattr,
+ struct module_kobject *mk, char *buffer)
+{
+ return sprintf(buffer, "%i\n", module_refcount(mk->mod));
+}
+
+static const struct module_attribute modinfo_refcnt =
+ __ATTR(refcnt, 0444, show_refcnt, NULL);
+
+static const struct module_attribute *const modinfo_attrs[] = {
+ &module_uevent,
+ &modinfo_version,
+ &modinfo_srcversion,
+ &modinfo_initstate,
+ &modinfo_coresize,
+#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
+ &modinfo_datasize,
+#endif
+ &modinfo_initsize,
+ &modinfo_taint,
+#ifdef CONFIG_MODULE_UNLOAD
+ &modinfo_refcnt,
+#endif
+ NULL,
+};
+
/*
* /sys/module/foo/sections stuff
* J. Corbet <[email protected]>
@@ -278,8 +416,6 @@ static void module_remove_modinfo_attrs(struct module *mod,
int end)
if (!attr->attr.name)
break;
sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
- if (attr->free)
- attr->free(mod);
}
kfree(mod->modinfo_attrs);
}
@@ -292,7 +428,7 @@ static int module_add_modinfo_attrs(struct module *mod)
int i;
mod->modinfo_attrs = kzalloc((sizeof(struct module_attribute) *
- (modinfo_attrs_count + 1)),
+ (ARRAY_SIZE(modinfo_attrs) + 1)),
GFP_KERNEL);
if (!mod->modinfo_attrs)
return -ENOMEM;
