diff mbox series

[2/2] module: add support to avoid duplicates early on load

Message ID 20230524213620.3509138-3-mcgrof@kernel.org (mailing list archive)
State New
Headers show
Series module: avoid all memory pressure due to duplicates | expand

Commit Message

Luis Chamberlain May 24, 2023, 9:36 p.m. UTC
Add support to use the new kread_uniq_fd() to avoid duplicate kernel
reads on modules. At the cost of about ~945 bytes to your kernel size,
enabling this on a 255 CPU x86_64 qemu guest this saves about ~1.8 GiB
of memory during boot which would otherwise be free'd, and reduces boot
time by about ~11 seconds.

Userspace loads modules through finit_module(), this in turn will
use vmalloc space up to 3 times:

  a) The kernel_read_file() call
  b) Optional module decompression
  c) Our final copy of the module

Commit 064f4536d139 ("module: avoid allocation if module is already
present and ready") shows a graph of the amount of vmalloc space
observed allocated but freed for duplicate module request which end
up in the trash bin. Since there is a linear relationship with the
number of CPUs eventually this will bite us and you end up not being
able to boot. That commit put a stop gap for c) but to avoid the
vmalloc() space wasted on a) and b) we need to detect duplicates
earlier.

We could just have userspace fix this, but as reviewed at LSFMM 2023
this year in Vancouver, fixing this in userspace can be complex and we
also can't know when userpace is fixed. Fixing this in kernel turned
out to be easy with the inode and with a simple kconfig option we can
let users / distros decide if this full stop gap is worthy to enable.

With this enabled I'm now able to see 0 bytes wasted on vmalloc space
due to duplicates.

Before:
 # sudo systemd-analyze
 Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s
 graphical.target reached after 44.178s in userspace.

 # grep "Virtual mem wasted bytes" /sys/kernel/debug/modules/stats
 Virtual mem wasted bytes       1949006968

So ~1.8 GiB... of vmalloc space wasted during boot.

After:

 # sudo systemd-analyze
 Startup finished in 29.883s (kernel) + 45.817s (userspace) = 1min 15.700s
 graphical.target reached after 45.682s in userspace.

 # grep "Virtual mem wasted bytes" /sys/kernel/debug/modules/stats
 Virtual mem wasted bytes       0

Suggested-by: Lennart Poettering <lennart@poettering.net>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 include/linux/module.h   |  1 +
 kernel/module/Kconfig    | 20 ++++++++++++++++++++
 kernel/module/internal.h |  1 +
 kernel/module/main.c     | 19 ++++++++++++-------
 4 files changed, 34 insertions(+), 7 deletions(-)

Comments

Petr Pavlu May 25, 2023, 11:40 a.m. UTC | #1
On 5/24/23 23:36, Luis Chamberlain wrote:
> Add support to use the new kread_uniq_fd() to avoid duplicate kernel
> reads on modules. At the cost of about ~945 bytes to your kernel size,
> enabling this on a 255 CPU x86_64 qemu guest this saves about ~1.8 GiB
> of memory during boot which would otherwise be free'd, and reduces boot
> time by about ~11 seconds.
> 
> Userspace loads modules through finit_module(), this in turn will
> use vmalloc space up to 3 times:
> 
>   a) The kernel_read_file() call
>   b) Optional module decompression
>   c) Our final copy of the module
> 
> Commit 064f4536d139 ("module: avoid allocation if module is already
> present and ready") shows a graph of the amount of vmalloc space
> observed allocated but freed for duplicate module request which end
> up in the trash bin. Since there is a linear relationship with the
> number of CPUs eventually this will bite us and you end up not being
> able to boot. That commit put a stop gap for c) but to avoid the
> vmalloc() space wasted on a) and b) we need to detect duplicates
> earlier.
> 
> We could just have userspace fix this, but as reviewed at LSFMM 2023
> this year in Vancouver, fixing this in userspace can be complex and we
> also can't know when userpace is fixed. Fixing this in kernel turned
> out to be easy with the inode and with a simple kconfig option we can
> let users / distros decide if this full stop gap is worthy to enable.

kmod normally uses finit_module() only if a module is not compressed,
otherwise it decompresses it first and then invokes init_module().

Looking at Fedora and openSUSE Tumbleweed, they compress kernel modules
with xz and zstd, respectively. They also have their kernels built
without any CONFIG_MODULE_COMPRESS_{GZIP,XZ,ZSTD} options.

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.

-- Petr
Linus Torvalds May 25, 2023, 4:07 p.m. UTC | #2
On Thu, May 25, 2023 at 4:40 AM Petr Pavlu <petr.pavlu@suse.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.

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

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

for some udev background.

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.

                  Linus
Greg KH May 25, 2023, 4:42 p.m. UTC | #3
On Thu, May 25, 2023 at 09:07:23AM -0700, Linus Torvalds wrote:
> > 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.

I totally agree.  I also agree that this doesn't really seem to be any
sort of "bug" in that no memory leaks, and when userspace calms down,
all goes back to normal.  So hacks in the vfs layer for this is not
good, let's not paper over userspace code that we have control over with
kernel changes.

Luis, I asked last time what modules are being asked by the kernel to be
loaded thousands of times at boot and can't seem to find an answer
anywhere, did I miss that?  This should be very easy to handle in
userspace if systems need it, so that begs the questions, what types of
systems need this?  We have handled booting with tens of thousands of
devices attached for decades now with no reports of boot/udev/kmod
issues before, what has recently changed to cause issues?

thanks,

greg k-h
Lucas De Marchi May 25, 2023, 4:54 p.m. UTC | #4
On Thu, May 25, 2023 at 01:40:32PM +0200, Petr Pavlu wrote:
>On 5/24/23 23:36, Luis Chamberlain wrote:
>> Add support to use the new kread_uniq_fd() to avoid duplicate kernel
>> reads on modules. At the cost of about ~945 bytes to your kernel size,
>> enabling this on a 255 CPU x86_64 qemu guest this saves about ~1.8 GiB
>> of memory during boot which would otherwise be free'd, and reduces boot
>> time by about ~11 seconds.
>>
>> Userspace loads modules through finit_module(), this in turn will
>> use vmalloc space up to 3 times:
>>
>>   a) The kernel_read_file() call
>>   b) Optional module decompression
>>   c) Our final copy of the module
>>
>> Commit 064f4536d139 ("module: avoid allocation if module is already
>> present and ready") shows a graph of the amount of vmalloc space
>> observed allocated but freed for duplicate module request which end
>> up in the trash bin. Since there is a linear relationship with the
>> number of CPUs eventually this will bite us and you end up not being
>> able to boot. That commit put a stop gap for c) but to avoid the
>> vmalloc() space wasted on a) and b) we need to detect duplicates
>> earlier.
>>
>> We could just have userspace fix this, but as reviewed at LSFMM 2023
>> this year in Vancouver, fixing this in userspace can be complex and we
>> also can't know when userpace is fixed. Fixing this in kernel turned
>> out to be easy with the inode and with a simple kconfig option we can
>> let users / distros decide if this full stop gap is worthy to enable.
>
>kmod normally uses finit_module() only if a module is not compressed,
>otherwise it decompresses it first and then invokes init_module().

that is for historical reasons, because the kernel didn't support to
uncompress the module by itself.

>
>Looking at Fedora and openSUSE Tumbleweed, they compress kernel modules
>with xz and zstd, respectively. They also have their kernels built
>without any CONFIG_MODULE_COMPRESS_{GZIP,XZ,ZSTD} options.
>
>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.

true, but the change in kmod should be trivial now that the kernel has
the proper support in place and the algorithms support match the ones
kmod has.  I will take a look at switching the logic around to just pass
the fd to the kernel so it can also deduplicate the requests.

thanks for the reminder,

Lucas De Marchi

>
>-- Petr
Linus Torvalds May 25, 2023, 5:52 p.m. UTC | #5
On Thu, May 25, 2023 at 9:07 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.

Well, it boots here, so it must be perfect.

That said, I didn't add any debugging code, and I didn't test it on
any odd setups, and I've never had any problems before. So I don't
actually know if the patch *does* anything.

But it did boot..

               Linus
Luis Chamberlain May 25, 2023, 6:22 p.m. UTC | #6
On Thu, May 25, 2023 at 05:42:10PM +0100, Greg KH wrote:
> Luis, I asked last time what modules are being asked by the kernel to be
> loaded thousands of times at boot and can't seem to find an answer
> anywhere, did I miss that?

Yes you missed it, I had explained it:

https://lore.kernel.org/all/ZEGopJ8VAYnE7LQ2@bombadil.infradead.org/

"My best assessment of the situation is that each CPU in udev ends up
triggering a load of duplicate set of modules, not just one, but *a
lot*. Not sure what heuristics udev uses to load a set of modules per
CPU."

Petr Pavlu then finishes the assessment:

https://lore.kernel.org/all/23bd0ce6-ef78-1cd8-1f21-0e706a00424a@suse.com/

But let me quote it, so it is not missed:

"My understanding is that udev workers are forked. An initial kmod
context is created by the main udevd process but no sharing happens
after the fork.  It means that the mentioned memory pool logic doesn't
really kick in.

Multiple parallel load requests come from multiple udev workers, for
instance, each handling an udev event for one CPU device and making the
exactly same requests as all others are doing at the same time.

The optimization idea would be to recognize these duplicate requests at
the udevd/kmod level and converge them."

> This should be very easy to handle in
> userspace if systems need it, so that begs the questions, what types of
> systems need this? 

I had explained, this has existed for a long time.

> We have handled booting with tens of thousands of
> devices attached for decades now with no reports of boot/udev/kmod
> issues before, what has recently changed to cause issues?

Doesn't mean this didn't happen before, just because memory is freed due
to duplicates does not mean that the memory pressure induced by them is
not stupid. It is stupid, but hasn't come up as a possible real issue
nowadays where systems require more vmalloc space used during boot with
new features. I had explained also the context where this came from.
David Hildenbrand had reported failure to boot on many CPUs.  If you
induce more vmap memory pressure on boot with multiple CPUs eventually
you can't boot. Enabling KASAN will make this worse today.

  Luis
Lucas De Marchi May 25, 2023, 6:45 p.m. UTC | #7
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@suse.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
Linus Torvalds May 25, 2023, 9:12 p.m. UTC | #8
On Thu, May 25, 2023 at 11:45 AM Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
>
> Are you willig to merge (a possibly improved version of) your patch
> or the userspace change is still something that would be desired?

I think a user space change should still be something that people
should look at, particularly as the kernel side patch I'm willing to
accept doesn't catch the "completely serial" cases, only the "trying
to load at the same time that the same module is literally busy being
loaded".

But I've cleaned up my patch a bit, and while the cleaned-up version
is rather larger as a patch (mainly because of just also re-organizing
the finit_module() code to do all the 'struct file' prep), I'm
actually pretty happy with this attached patch conceptually.

In this form, it actually "makes sense" to me, rather than being just
clearly a workaround.  Also, unlike the previous patch, this doesn't
actually make any changes to the basic kernel_read_file() set of
functions, it's all done by the module loading code itself.

