[RFC] Revert "kernel/params.c: defer most of param_sysfs_init() to late_initcall time"

[RFC] Revert "kernel/params.c: defer most of param_sysfs_init() to late_initcall time"

[Shyam Saini]

This reverts commit 96a1a2412acba8c057c041833641d9b7dbf52170,
as it breaks the creation of /sys/module/<built-in-module>/drivers.

The reverted commit changed the initcall order for
param_sysfs_builtin() from subsys_initcall() to late_initcall(),
which impacts the module_kset list and its population.

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
at this point due to the reverted change. As a consequence,
module_add_driver() returns early without calling make_driver_name().
Therefore, /sys/module/<built-in-module>/drivers is never created.

This breaks user-space applications for eg: DPDK, which expect
/sys/module/vfio_pci/drivers/pci:vfio-pci/new_id to be present.

This revert restores the initcall order for param_sysfs_builtin()
and fixes the above mentioned issue.

Fixes: 96a1a2412acb ("kernel/params.c: defer most of param_sysfs_init() to late_initcall time")
Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
---
 kernel/params.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 0074d29c9b80..890b44d36b1e 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -958,11 +958,7 @@ const struct kobj_type module_ktype = {
 };
 
 /*
- * param_sysfs_init - create "module" kset
- *
- * This must be done before the initramfs is unpacked and
- * request_module() thus becomes possible, because otherwise the
- * module load would fail in mod_sysfs_init.
+ * param_sysfs_init - wrapper for built-in params support
  */
 static int __init param_sysfs_init(void)
 {
@@ -973,24 +969,11 @@ static int __init param_sysfs_init(void)
 		return -ENOMEM;
 	}
 
-	return 0;
-}
-subsys_initcall(param_sysfs_init);
-
-/*
- * param_sysfs_builtin_init - add sysfs version and parameter
- * attributes for built-in modules
- */
-static int __init param_sysfs_builtin_init(void)
-{
-	if (!module_kset)
-		return -ENOMEM;
-
 	version_sysfs_builtin();
 	param_sysfs_builtin();
 
 	return 0;
 }
-late_initcall(param_sysfs_builtin_init);
+subsys_initcall(param_sysfs_init);
 
 #endif /* CONFIG_SYSFS */
-- 
2.34.1

Re: [RFC] Revert "kernel/params.c: defer most of param_sysfs_init() to late_initcall time"

[Rasmus Villemoes]

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

> This reverts commit 96a1a2412acba8c057c041833641d9b7dbf52170,
> as it breaks the creation of /sys/module/<built-in-module>/drivers.
>
> The reverted commit changed the initcall order for
> param_sysfs_builtin() from subsys_initcall() to late_initcall(),
> which impacts the module_kset list and its population.
>
> 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
> at this point due to the reverted change. As a consequence,
> module_add_driver() returns early without calling make_driver_name().
> Therefore, /sys/module/<built-in-module>/drivers is never created.
>
> This breaks user-space applications for eg: DPDK, which expect
> /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id to be present.
>

:(

Unfortunately, the init time saved by the mentioned commit is important
for some boards, and reverting that commit now will mean that some of
those boards may spuriously fail to boot due to random timing and the
external watchdog firing.

I wonder why this has taken 2+ years to be noticed?

Since the problem is that /sys/module/vfio_pci is not (yet) created as a side
effect of version_sysfs_builtin() and/or param_sysfs_builtin(), both of
which use locate_module_kobject() to lookup-or-create that, maybe the
code in module_add_driver() could be reworked to use that.

It's of course not entirely trivial, as locate_module_kobject() is
currently __init and static, but that should be fixable if we want to go
that route. Maybe it should be refactored a little to be callable from
the builtin-module branch of module_add_driver(), but ignoring that,
something like

diff --git a/drivers/base/module.c b/drivers/base/module.c
index 5bc71bea883a..6b32c5fec283 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);
+		mk = locate_module_kobject(drv->mod_name);
+		if (mk) {
 			/* remember our module structure */
 			drv->p->mkobj = mk;
-			/* kset_find_obj took a reference */
-			kobject_put(mkobj);
+			/* locate_module_kobject took a reference */
+			kobject_put(&mk->mkobj);
 		}
 	}
 
It also seems to me that if a module has neither a MODULE_VERSION nor
any module parameters, /sys/module/<modname> wouldn't be created as part
of that param_sysfs_builtin(), regardless of commit 96a1a2412acb. So
relying on that sysfs directory existing seems to be a little fragile;
IOW I do think that module_add_driver() should itself ensure that the
module_kobject exists.

Rasmus

[RFC] Revert "kernel/params.c: defer most of param_sysfs_init() to late_initcall time"

[Shyam Saini]

Hi Rasmus,

> On Thu, Jan 30 2025, Shyam Saini <shyamsaini@linux.microsoft.com> wrote:
> 
> > This reverts commit 96a1a2412acba8c057c041833641d9b7dbf52170,
> > as it breaks the creation of /sys/module/<built-in-module>/drivers.
> >
> > The reverted commit changed the initcall order for
> > param_sysfs_builtin() from subsys_initcall() to late_initcall(),
> > which impacts the module_kset list and its population.
> >
> > 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
> > at this point due to the reverted change. As a consequence,
> > module_add_driver() returns early without calling make_driver_name().
> > Therefore, /sys/module/<built-in-module>/drivers is never created.
> >
> > This breaks user-space applications for eg: DPDK, which expect
> > /sys/module/vfio_pci/drivers/pci:vfio-pci/new_id to be present.
> >
> 
> :(
> 
> Unfortunately, the init time saved by the mentioned commit is important
> for some boards, and reverting that commit now will mean that some of
> those boards may spuriously fail to boot due to random timing and the
> external watchdog firing.
> 
> I wonder why this has taken 2+ years to be noticed?

Unfortunately, we couldn't detect it earlier, sorry about that.
I am fairly suprised no one else reported this issue.

> Since the problem is that /sys/module/vfio_pci is not (yet) created as a side
> effect of version_sysfs_builtin() and/or param_sysfs_builtin(), both of
> which use locate_module_kobject() to lookup-or-create that, maybe the
> code in module_add_driver() could be reworked to use that.
> 
> It's of course not entirely trivial, as locate_module_kobject() is
> currently __init and static, but that should be fixable if we want to go
> that route. Maybe it should be refactored a little to be callable from
> the builtin-module branch of module_add_driver(), but ignoring that,
> something like
> 
> diff --git a/drivers/base/module.c b/drivers/base/module.c
> index 5bc71bea883a..6b32c5fec283 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);
> +		mk = locate_module_kobject(drv->mod_name);
> +		if (mk) {
>  			/* remember our module structure */
>  			drv->p->mkobj = mk;
> -			/* kset_find_obj took a reference */
> -			kobject_put(mkobj);
> +			/* locate_module_kobject took a reference */
> +			kobject_put(&mk->mkobj);
>  		}
>  	}
>  
> It also seems to me that if a module has neither a MODULE_VERSION nor
> any module parameters, /sys/module/<modname> wouldn't be created as part
> of that param_sysfs_builtin(), regardless of commit 96a1a2412acb. So
> relying on that sysfs directory existing seems to be a little fragile;
> IOW I do think that module_add_driver() should itself ensure that the
> module_kobject exists.

I have reworked this patch as per your suggestions, will send it shortly.

Thanks,
Shyam

[RFC] Revert "kernel/params.c: defer most of param_sysfs_init() to late_initcall time"

[Shyam Saini]

Adding more folks as per Luis's suggestion, sorry for the noise.