Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Fri, 24 Mar 2023 13:28:51 -0700 | Subject | Re: [RFC 00/12] module: avoid userspace pressure on unwanted allocations |
| |
On Fri, Mar 24, 2023 at 1:00 PM Luis Chamberlain <mcgrof@kernel.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
| |