Luis, would you mind testing this version on your load? It still won't
actually handle the purely serial case, so there *will* be those
spurious double module reads from different CPU's just doing the
things serially, but the exclusive file access region has been
extended to not just cover the actual file content reading, but to
cover the whole "turn it into a a real module" part too.

Also, this does *not* update some of the comments in the module
loading. I changed finit_module to use "kernel_read_file()" instead of
"kernel_read_file_from_fd()", since it actually now has to look up the
file descriptor anyway. But the comments still talk about that
"from_fd" thing.

Anyway, this is back to "ENTIRELY UNTESTED" territory, in that I've
compiled this, but haven't booted it. The changes look obvious, but
hey, mistakes happen.

And the commit message is just a place-holder. Obviously. I won't sign
off on this or write more of a commit message until it has had some
real testing.

                  Linus
Luis Chamberlain May 25, 2023, 10:02 p.m. UTC | #9
On Thu, May 25, 2023 at 02:12:49PM -0700, Linus Torvalds wrote:
> On Thu, May 25, 2023 at 11:45 AM Lucas De Marchi
> <lucas.demarchi@intel.com> wrote:
> >
> > Are you willig to merge (a possibly improved version of) your patch
> > or the userspace change is still something that would be desired?
> 
> I think a user space change should still be something that people
> should look at, particularly as the kernel side patch I'm willing to
> accept doesn't catch the "completely serial" cases, only the "trying
> to load at the same time that the same module is literally busy being
> loaded".
> 
> But I've cleaned up my patch a bit, and while the cleaned-up version
> is rather larger as a patch (mainly because of just also re-organizing
> the finit_module() code to do all the 'struct file' prep), I'm
> actually pretty happy with this attached patch conceptually.
> 
> In this form, it actually "makes sense" to me, rather than being just
> clearly a workaround.  Also, unlike the previous patch, this doesn't
> actually make any changes to the basic kernel_read_file() set of
> functions, it's all done by the module loading code itself.
> 
> Luis, would you mind testing this version on your load? It still won't
> actually handle the purely serial case, so there *will* be those
> spurious double module reads from different CPU's just doing the
> things serially, but the exclusive file access region has been
> extended to not just cover the actual file content reading, but to
> cover the whole "turn it into a a real module" part too.
> 
> Also, this does *not* update some of the comments in the module
> loading. I changed finit_module to use "kernel_read_file()" instead of
> "kernel_read_file_from_fd()", since it actually now has to look up the
> file descriptor anyway. But the comments still talk about that
> "from_fd" thing.
> 
> Anyway, this is back to "ENTIRELY UNTESTED" territory, in that I've
> compiled this, but haven't booted it. The changes look obvious, but
> hey, mistakes happen.
> 
> And the commit message is just a place-holder. Obviously. I won't sign
> off on this or write more of a commit message until it has had some
> real testing.

With 255 vcpus:

Before:

vagrant@kmod ~ $ sudo systemd-analyze                                            
Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s        
graphical.target reached after 44.178s in userspace.  

root@kmod ~ # grep "Virtual mem wasted bytes"
/sys/kernel/debug/modules/stats    
 Virtual mem wasted bytes       1949006968                                       

So ~1.8 GiB.

After:

root@kmod ~ # systemd-analyze 
Startup finished in 35.872s (kernel) + 41.715s (userspace) = 1min 17.588s 
graphical.target reached after 41.594s in userspace.

root@kmod ~ # cat /sys/kernel/debug/modules/stats
         Mods ever loaded       66
     Mods failed on kread       0
Mods failed on decompress       0
  Mods failed on becoming       1
      Mods failed on load       0
        Total module size       11268096
      Total mod text size       4149248
       Failed kread bytes       0
  Failed decompress bytes       0
    Failed becoming bytes       474688
        Failed kmod bytes       0
 Virtual mem wasted bytes       474688
         Average mod size       170729
    Average mod text size       62868
  Avg fail becoming bytes       474688
Duplicate failed modules:
              Module-name        How-many-times                    Reason
                   cryptd                     1                  Becoming

root@kmod ~ # du -b /lib/modules/6.3.0-next-20230505+/kernel/crypto/cryptd.ko
475409 /lib/modules/6.3.0-next-20230505+/kernel/crypto/cryptd.ko

So yeah definitely a pretty good improvement. Sometimes the system boots
without any duplicates at all, for some reason Vs the previous attempt.

Tested-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis
Linus Torvalds May 26, 2023, 1:39 a.m. UTC | #10
On Thu, May 25, 2023 at 3:02 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> So yeah definitely a pretty good improvement. Sometimes the system boots
> without any duplicates at all, for some reason Vs the previous attempt.
>
> Tested-by: Luis Chamberlain <mcgrof@kernel.org>

Ok, I decided to just move it from my experimental tree to my main tree.

I think I used about three times the time and effort (and lines of
text) on writing the commit message compared to what I did on the
patch itself.

I tried to lay out the background and the implications of the change -
it may be pretty darn simple, but it does have some subtle issues.

Anyway: I've committed it to my tree. This is not necessarily the best
time to do that, but let's get this behind us, and in particular,
let's get it out and into wider testing asap.

If it causes any problems what-so-ever, I'll just revert it very
aggressively (unless the problem is trivially and obviously fixable).
It is, after all, not a fix for a _kernel_ bug per se, and whil eI
think the patch is very benign, it does change user-visible behavior.
Very intentionally so, but still..

                         Linus
Johan Hovold May 29, 2023, 8:58 a.m. UTC | #11
On Thu, May 25, 2023 at 06:39:52PM -0700, Linus Torvalds wrote:

> Ok, I decided to just move it from my experimental tree to my main tree.
> 
> I think I used about three times the time and effort (and lines of
> text) on writing the commit message compared to what I did on the
> patch itself.
> 
> I tried to lay out the background and the implications of the change -
> it may be pretty darn simple, but it does have some subtle issues.
> 
> Anyway: I've committed it to my tree. This is not necessarily the best
> time to do that, but let's get this behind us, and in particular,
> let's get it out and into wider testing asap.
> 
> If it causes any problems what-so-ever, I'll just revert it very
> aggressively (unless the problem is trivially and obviously fixable).
> It is, after all, not a fix for a _kernel_ bug per se, and whil eI
> think the patch is very benign, it does change user-visible behavior.
> Very intentionally so, but still..

This change breaks module loading during boot on the Lenovo Thinkpad
X13s (aarch64).

Specifically it results in indefinite probe deferral of the display and
USB (ethernet) which makes it a pain to debug. Typing in the dark to
acquire some logs reveals that other modules are missing as well.

Fortunately commit 9828ed3f695a ("module: error out early on concurrent
load of the same module file") stood out when skimming the changes that
went into -rc4, and reverting it make all the expected modules be loaded
again.

I have not tried to figure out exactly why things break, but it does
seem like this one should be reverted.

Johan
Linus Torvalds May 29, 2023, 11 a.m. UTC | #12
On Mon, May 29, 2023 at 4:58 AM Johan Hovold <johan@kernel.org> wrote:
>
> I have not tried to figure out exactly why things break, but it does
> seem like this one should be reverted.

Yes, I have done so.

However, can I ask you to just verify that it was purely the exclusive
open part, and it wasn't that I messed up something else. IOW, can you
replace the

        return exclusive_deny_write_access(file);

in prepare_file_for_module_load() with just a "return 0", and remove the

                allow_write_access(f.file);

line in finit_module()?

That's obviously _instead_ of the revert that I already pushed out,
just to verify that "yup, it's that part, not something silly
elsewhere"

I do wonder what it is that is different in your setup, and maybe you
could also enable the

        pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);

in finit_module() while you are at it? Since you'd be editing that
file anyway for the test, just change the pr_debug() to a printk() and
then do

    dmesg | grep finit_module

to see what it all results in (on a working kernel, of course).

                    Linus
Johan Hovold May 29, 2023, 12:44 p.m. UTC | #13
On Mon, May 29, 2023 at 07:00:05AM -0400, Linus Torvalds wrote:

> However, can I ask you to just verify that it was purely the exclusive
> open part, and it wasn't that I messed up something else. IOW, can you
> replace the
> 
>         return exclusive_deny_write_access(file);
> 
> in prepare_file_for_module_load() with just a "return 0", and remove the
> 
>                 allow_write_access(f.file);
> 
> line in finit_module()?
> 
> That's obviously _instead_ of the revert that I already pushed out,
> just to verify that "yup, it's that part, not something silly
> elsewhere"

Yes, those two changes are enough to make the problem go away.

> I do wonder what it is that is different in your setup, and maybe you
> could also enable the
> 
>         pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);

Below is the corresponding output with a working kernel: 174 requests
for the 131 modules that end up being loaded (without the revert there
is only around 110 modules loaded).

There is some probe deferral and async probing going on during normal
boot which may be part of the explanation.

Johan

