Message-ID: <e8ba9f36-e33d-4c53-9d34-ff611cb1c221@suse.com>
Date: Tue, 25 Feb 2025 09:33:10 +0100
From: Petr Pavlu <petr.pavlu@...e.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Shyam Saini <shyamsaini@...ux.microsoft.com>,
 linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org,
 code@...icks.com, christophe.leroy@...roup.eu, hch@...radead.org,
 mcgrof@...nel.org, frkaya@...ux.microsoft.com, vijayb@...ux.microsoft.com,
 linux@...ssschuh.net, samitolvanen@...gle.com, da.gomez@...sung.com,
 gregkh@...uxfoundation.org, rafael@...nel.org, dakr@...nel.org
Subject: Re: [PATCH v3 2/4] kernel: refactor lookup_or_create_module_kobject()

On 2/21/25 11:42, Rasmus Villemoes wrote:
> On Thu, Feb 13 2025, Petr Pavlu <petr.pavlu@...e.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

Powered by blists - more mailing lists