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

[Rasmus Villemoes]

On Thu, Feb 13 2025, Petr Pavlu <petr.pavlu@xxxxxxxx> 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