[    0.669112] finit_module: fd=3, uargs=00000000b461506c, flags=0
[    0.674144] finit_module: fd=4, uargs=00000000b461506c, flags=0
[    0.676783] finit_module: fd=5, uargs=00000000b461506c, flags=0
[    0.678920] finit_module: fd=3, uargs=00000000b461506c, flags=0
[    0.837967] finit_module: fd=5, uargs=0000000000157d9f, flags=0
[    0.839414] finit_module: fd=3, uargs=00000000b461506c, flags=0
[    0.844129] finit_module: fd=4, uargs=00000000b461506c, flags=0
[    0.845016] finit_module: fd=3, uargs=00000000b461506c, flags=0
[    0.849132] finit_module: fd=3, uargs=00000000b461506c, flags=0
[    0.849460] finit_module: fd=4, uargs=00000000b461506c, flags=0
[    3.345004] finit_module: fd=4, uargs=00000000e3e6c6d2, flags=0
[    3.364302] finit_module: fd=4, uargs=0000000095136ea7, flags=0
[    3.371928] finit_module: fd=5, uargs=0000000095136ea7, flags=0
[    4.099183] finit_module: fd=3, uargs=00000000ce2d6f3e, flags=0
[    4.103156] finit_module: fd=3, uargs=000000004e3e14c2, flags=0
[    4.713558] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.715608] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.717620] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.717910] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.719517] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.725862] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.726730] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.727018] finit_module: fd=14, uargs=00000000262da138, flags=0
[    4.730525] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.749602] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.749675] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.749678] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.774117] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.795307] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.797327] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.798405] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.799140] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.800850] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.807306] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.807313] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.807321] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.807394] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.807463] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.807525] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.807530] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.807590] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.811469] finit_module: fd=0, uargs=0000000080fab15b, flags=0
[    4.845851] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.845875] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.846282] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.846363] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.846363] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.846669] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.846994] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.847005] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.847194] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.847356] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.847448] finit_module: fd=17, uargs=00000000262da138, flags=0
[    4.847556] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.847651] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.848175] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.850005] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.850485] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.866031] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.866032] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.866381] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.866711] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.867757] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.868360] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.886043] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.886046] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.886046] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.886060] finit_module: fd=17, uargs=00000000262da138, flags=0
[    4.886114] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.886140] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.886326] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.886716] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.887210] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.887451] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.887811] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.887963] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.892066] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.896048] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.896070] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.896092] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.896157] finit_module: fd=18, uargs=00000000262da138, flags=0
[    4.896193] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.896211] finit_module: fd=18, uargs=00000000262da138, flags=0
[    4.896737] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.896751] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.897174] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.897343] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.897388] finit_module: fd=19, uargs=00000000262da138, flags=0
[    4.897555] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.897592] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.899657] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.899973] finit_module: fd=6, uargs=00000000262da138, flags=0
[    4.900316] finit_module: fd=17, uargs=00000000262da138, flags=0
[    4.901188] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.901668] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.901708] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.902030] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.903964] finit_module: fd=14, uargs=00000000262da138, flags=0
[    4.905243] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.907083] finit_module: fd=17, uargs=00000000262da138, flags=0
[    4.907480] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.907519] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.909481] finit_module: fd=13, uargs=00000000262da138, flags=0
[    4.911705] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.912056] finit_module: fd=18, uargs=00000000262da138, flags=0
[    4.912079] finit_module: fd=15, uargs=00000000262da138, flags=0
[    4.915340] finit_module: fd=19, uargs=00000000262da138, flags=0
[    4.933199] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.933585] finit_module: fd=18, uargs=00000000262da138, flags=0
[    4.935169] finit_module: fd=20, uargs=00000000262da138, flags=0
[    4.956021] finit_module: fd=19, uargs=00000000262da138, flags=0
[    4.956797] finit_module: fd=16, uargs=00000000262da138, flags=0
[    4.959865] finit_module: fd=17, uargs=00000000262da138, flags=0
[    4.964171] finit_module: fd=17, uargs=00000000262da138, flags=0
[    4.977073] finit_module: fd=18, uargs=00000000262da138, flags=0
[    4.980167] finit_module: fd=18, uargs=00000000262da138, flags=0
[    5.043379] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.053709] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.131232] finit_module: fd=15, uargs=00000000262da138, flags=0
[    5.140785] finit_module: fd=19, uargs=00000000262da138, flags=0
[    5.186244] finit_module: fd=18, uargs=00000000262da138, flags=0
[    5.186247] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.186252] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.186451] finit_module: fd=15, uargs=00000000262da138, flags=0
[    5.186507] finit_module: fd=13, uargs=00000000262da138, flags=0
[    5.187345] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.190282] finit_module: fd=15, uargs=00000000262da138, flags=0
[    5.195744] finit_module: fd=13, uargs=00000000262da138, flags=0
[    5.198242] finit_module: fd=16, uargs=00000000262da138, flags=0
[    5.198271] finit_module: fd=20, uargs=00000000262da138, flags=0
[    5.222394] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.222395] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.222407] finit_module: fd=16, uargs=00000000262da138, flags=0
[    5.222430] finit_module: fd=21, uargs=00000000262da138, flags=0
[    5.222432] finit_module: fd=16, uargs=00000000262da138, flags=0
[    5.222443] finit_module: fd=15, uargs=00000000262da138, flags=0
[    5.229650] finit_module: fd=22, uargs=00000000262da138, flags=0
[    5.257981] finit_module: fd=6, uargs=00000000262da138, flags=0
[    5.313560] finit_module: fd=6, uargs=00000000262da138, flags=0
[    6.144316] finit_module: fd=6, uargs=00000000262da138, flags=0
[    6.178956] finit_module: fd=6, uargs=00000000262da138, flags=0
[    6.178961] finit_module: fd=6, uargs=00000000262da138, flags=0
[    6.182057] finit_module: fd=13, uargs=00000000262da138, flags=0
[    6.182067] finit_module: fd=13, uargs=00000000262da138, flags=0
[    6.243708] finit_module: fd=0, uargs=00000000f9e4f67e, flags=0
[    6.249397] finit_module: fd=0, uargs=000000001b26db10, flags=0
[    6.249904] finit_module: fd=1, uargs=000000001b26db10, flags=0
[    6.250626] finit_module: fd=2, uargs=000000001b26db10, flags=0
[    6.251515] finit_module: fd=3, uargs=000000001b26db10, flags=0
[    6.254112] finit_module: fd=4, uargs=000000001b26db10, flags=0
[    6.255129] finit_module: fd=6, uargs=00000000262da138, flags=0
[    6.255504] finit_module: fd=6, uargs=00000000262da138, flags=0
[    6.259256] finit_module: fd=0, uargs=00000000e1b6cfe4, flags=0
[    6.264136] finit_module: fd=0, uargs=000000004070418f, flags=0
[    6.265227] finit_module: fd=1, uargs=000000004070418f, flags=0
[    6.270175] finit_module: fd=0, uargs=0000000092757077, flags=0
[    6.271230] finit_module: fd=1, uargs=0000000092757077, flags=0
[    6.322960] finit_module: fd=3, uargs=00000000fb904223, flags=0
[    6.373125] finit_module: fd=0, uargs=00000000e2cdc73f, flags=0
[    6.380061] finit_module: fd=0, uargs=000000000591e4e9, flags=0
[    6.392296] finit_module: fd=0, uargs=0000000088d2796a, flags=0
[    6.464595] finit_module: fd=0, uargs=000000001aa8b42e, flags=0
[    6.479839] finit_module: fd=0, uargs=00000000de50a030, flags=0
[    6.488790] finit_module: fd=0, uargs=00000000cbcb6a65, flags=0
[    6.492573] finit_module: fd=1, uargs=00000000cbcb6a65, flags=0
[    6.514903] finit_module: fd=0, uargs=000000006f393376, flags=0
[    7.989970] finit_module: fd=0, uargs=00000000c4594f52, flags=0
[   11.492886] finit_module: fd=13, uargs=00000000262da138, flags=0
[   11.639532] finit_module: fd=6, uargs=00000000262da138, flags=0
[   11.640048] finit_module: fd=6, uargs=00000000262da138, flags=0
[   11.640997] finit_module: fd=15, uargs=00000000262da138, flags=0
[   11.641049] finit_module: fd=6, uargs=00000000262da138, flags=0
[   11.775051] finit_module: fd=6, uargs=00000000262da138, flags=0
[   11.776806] finit_module: fd=6, uargs=00000000262da138, flags=0
Johan Hovold May 29, 2023, 3:18 p.m. UTC | #14
> On Mon, May 29, 2023 at 07:00:05AM -0400, Linus Torvalds wrote:
> > I do wonder what it is that is different in your setup

I took a closer look at some of the modules that failed to load and
noticed a pattern in that they have dependencies that are needed by more
than one device.

If attempts to load the drivers for two such devices are made in
parallel, only one of them may be successful in loading the shared
dependency which means that module loading is now aborted for the other.

I took a quick look at the kmod code

	https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/tree/libkmod/libkmod-module.c#n1305

and it does seem like this could happen if we start returning errors
when a module is already in the process of being loaded.

Johan
Linus Torvalds May 29, 2023, 5:47 p.m. UTC | #15
On Mon, May 29, 2023 at 8:44 AM Johan Hovold <johan@kernel.org> wrote:
>
> Yes, those two changes are enough to make the problem go away.

Ok, good. Expected, but just verifying that it wasn't some silly
incidental thinko.

> > I do wonder what it is that is different in your setup, and maybe you
> > could also enable the
> >
> >         pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
>
> Below is the corresponding output with a working kernel: 174 requests
> for the 131 modules that end up being loaded (without the revert there
> is only around 110 modules loaded).

Ok, your setup doesn't sound *too* different from mine. I have 176
kernel modules on my laptop right now, and that exclusive open
obviously worked fine for me.

But it could easily be some random small difference just from
different hardware, so...

And yeah, that dmesg output is useless, I didn't think of the fact
that it only prints out the file descriptor, not the actual path to
the file. In fact, without that change in place, the module code never
actually looks at the file and leaves it all to
kernel_read_file_from_fd().

With my change, it woul dhave been trivial to use "%pD" and point it
at the file pointer instead, and get the dentry name that way, but
never mind.  I think you're entirely right that it's probably due to a
shared dependency module, and I just didn't happen to trigger that
case.

Sadly, the whole idea was to figure out the exclusion so early that we
don't have the module data structures lookup up yet, so there's no
really obvious thing to serialize the load on.

I'll have to think about this more. Serializing on a per-inode lock
would seem to be the simplest thing, but they are all for IO, and we
can't just take them over the read.

                     Linus
Linus Torvalds May 30, 2023, 1:55 a.m. UTC | #16
On Mon, May 29, 2023 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
>
> I took a closer look at some of the modules that failed to load and
> noticed a pattern in that they have dependencies that are needed by more
> than one device.

Ok, this is a "maybe something like this" RFC series of two patches -
one trivial one to re-organize things a bit so that we can then do the
real one which uses a filter based on the inode pointer to return an
"idempotent return value" for module loads that share the same inode.

It's entirely untested, and since I'm on the road I'm going to not
really be able to test it. It compiles for me, and the code looks
fairly straightforward, but it's probably buggy.

It's very loosely based on Luis' attempt,  but it
 (a) is internal to module loading
 (b) uses a reliable cookie
 (c) doesn't leave the cookie around randomly for later
 (d) has seen absolutely no testing

Put another way: if somebody wants to play with this, please treat it
as a starting point, not the final thing. You might need to debug
things, and fix silly mistakes.

The idea is to just have a simple hash list of currently executing
module loads, protected by a trivial spinlock. Every module loader
adds itself to the right hash list, and if they were the *first* one
(ie no other pending module loads for that inode), will actually do
the module load.

Everybody who *isn't* the first one will just wait for completion and
return the same error code that the first one returned.

This is technically bogus. The first one might fail due to arguments.
So the cookie shouldn't be just the inode, it should be the inode and
a hash of the arguments or something like that. But it is what it is,
and apart from possible show-stopper bugs this is no worse than the
failed "exclusive write deny" attempt. IOW - maybe worth trying?

And if *that* didn't sell people on this patch series, I don't know
what will. I should be in marketing! Two drink minimums, here I come!

               Linus
Johan Hovold May 30, 2023, 9:40 a.m. UTC | #17
On Mon, May 29, 2023 at 09:55:15PM -0400, Linus Torvalds wrote:
> On Mon, May 29, 2023 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > I took a closer look at some of the modules that failed to load and
> > noticed a pattern in that they have dependencies that are needed by more
> > than one device.
> 
> Ok, this is a "maybe something like this" RFC series of two patches -
> one trivial one to re-organize things a bit so that we can then do the
> real one which uses a filter based on the inode pointer to return an
> "idempotent return value" for module loads that share the same inode.
> 
> It's entirely untested, and since I'm on the road I'm going to not
> really be able to test it. It compiles for me, and the code looks
> fairly straightforward, but it's probably buggy.
> 
> It's very loosely based on Luis' attempt,  but it
>  (a) is internal to module loading
>  (b) uses a reliable cookie
>  (c) doesn't leave the cookie around randomly for later
>  (d) has seen absolutely no testing
> 
> Put another way: if somebody wants to play with this, please treat it
> as a starting point, not the final thing. You might need to debug
> things, and fix silly mistakes.

