linux-kernel - Re: [PATCH v3 2/4] kernel: refactor lookup_or_create_module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wmdjo9yp.fsf@prevas.dk>
Date: Fri, 21 Feb 2025 11:42:22 +0100
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Petr Pavlu <petr.pavlu@...e.com>
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 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.

> 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