[v2 0/3] Properly handle module_kobject creation

[v2 0/3] Properly handle module_kobject creation

[Shyam Saini]

Hi Everyone,

This patch series fixes handling of module_kobject creation.
A driver expect module_kset list populated with its corresponding
module_kobject to create its /sys/module/<built-in-module>/drivers
directory.

Since,
[1] commit 96a1a2412acb ("kernel/params.c: defer most of param_sysfs_init() to late_initcall time")
Call to populate module_kset list is deferred to save init time so that
external watchdog doesn't fireup on some boards and Linux can take
responsibility of feeding watchdog before it spuriously resets the
system. However, [1] this fix caused another issue i.e, consumers
of module_kset can't get related module_kobject during driver
initialisation and hence can't create their
/sys/module/<built-in-module>/drivers directory.

Consequently, [1] breaks user-space applications for eg: DPDK, which
expects /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id to be present.

The second issue was reported and the [2] revert of [1] was
proposed. However, [2] the Revert doesn't address original issue
reported in [1].

This patch series addresses both issues reported in [1] and [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=96a1a2412acb
[2] https://lore.kernel.org/lkml/20250130225803.321004-1-shyamsaini@linux.microsoft.com/

Thanks,
Shyam

---
Changes in v2:
- Undo moving of to_module* and lookup_or_create_module_kobject() to module.h
- Refactor and globalize lookup_or_create_module_kobject()
- move __modinit macro construct to module.h
- Fix commit message typos

Changes in v1: 
- Rename locate_module_kobject() to lookup_or_create_module_kobject() to accurately
  describe its operations.
- Moves lookup_or_create_module_kobject() and to_module* macros to module.h, so
  that driver code can use these.
- Handle module_kobject creation and population of module_kset list to fix [1]
  and [2] issues.
- Link: https://lore.kernel.org/lkml/20250204052222.1611510-1-shyamsaini@linux.microsoft.com/

Changes in RFC:
         Reverts commit 96a1a2412acb ("kernel/params.c: defer most of param_sysfs_init() to late_initcall time")
---

Shyam Saini (3):
  kernel: param: rename locate_module_kobject
  kernel: refactor and globalize lookup_or_create_module_kobject()
  drivers: base: handle module_kobject creation

 drivers/base/module.c  | 13 ++++-------
 include/linux/module.h |  8 +++++++
 kernel/params.c        | 53 ++++++++++++++++++------------------------
 3 files changed, 36 insertions(+), 38 deletions(-)

-- 
2.34.1

[v2 1/3] kernel: param: rename locate_module_kobject

[Shyam Saini]

The locate_module_kobject() function looks up an existing
module_kobject for a given module name. If it cannot find the
corresponding module_kobject, it creates one for the given name.

This commit renames locate_module_kobject() to
lookup_or_create_module_kobject() to better describe its operations.

This doesn't change anything functionality wise.

Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
---
 kernel/params.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 0074d29c9b80..4b43baaf7c83 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -763,7 +763,7 @@ void destroy_params(const struct kernel_param *params, unsigned num)
 			params[i].ops->free(params[i].arg);
 }
 