With the missing spinlock initialisation fixed:

-static struct spinlock idem_lock;
+static DEFINE_SPINLOCK(idem_lock);

this passes basic smoke testing and allows the X13s to boot.

It does not seem to have any significant impact on boot time, but it
avoids some of the unnecessary load attempts as intended:

Before:

         Mods ever loaded       131
     Mods failed on kread       0
Mods failed on decompress       0
  Mods failed on becoming       24
      Mods failed on load       14
        Total module size       12587008
      Total mod text size       5058560
       Failed kread bytes       0
  Failed decompress bytes       0
    Failed becoming bytes       2437992
        Failed kmod bytes       1858992
 Virtual mem wasted bytes       4296984
         Average mod size       96085
    Average mod text size       38615
  Avg fail becoming bytes       101583
  Average fail load bytes       132786

After:

         Mods ever loaded       131
     Mods failed on kread       0
Mods failed on decompress       0
  Mods failed on becoming       4
      Mods failed on load       0
        Total module size       12587008
      Total mod text size       5058560
       Failed kread bytes       0
  Failed decompress bytes       0
    Failed becoming bytes       109776
        Failed kmod bytes       0
 Virtual mem wasted bytes       109776
         Average mod size       96085
    Average mod text size       38615
  Avg fail becoming bytes       27444

Johan
Johan Hovold May 30, 2023, 10:01 a.m. UTC | #18
On Mon, May 29, 2023 at 01:47:28PM -0400, Linus Torvalds wrote:
> On Mon, May 29, 2023 at 8:44 AM Johan Hovold <johan@kernel.org> wrote:

> > > I do wonder what it is that is different in your setup, and maybe you
> > > could also enable the
> > >
> > >         pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
> >
> > Below is the corresponding output with a working kernel: 174 requests
> > for the 131 modules that end up being loaded (without the revert there
> > is only around 110 modules loaded).

> And yeah, that dmesg output is useless, I didn't think of the fact
> that it only prints out the file descriptor, not the actual path to
> the file. In fact, without that change in place, the module code never
> actually looks at the file and leaves it all to
> kernel_read_file_from_fd().

Yeah, I added a printk with the module name to load_module() to make
some sense of it.
 
> With my change, it woul dhave been trivial to use "%pD" and point it
> at the file pointer instead, and get the dentry name that way, but
> never mind.  I think you're entirely right that it's probably due to a
> shared dependency module, and I just didn't happen to trigger that
> case.

For completeness, here's a corresponding log from when running with your
RFC. Those duplicate requests that now wait for loading to complete
would have failed, and that explains why some modules like
qcom-spmi-adc5 and qcom-spmi-adc-tm5 that both depend on
qcom-vadc-common would in turn fail to load.

Johan

