diff mbox series

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

Message ID 20250211214842.1806521-3-shyamsaini@linux.microsoft.com (mailing list archive)
State New
Headers show
Series Properly handle module_kobject creation | expand

Checks

Context Check Description
mcgrof/vmtest-modules-next-PR fail PR summary
mcgrof/vmtest-modules-next-VM_Test-0 success Logs for Run CI tests
mcgrof/vmtest-modules-next-VM_Test-1 success Logs for Run CI tests
mcgrof/vmtest-modules-next-VM_Test-2 fail Logs for cleanup / Archive results and cleanup
mcgrof/vmtest-modules-next-VM_Test-3 fail Logs for cleanup / Archive results and cleanup
mcgrof/vmtest-modules-next-VM_Test-4 fail Logs for setup / Setup kdevops environment
mcgrof/vmtest-modules-next-VM_Test-5 fail Logs for setup / Setup kdevops environment

Commit Message

Shyam Saini Feb. 11, 2025, 9:48 p.m. UTC
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(-)

Comments

Petr Pavlu Feb. 13, 2025, 3:55 p.m. UTC | #1
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.
Rasmus Villemoes Feb. 21, 2025, 10:42 a.m. UTC | #2
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
diff mbox series

Patch

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;
 }