[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHk-=whkj6=wyi201JXkw9iT_eTUTsSx+Yb9d4OgmZFjDJA18g@mail.gmail.com>
Date: Fri, 24 Mar 2023 13:28:51 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: David Hildenbrand <david@...hat.com>,
Kees Cook <keescook@...omium.org>,
linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org,
pmladek@...e.com, petr.pavlu@...e.com, prarit@...hat.com,
christophe.leroy@...roup.eu, song@...nel.org, dave@...olabs.net,
fan.ni@...sung.com, vincent.fu@...sung.com,
a.manzanares@...sung.com, colin.i.king@...il.com
Subject: Re: [RFC 00/12] module: avoid userspace pressure on unwanted allocations
On Fri, Mar 24, 2023 at 1:00 PM Luis Chamberlain <mcgrof@...nel.org> wrote:
>
> On the modules side of things we can be super defensive on the second
> vmalloc allocation defensive [0] but other than this the initial kread
> also needs care too.
Please don't re-implement semaphores. They are a *classic* concurrency limiter.
In fact, probably *THE* classic one.
So just do something like this instead:
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2937,6 +2937,11 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
return load_module(&info, uargs, 0);
}
+#define CONCURRENCY_LIMITER(name, n) \
+ struct semaphore name = __SEMAPHORE_INITIALIZER(name, n)
+
+static CONCURRENCY_LIMITER(module_loading_concurrency, 50);
+
SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs,
int, flags)
{
struct load_info info = { };
@@ -2955,8 +2960,12 @@ SYSCALL_DEFINE3(finit_module, int, fd, const
char __user *, uargs, int, flags)
|MODULE_INIT_COMPRESSED_FILE))
return -EINVAL;
+ err = down_killable(&module_loading_concurrency);
+ if (err)
+ return err;
len = kernel_read_file_from_fd(fd, 0, &buf, INT_MAX, NULL,
READING_MODULE);
+ up(&module_loading_concurrency);
if (len < 0)
return len;
NOTE! Entirely untested. Surprise surprise.
I'm a tiny bit worried about deadlocks here, so somebody needs to make
sure that the kernel_read_file_from_fd() case cannot possibly in turn
cause a "request_module()" recursion.
We most definitely have had module recursion before, but I *think*
it's always just in the module init function (ie one module requests
another when ithe mod->init() function is called).
I think by the time we have opened the module file descriptors and are
just reading the data, we should be ok and the above would never
deadlock, but I want people to at least think about it.
Of course, with that "max 50 concurrent loaders" limit, maybe it's
never an issue anyway. Even if you get a recursion a few deep, at most
you just wait for somebody else to get out of their loaders. Unless
*everybody* ends up waiting on some progress.
Linus
Powered by blists - more mailing lists