[    0.633127] init_module_from_file - i2c-core.ko
[    0.638320] init_module_from_file - i2c-hid.ko
[    0.640654] init_module_from_file - i2c-hid-of.ko
[    0.642572] init_module_from_file - i2c-qcom-geni.ko
[    0.826911] init_module_from_file - hid-multitouch.ko
[    0.827861] init_module_from_file - nvme-core.ko
[    0.833011] init_module_from_file - nvme.ko
[    0.835558] init_module_from_file - phy-qcom-qmp-pcie.ko
[    0.841050] init_module_from_file - crc8.ko
[    0.841371] init_module_from_file - pcie-qcom.ko
[    3.390182] init_module_from_file - ipv6.ko
[    3.402261] init_module_from_file - x_tables.ko
[    3.406573] init_module_from_file - ip_tables.ko
[    4.180345] init_module_from_file - dm-mod.ko
[    4.186611] init_module_from_file - drm.ko
[    4.793481] init_module_from_file - pwm_bl.ko
[    4.798935] init_module_from_file - soundwire-bus.ko
[    4.802551] init_module_from_file - qmi_helpers.ko
[    4.805664] init_module_from_file - socinfo.ko
[    4.814729] init_module_from_file - pdr_interface.ko
[    4.830809] init_module_from_file - soundcore.ko
[    4.832227] init_module_from_file - qcom-wdt.ko
[    4.832242] init_module_from_file - qcom-rng.ko
[    4.832311] init_module_from_file - icc-osm-l3.ko
[    4.867903] init_module_from_file - pmic_glink.ko
[    4.867904] init_module_from_file - snd.ko
[    4.868045] init_module_from_file - mdt_loader.ko
[    4.890274] init_module_from_file - snd.ko
[    4.893962] init_module_from_file - snd.ko
[    4.894620] init_module_from_file - snd.ko
[    4.896162] init_module_from_file - snd.ko
[    4.896797] init_module_from_file - snd.ko
[    4.896907] init_module_from_file - snd.ko
[    4.898087] init_module_from_file - snd.ko
[    4.907548] init_module_from_file - qcom_sysmon.ko
[    4.907560] init_module_from_file - qcom_sysmon.ko
[    4.918690] init_module_from_file - snd.ko - waited, ret = 0
[    4.918692] init_module_from_file - snd.ko - waited, ret = 0
[    4.918694] init_module_from_file - snd.ko - waited, ret = 0
[    4.918695] init_module_from_file - snd.ko - waited, ret = 0
[    4.918699] init_module_from_file - snd.ko - waited, ret = 0
[    4.918700] init_module_from_file - snd.ko - waited, ret = 0
[    4.920849] init_module_from_file - snd.ko - waited, ret = 0
[    4.937139] init_module_from_file - qrtr.ko
[    4.937163] init_module_from_file - icc-bwmon.ko
[    4.937185] init_module_from_file - icc-bwmon.ko
[    4.938603] init_module_from_file - qcom_sysmon.ko - waited, ret = 0
[    4.939866] init_module_from_file - snd-timer.ko
[    4.939877] init_module_from_file - snd-timer.ko
[    4.939885] init_module_from_file - snd-timer.ko
[    4.939897] init_module_from_file - snd-timer.ko
[    4.939905] init_module_from_file - snd-timer.ko
[    4.939914] init_module_from_file - snd-timer.ko
[    4.939982] init_module_from_file - snd-timer.ko
[    4.940050] init_module_from_file - snd-timer.ko
[    4.943465] init_module_from_file - pinctrl-lpass-lpi.ko
[    4.943493] init_module_from_file - phy-qcom-snps-femto-v2.ko
[    4.943512] init_module_from_file - phy-qcom-snps-femto-v2.ko
[    4.944176] init_module_from_file - qcom_q6v5.ko
[    4.944362] init_module_from_file - qcom_q6v5.ko
[    4.946140] init_module_from_file - qcom_q6v5.ko - waited, ret = 0
[    4.946758] init_module_from_file - snd-timer.ko - waited, ret = 0
[    4.946762] init_module_from_file - snd-timer.ko - waited, ret = 0
[    4.946919] init_module_from_file - snd-timer.ko - waited, ret = 0
[    4.947484] init_module_from_file - snd-timer.ko - waited, ret = 0
[    4.947827] init_module_from_file - snd-timer.ko - waited, ret = 0
[    4.948101] init_module_from_file - snd-timer.ko - waited, ret = 0
[    4.948163] init_module_from_file - snd-timer.ko - waited, ret = 0
[    4.948483] init_module_from_file - phy-qcom-snps-femto-v2.ko - waited, ret = 0
[    4.950639] init_module_from_file - icc-bwmon.ko - waited, ret = 0
[    4.951178] init_module_from_file - snd-pcm.ko
[    4.951279] init_module_from_file - snd-pcm.ko
[    4.951616] init_module_from_file - qcom_glink_smem.ko
[    4.952049] init_module_from_file - snd-pcm.ko
[    4.952122] init_module_from_file - snd-pcm.ko
[    4.952132] init_module_from_file - snd-pcm.ko
[    4.952185] init_module_from_file - snd-pcm.ko
[    4.952231] init_module_from_file - pinctrl-sc8280xp-lpass-lpi.ko
[    4.952238] init_module_from_file - snd-pcm.ko
[    4.952326] init_module_from_file - qcom_glink_smem.ko
[    4.957870] init_module_from_file - snd-pcm.ko
[    4.971513] init_module_from_file - typec.ko
[    4.971529] init_module_from_file - typec.ko
[    4.971544] init_module_from_file - phy-qcom-edp.ko
[    4.971619] init_module_from_file - typec.ko
[    4.971806] init_module_from_file - snd-pcm.ko
[    4.971961] init_module_from_file - qcom_common.ko
[    4.972583] init_module_from_file - qcom_common.ko
[    4.972676] init_module_from_file - rfkill.ko
[    4.972898] init_module_from_file - qcom_stats.ko
[    4.973031] init_module_from_file - typec.ko
[    4.973917] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.974024] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.974090] init_module_from_file - qcom_common.ko - waited, ret = 0
[    4.974134] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.974205] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.974265] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.974307] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.974385] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.974436] init_module_from_file - snd-pcm.ko - waited, ret = 0
[    4.979168] init_module_from_file - qcom_pil_info.ko
[    4.979352] init_module_from_file - snd-compress.ko
[    4.979399] init_module_from_file - qcom_pil_info.ko
[    4.979668] init_module_from_file - snd-compress.ko
[    4.979912] init_module_from_file - snd-compress.ko
[    4.980022] init_module_from_file - rtc-pm8xxx.ko
[    4.980107] init_module_from_file - snd-compress.ko
[    4.980216] init_module_from_file - sysimgblt.ko
[    4.980490] init_module_from_file - nvmem_qcom-spmi-sdam.ko
[    4.980593] init_module_from_file - snd-compress.ko
[    4.981109] init_module_from_file - snd-compress.ko
[    4.981136] init_module_from_file - qcom_pil_info.ko - waited, ret = 0
[    4.981285] init_module_from_file - snd-compress.ko
[    4.981378] init_module_from_file - typec.ko - waited, ret = 0
[    4.981382] init_module_from_file - typec.ko - waited, ret = 0
[    4.981402] init_module_from_file - snd-compress.ko - waited, ret = 0
[    4.981404] init_module_from_file - snd-compress.ko - waited, ret = 0
[    4.981412] init_module_from_file - typec.ko - waited, ret = 0
[    4.981453] init_module_from_file - snd-compress.ko - waited, ret = 0
[    4.981482] init_module_from_file - snd-compress.ko - waited, ret = 0
[    4.981516] init_module_from_file - snd-compress.ko - waited, ret = 0
[    4.981620] init_module_from_file - snd-compress.ko
[    4.982970] init_module_from_file - snd-compress.ko
[    4.985063] init_module_from_file - qcom_q6v5_pas.ko
[    4.985132] init_module_from_file - qcom_q6v5_pas.ko
[    4.985200] init_module_from_file - snd-soc-core.ko
[    4.985222] init_module_from_file - qcom-vadc-common.ko
[    4.985254] init_module_from_file - sysfillrect.ko
[    4.985654] init_module_from_file - snd-soc-core.ko
[    4.985697] init_module_from_file - sysfillrect.ko
[    4.985862] init_module_from_file - snd-soc-core.ko
[    4.985883] init_module_from_file - ecc.ko
[    4.985950] init_module_from_file - reboot-mode.ko
[    4.985982] init_module_from_file - snd-soc-core.ko
[    4.986159] init_module_from_file - snd-soc-core.ko
[    4.986304] init_module_from_file - snd-soc-core.ko
[    4.986829] init_module_from_file - qcom-vadc-common.ko
[    4.987106] init_module_from_file - snd-soc-core.ko
[    4.987546] init_module_from_file - qcom-vadc-common.ko - waited, ret = 0
[    4.987889] init_module_from_file - snd-soc-core.ko
[    4.988198] init_module_from_file - snd-soc-core.ko
[    4.989151] init_module_from_file - industrialio.ko
[    4.989220] init_module_from_file - industrialio.ko
[    4.989588] init_module_from_file - gpio-sbu-mux.ko
[    4.989651] init_module_from_file - gpio-sbu-mux.ko
[    4.990037] init_module_from_file - qcom-pon.ko
[    4.991354] init_module_from_file - mhi.ko
[    4.992889] init_module_from_file - industrialio.ko - waited, ret = 0
[    4.993473] init_module_from_file - cfg80211.ko
[    4.994053] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994073] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994103] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994107] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994129] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994147] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994166] init_module_from_file - sysfillrect.ko
[    4.994176] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994206] init_module_from_file - snd-soc-core.ko - waited, ret = 0
[    4.994319] init_module_from_file - qcom-spmi-adc5.ko
[    4.994621] init_module_from_file - ecdh_generic.ko
[    4.994975] init_module_from_file - snd-soc-lpass-macro-common.ko
[    4.994999] init_module_from_file - qcom-spmi-adc-tm5.ko
[    4.996797] init_module_from_file - gpio-sbu-mux.ko - waited, ret = 0
[    4.997487] init_module_from_file - snd-soc-qcom-sdw.ko
[    4.997511] init_module_from_file - snd-soc-lpass-macro-common.ko
[    4.997546] init_module_from_file - snd-soc-wcd-mbhc.ko
[    4.997573] init_module_from_file - sysfillrect.ko - waited, ret = 0
[    4.997754] init_module_from_file - syscopyarea.ko
[    4.997762] init_module_from_file - syscopyarea.ko
[    4.999053] init_module_from_file - sysfillrect.ko - waited, ret = 0
[    4.999247] init_module_from_file - snd-soc-lpass-macro-common.ko
[    4.999482] init_module_from_file - snd-soc-lpass-wsa-macro.ko
[    4.999587] init_module_from_file - soundwire-qcom.ko
[    5.000338] init_module_from_file - soundwire-qcom.ko
[    5.000745] init_module_from_file - soundwire-qcom.ko
[    5.001008] init_module_from_file - syscopyarea.ko
[    5.015734] init_module_from_file - bluetooth.ko
[    5.016020] init_module_from_file - regmap-sdw.ko
[    5.016434] init_module_from_file - snd-soc-qcom-common.ko
[    5.017416] init_module_from_file - syscopyarea.ko - waited, ret = 0
[    5.018912] init_module_from_file - snd-soc-lpass-macro-common.ko - waited, ret = 0
[    5.018952] init_module_from_file - syscopyarea.ko - waited, ret = 0
[    5.019196] init_module_from_file - snd-soc-lpass-macro-common.ko - waited, ret = 0
[    5.020220] init_module_from_file - drm_kms_helper.ko
[    5.020230] init_module_from_file - snd-soc-lpass-rx-macro.ko
[    5.023262] init_module_from_file - drm_kms_helper.ko
[    5.023366] init_module_from_file - snd-soc-wcd938x-sdw.ko
[    5.023705] init_module_from_file - snd-soc-lpass-va-macro.ko
[    5.024133] init_module_from_file - drm_kms_helper.ko
[    5.024223] init_module_from_file - snd-soc-sc8280xp.ko
[    5.024814] init_module_from_file - snd-soc-lpass-tx-macro.ko
[    5.027886] init_module_from_file - drm_kms_helper.ko - waited, ret = 0
[    5.027937] init_module_from_file - drm_kms_helper.ko - waited, ret = 0
[    5.028550] init_module_from_file - phy-qcom-qmp-combo.ko
[    5.028828] init_module_from_file - snd-soc-wcd938x.ko
[    5.029106] init_module_from_file - drm_dp_aux_bus.ko
[    5.030265] init_module_from_file - phy-qcom-qmp-combo.ko
[    5.037773] init_module_from_file - qcom_q6v5_pas.ko - waited, ret = 0
[    5.038034] init_module_from_file - soundwire-qcom.ko - waited, ret = 0
[    5.038548] init_module_from_file - libarc4.ko
[    5.038551] init_module_from_file - soundwire-qcom.ko - waited, ret = 0
[    5.042176] init_module_from_file - btqca.ko
[    5.044433] init_module_from_file - mac80211.ko
[    5.044480] init_module_from_file - drm_display_helper.ko
[    5.046672] init_module_from_file - qcom-spmi-temp-alarm.ko
[    5.048163] init_module_from_file - hci_uart.ko
[    5.056843] init_module_from_file - gpu-sched.ko
[    5.073356] init_module_from_file - led-class-multicolor.ko
[    5.087906] init_module_from_file - qcom-spmi-temp-alarm.ko
[    5.106010] init_module_from_file - ath11k.ko
[    5.106084] init_module_from_file - msm.ko
[    5.125345] init_module_from_file - leds-qcom-lpg.ko
[    5.150715] init_module_from_file - ath11k_pci.ko
[    5.157163] init_module_from_file - qcom-spmi-temp-alarm.ko - waited, ret = 0
[    5.157894] init_module_from_file - qcom_battmgr.ko
[    5.158102] init_module_from_file - phy-qcom-qmp-combo.ko - waited, ret = 0
[    5.159204] init_module_from_file - pmic_glink_altmode.ko
[    5.258217] init_module_from_file - qrtr-smd.ko
[    5.258238] init_module_from_file - qrtr-smd.ko
[    5.258294] init_module_from_file - rpmsg_char.ko
[    5.259157] init_module_from_file - apr.ko
[    5.259160] init_module_from_file - rpmsg_char.ko
[    5.276634] init_module_from_file - qrtr-smd.ko - waited, ret = 0
[    5.277698] init_module_from_file - fastrpc.ko
[    5.278293] init_module_from_file - rpmsg_char.ko - waited, ret = 0
[    5.278926] init_module_from_file - rpmsg_ctrl.ko
[    5.279642] init_module_from_file - rpmsg_ctrl.ko
[    5.281399] init_module_from_file - rpmsg_ctrl.ko - waited, ret = 0
[    5.330747] init_module_from_file - snd-soc-hdmi-codec.ko
[    5.330826] init_module_from_file - snd-soc-hdmi-codec.ko
[    5.333130] init_module_from_file - snd-soc-hdmi-codec.ko
[    5.333201] init_module_from_file - snd-soc-hdmi-codec.ko - waited, ret = 0
[    5.333214] init_module_from_file - snd-soc-hdmi-codec.ko - waited, ret = 0
[    5.396803] init_module_from_file - panel-edp.ko
[    6.101509] init_module_from_file - qrtr-mhi.ko
[    6.196359] init_module_from_file - iptable_filter.ko
[    6.203333] init_module_from_file - nf_defrag_ipv4.ko
[    6.203903] init_module_from_file - nf_defrag_ipv6.ko
[    6.204628] init_module_from_file - libcrc32c.ko
[    6.204727] init_module_from_file - snd-q6apm.ko
[    6.204985] init_module_from_file - snd-q6apm.ko
[    6.205007] init_module_from_file - nf_conntrack.ko
[    6.207137] init_module_from_file - xt_conntrack.ko
[    6.211214] init_module_from_file - xt_tcpudp.ko
[    6.215535] init_module_from_file - nf_reject_ipv4.ko
[    6.216071] init_module_from_file - ipt_REJECT.ko
[    6.222201] init_module_from_file - nf_log_syslog.ko
[    6.223050] init_module_from_file - xt_LOG.ko
[    6.241544] init_module_from_file - ip6_tables.ko
[    6.286511] init_module_from_file - af_alg.ko
[    6.293272] init_module_from_file - algif_hash.ko
[    6.304773] init_module_from_file - md5.ko
[    6.333540] init_module_from_file - mii.ko
[    6.333550] init_module_from_file - mii.ko
[    6.334785] init_module_from_file - mii.ko - waited, ret = 0
[    6.335170] init_module_from_file - r8152.ko
[    6.335282] init_module_from_file - r8152.ko
[    6.370285] init_module_from_file - algif_skcipher.ko
[    6.384424] init_module_from_file - ecb.ko
[    6.392643] init_module_from_file - libdes.ko
[    6.397046] init_module_from_file - des_generic.ko
[    6.416039] init_module_from_file - cbc.ko
[    6.643702] init_module_from_file - r8152.ko - waited, ret = 0
[    7.940666] init_module_from_file - michael_mic.ko
[   11.233648] init_module_from_file - snd-q6apm.ko - waited, ret = 0
[   11.234264] init_module_from_file - q6prm.ko
[   11.303802] init_module_from_file - q6apm-dai.ko
[   11.304498] init_module_from_file - snd-q6dsp-common.ko
[   11.305476] init_module_from_file - q6apm-lpass-dais.ko
[   11.309858] init_module_from_file - q6prm-clocks.ko
[   11.446919] init_module_from_file - snd-soc-wsa883x.ko
[   11.446953] init_module_from_file - snd-soc-wsa883x.ko
[   11.456469] init_module_from_file - snd-soc-wsa883x.ko - waited, ret = 0
Luis Chamberlain May 30, 2023, 4:22 p.m. UTC | #19
On Mon, May 29, 2023 at 09:55:15PM -0400, Linus Torvalds wrote:
> On Mon, May 29, 2023 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > I took a closer look at some of the modules that failed to load and
> > noticed a pattern in that they have dependencies that are needed by more
> > than one device.
> 
> Ok, this is a "maybe something like this" RFC series of two patches -
> one trivial one to re-organize things a bit so that we can then do the
> real one which uses a filter based on the inode pointer to return an
> "idempotent return value" for module loads that share the same inode.
> 
> It's entirely untested, and since I'm on the road I'm going to not
> really be able to test it. It compiles for me, and the code looks
> fairly straightforward, but it's probably buggy.
> 
> It's very loosely based on Luis' attempt,  but it
>  (a) is internal to module loading
>  (b) uses a reliable cookie
>  (c) doesn't leave the cookie around randomly for later
>  (d) has seen absolutely no testing
> 
> Put another way: if somebody wants to play with this, please treat it
> as a starting point, not the final thing. You might need to debug
> things, and fix silly mistakes.
> 
> The idea is to just have a simple hash list of currently executing
> module loads, protected by a trivial spinlock. Every module loader
> adds itself to the right hash list, and if they were the *first* one
> (ie no other pending module loads for that inode), will actually do
> the module load.
> 
> Everybody who *isn't* the first one will just wait for completion and
> return the same error code that the first one returned.

That's also a hell much more snazzier MODULE_DEBUG_AUTOLOAD_DUPS if we
ever wanted to do something similar there if we wanted to also
join request_module() calls, instead of it hiding under debug.

