[PATCH v3 0/4] Properly handle module_kobject creation

[PATCH v3 0/4] 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 the 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 v3:
  - Split refactoring and globalisation of lookup_or_create_module_kobject() into two patches
  - Fix commit message to better describe the change.
  - Use IS_ENABLED instead of #ifdef macro construct
  - Undo the moving of __modinit macro construct into module.h
  - drop __modinit from lookup_or_create_module_kobject() declaration

Changes in v2:
- Undo the moving of to_module* and lookup_or_create_module_kobject() into 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 (4):
  kernel: param: rename locate_module_kobject
  kernel: refactor lookup_or_create_module_kobject()
  kernel: globalize lookup_or_create_module_kobject()
  drivers: base: handle module_kobject creation

 drivers/base/module.c  | 13 +++++------
 include/linux/module.h |  2 ++
 kernel/params.c        | 49 ++++++++++++++++++++----------------------
 3 files changed, 30 insertions(+), 34 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] 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

Re: [PATCH v3 1/4] kernel: param: rename locate_module_kobject

[Petr Pavlu]

On 2/11/25 22:48, Shyam Saini wrote:
> 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);

Nit: Please restore the accidentally removed empty line.

-- 
Thanks,
Petr

[PATCH v3 2/4] kernel: refactor lookup_or_create_module_kobject()

[Shyam Saini]

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.

Since we are here, also start using IS_ENABLED instead of #ifdef
construct.

Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Shyam Saini <shyamsaini@linux.microsoft.com>
---
 kernel/params.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 4b43baaf7c83..6e87aef876b2 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -770,31 +770,28 @@ static struct module_kobject * __init lookup_or_create_module_kobject(const char
 	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;
-		}
+	if (kobj)
+		return to_module_kobject(kobj);
 
-		/* So that we hold reference in both cases. */
-		kobject_get(&mk->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);
+	if (IS_ENABLED(CONFIG_MODULES) && !err)
+		err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
+	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: [PATCH v3 2/4] kernel: refactor lookup_or_create_module_kobject()

[Petr Pavlu]

On 2/11/25 22:48, Shyam Saini wrote:
> 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.

I think some error reporting should be cleaned up here.

The current situation is that locate_module_kobject() can fail in
several cases and all these situations are loudly reported by the
function, either by BUG_ON() or pr_crit(). Consistently with that, both
its current callers version_sysfs_builtin() and kernel_add_sysfs_param()
don't do any reporting if locate_module_kobject() fails; they simply
return.

The series seems to introduce two somewhat suboptimal cases.

With this patch, when either version_sysfs_builtin() or
kernel_add_sysfs_param() calls lookup_or_create_module_kobject() and it
fails because of a potential kzalloc() error, the problem is silently
ignored.

Similarly, in the patch #4, when module_add_driver() calls
lookup_or_create_module_kobject() and the function fails, the problem
may or may not be reported, depending on the error.

I'd suggest something as follows:
* Drop the pr_crit() reporting in lookup_or_create_module_kobject().
* Have version_sysfs_builtin() and kernel_add_sysfs_param() log an error
  when lookup_or_create_module_kobject() fails. Using BUG_ON() might be
  appropriate, as that is already what is used in
  kernel_add_sysfs_param()?
* Update module_add_driver() to propagate any error from
  lookup_or_create_module_kobject() up the stack.

-- 
Thanks,
Petr

Re: [PATCH v3 2/4] kernel: refactor lookup_or_create_module_kobject()

[Rasmus Villemoes]

On Thu, Feb 13 2025, Petr Pavlu <petr.pavlu@suse.com> wrote:

> On 2/11/25 22:48, Shyam Saini wrote:
>> 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.
>
> I think some error reporting should be cleaned up here.
>
> The current situation is that locate_module_kobject() can fail in
> several cases and all these situations are loudly reported by the
> function, either by BUG_ON() or pr_crit(). Consistently with that, both
> its current callers version_sysfs_builtin() and kernel_add_sysfs_param()
> don't do any reporting if locate_module_kobject() fails; they simply
> return.
>
> The series seems to introduce two somewhat suboptimal cases.
>
> With this patch, when either version_sysfs_builtin() or
> kernel_add_sysfs_param() calls lookup_or_create_module_kobject() and it
> fails because of a potential kzalloc() error, the problem is silently
> ignored.
>

No, because (IIUC) when a basic allocation via kzalloc fails, the kernel
complains loudly already; there's no reason for every caller of
kmalloc() and friends to add their own pr_err("kmalloc failed"), that
just bloats the kernel .text.

> Similarly, in the patch #4, when module_add_driver() calls
> lookup_or_create_module_kobject() and the function fails, the problem
> may or may not be reported, depending on the error.
>
> I'd suggest something as follows:
> * Drop the pr_crit() reporting in lookup_or_create_module_kobject().

No, because that reports on something other than an allocation failing
(of course, it could be that the reason kobject_init_and_add or
sysfs_create_file failed was an allocation failure, but it could
also be something else), so reporting there is the right thing to do.

> * Have version_sysfs_builtin() and kernel_add_sysfs_param() log an error
>   when lookup_or_create_module_kobject() fails. Using BUG_ON() might be
>   appropriate, as that is already what is used in
>   kernel_add_sysfs_param()?

No, BUG_ON is almost never appropriate, and certainly not for something
which the machine can easily survice, albeit perhaps with some
functionality not available. That this had a BUG_ON is simply a
historical artefact, nothing more, and borderline acceptable as lazy
error handling in __init code, as small allocations during system init
simply don't fail (and if they did, the system would be unusable
anyway).

Rasmus

Re: [PATCH v3 2/4] kernel: refactor lookup_or_create_module_kobject()

[Petr Pavlu]

On 2/21/25 11:42, Rasmus Villemoes wrote:
> On Thu, Feb 13 2025, Petr Pavlu <petr.pavlu@suse.com> wrote:
> 
>> On 2/11/25 22:48, Shyam Saini wrote:
>>> 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.
>>
>> I think some error reporting should be cleaned up here.
>>
>> The current situation is that locate_module_kobject() can fail in
>> several cases and all these situations are loudly reported by the
>> function, either by BUG_ON() or pr_crit(). Consistently with that, both
>> its current callers version_sysfs_builtin() and kernel_add_sysfs_param()
>> don't do any reporting if locate_module_kobject() fails; they simply
>> return.
>>
>> The series seems to introduce two somewhat suboptimal cases.
>>
>> With this patch, when either version_sysfs_builtin() or
>> kernel_add_sysfs_param() calls lookup_or_create_module_kobject() and it
>> fails because of a potential kzalloc() error, the problem is silently
>> ignored.
>>
> 
> No, because (IIUC) when a basic allocation via kzalloc fails, the kernel
> complains loudly already; there's no reason for every caller of
> kmalloc() and friends to add their own pr_err("kmalloc failed"), that
> just bloats the kernel .text.

I wasn't suggesting to log a kmalloc() error specifically. The idea was
that if lookup_or_create_module_kobject() fails for whatever reason,
kernel_add_sysfs_param() and version_sysfs_builtin() should log the
failure to create/get the kobject.

> 
>> Similarly, in the patch #4, when module_add_driver() calls
>> lookup_or_create_module_kobject() and the function fails, the problem
>> may or may not be reported, depending on the error.
>>
>> I'd suggest something as follows:
>> * Drop the pr_crit() reporting in lookup_or_create_module_kobject().
> 
> No, because that reports on something other than an allocation failing
> (of course, it could be that the reason kobject_init_and_add or
> sysfs_create_file failed was an allocation failure, but it could
> also be something else), so reporting there is the right thing to do.

The error message says:
pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", name, err);

