On Wed, 7 Aug 2024 at 23:17, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
>
> OK I tracked it down to a recursive module load that hangs because
> of this commit:
>
> commit 9b9879fc03275ffe0da328cf5b864d9e694167c8
> Author: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Date: Mon May 29 21:39:51 2023 -0400
>
> modules: catch concurrent module loads, treat them as idempotent
>
> So what's happening here is that the first modprobe tries to load
> a fallback CBC implementation, in doing so it triggers a load of
> the exact same module due to module aliases.
Ahh. That would indeed be very wrong, but yes, the fact that it now
just ends up hanging is very annoying and not helpful for debugging.
Sadly, the "return -EBUSY" that the code initially did caused problems
because when the loading isn't recursive, but just concurrent (because
two separate users show up at the same time), you really do want to
wait for the original loader.
> Now I presume this used to just fail immediately which is OK because
> user-space would then try to load other aliases of ecb(aes). But it
> now hangs which causes the whole thing to freeze until a timeout
> hits somwhere along the line.
What used to happen is that the recursive loader would *also* load the
module into memory (so it's loaded twice), but then the two loaders at
the end get serialized by the 'module_mutex' when it does the
add_unformed_module()
So at that point the module_patient_check_exists() would notice that
one or the other loaded module is a duplicate, and would exit with an
error (EBUSY or EEXIST depending on whether the "winning" module got
to MODULE_STATE_LIVE in the meantime).
The new code exists exactly because on big machines, the "load X
modules concurrently" was a huge cost, and would use literally
gigabytes of memory loading the same duplicated module multiple times
concurrently, only for all but one of them to fail.
So now we catch that "we're already loading this module" early, and
wait for the first loader to do it all.
But yes, it does mean that you can't recursively load the same module
from the kernel.
... which was obviously always bogus, but now it's actively very wrong.
Sadly, loading modules recursively is very much required in general
(because modules depend on each other). So we do need to deal with
that. It's only loading the *same* module recurively that is very very
bad.
I guess we could at least add a timeout and a big fat warning when it
triggers. But what's the right timeout? Sometimes module loading can
really be very slow, if it problems hardware.
Let me think about this, because the new behavior is obviously not
great for this situation, even if it was triggered by a different
kernel bug / misfeature. From a debugging standpoint, that "silent
hang" is most definitely bad.
It did apparently take a long time for people to notice (that module
loading behavior is over a year old by now). How hard is it to just
fix the recursive load?
ANYWAY.
While I think some more about this, does this attached patch at least
give you an error printout? It won't fix the situation, but at least
the "silently wait forever" should turn into a "wait forever but with
a warning".
Which isn't perfect, but is better, and would presumably have made it
a whole lot easier to debug this nasty situation.
Hmm?
(Please note: ENTIRELY UNTESTED! It compiles for me, but I might have
done something incredibly stupid and maybe there's some silly and
fatal bug in what _appears_ trivially correct).
Linus
kernel/module/main.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index d9592195c5bb..4150a546a00a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3183,15 +3183,29 @@ static int idempotent_init_module(struct file *f, const char __user * uargs, int
if (!f || !(f->f_mode & FMODE_READ))
return -EBADF;
- /* See if somebody else is doing the operation? */
- if (idempotent(&idem, file_inode(f))) {
- wait_for_completion(&idem.complete);
- return idem.ret;
+ /* Are we the winners of the race and get to do this? */
+ if (!idempotent(&idem, file_inode(f))) {
+ int ret = init_module_from_file(f, uargs, flags);
+ return idempotent_complete(&idem, ret);
}
- /* Otherwise, we'll do it and complete others */
- return idempotent_complete(&idem,
- init_module_from_file(f, uargs, flags));
+ /*
+ * Somebody else won the race and is loading the module.
+ *
+ * We have to wait for it forever, since our 'idem' is
+ * on the stack and the list entry stays there until
+ * completed (but we could fix it under the idem_lock)
+ *
+ * It's also unclear what a real timeout might be,
+ * but we could maybe at least make this killable
+ * and remove the idem entry in that case?
+ */
+ for (;;) {
+ int ret = wait_for_completion_timeout(&idem.complete, 10*HZ);
+ if (likely(!ret))
+ return idem.ret;
+ pr_warn_once("module '%pD' taking a long time to load", f);
+ }
}
SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