> This is technically bogus. The first one might fail due to arguments.

For boot it's fine, as I can't think of boot wanting to support trying
to load a module with different arguments but who knows. But I can't
see it sensible to issue concurrent multiple requests for modules
with different arguments without waiting in userspace for the first
to fail.

Even post-boot, doing that sounds rather insane, but it would certainly
be a compromise and should probably be clearly documented. I think just
a comment acknolwedging that corner case seems sensible.

Because we won't be able to get the arguments until we process the
module, so it would be too late for this optimization on kread. So it is
why I had also stuck to the original feature being in kread, as then it
provides a uniq kread call and the caller is aware of it. But indeed I
had not considered the effects of arguments.

Lucas, any thoughts from modules kmod userspace perspective into
supporting anyone likely issuing concurrent modules requests with
differing arguments?

> So the cookie shouldn't be just the inode, it should be the inode and
> a hash of the arguments or something like that.

Personally I think it's a fine optimization without the arguments.

> But it is what it is,
> and apart from possible show-stopper bugs this is no worse than the
> failed "exclusive write deny" attempt. IOW - maybe worth trying?

The only thing I can think of is allowing threads other than the
first one to complete before the one that actually loaded the
module. I thought about this race for module auto-loading, see
the comment in kmod_dup_request_announce(), so that just
further delays the completion to other thread with a stupid
queue_work(). That seems more important for module auto-loading
duplicates than for boot finit_module() duplicates. But not sure
if odering matters in the end due to a preemtible kernel and maybe
that concern is hysteria.

> And if *that* didn't sell people on this patch series, I don't know
> what will. I should be in marketing! Two drink minimums, here I come!

Sold:

on 255 vcpus 0 duplicates found with this setup:

root@kmod ~ # cat /sys/kernel/debug/modules/stats
         Mods ever loaded       66
     Mods failed on kread       0
Mods failed on decompress       0
  Mods failed on becoming       0
      Mods failed on load       0
        Total module size       11268096
      Total mod text size       4149248
       Failed kread bytes       0
  Failed decompress bytes       0
    Failed becoming bytes       0
        Failed kmod bytes       0
 Virtual mem wasted bytes       0
         Average mod size       170729
    Average mod text size       62868

So:

Tested-by: Luis Chamberlain <mcgrof@kernel.org>

In terms of bootup timing:

Before:
Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s        
graphical.target reached after 44.178s in userspace.                             
                                                                                 
After:                                                                           
Startup finished in 23.995s (kernel) + 40.350s (userspace) = 1min 4.345s         
graphical.target reached after 40.226s in userspace. 

So other than the brain farts above, I think this is pretty sexy.

  Luis
Lucas De Marchi May 30, 2023, 5:16 p.m. UTC | #20
On Tue, May 30, 2023 at 09:22:14AM -0700, Luis Chamberlain wrote:
>On Mon, May 29, 2023 at 09:55:15PM -0400, Linus Torvalds wrote:
>> On Mon, May 29, 2023 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
>> >
>> > I took a closer look at some of the modules that failed to load and
>> > noticed a pattern in that they have dependencies that are needed by more
>> > than one device.
>>
>> Ok, this is a "maybe something like this" RFC series of two patches -
>> one trivial one to re-organize things a bit so that we can then do the
>> real one which uses a filter based on the inode pointer to return an
>> "idempotent return value" for module loads that share the same inode.
>>
>> It's entirely untested, and since I'm on the road I'm going to not
>> really be able to test it. It compiles for me, and the code looks
>> fairly straightforward, but it's probably buggy.
>>
>> It's very loosely based on Luis' attempt,  but it
>>  (a) is internal to module loading
>>  (b) uses a reliable cookie
>>  (c) doesn't leave the cookie around randomly for later
>>  (d) has seen absolutely no testing
>>
>> Put another way: if somebody wants to play with this, please treat it
>> as a starting point, not the final thing. You might need to debug
>> things, and fix silly mistakes.
>>
>> The idea is to just have a simple hash list of currently executing
>> module loads, protected by a trivial spinlock. Every module loader
>> adds itself to the right hash list, and if they were the *first* one
>> (ie no other pending module loads for that inode), will actually do
>> the module load.
>>
>> Everybody who *isn't* the first one will just wait for completion and
>> return the same error code that the first one returned.
>
>That's also a hell much more snazzier MODULE_DEBUG_AUTOLOAD_DUPS if we
>ever wanted to do something similar there if we wanted to also
>join request_module() calls, instead of it hiding under debug.
>
>> This is technically bogus. The first one might fail due to arguments.
>
>For boot it's fine, as I can't think of boot wanting to support trying
>to load a module with different arguments but who knows. But I can't
>see it sensible to issue concurrent multiple requests for modules
>with different arguments without waiting in userspace for the first
>to fail.
>
>Even post-boot, doing that sounds rather insane, but it would certainly
>be a compromise and should probably be clearly documented. I think just
>a comment acknolwedging that corner case seems sensible.
>
>Because we won't be able to get the arguments until we process the
>module, so it would be too late for this optimization on kread. So it is
>why I had also stuck to the original feature being in kread, as then it
>provides a uniq kread call and the caller is aware of it. But indeed I
>had not considered the effects of arguments.
>
>Lucas, any thoughts from modules kmod userspace perspective into
>supporting anyone likely issuing concurrent modules requests with
>differing arguments?

Changing module params like that without first explicitly removing the
module was never supported by kmod or module-init-tools (I'm not digging
the history before 2.6 kernel)

During boot, note there is already a shortcut
if we have the sysfs node already in the "live" state or if the module is
built-in. In that case we will return success or -EEXIST (if the
KMOD_PROBE_IGNORE_LOADED flag was passed). To be 100% equivalent when
covering the window between the first finit_module() and the sysfs node
being created, then we could add a similar flag to finit_module() and
return -EEXIST. Note however that likbmod will already clear the error
when no flag is passed, which is the normal case during boot.

The only scenario I can think of during boot in which the module params
could change would be when a buggy initrd is created, i.e.
/etc/modprobed.d/*.conf is in the rootfs but absent from initrd.

So returning the same error code seems good to me.

thanks
Lucas De Marchi

>
>> So the cookie shouldn't be just the inode, it should be the inode and
>> a hash of the arguments or something like that.
>
>Personally I think it's a fine optimization without the arguments.
>
>> But it is what it is,
>> and apart from possible show-stopper bugs this is no worse than the
>> failed "exclusive write deny" attempt. IOW - maybe worth trying?
>
>The only thing I can think of is allowing threads other than the
>first one to complete before the one that actually loaded the
>module. I thought about this race for module auto-loading, see
>the comment in kmod_dup_request_announce(), so that just
>further delays the completion to other thread with a stupid
>queue_work(). That seems more important for module auto-loading
>duplicates than for boot finit_module() duplicates. But not sure
>if odering matters in the end due to a preemtible kernel and maybe
>that concern is hysteria.
>
>> And if *that* didn't sell people on this patch series, I don't know
>> what will. I should be in marketing! Two drink minimums, here I come!
>
>Sold:
>
>on 255 vcpus 0 duplicates found with this setup:
>
>root@kmod ~ # cat /sys/kernel/debug/modules/stats
>         Mods ever loaded       66
>     Mods failed on kread       0
>Mods failed on decompress       0
>  Mods failed on becoming       0
>      Mods failed on load       0
>        Total module size       11268096
>      Total mod text size       4149248
>       Failed kread bytes       0
>  Failed decompress bytes       0
>    Failed becoming bytes       0
>        Failed kmod bytes       0
> Virtual mem wasted bytes       0
>         Average mod size       170729
>    Average mod text size       62868
>
>So:
>
>Tested-by: Luis Chamberlain <mcgrof@kernel.org>
>
>In terms of bootup timing:
>
>Before:
>Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s
>graphical.target reached after 44.178s in userspace.
>
>After:
>Startup finished in 23.995s (kernel) + 40.350s (userspace) = 1min 4.345s
>graphical.target reached after 40.226s in userspace.
>
>So other than the brain farts above, I think this is pretty sexy.
>
>  Luis
Luis Chamberlain May 30, 2023, 7:41 p.m. UTC | #21
On Tue, May 30, 2023 at 10:16:22AM -0700, Lucas De Marchi wrote:
> On Tue, May 30, 2023 at 09:22:14AM -0700, Luis Chamberlain wrote:
> > Lucas, any thoughts from modules kmod userspace perspective into
> > supporting anyone likely issuing concurrent modules requests with
> > differing arguments?
> 
> Changing module params like that without first explicitly removing the
> module was never supported by kmod or module-init-tools (I'm not digging
> the history before 2.6 kernel)

That's good enough.

> During boot, note there is already a shortcut
> if we have the sysfs node already in the "live" state or if the module is
> built-in. In that case we will return success or -EEXIST (if the
> KMOD_PROBE_IGNORE_LOADED flag was passed).

Linus' code would make duplicates wait and share the same return value,
ie, no new odd error code is returned. Or are you suggesting -EEXIST
should be returned to duplicates if the module succeeded to load
instead of 0 ?

> The only scenario I can think of during boot in which the module params
> could change would be when a buggy initrd is created, i.e.
> /etc/modprobed.d/*.conf is in the rootfs but absent from initrd.

This helps thanks.

> So returning the same error code seems good to me.

OK thanks! So just to confirm, it seems fine to return the same error
code if duplicates wait, or do you prefer for some reason for there to
be an exception and return -EEXIST if the module did succeed in the
duplicate case?

  Luis
Linus Torvalds May 30, 2023, 10:17 p.m. UTC | #22
On Tue, May 30, 2023 at 3:41 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> OK thanks! So just to confirm, it seems fine to return the same error
> code if duplicates wait, or do you prefer for some reason for there to
> be an exception and return -EEXIST if the module did succeed in the
> duplicate case?

I think either should be fine, since either was possible before.

By definition, these are module loads being done in parallel, and so
any of them "could" have been the first, and returned success before.

And by extension, any of them could have been not first, and returned
-EEXIST if somebody else loaded the same module first.

So that "somebody else did a load" code:

        if (idempotent(&idem, file_inode(f))) {
                wait_for_completion(&idem.complete);
                return idem.ret;
        }

could certainly have made the return value be something like

        return idem.ret ? : -EEXIST;

instead of that "return idem.ret".

But it does seem simpler - and more in line with the conceptual
"loading the same module is an idempotent operation" of the patch -
to just always return the success value to all of them.

After all, they all did in some sense succeed to get that module
loaded, even if it was a communal effort, and some threads did more
than others...

As mentioned, I don't think it can matter either way, since any of the
callers might as well have been the successful one, and they would
basically have to act the same way regardless (ie "somebody else
succeeded" and "you succeeded" are basically equivalent return
values). If the module was a prerequisite for another module being
loaded, either -EEXIST or 0 _is_ a success case.

             Linus
Dan Williams May 30, 2023, 10:45 p.m. UTC | #23
Linus Torvalds wrote:
> On Mon, May 29, 2023 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > I took a closer look at some of the modules that failed to load and
> > noticed a pattern in that they have dependencies that are needed by more
> > than one device.
> 
> Ok, this is a "maybe something like this" RFC series of two patches -
> one trivial one to re-organize things a bit so that we can then do the
> real one which uses a filter based on the inode pointer to return an
> "idempotent return value" for module loads that share the same inode.
> 
> It's entirely untested, and since I'm on the road I'm going to not
> really be able to test it. It compiles for me, and the code looks
> fairly straightforward, but it's probably buggy.

At least for me, these 2 patches, plus Johan's spinlock fixup, are:

Tested-by: Dan Williams <dan.j.williams@intel.com>

...on the test that was previously broken on plain v6.4-rc4. I have
these cherry picked on top of v6.4-rc4:

 Revert "module: error out early on concurrent load of the same module file"
 module: split up 'finit_module()' into init_module_from_file() helper
 modules: catch concurrent module loads, take two
Luis Chamberlain May 31, 2023, 12:31 a.m. UTC | #24
On Tue, May 30, 2023 at 09:22:14AM -0700, Luis Chamberlain wrote:
> The only thing I can think of is allowing threads other than the
> first one to complete before the one that actually loaded the
> module. I thought about this race for module auto-loading, see
> the comment in kmod_dup_request_announce(), so that just
> further delays the completion to other thread with a stupid
> queue_work(). That seems more important for module auto-loading
> duplicates than for boot finit_module() duplicates. But not sure
> if odering matters in the end due to a preemtible kernel and maybe
> that concern is hysteria.

I think I'm OK to accept this ordering concern as hysteria for now.

  Luis
Lucas De Marchi May 31, 2023, 5:30 a.m. UTC | #25
On Tue, May 30, 2023 at 06:17:11PM -0400, Linus Torvalds wrote:
>On Tue, May 30, 2023 at 3:41 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>
>> OK thanks! So just to confirm, it seems fine to return the same error
>> code if duplicates wait, or do you prefer for some reason for there to
>> be an exception and return -EEXIST if the module did succeed in the
>> duplicate case?
>
>I think either should be fine, since either was possible before.
>
>By definition, these are module loads being done in parallel, and so
>any of them "could" have been the first, and returned success before.
>
>And by extension, any of them could have been not first, and returned
>-EEXIST if somebody else loaded the same module first.
>
>So that "somebody else did a load" code:
>
>        if (idempotent(&idem, file_inode(f))) {
>                wait_for_completion(&idem.complete);
>                return idem.ret;
>        }
>
>could certainly have made the return value be something like
>
>        return idem.ret ? : -EEXIST;

yes, this is what I had in mind.

>
>instead of that "return idem.ret".
>
>But it does seem simpler - and more in line with the conceptual
>"loading the same module is an idempotent operation" of the patch -
>to just always return the success value to all of them.
>
>After all, they all did in some sense succeed to get that module
>loaded, even if it was a communal effort, and some threads did more
>than others...
>
>As mentioned, I don't think it can matter either way, since any of the
>callers might as well have been the successful one, and they would
>basically have to act the same way regardless (ie "somebody else
>succeeded" and "you succeeded" are basically equivalent return

agreed, it will just be a slightly different behavior if finit_module()
is called twice and the first call is already in the process of
initializing the module, i.e. complete_formation() was already called,
putting the module in the MODULE_STATE_COMING state, as per
kernel/module/main.c:add_unformed_module():

	/*
	 * We are here only when the same module was being loaded. Do
	 * not try to load it again right now. It prevents long delays
	 * caused by serialized module load failures. It might happen
	 * when more devices of the same type trigger load of
	 * a particular module.
	 */
	if (old && old->state == MODULE_STATE_LIVE)
		err = -EEXIST;
	else
		err = -EBUSY;
	goto out;

