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

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:

quoted

On Thu, Feb 13 2025, Petr Pavlu [off-list ref] wrote:

quoted

On 2/11/25 22:48, Shyam Saini wrote:

quoted

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.

quoted

quoted

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