-static struct module_kobject * __init locate_module_kobject(const char *name)
+static struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
 {
 	struct module_kobject *mk;
 	struct kobject *kobj;
@@ -805,10 +805,9 @@ static void __init kernel_add_sysfs_param(const char *name,
 	struct module_kobject *mk;
 	int err;
 
-	mk = locate_module_kobject(name);
+	mk = lookup_or_create_module_kobject(name);
 	if (!mk)
 		return;
-
 	/* We need to remove old parameters before adding more. */
 	if (mk->mp)
 		sysfs_remove_group(&mk->kobj, &mk->mp->grp);
@@ -876,7 +875,7 @@ static void __init version_sysfs_builtin(void)
 	int err;
 
 	for (vattr = __start___modver; vattr < __stop___modver; vattr++) {
-		mk = locate_module_kobject(vattr->module_name);
+		mk = lookup_or_create_module_kobject(vattr->module_name);
 		if (mk) {
 			err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
 			WARN_ON_ONCE(err);
-- 
2.34.1

[v2 2/3] kernel: refactor and globalize lookup_or_create_module_kobject()

[Shyam Saini]

lookup_or_create_module_kobject() is static and marked as __init,
this is not ideal for global usage.

Fix this limitation by refactoring and declaring this as global:
 - Refactor it by removing BUG_ON() and 'if else' construct by returning
   early
 - Remove static and __init markers from the function and add its
   declaration in module.h
 - Mark this function as "__modinit". To facilitate this, move the
   __modinit macro construct to module.h

Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
---
 include/linux/module.h |  8 +++++++
 kernel/params.c        | 48 ++++++++++++++++++------------------------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 12f8a7d4fc1c..57d09b4e4385 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -162,6 +162,14 @@ extern void cleanup_module(void);
 #define __INITRODATA_OR_MODULE __INITRODATA
 #endif /*CONFIG_MODULES*/
 
+#ifdef CONFIG_MODULES
+#define __modinit
+#else
+#define __modinit __init
+#endif
+
+struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name);
+
 /* Generic info of form tag = "info" */
 #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
 
diff --git a/kernel/params.c b/kernel/params.c
index 4b43baaf7c83..5d16696b1daa 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -595,12 +595,6 @@ static ssize_t param_attr_store(const struct module_attribute *mattr,
 }
 #endif
 
-#ifdef CONFIG_MODULES
-#define __modinit
-#else
-#define __modinit __init
-#endif
-
 #ifdef CONFIG_SYSFS
 void kernel_param_lock(struct module *mod)
 {
@@ -763,38 +757,38 @@ void destroy_params(const struct kernel_param *params, unsigned num)
 			params[i].ops->free(params[i].arg);
 }
 
-static struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
+struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name)
 {
 	struct module_kobject *mk;
 	struct kobject *kobj;
 	int err;
 
 	kobj = kset_find_obj(module_kset, name);
-	if (kobj) {
-		mk = to_module_kobject(kobj);
-	} else {
-		mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
-		BUG_ON(!mk);
-
-		mk->mod = THIS_MODULE;
-		mk->kobj.kset = module_kset;
-		err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
+	if (kobj)
+		return to_module_kobject(kobj);
+
+	mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
+	if (!mk)
+		return NULL;
+
+	mk->mod = THIS_MODULE;
+	mk->kobj.kset = module_kset;
+	err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
 					   "%s", name);
 #ifdef CONFIG_MODULES
-		if (!err)
-			err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
+	if (!err)
+		err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
 #endif
-		if (err) {
-			kobject_put(&mk->kobj);
-			pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
-				name, err);
-			return NULL;
-		}
-
-		/* So that we hold reference in both cases. */
-		kobject_get(&mk->kobj);
+	if (err) {
+		kobject_put(&mk->kobj);
+		pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
+			name, err);
+		return NULL;
 	}
 
+	/* So that we hold reference in both cases. */
+	kobject_get(&mk->kobj);
+
 	return mk;
 }
 
-- 
2.34.1

[v2 3/3] drivers: base: handle module_kobject creation

[Shyam Saini]

module_add_driver() relies on module_kset list for
/sys/module/<built-in-module>/drivers directory creation.

Since,
commit 96a1a2412acba ("kernel/params.c: defer most of param_sysfs_init() to late_initcall time")
drivers which are initialized from subsys_initcall() or any other
higher precedence initcall couldn't find the related kobject entry
in the module_kset list because module_kset is not fully populated
by the time module_add_driver() refers it. As a consequence,
module_add_driver() returns early without calling make_driver_name().
Therefore, /sys/module/<built-in-module>/drivers is never created.

Fix this issue by letting module_add_driver() handle module_kobject
creation itself.

Fixes: 96a1a2412acb ("kernel/params.c: defer most of param_sysfs_init() to late_initcall time")
Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
---
 drivers/base/module.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/base/module.c b/drivers/base/module.c
index 5bc71bea883a..218aaa096455 100644
--- a/drivers/base/module.c
+++ b/drivers/base/module.c
@@ -42,16 +42,13 @@ int module_add_driver(struct module *mod, const struct device_driver *drv)
 	if (mod)
 		mk = &mod->mkobj;
 	else if (drv->mod_name) {
-		struct kobject *mkobj;
-
-		/* Lookup built-in module entry in /sys/modules */
-		mkobj = kset_find_obj(module_kset, drv->mod_name);
-		if (mkobj) {
-			mk = container_of(mkobj, struct module_kobject, kobj);
+		/* Lookup or create built-in module entry in /sys/modules */
+		mk = lookup_or_create_module_kobject(drv->mod_name);
+		if (mk) {
 			/* remember our module structure */
 			drv->p->mkobj = mk;
-			/* kset_find_obj took a reference */
-			kobject_put(mkobj);
+			/* lookup_or_create_module_kobject took a reference */
+			kobject_put(&mk->kobj);
 		}
 	}
 
-- 
2.34.1

Re: [v2 2/3] kernel: refactor and globalize lookup_or_create_module_kobject()

[Thomas Weißschuh]

On 2025-02-06 21:45:37-0800, Shyam Saini wrote:
> lookup_or_create_module_kobject() is static and marked as __init,
> this is not ideal for global usage.

FYI missing "PATCH" in patch subject.

> Fix this limitation by refactoring and declaring this as global:
>  - Refactor it by removing BUG_ON() and 'if else' construct by returning
>    early

This does look like an unrelated change, could be in its own patch.

>  - Remove static and __init markers from the function and add its
>    declaration in module.h
>  - Mark this function as "__modinit". To facilitate this, move the
>    __modinit macro construct to module.h
> 
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> ---
>  include/linux/module.h |  8 +++++++
>  kernel/params.c        | 48 ++++++++++++++++++------------------------
>  2 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..57d09b4e4385 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -162,6 +162,14 @@ extern void cleanup_module(void);
>  #define __INITRODATA_OR_MODULE __INITRODATA
>  #endif /*CONFIG_MODULES*/
>  
> +#ifdef CONFIG_MODULES
> +#define __modinit
> +#else
> +#define __modinit __init
> +#endif
> +
> +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name);