in userspace we already deal with that in a special way and should be
compatible with returning 0 for all practical purposes.

thanks
Lucas De Marchi

>values). If the module was a prerequisite for another module being
>loaded, either -EEXIST or 0 _is_ a success case.
>
>             Linus
David Hildenbrand May 31, 2023, 7:51 a.m. UTC | #26
On 30.05.23 18:22, Luis Chamberlain wrote:
> On Mon, May 29, 2023 at 09:55:15PM -0400, Linus Torvalds wrote:
>> On Mon, May 29, 2023 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
>>>
>>> I took a closer look at some of the modules that failed to load and
>>> noticed a pattern in that they have dependencies that are needed by more
>>> than one device.
>>
>> Ok, this is a "maybe something like this" RFC series of two patches -
>> one trivial one to re-organize things a bit so that we can then do the
>> real one which uses a filter based on the inode pointer to return an
>> "idempotent return value" for module loads that share the same inode.
>>
>> It's entirely untested, and since I'm on the road I'm going to not
>> really be able to test it. It compiles for me, and the code looks
>> fairly straightforward, but it's probably buggy.
>>
>> It's very loosely based on Luis' attempt,  but it
>>   (a) is internal to module loading
>>   (b) uses a reliable cookie
>>   (c) doesn't leave the cookie around randomly for later
>>   (d) has seen absolutely no testing
>>
>> Put another way: if somebody wants to play with this, please treat it
>> as a starting point, not the final thing. You might need to debug
>> things, and fix silly mistakes.
>>
>> The idea is to just have a simple hash list of currently executing
>> module loads, protected by a trivial spinlock. Every module loader
>> adds itself to the right hash list, and if they were the *first* one
>> (ie no other pending module loads for that inode), will actually do
>> the module load.
>>
>> Everybody who *isn't* the first one will just wait for completion and
>> return the same error code that the first one returned.
> 
> That's also a hell much more snazzier MODULE_DEBUG_AUTOLOAD_DUPS if we
> ever wanted to do something similar there if we wanted to also
> join request_module() calls, instead of it hiding under debug.
> 
>> This is technically bogus. The first one might fail due to arguments.
> 
> For boot it's fine, as I can't think of boot wanting to support trying
> to load a module with different arguments but who knows. But I can't
> see it sensible to issue concurrent multiple requests for modules
> with different arguments without waiting in userspace for the first
> to fail.
> 
> Even post-boot, doing that sounds rather insane, but it would certainly
> be a compromise and should probably be clearly documented. I think just
> a comment acknolwedging that corner case seems sensible.
> 
> Because we won't be able to get the arguments until we process the
> module, so it would be too late for this optimization on kread. So it is
> why I had also stuck to the original feature being in kread, as then it
> provides a uniq kread call and the caller is aware of it. But indeed I
> had not considered the effects of arguments.
> 
> Lucas, any thoughts from modules kmod userspace perspective into
> supporting anyone likely issuing concurrent modules requests with
> differing arguments?
> 
>> So the cookie shouldn't be just the inode, it should be the inode and
>> a hash of the arguments or something like that.
> 
> Personally I think it's a fine optimization without the arguments.
> 
>> But it is what it is,
>> and apart from possible show-stopper bugs this is no worse than the
>> failed "exclusive write deny" attempt. IOW - maybe worth trying?
> 
> The only thing I can think of is allowing threads other than the
> first one to complete before the one that actually loaded the
> module. I thought about this race for module auto-loading, see
> the comment in kmod_dup_request_announce(), so that just
> further delays the completion to other thread with a stupid
> queue_work(). That seems more important for module auto-loading
> duplicates than for boot finit_module() duplicates. But not sure
> if odering matters in the end due to a preemtible kernel and maybe
> that concern is hysteria.
> 
>> And if *that* didn't sell people on this patch series, I don't know
>> what will. I should be in marketing! Two drink minimums, here I come!
> 
> Sold:
> 
> on 255 vcpus 0 duplicates found with this setup:
> 
> root@kmod ~ # cat /sys/kernel/debug/modules/stats
>           Mods ever loaded       66
>       Mods failed on kread       0
> Mods failed on decompress       0
>    Mods failed on becoming       0
>        Mods failed on load       0
>          Total module size       11268096
>        Total mod text size       4149248
>         Failed kread bytes       0
>    Failed decompress bytes       0
>      Failed becoming bytes       0
>          Failed kmod bytes       0
>   Virtual mem wasted bytes       0
>           Average mod size       170729
>      Average mod text size       62868
> 
> So:
> 
> Tested-by: Luis Chamberlain <mcgrof@kernel.org>
> 
> In terms of bootup timing:
> 
> Before:
> Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s
> graphical.target reached after 44.178s in userspace.
>                                                                                   
> After:
> Startup finished in 23.995s (kernel) + 40.350s (userspace) = 1min 4.345s
> graphical.target reached after 40.226s in userspace.

I'll try grabbing the system where we saw the KASAN-related issues [1] 
and give it a churn with and without the two patches. Might take a bit 
(~1 day), unfortunately.

