[v1 0/3] Properly handle module_kboject creation

[v1 0/3] Properly handle module_kboject creation

[Shyam Saini]

Hi Everyone,

This patch series fixes handling of module_kboject creation.
A driver expect module_kset list populated with its corresponding
module_kboject 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 deffered 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_kboject 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].

Changes since initial RFC(Based on Rasmus's suggestions):
Patch 1: Renames locate_module_kobject() to lookup_or_create_module_kobject(),
         to accurately describe its operations.
Patch 2: Moves lookup_or_create_module_kobject() and to_module* macros to
         module.h, so that driver code can use these.
Patch 3: Handles module_kboject creation and population of module_kset list
         to fix [1] and [2] issues.

[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

Shyam Saini (3):
  kernel: param: rename locate_module_kobject
  include: move lookup_or_create_module_kobject()/to_module* to module.h
  drivers: base: handle module_kboject creation

 drivers/base/module.c  | 13 +++++--------
 include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++
 kernel/params.c        | 44 ++----------------------------------------
 3 files changed, 46 insertions(+), 50 deletions(-)

-- 
2.34.1

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

[Shyam Saini]

locate_module_kobject() look up for existing module_kobject
For given module name locate_module_kobject() look up for
corresponding module_kobject if it exists. If it coudln't
find its corresponding module_kobject then it creates one
for the given name.
Rename 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

[v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h

[Shyam Saini]

Move the following to module.h to allow common usages:
 - lookup_or_create_module_kobject()
 - to_module_attr
 - to_module_kobject

This makes lookup_or_create_module_kobject() as inline.

Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
---
 include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
 kernel/params.c        | 39 ---------------------------------------
 2 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 12f8a7d4fc1c..ba5f5e6c3927 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
+/* sysfs output in /sys/modules/XYZ/parameters/ */
+#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
+#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
 extern struct kset *module_kset;
 extern const struct kobj_type module_ktype;
+
+static inline struct module_kobject * __init 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,
+					   "%s", name);
+#ifdef CONFIG_MODULES
+		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);
+	}
+
+	return mk;
+}
+
 #endif /* CONFIG_SYSFS */
 
 #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
diff --git a/kernel/params.c b/kernel/params.c
index 4b43baaf7c83..3c0798b79f76 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -537,10 +537,6 @@ const struct kernel_param_ops param_ops_string = {
 };
 EXPORT_SYMBOL(param_ops_string);
 
-/* sysfs output in /sys/modules/XYZ/parameters/ */
-#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
-#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
-
 struct param_attribute
 {
 	struct module_attribute mattr;
@@ -763,41 +759,6 @@ 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 *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,
-					   "%s", name);
-#ifdef CONFIG_MODULES
-		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);
-	}
-
-	return mk;
-}
-
 static void __init kernel_add_sysfs_param(const char *name,
 					  const struct kernel_param *kparam,
 					  unsigned int name_skip)
-- 
2.34.1

[v1 3/3] drivers: base: handle module_kboject creation

[Shyam Saini]

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

Since,
commit 96a1a2412acb ("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: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h

[Christoph Hellwig]

> +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)

Crazily unreadable line.

Either way this is not a function that should be inline in a header.
Both due to sheer size, but also due to what it does and what it pulls
in.

> +{
> +	struct module_kobject *mk;
> +	struct kobject *kobj;
> +	int err;
> +
> +	kobj = kset_find_obj(module_kset, name);
> +	if (kobj) {
> +		mk = to_module_kobject(kobj);
> +	} else {

And I know this is just moving the code, but an ealy return here
would significanty clean up the function..

> +		mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> +		BUG_ON(!mk);

.. and a BUG_ON on allocation failure is a no-go.

Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h

[Christophe Leroy]

Le 04/02/2025 à 06:22, Shyam Saini a écrit :
> [Vous ne recevez pas souvent de courriers de shyamsaini@linux.microsoft.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> Move the following to module.h to allow common usages:
>   - lookup_or_create_module_kobject()
>   - to_module_attr
>   - to_module_kobject
> 
> This makes lookup_or_create_module_kobject() as inline.

Why an inline ? It looks quite big for an inline. Why not just make it 
global ?

> 
> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> ---
>   include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
>   kernel/params.c        | 39 ---------------------------------------
>   2 files changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..ba5f5e6c3927 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
>   #endif /* CONFIG_MODULES */
> 
>   #ifdef CONFIG_SYSFS
> +/* sysfs output in /sys/modules/XYZ/parameters/ */
> +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
>   extern struct kset *module_kset;
>   extern const struct kobj_type module_ktype;
> +
> +static inline struct module_kobject * __init 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);

Would look cleaner without the else. Something like:

	if (kobj)
		return to_module_kobject(kobj);

	mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
...


> +       } else {
> +               mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> +               BUG_ON(!mk);

Why a BUG_ON() ? Why not just return NULL and let the caller handle the 
failure ?

> +
> +               mk->mod = THIS_MODULE;
> +               mk->kobj.kset = module_kset;
> +               err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> +                                          "%s", name);
> +#ifdef CONFIG_MODULES

This #ifdef is not needed, module_uevent is declared at all time in 
module.h, IS_ENABLED(CONFIG_MODULES) should be enough.

> +               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);
> +       }
> +
> +       return mk;
> +}
> +
>   #endif /* CONFIG_SYSFS */
> 
>   #define symbol_request(x) try_then_request_module(symbol_get(x), "symbol:" #x)
> diff --git a/kernel/params.c b/kernel/params.c
> index 4b43baaf7c83..3c0798b79f76 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -537,10 +537,6 @@ const struct kernel_param_ops param_ops_string = {
>   };
>   EXPORT_SYMBOL(param_ops_string);
> 
> -/* sysfs output in /sys/modules/XYZ/parameters/ */
> -#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> -#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> -
>   struct param_attribute
>   {
>          struct module_attribute mattr;
> @@ -763,41 +759,6 @@ 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 *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,
> -                                          "%s", name);
> -#ifdef CONFIG_MODULES
> -               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);
> -       }
> -
> -       return mk;
> -}
> -
>   static void __init kernel_add_sysfs_param(const char *name,
>                                            const struct kernel_param *kparam,
>                                            unsigned int name_skip)
> --
> 2.34.1
> 
> 

Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h