__init / __modinit is not necessary on the declaration.
You can remove it here and keep the #define private.

> +
>  /* Generic info of form tag = "info" */
>  #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
>  
> diff --git a/kernel/params.c b/kernel/params.c
> index 4b43baaf7c83..5d16696b1daa 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -595,12 +595,6 @@ static ssize_t param_attr_store(const struct module_attribute *mattr,
>  }
>  #endif
>  
> -#ifdef CONFIG_MODULES
> -#define __modinit
> -#else
> -#define __modinit __init
> -#endif
> -
>  #ifdef CONFIG_SYSFS
>  void kernel_param_lock(struct module *mod)
>  {
> @@ -763,38 +757,38 @@ void destroy_params(const struct kernel_param *params, unsigned num)
>  			params[i].ops->free(params[i].arg);
>  }
>  
> -static struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
> +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name)
>  {
>  	struct module_kobject *mk;
>  	struct kobject *kobj;
>  	int err;
>  
>  	kobj = kset_find_obj(module_kset, name);
> -	if (kobj) {
> -		mk = to_module_kobject(kobj);
> -	} else {
> -		mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> -		BUG_ON(!mk);
> -
> -		mk->mod = THIS_MODULE;
> -		mk->kobj.kset = module_kset;
> -		err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> +	if (kobj)
> +		return to_module_kobject(kobj);
> +
> +	mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> +	if (!mk)
> +		return NULL;
> +
> +	mk->mod = THIS_MODULE;
> +	mk->kobj.kset = module_kset;
> +	err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
>  					   "%s", name);
>  #ifdef CONFIG_MODULES

As you are cleaning this up anyways: The #ifdef above should become

if (IS_ENABLED(CONFIG_MODULES))

> -		if (!err)
> -			err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> +	if (!err)
> +		err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
>  #endif
> -		if (err) {
> -			kobject_put(&mk->kobj);
> -			pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> -				name, err);
> -			return NULL;
> -		}
> -
> -		/* So that we hold reference in both cases. */
> -		kobject_get(&mk->kobj);
> +	if (err) {
> +		kobject_put(&mk->kobj);
> +		pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> +			name, err);
> +		return NULL;
>  	}
>  
> +	/* So that we hold reference in both cases. */
> +	kobject_get(&mk->kobj);
> +
>  	return mk;
>  }
>  
> -- 
> 2.34.1
> 

Re: [v2 2/3] kernel: refactor and globalize lookup_or_create_module_kobject()

[Rasmus Villemoes]

On Thu, Feb 06 2025, Shyam Saini <shyamsaini@linux.microsoft.com> wrote:

> lookup_or_create_module_kobject() is static and marked as __init,
> this is not ideal for global usage.
>
> Fix this limitation by refactoring and declaring this as global:
>  - Refactor it by removing BUG_ON() and 'if else' construct by returning
>    early
>  - Remove static and __init markers from the function and add its
>    declaration in module.h
>  - Mark this function as "__modinit". To facilitate this, move the
>    __modinit macro construct to module.h
>

I think this does too much in one patch.

More or less any time you find yourself writing such a bullet list, ask
yourself it that doesn't mean you're really writing three distinct
commit messages (and thus should make it three patches).

This also forces you to explicitly justify the removal of BUG_ON(),
instead of merely stating that the patch does that (the "what" is clear
from the patch itself, the commit message is for the "why"). Something like

  In the unlikely event of the allocation failing, it is better to let
  the machine boot with a not fully populated sysfs than to kill it with
  this BUG_ON(). All callers are already prepared for
  lookup_or_create_module_kobject() returning NULL.

  This is also preparation for calling this function from non-__init
  code, where using BUG_ON for allocation failure handling is not
  acceptable.

That will also make it much easier to verify that the refactoring commit
does only that and doesn't accidentally introduce some subtle change.

Actually, since you're not moving the function anymore (and shouldn't),
you don't really _need_ to include that refactoring patch, but it would be
a nice thing to have since you're touching this code anyway.


> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> ---
>  include/linux/module.h |  8 +++++++
>  kernel/params.c        | 48 ++++++++++++++++++------------------------
>  2 files changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..57d09b4e4385 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -162,6 +162,14 @@ extern void cleanup_module(void);
>  #define __INITRODATA_OR_MODULE __INITRODATA
>  #endif /*CONFIG_MODULES*/
>  
> +#ifdef CONFIG_MODULES
> +#define __modinit
> +#else
> +#define __modinit __init
> +#endif
> +
> +struct module_kobject __modinit * lookup_or_create_module_kobject(const char *name);
> +

No, the section placement doesn't need to, and should not, be on the
declaration, that just belongs with the definition. So there's no reason
to move that __modinit definition and make it globally visible.

Rasmus