Message-ID: <6gwjomw6sxxmlglxfoilelswv4hgygqelomevb4k4wrlrk3gtm@wrakbmwztgeu>
Date:   Thu, 25 May 2023 11:45:33 -0700
From:   Lucas De Marchi <lucas.demarchi@...el.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
CC:     Petr Pavlu <petr.pavlu@...e.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
        <song@...nel.org>, <lucas.de.marchi@...il.com>,
        <christophe.leroy@...roup.eu>, <peterz@...radead.org>,
        <rppt@...nel.org>, <dave@...olabs.net>, <willy@...radead.org>,
        <vbabka@...e.cz>, <mhocko@...e.com>, <dave.hansen@...ux.intel.com>,
        <colin.i.king@...il.com>, <jim.cromie@...il.com>,
        <catalin.marinas@....com>, <jbaron@...mai.com>,
        <rick.p.edgecombe@...el.com>, <yujie.liu@...el.com>,
        <david@...hat.com>, <tglx@...utronix.de>, <hch@....de>,
        <patches@...ts.linux.dev>, <linux-modules@...r.kernel.org>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        <pmladek@...e.com>, <prarit@...hat.com>, <lennart@...ttering.net>
Subject: Re: [PATCH 2/2] module: add support to avoid duplicates early on load

On Thu, May 25, 2023 at 09:07:23AM -0700, Linus Torvalds wrote:
>On Thu, May 25, 2023 at 4:40 AM Petr Pavlu <petr.pavlu@...e.com> wrote:
>>
>> kmod normally uses finit_module() only if a module is not compressed,
>> otherwise it decompresses it first and then invokes init_module().
>
>Note that it would probably be good to teach Fedora and SuSE to use
>the kernel-side decompression, if only because we have it and would
>like to try to avoid using the old "load contents from user memory".
>
>Mainly because it allows security modules to actively check for
>tampering (ie things like verity etc). Long-term, it would be good to
>just deprecate the old init_module() entirely.

Right... I was trying to remember why that wasn't done yet since I
thought it was. The in-kernel decompression is much more recent than
the finit_module. Commit b1ae6dc41eaa ("module: add in-kernel support for decompressing")
was actually the one allowing to decompress on the kernel side
and  commit 169a58ad824d ("module/decompress: Support zstd in-kernel decompression")
brought the algo support on the kernel and userspace side to parity.

I will teach kmod to take the proper path considering the in-kernel
decompression availability.

>
>But yes:
>
>> It means that these and similarly organized distributions end up using
>> init_module(), and adding complexity to optimize finit_module() wouldn't
>> actually help in their case.
>
>Yeah, I think the real bug is absolutely in udev, and trying to load
>the same module hundreds of times is very very wrong. So I think the
>"mitigate it in the kernel" is at most a quick hack to fix user-space
>brokenness.
>
>And I don't think 1/2 is acceptable as that "quick hack". Not at all.
>It also seems fundamentally buggy, as it uses purely the inode number
>as the file identity, which means that it does bad things across
>filesystem limits.
>
>That said, I posted an alternate patch that I think _is_ valid as that
>quick hack. I don't love it, but it sure is simpler (and avoids the
>i_ino bug):
>
>    https://lore.kernel.org/lkml/CAHk-=wgKu=tJf1bm_dtme4Hde4zTB=_7EdgR8avsDRK4_jD+uA@mail.gmail.com/
>
>that patch hasn't seen any testing, and for all I know it won't even
>boot because of some thinko, but I think it would be acceptable as a
>workaround if it does work.
>
>But no, it's not some kind of "fix" for the bug, and yes, using
>init_module() rather than finit_module() will circumvent the quick
>hack. The true fix would be for udev to do proper handling of its data
>structures instead of randomly spraying duplicate module loading
>events.
>
>I don't know why udev does what it does. From what Luis told me,
>apparently it's just forking stuff and keeping all its data structures
>in memory, and has no actual consistency or locking or memory of what
>it has done. Luis pointed me at

It's a long time I don't touch that udev code, but my understanding
is that it first creates the kmod context and then starts to fork workers
(up to a limit) as the events arrive and there are no idle workers available.
At this point each of them have a separate kmod context derived from the
initial context. I was told the workers are needed because
a) they must be resilient to crashing without catastrophic consequences and
b) the kernel floods udev with thousands of netlink events during boot.
c) unrelated netlink events can't wait a module to be loaded, for example.

If the above is true (need confirmation from udev devs), then what could
be done on the userspace side would be:

1) do the modalias lookup first, before delegating the module load part
    to the workers. That will translate the modalias to the module name,
2) hand over to the worker the module loading part by name, not by alias,
    iff there isn't one being done for that already by other workers -
    workers need to share some state with the main process.

With this the dedup can happen based on the *module name*. I was told
a dedup based on the aliases is not effective as there are slight
changes on the modaliases being sent on boot leading to the same module.


>    https://lore.kernel.org/all/23bd0ce6-ef78-1cd8-1f21-0e706a00424a@suse.com/T/#u
>
>for some udev background.

the synchronization point in the kernel side rather than on userspace
used to be cheap and the race window smaller.  About the race: libkmod
already checks if there's a module being loaded before actually loading
it, however there is a race between the initstate file being created by
the kernel side and new requests arriving to load the same module.

>
>It's been about a decade since I looked at udev sources, and none of
>this encourages me to take a second look, so all of the above may be
>me misunderstanding just exactly what the udev problem is. But for
>that 'finit' case, we *could* try that simple hack of mine.
>
>I say "hack", but the patch really is pretty simple, and the concept
>of "exclusive special access" certainly is not some hack in itself.
>It's just not anything we've ever done before. So the hackishness from
>that exclusive_deny_write_access() thing in my patch is mainly that it
>shouldn't be needed at all (and that the exclusivity should probably
>be set some other way).
>
>Comments welcome.

Thinking only on the finit_module case and given libkmod will be
changed to prefer that path, it's not clear if it's preferred
to dedup on module name (userspace) or inode (kernel). Also worth
mentioning that both of them only protect against the window of calling
finit_module() and having a initstate file created by the kernel: if the
file exists in the coming or live states, libkmod will already do the
shortcut.

Are you willig to merge (a possibly improved version of) your patch
or the userspace change is still something that
would be desired?  Doing that on the kernel has the small advantage
that it also synchronizes requests from sources other than udev,
but I don't think we would have many to justify.

Lucas De Marchi

>
>                  Linus

Powered by blists - more mailing lists