[Rasmus Villemoes]

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

> Move the following to module.h to allow common usages:
>  - lookup_or_create_module_kobject()
>  - to_module_attr
>  - to_module_kobject
>
> This makes lookup_or_create_module_kobject() as inline.
>
> Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> ---
>  include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
>  kernel/params.c        | 39 ---------------------------------------
>  2 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 12f8a7d4fc1c..ba5f5e6c3927 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
>  #endif /* CONFIG_MODULES */
>  
>  #ifdef CONFIG_SYSFS
> +/* sysfs output in /sys/modules/XYZ/parameters/ */
> +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
>  extern struct kset *module_kset;
>  extern const struct kobj_type module_ktype;
> +
> +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)

As others have said, this is way too big for an inline. Also, __init is
at best harmless (if it is indeed inlined into all callers), but if for
whatever reason the compiler decides to emit an OOL version, we
definitely do not want that in .init.text as we are now calling it from
non-init code.

> +{
> +	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);
> +

And while the BUG_ON() is borderline acceptable in the current,
only-used-during-init, state, that definitely must go away once the
function can be called from non-init code.

This is what I alluded to when I said that the function might need some
refactoring before module_add_driver can start using it.

I'd say that the BUG_ON can simply be removed and replaced by a return
NULL; an "impossible" error case that can already be hit by some of the
code below, so callers have to be prepared for it anyway. If the
allocation fails (during early boot or later), the allocator already
makes a lot of noise, so there's no reason to even do a WARN_ON, and
since it "only" affects certain /sys objects, it's probably better to
let the machine try to complete boot instead of killing it.

Also, I agree with the suggestion to drop/outdent the whole else{} branch by
changing the if() to do "return to_module_kobject(kobj);".

To summarize:

- refactor to do an early return

- drop BUG_ON, replace by return NULL.

- drop static and __init, perhaps change __init to __modinit (which is a
  pre-existing #define in params.c, so the function remains __init if
  !CONFIG_MODULES), add an appropriate declaration somewhere, and if you
  like, also do the rename

- make use of it from module_add_driver()

Rasmus

Re: [v1 2/3] include: move lookup_or_create_module_kobject()/to_module* to module.h

[Shyam Saini]

Hi Everyone,

On Wed, Feb 05, 2025 at 09:43:12AM +0100, Rasmus Villemoes wrote:
> On Mon, Feb 03 2025, Shyam Saini <shyamsaini@linux.microsoft.com> wrote:
> 
> > Move the following to module.h to allow common usages:
> >  - lookup_or_create_module_kobject()
> >  - to_module_attr
> >  - to_module_kobject
> >
> > This makes lookup_or_create_module_kobject() as inline.
> >
> > Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
> > ---
> >  include/linux/module.h | 39 +++++++++++++++++++++++++++++++++++++++
> >  kernel/params.c        | 39 ---------------------------------------
> >  2 files changed, 39 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 12f8a7d4fc1c..ba5f5e6c3927 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -894,8 +894,47 @@ static inline void *module_writable_address(struct module *mod, void *loc)
> >  #endif /* CONFIG_MODULES */
> >  
> >  #ifdef CONFIG_SYSFS
> > +/* sysfs output in /sys/modules/XYZ/parameters/ */
> > +#define to_module_attr(n) container_of_const(n, struct module_attribute, attr)
> > +#define to_module_kobject(n) container_of(n, struct module_kobject, kobj)
> >  extern struct kset *module_kset;
> >  extern const struct kobj_type module_ktype;
> > +
> > +static inline struct module_kobject * __init lookup_or_create_module_kobject(const char *name)
> 
> As others have said, this is way too big for an inline. Also, __init is
> at best harmless (if it is indeed inlined into all callers), but if for
> whatever reason the compiler decides to emit an OOL version, we
> definitely do not want that in .init.text as we are now calling it from
> non-init code.
> 
> > +{
> > +	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);
> > +
> 
> And while the BUG_ON() is borderline acceptable in the current,
> only-used-during-init, state, that definitely must go away once the
> function can be called from non-init code.
> 
> This is what I alluded to when I said that the function might need some
> refactoring before module_add_driver can start using it.
> 
> I'd say that the BUG_ON can simply be removed and replaced by a return
> NULL; an "impossible" error case that can already be hit by some of the
> code below, so callers have to be prepared for it anyway. If the
> allocation fails (during early boot or later), the allocator already
> makes a lot of noise, so there's no reason to even do a WARN_ON, and
> since it "only" affects certain /sys objects, it's probably better to
> let the machine try to complete boot instead of killing it.
> 
> Also, I agree with the suggestion to drop/outdent the whole else{} branch by
> changing the if() to do "return to_module_kobject(kobj);".
> 
> To summarize:
> 
> - refactor to do an early return
> 
> - drop BUG_ON, replace by return NULL.
> 
> - drop static and __init, perhaps change __init to __modinit (which is a
>   pre-existing #define in params.c, so the function remains __init if
>   !CONFIG_MODULES), add an appropriate declaration somewhere, and if you
>   like, also do the rename
> 
> - make use of it from module_add_driver()

I have addressed these feedback in v2, thank you for the reviews.

Thanks,
Shyam