Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753370AbcLHU2L (ORCPT ); Thu, 8 Dec 2016 15:28:11 -0500 Received: from mail-io0-f177.google.com ([209.85.223.177]:33178 "EHLO mail-io0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752257AbcLHU2J (ORCPT ); Thu, 8 Dec 2016 15:28:09 -0500 MIME-Version: 1.0 In-Reply-To: <20161208194814.2485-1-mcgrof@kernel.org> References: <20161208184801.1689-1-mcgrof@kernel.org> <20161208194814.2485-1-mcgrof@kernel.org> From: Kees Cook Date: Thu, 8 Dec 2016 12:28:07 -0800 X-Google-Sender-Auth: x1DcWiL6Vw9-TV8REq7kMBsEl1E Message-ID: Subject: Re: [RFC 03/10] kmod: add dynamic max concurrent thread count To: "Luis R. Rodriguez" Cc: shuah@kernel.org, Jessica Yu , Rusty Russell , "Eric W. Biederman" , Dmitry Torokhov , Arnaldo Carvalho de Melo , Jonathan Corbet , martin.wilck@suse.com, Michal Marek , Petr Mladek , hare@suse.com, rwright@hpe.com, Jeff Mahoney , DSterba@suse.com, fdmanana@suse.com, neilb@suse.com, Guenter Roeck , rgoldwyn@suse.com, subashab@codeaurora.org, Heinrich Schuchardt , Aaron Tomlin , mbenes@suse.cz, "Paul E. McKenney" , Dan Williams , Josh Poimboeuf , "David S. Miller" , Ingo Molnar , Andrew Morton , Linus Torvalds , linux-kselftest@vger.kernel.org, "linux-doc@vger.kernel.org" , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8872 Lines: 216 On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez wrote: > We currently statically limit the number of modprobe threads which > we allow to run concurrently to 50. As per Keith Owens, this was a > completely arbitrary value, and it was set in the 2.3.38 days [0] > over 16 years ago in year 2000. > > Although we haven't yet hit our lower limits, experimentation [1] > shows that when and if we hit this limit in the worst case, will be > fatal -- consider get_fs_type() failures upon mount on a system which > has many partitions, some of which might even be with the same > filesystem. Its best to be prudent and increase and set this > value to something more sensible which ensures we're far from hitting > the limit and also allows default build/user run time override. > > The worst case is fatal given that once a module fails to load there > is a period of time during which subsequent request for the same module > will fail, so in the case of partitions its not just one request that > could fail, but whole series of partitions. This later issue of a > module request failure domino effect can be addressed later, but > increasing the limit to something more meaninful should at least give us > enough cushion to avoid this for a while. > > Set this value up with a bit more meaninful modern limits: > > Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) > Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL) > > Also allow the default max limit to be further fine tuned at compile > time and at initialization at run time at boot up using the kernel > parameter: max_modprobes. > > [0] https://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44 > [1] https://github.com/mcgrof/test_request_module > > Signed-off-by: Luis R. Rodriguez > --- > Documentation/admin-guide/kernel-parameters.txt | 7 ++++ > include/linux/kmod.h | 3 +- > init/Kconfig | 23 +++++++++++++ > init/main.c | 1 + > kernel/kmod.c | 43 ++++++++++++++++--------- > 5 files changed, 61 insertions(+), 16 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index be2d6d0a03a4..92bcccc65ea4 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1700,6 +1700,13 @@ > > keepinitrd [HW,ARM] > > + kmod.max_modprobes [KNL] > + This lets you set the max allowed of concurrent > + modprobes threads possible on a system overriding the > + default heuristic of: > + > + min(max_threads/2, 2 << CONFIG_MAX_KMOD_CONCURRENT) > + > kernelcore= [KNL,X86,IA-64,PPC] > Format: nn[KMGTPE] | "mirror" > This parameter > diff --git a/include/linux/kmod.h b/include/linux/kmod.h > index fcfd2bf14d3f..15783cd7f056 100644 > --- a/include/linux/kmod.h > +++ b/include/linux/kmod.h > @@ -38,13 +38,14 @@ int __request_module(bool wait, const char *name, ...); > #define request_module_nowait(mod...) __request_module(false, mod) > #define try_then_request_module(x, mod...) \ > ((x) ?: (__request_module(true, mod), (x))) > +void init_kmod_umh(void); > #else > static inline int request_module(const char *name, ...) { return -ENOSYS; } > static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; } > +static inline void init_kmod_umh(void) { } > #define try_then_request_module(x, mod...) (x) > #endif > > - > struct cred; > struct file; > > diff --git a/init/Kconfig b/init/Kconfig > index 271692a352f1..da2c25746937 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -2111,6 +2111,29 @@ config TRIM_UNUSED_KSYMS > > If unsure, or if you need to build out-of-tree modules, say N. > > +config MAX_KMOD_CONCURRENT > + int "Max allowed concurrent request_module() calls (6=>64, 10=>1024)" > + range 0 14 > + default 6 if !BASE_SMALL > + default 7 if BASE_SMALL > + help > + The kernel restricts the number of possible concurrent calls to > + request_module() to help avoid a recursive loop possible with > + modules. The default maximum number of concurrent threads allowed > + to run request_module() will be: > + > + max_modprobes = min(max_threads/2, 2 << CONFIG_MAX_KMOD_CONCURRENT); > + > + The value set in CONFIG_MAX_KMOD_CONCURRENT represents then the power > + of 2 value used at boot time for the above computation. You can > + override the default built value using the kernel parameter: > + > + kmod.max_modprobes=4096 > + > + We set this to default to 64 (2^6) concurrent modprobe threads for > + small systems, for larger systems this defaults to 128 (2^7) > + concurrent modprobe threads. > + > endif # MODULES > > config MODULES_TREE_LOOKUP > diff --git a/init/main.c b/init/main.c > index 8161208d4ece..1fa441aa32c6 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -638,6 +638,7 @@ asmlinkage __visible void __init start_kernel(void) > thread_stack_cache_init(); > cred_init(); > fork_init(); > + init_kmod_umh(); > proc_caches_init(); > buffer_init(); > key_init(); > diff --git a/kernel/kmod.c b/kernel/kmod.c > index 0277d1216f80..cb6f7ca7b8a5 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -44,6 +44,9 @@ > #include > > extern int max_threads; > +unsigned int max_modprobes; > +module_param(max_modprobes, uint, 0644); > +MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent modprobes"); > > #define CAP_BSET (void *)1 > #define CAP_PI (void *)2 > @@ -125,10 +128,8 @@ int __request_module(bool wait, const char *fmt, ...) > { > va_list args; > char module_name[MODULE_NAME_LEN]; > - unsigned int max_modprobes; > int ret; > static atomic_t kmod_concurrent = ATOMIC_INIT(0); > -#define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */ > static int kmod_loop_msg; > > /* > @@ -152,19 +153,6 @@ int __request_module(bool wait, const char *fmt, ...) > if (ret) > return ret; > > - /* If modprobe needs a service that is in a module, we get a recursive > - * loop. Limit the number of running kmod threads to max_threads/2 or > - * MAX_KMOD_CONCURRENT, whichever is the smaller. A cleaner method > - * would be to run the parents of this process, counting how many times > - * kmod was invoked. That would mean accessing the internals of the > - * process tables to get the command line, proc_pid_cmdline is static > - * and it is not worth changing the proc code just to handle this case. > - * KAO. > - * > - * "trace the ppid" is simple, but will fail if someone's > - * parent exits. I think this is as good as it gets. --RR > - */ > - max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT); > atomic_inc(&kmod_concurrent); > if (atomic_read(&kmod_concurrent) > max_modprobes) { > /* We may be blaming an innocent here, but unlikely */ > @@ -186,6 +174,31 @@ int __request_module(bool wait, const char *fmt, ...) > return ret; > } > EXPORT_SYMBOL(__request_module); > + > +/* > + * If modprobe needs a service that is in a module, we get a recursive > + * loop. Limit the number of running kmod threads to max_threads/2 or > + * CONFIG_MAX_KMOD_CONCURRENT, whichever is the smaller. A cleaner method > + * would be to run the parents of this process, counting how many times > + * kmod was invoked. That would mean accessing the internals of the > + * process tables to get the command line, proc_pid_cmdline is static > + * and it is not worth changing the proc code just to handle this case. > + * > + * "trace the ppid" is simple, but will fail if someone's > + * parent exits. I think this is as good as it gets. > + * > + * You can override with with a kernel parameter, for instance to allow > + * 4096 concurrent modprobe instances: > + * > + * kmod.max_modprobes=4096 > + */ > +void __init init_kmod_umh(void) What does umh mean? > +{ > + if (!max_modprobes) > + max_modprobes = min(max_threads/2, > + 2 << CONFIG_MAX_KMOD_CONCURRENT); > +} > + > #endif /* CONFIG_MODULES */ > > static void call_usermodehelper_freeinfo(struct subprocess_info *info) > -- > 2.10.1 > -- Kees Cook Nexus Security