I think it was appropriate for locate_module_kobject() to do this error
reporting when the function was only called by code in kernel/params.c
and in the boot context. Now that the patch makes the function available
to external callers, I'm not sure if it should remain reporting this
error.

The function itself shouldn't directly make the system unstable when it
fails. Each caller can appropriately determine how to handle the error.
Functions kernel_add_sysfs_param() and version_sysfs_builtin() don't
have much of an option and can only log it, but module_add_driver()
could roll back its operation.

That's it, I guess my question is really whether module_add_driver()
should do that and handle errors from lookup_or_create_module_kobject()
by propagating them up the call stack. That would be my default if
writing this code from scratch. I don't quite see why calling
lookup_or_create_module_kobject() is special in this regard.

The rest of my suggestion flows from that. Function
lookup_or_create_module_kobject() should then similarly propagate any
error upwards. However, functions kernel_add_sysfs_param() and
version_sysfs_builtin() can't really do that and so actually need to
report the error themselves.

What do you think?

> 
>> * Have version_sysfs_builtin() and kernel_add_sysfs_param() log an error
>>   when lookup_or_create_module_kobject() fails. Using BUG_ON() might be
>>   appropriate, as that is already what is used in
>>   kernel_add_sysfs_param()?
> 
> No, BUG_ON is almost never appropriate, and certainly not for something
> which the machine can easily survice, albeit perhaps with some
> functionality not available. That this had a BUG_ON is simply a
> historical artefact, nothing more, and borderline acceptable as lazy
> error handling in __init code, as small allocations during system init
> simply don't fail (and if they did, the system would be unusable
> anyway).

I agree, just to clarify.. the reason why I mentioned using BUG_ON() in
this case was only for consistency with what's already in
kernel_add_sysfs_param(). I think it would be good to clean up other
BUG_ON() calls in that function later, but it didn't seem immediately
necessary to me as part of this series.

-- 
Thanks,
Petr

Re: [PATCH v3 2/4] kernel: refactor lookup_or_create_module_kobject()

[Shyam Saini]

Hi Petr,

Thank you for the reviews

On Tue, Feb 25, 2025 at 09:33:10AM +0100, Petr Pavlu wrote:
> On 2/21/25 11:42, Rasmus Villemoes wrote:
> > On Thu, Feb 13 2025, Petr Pavlu <petr.pavlu@suse.com> wrote:
> > 
> >> On 2/11/25 22:48, Shyam Saini wrote:
> >>> 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.
> >>
> >> I think some error reporting should be cleaned up here.
> >>
> >> The current situation is that locate_module_kobject() can fail in
> >> several cases and all these situations are loudly reported by the
> >> function, either by BUG_ON() or pr_crit(). Consistently with that, both
> >> its current callers version_sysfs_builtin() and kernel_add_sysfs_param()
> >> don't do any reporting if locate_module_kobject() fails; they simply
> >> return.
> >>
> >> The series seems to introduce two somewhat suboptimal cases.
> >>
> >> With this patch, when either version_sysfs_builtin() or
> >> kernel_add_sysfs_param() calls lookup_or_create_module_kobject() and it
> >> fails because of a potential kzalloc() error, the problem is silently
> >> ignored.
> >>
> > 
> > No, because (IIUC) when a basic allocation via kzalloc fails, the kernel
> > complains loudly already; there's no reason for every caller of
> > kmalloc() and friends to add their own pr_err("kmalloc failed"), that
> > just bloats the kernel .text.
> 
> I wasn't suggesting to log a kmalloc() error specifically. The idea was
> that if lookup_or_create_module_kobject() fails for whatever reason,
> kernel_add_sysfs_param() and version_sysfs_builtin() should log the
> failure to create/get the kobject.
> 
> > 
> >> Similarly, in the patch #4, when module_add_driver() calls
> >> lookup_or_create_module_kobject() and the function fails, the problem
> >> may or may not be reported, depending on the error.
> >>
> >> I'd suggest something as follows:
> >> * Drop the pr_crit() reporting in lookup_or_create_module_kobject().
> > 
> > No, because that reports on something other than an allocation failing
> > (of course, it could be that the reason kobject_init_and_add or
> > sysfs_create_file failed was an allocation failure, but it could
> > also be something else), so reporting there is the right thing to do.
> 
> The error message says:
> pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", name, err);
> 
> I think it was appropriate for locate_module_kobject() to do this error
> reporting when the function was only called by code in kernel/params.c
> and in the boot context. Now that the patch makes the function available
> to external callers, I'm not sure if it should remain reporting this
> error.
> 
> The function itself shouldn't directly make the system unstable when it
> fails. Each caller can appropriately determine how to handle the error.
> Functions kernel_add_sysfs_param() and version_sysfs_builtin() don't
> have much of an option and can only log it, but module_add_driver()
> could roll back its operation.
> 

