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

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

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.

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