[1] https://lkml.kernel.org/r/20221013180518.217405-1-david@redhat.com
Luis Chamberlain May 31, 2023, 4:57 p.m. UTC | #27
On Wed, May 31, 2023 at 09:51:41AM +0200, David Hildenbrand wrote:
> On 30.05.23 18:22, Luis Chamberlain wrote:
> > On Mon, May 29, 2023 at 09:55:15PM -0400, Linus Torvalds wrote:
> > > On Mon, May 29, 2023 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
> > > > 
> > > > I took a closer look at some of the modules that failed to load and
> > > > noticed a pattern in that they have dependencies that are needed by more
> > > > than one device.
> > > 
> > > Ok, this is a "maybe something like this" RFC series of two patches -
> > > one trivial one to re-organize things a bit so that we can then do the
> > > real one which uses a filter based on the inode pointer to return an
> > > "idempotent return value" for module loads that share the same inode.
> > > 
> > > It's entirely untested, and since I'm on the road I'm going to not
> > > really be able to test it. It compiles for me, and the code looks
> > > fairly straightforward, but it's probably buggy.
> > > 
> > > It's very loosely based on Luis' attempt,  but it
> > >   (a) is internal to module loading
> > >   (b) uses a reliable cookie
> > >   (c) doesn't leave the cookie around randomly for later
> > >   (d) has seen absolutely no testing
> > > 
> > > Put another way: if somebody wants to play with this, please treat it
> > > as a starting point, not the final thing. You might need to debug
> > > things, and fix silly mistakes.
> > > 
> > > The idea is to just have a simple hash list of currently executing
> > > module loads, protected by a trivial spinlock. Every module loader
> > > adds itself to the right hash list, and if they were the *first* one
> > > (ie no other pending module loads for that inode), will actually do
> > > the module load.
> > > 
> > > Everybody who *isn't* the first one will just wait for completion and
> > > return the same error code that the first one returned.
> > 
> > That's also a hell much more snazzier MODULE_DEBUG_AUTOLOAD_DUPS if we
> > ever wanted to do something similar there if we wanted to also
> > join request_module() calls, instead of it hiding under debug.
> > 
> > > This is technically bogus. The first one might fail due to arguments.
> > 
> > For boot it's fine, as I can't think of boot wanting to support trying
> > to load a module with different arguments but who knows. But I can't
> > see it sensible to issue concurrent multiple requests for modules
> > with different arguments without waiting in userspace for the first
> > to fail.
> > 
> > Even post-boot, doing that sounds rather insane, but it would certainly
> > be a compromise and should probably be clearly documented. I think just
> > a comment acknolwedging that corner case seems sensible.
> > 
> > Because we won't be able to get the arguments until we process the
> > module, so it would be too late for this optimization on kread. So it is
> > why I had also stuck to the original feature being in kread, as then it
> > provides a uniq kread call and the caller is aware of it. But indeed I
> > had not considered the effects of arguments.
> > 
> > Lucas, any thoughts from modules kmod userspace perspective into
> > supporting anyone likely issuing concurrent modules requests with
> > differing arguments?
> > 
> > > So the cookie shouldn't be just the inode, it should be the inode and
> > > a hash of the arguments or something like that.
> > 
> > Personally I think it's a fine optimization without the arguments.
> > 
> > > But it is what it is,
> > > and apart from possible show-stopper bugs this is no worse than the
> > > failed "exclusive write deny" attempt. IOW - maybe worth trying?
> > 
> > The only thing I can think of is allowing threads other than the
> > first one to complete before the one that actually loaded the
> > module. I thought about this race for module auto-loading, see
> > the comment in kmod_dup_request_announce(), so that just
> > further delays the completion to other thread with a stupid
> > queue_work(). That seems more important for module auto-loading
> > duplicates than for boot finit_module() duplicates. But not sure
> > if odering matters in the end due to a preemtible kernel and maybe
> > that concern is hysteria.
> > 
> > > And if *that* didn't sell people on this patch series, I don't know
> > > what will. I should be in marketing! Two drink minimums, here I come!
> > 
> > Sold:
> > 
> > on 255 vcpus 0 duplicates found with this setup:
> > 
> > root@kmod ~ # cat /sys/kernel/debug/modules/stats
> >           Mods ever loaded       66
> >       Mods failed on kread       0
> > Mods failed on decompress       0
> >    Mods failed on becoming       0
> >        Mods failed on load       0
> >          Total module size       11268096
> >        Total mod text size       4149248
> >         Failed kread bytes       0
> >    Failed decompress bytes       0
> >      Failed becoming bytes       0
> >          Failed kmod bytes       0
> >   Virtual mem wasted bytes       0
> >           Average mod size       170729
> >      Average mod text size       62868
> > 
> > So:
> > 
> > Tested-by: Luis Chamberlain <mcgrof@kernel.org>
> > 
> > In terms of bootup timing:
> > 
> > Before:
> > Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s
> > graphical.target reached after 44.178s in userspace.
> > After:
> > Startup finished in 23.995s (kernel) + 40.350s (userspace) = 1min 4.345s
> > graphical.target reached after 40.226s in userspace.
> 
> I'll try grabbing the system where we saw the KASAN-related issues [1] and
> give it a churn with and without the two patches. Might take a bit (~1 day),
> unfortunately.
> 
> [1] https://lkml.kernel.org/r/20221013180518.217405-1-david@redhat.com

Great, don't forget:

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 82b0dcc1fe77..222015093eeb 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3066,7 +3066,7 @@ struct idempotent {
 
 #define IDEM_HASH_BITS 8
 static struct hlist_head idem_hash[1 << IDEM_HASH_BITS];
-static struct spinlock idem_lock;
+static DEFINE_SPINLOCK(idem_lock);
 
 static bool idempotent(struct idempotent *u, const void *cookie)
 {
Luis Chamberlain June 28, 2023, 6:52 p.m. UTC | #28
On Mon, Jun 05, 2023 at 08:28:41AM -0700, Luis Chamberlain wrote:
> On Mon, Jun 05, 2023 at 08:17:42AM -0700, Luis Chamberlain wrote:
> > We've gone down from ~6 GiB to ~6 MiB.
> 
> And just to also highlight, that was just for for the KASAN enabled case, and
> for !KASAN we went from ~18 GiB to 0.

Linus, were you thinking of including these patches in for v6.5-rc1?

  Luis
Linus Torvalds June 28, 2023, 8:14 p.m. UTC | #29
On Wed, 28 Jun 2023 at 11:52, Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> Linus, were you thinking of including these patches in for v6.5-rc1?

Heh, I don't even have them in my tree any more, I was assuming that
if they were good they'd be sent back to me...

I guess I can go fish them out from my old emails, but I actually was
expecting to just get them from you.

I do actually maintain my own branches for *some* things, and merge
them myself, but they tend to be for areas that I feel I'm
co-maintaining (ie notably vfs and mm that I still feel I'm involved
in).

In other areas, I may send out patches, but I don't feel like I'm a
maintainer, so I then think that "the real maintainer can decide if
these patches are good or not".

And I would very much hope that people don't take said patches _just_
because they come from me. They should be judged on their own merits,
and then occasionally people can mutter "Christ, what drugs is Linus
on _today_?" and just throw my patches in the garbage.

             Linus
Linus Torvalds June 28, 2023, 10:07 p.m. UTC | #30
On Wed, 28 Jun 2023 at 13:14, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> In other areas, I may send out patches, but I don't feel like I'm a
> maintainer, so I then think that "the real maintainer can decide if
> these patches are good or not".

Anyway, since I clearly didn't communicate clearly that I was throwing
the patch over to you, let me go and find it and just apply it myself.

           Linus
Linus Torvalds June 28, 2023, 11:17 p.m. UTC | #31
On Wed, 28 Jun 2023 at 15:07, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Anyway, since I clearly didn't communicate clearly that I was throwing
> the patch over to you, let me go and find it and just apply it myself.

Ok, I spent some time writing a better commit message for that change,
tried to make sure I got everybody's comments and Tested-by's sorted
out etc, and committed the end result.

I also did the actual module pull and your sysctl pull.

Can you please check that it all looks like you expected?

              Linus
Luis Chamberlain June 29, 2023, 12:18 a.m. UTC | #32
On Wed, Jun 28, 2023 at 04:17:10PM -0700, Linus Torvalds wrote:
> On Wed, 28 Jun 2023 at 15:07, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Anyway, since I clearly didn't communicate clearly that I was throwing
> > the patch over to you, let me go and find it and just apply it myself.
> 
> Ok, I spent some time writing a better commit message for that change,
> tried to make sure I got everybody's comments and Tested-by's sorted
> out etc, and committed the end result.
> 
> I also did the actual module pull and your sysctl pull.
> 
> Can you please check that it all looks like you expected?

Looks good to me, thanks for doing that. It certainly was not clear
to me that you had expected me to merge your patch just because you
had previously merged your own and later reverted it. But now I know
in the future I may just pick up your patches, instead of expecting
you to shortcut it, thanks!

  Luis
diff mbox series

Patch

diff --git a/include/linux/module.h b/include/linux/module.h
index 9e56763dff81..afc44df96278 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -557,6 +557,7 @@  struct module {
 	unsigned int printk_index_size;
 	struct pi_entry **printk_index_start;
 #endif
+	unsigned long i_ino;
 
 #ifdef CONFIG_MODULE_UNLOAD
 	/* What modules depend on me? */
diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig
index 33a2e991f608..85a6c7c5ddc0 100644
--- a/kernel/module/Kconfig
+++ b/kernel/module/Kconfig
@@ -157,6 +157,26 @@  config MODULE_UNLOAD_TAINT_TRACKING
 	  page (see bad_page()), the aforementioned details are also
 	  shown. If unsure, say N.
 
+config MODULE_KREAD_UNIQ
+	bool "Avoid duplicate module kernel reads"
+	select KREAD_UNIQ
+	help
+	  Enable this option to avoid vmalloc() space for duplicate module
+	  requests early before we can even check for the module name. This
+	  is useful to avoid duplicate module requests which userspace or kernel
+	  can issue. On some architectures such as x86_64 there is only 128 MiB
+	  of virtual memory space and since in the worst case we can end up
+	  allocating up to 3 times the module size in vmalloc space, avoiding
+	  duplicates can save virtual memory on boot.
+
+	  Enabling this will incrase your kernel by about 945 bytes, but can
+	  save considerable memory during bootup which would otherwise be freed
+	  and this in turn can help speed up kernel boot time.
+
+	  Disable this if you have enabled CONFIG_MODULE_STATS and have
+	  verified you see no duplicates or virtual memory being freed on
+	  bootup.
+
 config MODVERSIONS
 	bool "Module versioning support"
 	help
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index dc7b0160c480..7ea7f177f907 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -67,6 +67,7 @@  struct load_info {
 	unsigned int max_pages;
 	unsigned int used_pages;
 #endif
+	unsigned long i_ino;
 	struct {
 		unsigned int sym, str, mod, vers, info, pcpu;
 	} index;
diff --git a/kernel/module/main.c b/kernel/module/main.c
index ea7d0c7f3e60..e35900ee616a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1283,6 +1283,7 @@  static void free_module(struct module *mod)
 	kfree(mod->args);
 	percpu_modfree(mod);
 
+	kread_uniq_fd_free(mod->i_ino);
 	free_mod_mem(mod);
 }
 
@@ -1964,12 +1965,14 @@  static int copy_module_from_user(const void __user *umod, unsigned long len,
 	return err;
 }
 
-static void free_copy(struct load_info *info, int flags)
+static void free_copy(struct load_info *info, int flags, bool error)
 {
 	if (flags & MODULE_INIT_COMPRESSED_FILE)
 		module_decompress_cleanup(info);
 	else
 		vfree(info->hdr);
+	if (error)
+		kread_uniq_fd_free(info->i_ino);
 }
 
 static int rewrite_section_headers(struct load_info *info, int flags)
@@ -2965,7 +2968,7 @@  static int load_module(struct load_info *info, const char __user *uargs,
 	}
 
 	/* Get rid of temporary copy. */
-	free_copy(info, flags);
+	free_copy(info, flags, false);
 
 	/* Done! */
 	trace_module_load(mod);
@@ -3023,7 +3026,7 @@  static int load_module(struct load_info *info, const char __user *uargs,
 	 */
 	if (!module_allocated)
 		mod_stat_bump_becoming(info, flags);
-	free_copy(info, flags);
+	free_copy(info, flags, true);
 	return err;
 }
 
@@ -3068,11 +3071,12 @@  SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
 		      |MODULE_INIT_COMPRESSED_FILE))
 		return -EINVAL;
 
-	len = kernel_read_file_from_fd(fd, 0, &buf, INT_MAX, NULL,
-				       READING_MODULE);
+	len = kread_uniq_fd(fd, 0, &buf, &info.i_ino, INT_MAX, NULL, READING_MODULE);
 	if (len < 0) {
-		mod_stat_inc(&failed_kreads);
-		mod_stat_add_long(len, &invalid_kread_bytes);
+		if (len != -EBUSY) {
+			mod_stat_inc(&failed_kreads);
+			mod_stat_add_long(len, &invalid_kread_bytes);
+		}
 		return len;
 	}
 
@@ -3082,6 +3086,7 @@  SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
 		if (err) {
 			mod_stat_inc(&failed_decompress);
 			mod_stat_add_long(len, &invalid_decompress_bytes);
+			kread_uniq_fd_free(info.i_ino);
 			return err;
 		}
 	} else {