before this patch series [1] kset_find_obj() was called in module_add_driver()
and the function immediately returned when no valid module_kobject was found,

module_add_driver returns immediately if some error is encountered or module_kobject
is not found in lookup_or_create_module_kobject()
Since module_kobject() is anyway no-op if it [2] returns early so it shouldn't require
any rollback, right?

Assuming rollback is not required for module_add_driver() in lookup_or_create_module_kobject()
failure scenario, what should be the appropriate messaging from pr_crit() if it
fails in module_add_driver()?

[1] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/base/module.c#L48
[2] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/base/module.c#L58

Thanks,
Shyam

Re: [PATCH v3 2/4] kernel: refactor lookup_or_create_module_kobject()

[Petr Pavlu]

On 2/25/25 18:24, Shyam Saini wrote:
> On Tue, Feb 25, 2025 at 09:33:10AM +0100, Petr Pavlu wrote:
>> On 2/21/25 11:42, Rasmus Villemoes wrote:
>>> On Thu, Feb 13 2025, Petr Pavlu <petr.pavlu@suse.com> wrote:
>>>
>>>> On 2/11/25 22:48, Shyam Saini wrote:
>>>>> 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.
>>>>
>>>> I think some error reporting should be cleaned up here.
>>>>
>>>> The current situation is that locate_module_kobject() can fail in
>>>> several cases and all these situations are loudly reported by the
>>>> function, either by BUG_ON() or pr_crit(). Consistently with that, both
>>>> its current callers version_sysfs_builtin() and kernel_add_sysfs_param()
>>>> don't do any reporting if locate_module_kobject() fails; they simply
>>>> return.
>>>>
>>>> The series seems to introduce two somewhat suboptimal cases.
>>>>
>>>> With this patch, when either version_sysfs_builtin() or
>>>> kernel_add_sysfs_param() calls lookup_or_create_module_kobject() and it
>>>> fails because of a potential kzalloc() error, the problem is silently
>>>> ignored.
>>>>
>>>
>>> No, because (IIUC) when a basic allocation via kzalloc fails, the kernel
>>> complains loudly already; there's no reason for every caller of
>>> kmalloc() and friends to add their own pr_err("kmalloc failed"), that
>>> just bloats the kernel .text.
>>
>> I wasn't suggesting to log a kmalloc() error specifically. The idea was
>> that if lookup_or_create_module_kobject() fails for whatever reason,
>> kernel_add_sysfs_param() and version_sysfs_builtin() should log the
>> failure to create/get the kobject.
>>
>>>
>>>> Similarly, in the patch #4, when module_add_driver() calls
>>>> lookup_or_create_module_kobject() and the function fails, the problem
>>>> may or may not be reported, depending on the error.
>>>>
>>>> I'd suggest something as follows:
>>>> * Drop the pr_crit() reporting in lookup_or_create_module_kobject().
>>>
>>> No, because that reports on something other than an allocation failing
>>> (of course, it could be that the reason kobject_init_and_add or
>>> sysfs_create_file failed was an allocation failure, but it could
>>> also be something else), so reporting there is the right thing to do.
>>
>> The error message says:
>> pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n", name, err);
>>
>> I think it was appropriate for locate_module_kobject() to do this error
>> reporting when the function was only called by code in kernel/params.c
>> and in the boot context. Now that the patch makes the function available
>> to external callers, I'm not sure if it should remain reporting this
>> error.
>>
>> The function itself shouldn't directly make the system unstable when it
>> fails. Each caller can appropriately determine how to handle the error.
>> Functions kernel_add_sysfs_param() and version_sysfs_builtin() don't
>> have much of an option and can only log it, but module_add_driver()
>> could roll back its operation.
>>
> 
> before this patch series [1] kset_find_obj() was called in module_add_driver()
> and the function immediately returned when no valid module_kobject was found,
> 
> module_add_driver returns immediately if some error is encountered or module_kobject
> is not found in lookup_or_create_module_kobject()
> Since module_kobject() is anyway no-op if it [2] returns early so it shouldn't require
> any rollback, right?
> 
> Assuming rollback is not required for module_add_driver() in lookup_or_create_module_kobject()
> failure scenario, what should be the appropriate messaging from pr_crit() if it
> fails in module_add_driver()?
> 
> [1] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/base/module.c#L48
> [2] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/base/module.c#L58

Sorry, I partly misunderstood the different context in which
module_add_driver() is called. I still think my suggestion makes some
sense, but looking again, the current version seems ok to me too.

-- 
Thanks,
Petr

[PATCH v3 3/4] kernel: globalize lookup_or_create_module_kobject()

[Shyam Saini]

lookup_or_create_module_kobject() is marked as static and __init,
to make it global drop static keyword.
Since this function can be called from non-init code, use __modinit
instead of __init, __modinit marker will make it __init if
CONFIG_MODULES is not defined.

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

diff --git a/include/linux/module.h b/include/linux/module.h
index 12f8a7d4fc1c..aeaaf9e068eb 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -162,6 +162,8 @@ extern void cleanup_module(void);
 #define __INITRODATA_OR_MODULE __INITRODATA
 #endif /*CONFIG_MODULES*/
 
+struct module_kobject *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 6e87aef876b2..1073f8ebd5a6 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 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;
-- 
2.34.1

Re: [PATCH v3 3/4] kernel: globalize lookup_or_create_module_kobject()

[Petr Pavlu]

On 2/11/25 22:48, Shyam Saini wrote:
> lookup_or_create_module_kobject() is marked as static and __init,
> to make it global drop static keyword.
> Since this function can be called from non-init code, use __modinit
> instead of __init, __modinit marker will make it __init if
> CONFIG_MODULES is not defined.

Hm, we should later clean up kernel/params.c to use __init_or_module
from include/linux/module.h instead of its own __modinit implementation.

-- Petr

Re: [PATCH v3 3/4] kernel: globalize lookup_or_create_module_kobject()

[Rasmus Villemoes]

On Thu, Feb 13 2025, Petr Pavlu <petr.pavlu@suse.com> wrote:

> On 2/11/25 22:48, Shyam Saini wrote:
>> lookup_or_create_module_kobject() is marked as static and __init,
>> to make it global drop static keyword.
>> Since this function can be called from non-init code, use __modinit
>> instead of __init, __modinit marker will make it __init if
>> CONFIG_MODULES is not defined.
>
> Hm, we should later clean up kernel/params.c to use __init_or_module
> from include/linux/module.h instead of its own __modinit implementation.

Good catch, yes. Mind sending the patch?

Rasmus

[PATCH v3 4/4] 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: [PATCH v3 4/4] drivers: base: handle module_kobject creation

[Greg KH]

On Tue, Feb 11, 2025 at 01:48:42PM -0800, Shyam Saini wrote:
> 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
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot