Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1538798 > unrolled thread

[RFC 00/10] kmod: stress test driver, few fixes and enhancements

Back to article view | Back to linux.kernel

Contents

  [RFC 00/10] kmod: stress test driver, few fixes and enhancements "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 19:50 +0100
    [RFC 02/10] module: fix memory leak on early load_module() failures "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
      Re: [RFC 02/10] module: fix memory leak on early load_module() failures Kees Cook <keescook@chromium.org> - 2016-12-08 21:40 +0100
        Re: [RFC 02/10] module: fix memory leak on early load_module() failures "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 22:20 +0100
          Re: [RFC 02/10] module: fix memory leak on early load_module() failures Kees Cook <keescook@chromium.org> - 2016-12-08 22:20 +0100
      Re: [RFC 02/10] module: fix memory leak on early load_module()  failures Miroslav Benes <mbenes@suse.cz> - 2016-12-09 18:10 +0100
        Re: [RFC 02/10] module: fix memory leak on early load_module()  failures "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 10:00 +0100
      Re: [RFC 02/10] module: fix memory leak on early load_module()  failures Aaron Tomlin <atomlin@redhat.com> - 2016-12-15 19:50 +0100
    [RFC 08/10] sysctl: add support for unsigned int properly "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
    [RFC 05/10] kmod: return -EBUSY if modprobe limit is reached "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
    [RFC 07/10] kmod: use simplified rate limit printk "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
      Re: [RFC 07/10] kmod: use simplified rate limit printk Petr Mladek <pmladek@suse.com> - 2016-12-14 17:30 +0100
        Re: [RFC 07/10] kmod: use simplified rate limit printk Joe Perches <joe@perches.com> - 2016-12-14 17:50 +0100
        Re: [RFC 07/10] kmod: use simplified rate limit printk "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 09:50 +0100
    [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
      Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec Kees Cook <keescook@chromium.org> - 2016-12-08 21:40 +0100
        Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 22:10 +0100
          Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec Petr Mladek <pmladek@suse.com> - 2016-12-15 13:50 +0100
            Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 09:10 +0100
              Re: kmod: provide wrappers for kmod_concurrent inc/dec Jessica Yu <jeyu@redhat.com> - 2016-12-22 05:50 +0100
      Re: kmod: provide wrappers for kmod_concurrent inc/dec Jessica Yu <jeyu@redhat.com> - 2016-12-22 06:10 +0100
    [RFC 06/10] kmod: provide sanity check on kmod_concurrent access "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
      Re: [RFC 06/10] kmod: provide sanity check on kmod_concurrent access Petr Mladek <pmladek@suse.com> - 2016-12-14 17:10 +0100
        Re: [RFC 06/10] kmod: provide sanity check on kmod_concurrent access "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-14 18:20 +0100
      Re: [RFC 06/10] kmod: provide sanity check on kmod_concurrent access Petr Mladek <pmladek@suse.com> - 2016-12-15 14:00 +0100
    [RFC 10/10] kmod: add a sanity check on module loading "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
      Re: [RFC 10/10] kmod: add a sanity check on module loading Martin Wilck <mwilck@suse.com> - 2016-12-09 21:10 +0100
        Re: [RFC 10/10] kmod: add a sanity check on module loading Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-09 22:00 +0100
          Re: [RFC 10/10] kmod: add a sanity check on module loading "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-15 19:10 +0100
      Re: [RFC 10/10] kmod: add a sanity check on module loading Rusty Russell <rusty@rustcorp.com.au> - 2016-12-15 02:30 +0100
        Re: [RFC 10/10] kmod: add a sanity check on module loading "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 09:40 +0100
          Re: [RFC 10/10] kmod: add a sanity check on module loading Rusty Russell <rusty@rustcorp.com.au> - 2016-12-17 05:00 +0100
            Re: [RFC 10/10] kmod: add a sanity check on module loading Rusty Russell <rusty@rustcorp.com.au> - 2016-12-20 04:10 +0100
              Re: [RFC 10/10] kmod: add a sanity check on module loading "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-20 20:00 +0100
                Re: [RFC 10/10] kmod: add a sanity check on module loading Rusty Russell <rusty@rustcorp.com.au> - 2016-12-21 06:30 +0100
                  Re: [RFC 10/10] kmod: add a sanity check on module loading "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-21 14:10 +0100
                    Re: [RFC 10/10] kmod: add a sanity check on module loading Rusty Russell <rusty@rustcorp.com.au> - 2017-01-03 03:10 +0100
    [RFC 03/10] kmod: add dynamic max concurrent thread count "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
      Re: [RFC 03/10] kmod: add dynamic max concurrent thread count Kees Cook <keescook@chromium.org> - 2016-12-08 21:30 +0100
        Re: [RFC 03/10] kmod: add dynamic max concurrent thread count "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 22:10 +0100
      Re: [RFC 03/10] kmod: add dynamic max concurrent thread count Petr Mladek <pmladek@suse.com> - 2016-12-14 17:00 +0100
        Re: [RFC 03/10] kmod: add dynamic max concurrent thread count "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 09:50 +0100
    [RFC 09/10] kmod: add helpers for getting kmod count and limit "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100
      Re: [RFC 09/10] kmod: add helpers for getting kmod count and limit Petr Mladek <pmladek@suse.com> - 2016-12-15 18:00 +0100
        Re: [RFC 09/10] kmod: add helpers for getting kmod count and limit "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 09:00 +0100
    Re: [RFC 01/10] kmod: add test driver to stress test the module  loader "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-13 22:20 +0100
      Re: [RFC 01/10] kmod: add test driver to stress test the module  loader "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 08:50 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1546158 — Re: kmod: provide wrappers for kmod_concurrent inc/dec

+++ Luis R. Rodriguez [08/12/16 11:48 -0800]:
>kmod_concurrent is used as an atomic counter for enabling
>the allowed limit of modprobe calls, provide wrappers for it
>to enable this to be expanded on more easily. This will be done
>later.
>
>Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>---
> kernel/kmod.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
>diff --git a/kernel/kmod.c b/kernel/kmod.c
>index cb6f7ca7b8a5..049d7eabda38 100644
>--- a/kernel/kmod.c
>+++ b/kernel/kmod.c
>@@ -44,6 +44,9 @@
> #include <trace/events/module.h>
>
> extern int max_threads;
>+
>+static atomic_t kmod_concurrent = ATOMIC_INIT(0);
>+
> unsigned int max_modprobes;
> module_param(max_modprobes, uint, 0644);
> MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent modprobes");
>@@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
> 	return -ENOMEM;
> }
>
>+static int kmod_umh_threads_get(void)
>+{
>+	atomic_inc(&kmod_concurrent);
>+	if (atomic_read(&kmod_concurrent) < max_modprobes)

Should this not be <=? I think this only allows up to max_modprobes-1 concurrent threads.

>+		return 0;
>+	atomic_dec(&kmod_concurrent);
>+	return -ENOMEM;
>+}
>+
>+static void kmod_umh_threads_put(void)
>+{
>+	atomic_dec(&kmod_concurrent);
>+}
>+
> /**
>  * __request_module - try to load a kernel module
>  * @wait: wait (or not) for the operation to complete
>@@ -129,7 +146,6 @@ int __request_module(bool wait, const char *fmt, ...)
> 	va_list args;
> 	char module_name[MODULE_NAME_LEN];
> 	int ret;
>-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> 	static int kmod_loop_msg;
>
> 	/*
>@@ -153,8 +169,8 @@ int __request_module(bool wait, const char *fmt, ...)
> 	if (ret)
> 		return ret;
>
>-	atomic_inc(&kmod_concurrent);
>-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
>+	ret = kmod_umh_threads_get();
>+	if (ret) {
> 		/* We may be blaming an innocent here, but unlikely */
> 		if (kmod_loop_msg < 5) {
> 			printk(KERN_ERR
>@@ -162,15 +178,14 @@ int __request_module(bool wait, const char *fmt, ...)
> 			       module_name);
> 			kmod_loop_msg++;
> 		}
>-		atomic_dec(&kmod_concurrent);
>-		return -ENOMEM;
>+		return ret;
> 	}
>
> 	trace_module_request(module_name, wait, _RET_IP_);
>
> 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
>
>-	atomic_dec(&kmod_concurrent);
>+	kmod_umh_threads_put();
> 	return ret;
> }
> EXPORT_SYMBOL(__request_module);
>-- 
>2.10.1
>

[toc] | [prev] | [next] | [standalone]

#1538838 — [RFC 06/10] kmod: provide sanity check on kmod_concurrent access

Only decrement *iff* we're possitive. Warn if we've hit
a situation where the counter is already 0 after we're done
with a modprobe call, this would tell us we have an unaccounted
counter access -- this in theory should not be possible as
only one routine controls the counter, however preemption is
one case that could trigger this situation. Avoid that situation
by disabling preemptiong while we access the counter.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/kmod.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index ab38539f7e91..09cf35a2075a 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -113,16 +113,28 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) < max_modprobes)
-		return 0;
-	atomic_dec(&kmod_concurrent);
-	return -EBUSY;
+		goto out;
+
+	atomic_dec_if_positive(&kmod_concurrent);
+	ret = -EBUSY;
+out:
+	preempt_enable();
+	return 0;
 }
 
 static void kmod_umh_threads_put(void)
 {
-	atomic_dec(&kmod_concurrent);
+	int ret;
+
+	preempt_disable();
+	ret = atomic_dec_if_positive(&kmod_concurrent);
+	WARN_ON(ret < 0);
+	preempt_enable();
 }
 
 /**
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1542005 — Re: [RFC 06/10] kmod: provide sanity check on kmod_concurrent access

On Thu 2016-12-08 11:48:50, Luis R. Rodriguez wrote:
> Only decrement *iff* we're possitive. Warn if we've hit
> a situation where the counter is already 0 after we're done
> with a modprobe call, this would tell us we have an unaccounted
> counter access -- this in theory should not be possible as
> only one routine controls the counter, however preemption is
> one case that could trigger this situation. Avoid that situation
> by disabling preemptiong while we access the counter.

I am curious about it. How could enabled preemption cause that
the counter will get negative?

Unaccounted access would be possible if put() is called
without get() or if put() is called before get().

I do not see a way how the value might get negative when
the calls are paired and ordered.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1542066 — Re: [RFC 06/10] kmod: provide sanity check on kmod_concurrent access

On Wed, Dec 14, 2016 at 05:08:58PM +0100, Petr Mladek wrote:
> On Thu 2016-12-08 11:48:50, Luis R. Rodriguez wrote:
> > Only decrement *iff* we're possitive. Warn if we've hit
> > a situation where the counter is already 0 after we're done
> > with a modprobe call, this would tell us we have an unaccounted
> > counter access -- this in theory should not be possible as
> > only one routine controls the counter, however preemption is
> > one case that could trigger this situation. Avoid that situation
> > by disabling preemptiong while we access the counter.
> 
> I am curious about it. How could enabled preemption cause that
> the counter will get negative?

As the commit log describes today in theory this is not possible
was we have only have one routine controlling the counter. If we
were to expand this then such possibilities become more real.

> Unaccounted access would be possible if put() is called
> without get() or if put() is called before get().

Exactly, so buggy users of the get/put calls in future calls.
I can just drop the preemption disable / enable for now as it
should not be an issue now.

> I do not see a way how the value might get negative when
> the calls are paired and ordered.

Right, this just matches parity with module_put(), its perhaps
*preemptively* too cautious though so I could just drop the
preemption enable/disable for now as that would slow down
things a bit.

  Luis

[toc] | [prev] | [next] | [standalone]

#1542716 — Re: [RFC 06/10] kmod: provide sanity check on kmod_concurrent access

On Thu 2016-12-08 11:48:50, Luis R. Rodriguez wrote:
> Only decrement *iff* we're possitive. Warn if we've hit
> a situation where the counter is already 0 after we're done
> with a modprobe call, this would tell us we have an unaccounted
> counter access -- this in theory should not be possible as
> only one routine controls the counter, however preemption is
> one case that could trigger this situation. Avoid that situation
> by disabling preemptiong while we access the counter.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  kernel/kmod.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index ab38539f7e91..09cf35a2075a 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -113,16 +113,28 @@ static int call_modprobe(char *module_name, int wait)
>  
>  static int kmod_umh_threads_get(void)
>  {
> +	int ret = 0;
> +
> +	preempt_disable();
>  	atomic_inc(&kmod_concurrent);
>  	if (atomic_read(&kmod_concurrent) < max_modprobes)
> -		return 0;
> -	atomic_dec(&kmod_concurrent);
> -	return -EBUSY;
> +		goto out;

I though more about it and the disabled preemtion might make
sense here. It makes sure that we are not rescheduled here
and that kmod_concurrent is not increased by mistake for too long.

Well, it still would make sense to increment the value
only when it is under the limit and set the incremented
value using cmpxchg to avoid races.

I mean to use similar trick that is used by refcount_inc(), see
https://lkml.kernel.org/r/20161114174446.832175072@infradead.org


> +	atomic_dec_if_positive(&kmod_concurrent);
> +	ret = -EBUSY;
> +out:
> +	preempt_enable();
> +	return 0;
>  }
>  
>  static void kmod_umh_threads_put(void)
>  {
> -	atomic_dec(&kmod_concurrent);
> +	int ret;
> +
> +	preempt_disable();
> +	ret = atomic_dec_if_positive(&kmod_concurrent);
> +	WARN_ON(ret < 0);
> +	preempt_enable();

The disabled preemption does not make much sense here.
We do not need to tie the atomic operation and the WARN
together so tightly.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1538839 — [RFC 10/10] kmod: add a sanity check on module loading

kmod has an optimization in place whereby if a some kernel code
uses request_module() on a module already loaded we never bother
userspace as the module already is loaded. This is not true for
get_fs_type() though as it uses aliases.

Additionally kmod <= v19 was broken -- it returns 0 to modprobe calls,
assuming the kernel module is built-in, where really we have a race as
the module starts forming. kmod <= v19 has incorrect userspace heuristics,
a userspace kmod fix is available for it:

http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4

This changes kmod to address both:

 o Provides the alias optimization for get_fs_type() so modules already
   loaded do not get re-requested.

 o Provides a sanity test to verify modprobe's work

This is important given how any get_fs_type() users assert success
means we're ready to go, and tests with the new test_kmod stress driver
reveal that request_module() and get_fs_type() might fail for a few
other reasons. You don't need old kmod to fail on request_module() or
get_fs_type(), with the right system setup, these calls *can* fail
today.

Although this does get us in the business of keeping alias maps in
kernel, the the work to support and maintain this is trivial.
Aditionally, since it may be important get_fs_type() should not fail on
certain systems, this tightens things up a bit more.

The TL;DR:

kmod <= v19 will return 0 on modprobe calls if you are built-in,
however its heuristics for checking if you are built-in were broken.

It assumed that having the directory /sys/module/module-name
but not having the file /sys/module/module-name/initstate
is sufficient to assume a module is built-in.

The kernel loads the inittstate attribute *after* it creates the
directory. This is an issue when modprobe returns 0 for kernel calls
which assumes a return of 0 on request_module() can give you the
right to assert the module is loaded and live.

We cannot trust returns of modprobe as 0 in the kernel, we need to
verify that modules are live if modprobe return 0 but only if modules
*are* modules. The kernel heuristic we use to determine if a module is
built-in is that if modprobe returns 0 we know we must be built-in or
a module, but if we are a module clearly we must have a lingering kmod
dangling on our linked list. If there is no modules there we are *somewhat*
certain the module must be built in.

This is not enough though... we cannot easily work around this since the
kernel can use aliases to userspace for modules calls. For instance
fs/namespace.c uses fs-modulename for filesystesms on get_fs_type(), so
these need to be taken into consideration as well.

Using kmod <= 19 will give you a NULL get_fs_type() return even though
the module was loaded... That is a corner case, there are other failures
for request_module() though -- the other failures are not easy to
reproduce though but fortunately we have a stress test driver to help
with that now. Use the following tests:

 # tools/testing/selftests/kmod/kmod.sh -t 0008
 # tools/testing/selftests/kmod/kmod.sh -t 0009

You can more easily see this error if you have kmod <= v19 installed.

You will need to install kmod <= v19, be sure to install its modprobe
into /sbin/ as by default the 'make install' target does not replace
your own.

This test helps cure test_kmod cases 0008 0009 so enable them.

Reported-by: Martin Wilck <martin.wilck@suse.com>
Reported-by: Randy Wright <rwright@hpe.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/kmod.c                        | 73 ++++++++++++++++++++++++++++++++++++
 kernel/module.c                      | 11 ++++--
 tools/testing/selftests/kmod/kmod.sh |  9 ++---
 3 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index a0f449f77ed7..6bf0feab41d1 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -61,6 +61,11 @@ static DECLARE_RWSEM(umhelper_sem);
 
 #ifdef CONFIG_MODULES
 
+bool finished_loading(const char *name);
+int module_wait_until_finished(const char *name);
+struct module *find_module_all(const char *name, size_t len,
+			       bool even_unformed);
+
 /*
 	modprobe_path is set via /proc/sys.
 */
@@ -158,6 +163,72 @@ int get_kmod_umh_count(void)
 	return atomic_read(&kmod_concurrent);
 }
 
+static bool kmod_exists(char *name)
+{
+	struct module *mod;
+
+	mutex_lock(&module_mutex);
+	mod = find_module_all(name, strlen(name), true);
+	mutex_unlock(&module_mutex);
+
+	if (mod)
+		return true;
+
+	return false;
+}
+
+/*
+ * The assumption is this must be a module, it could still not be live though
+ * since kmod <= 19 returns 0 even if it was not ready yet.  Allow for force
+ * wait check in case you are stuck on old userspace.
+ */
+static int wait_for_kmod(char *name)
+{
+	int ret = 0;
+
+	if (!finished_loading(name))
+		ret = module_wait_until_finished(name);
+
+	return ret;
+}
+
+/*
+ * kmod <= 19 will tell us modprobe returned 0 even if the module
+ * is not ready yet, it does this because it checks the /sys/module/mod-name
+ * directory and if its created but the /sys/module/mod-name/initstate is not
+ * created it assumes you have a built-in driver. At this point the module
+ * is still unformed, and telling the kernel at any point via request_module()
+ * will cause issues given a lot of places in the kernel assert that the driver
+ * will be present and ready. We need to account for this.
+ *
+ * If we had a module and even if buggy modprobe returned 0, we know we'd at
+ * least have a dangling kmod entry we could fetch.
+ *
+ * If modprobe returned 0 and we cannot find a kmod entry this is a good
+ * indicator your by userspace and kernel space that what you have is built-in.
+ *
+ * If modprobe returned 0 and we can find a kmod entry we should air on the
+ * side of caution and wait for the module to become ready or going.
+ *
+ * In the worst case, for built-in, we have to check on the module list for
+ * as many aliases possible the kernel gives the module, if that is n, that
+ * n traversals on the module list.
+ */
+static int finished_kmod_load(char *name)
+{
+	int ret = 0;
+	bool is_fs = (strlen(name) > 3) && (strncmp(name, "fs-", 3) == 0);
+
+	if (kmod_exists(name)) {
+		ret = wait_for_kmod(name);
+	} else {
+		if (is_fs && kmod_exists(name + 3))
+			ret = wait_for_kmod(name + 3);
+	}
+
+	return ret;
+}
+
 /**
  * __request_module - try to load a kernel module
  * @wait: wait (or not) for the operation to complete
@@ -211,6 +282,8 @@ int __request_module(bool wait, const char *fmt, ...)
 	trace_module_request(module_name, wait, _RET_IP_);
 
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
+	if (!ret)
+		ret = finished_kmod_load(module_name);
 
 	kmod_umh_threads_put();
 	return ret;
diff --git a/kernel/module.c b/kernel/module.c
index e420ed67e533..bf854321dca0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -590,8 +590,8 @@ EXPORT_SYMBOL_GPL(find_symbol);
  * Search for module by name: must hold module_mutex (or preempt disabled
  * for read-only access).
  */
-static struct module *find_module_all(const char *name, size_t len,
-				      bool even_unformed)
+struct module *find_module_all(const char *name, size_t len,
+			       bool even_unformed)
 {
 	struct module *mod;
 
@@ -3325,7 +3325,7 @@ static int post_relocation(struct module *mod, const struct load_info *info)
 }
 
 /* Is this module of this name done loading?  No locks held. */
-static bool finished_loading(const char *name)
+bool finished_loading(const char *name)
 {
 	struct module *mod;
 	bool ret;
@@ -3486,6 +3486,11 @@ static int may_init_module(void)
 	return 0;
 }
 
+int module_wait_until_finished(const char *name)
+{
+	return wait_event_interruptible(module_wq, finished_loading(name));
+}
+
 /*
  * We try to place it in the list now to make sure it's unique before
  * we dedicate too many resources.  In particular, temporary percpu
diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index 9ea1864d8bae..ccf35b8d1671 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -382,7 +382,7 @@ kmod_test_0008()
 	let EXTRA=$MODPROBE_LIMIT/2
 	config_num_thread_limit_extra $EXTRA
 	config_trigger ${FUNCNAME[0]}
-	config_expect_result ${FUNCNAME[0]} -EINVAL
+	config_expect_result ${FUNCNAME[0]} SUCCESS
 }
 
 kmod_test_0009()
@@ -392,7 +392,7 @@ kmod_test_0009()
 	#let EXTRA=$MODPROBE_LIMIT/3
 	config_num_thread_limit_extra 5
 	config_trigger ${FUNCNAME[0]}
-	config_expect_result ${FUNCNAME[0]} -EINVAL
+	config_expect_result ${FUNCNAME[0]} SUCCESS
 }
 
 trap "test_finish" EXIT
@@ -442,8 +442,7 @@ kmod_test_0004
 kmod_test_0005
 kmod_test_0006
 kmod_test_0007
-
-#kmod_test_0008
-#kmod_test_0009
+kmod_test_0008
+kmod_test_0009
 
 exit 0
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1539638 — Re: [RFC 10/10] kmod: add a sanity check on module loading

On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
> 
> Although this does get us in the business of keeping alias maps in
> kernel, the the work to support and maintain this is trivial.

You've implemented a special treatment for request_module("fs-$X")in
finished_kmod_load(), but there are many more aliases defined (and
used) in the kernel. Do you plan to implement special code for "char-
major-$X", "crypto-$X", "binfmt-$X" etc. later? 

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1539655 — Re: [RFC 10/10] kmod: add a sanity check on module loading

On Fri, Dec 9, 2016 at 12:03 PM, Martin Wilck <mwilck@suse.com> wrote:
> On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
>>
>> Although this does get us in the business of keeping alias maps in
>> kernel, the the work to support and maintain this is trivial.
>
> You've implemented a special treatment for request_module("fs-$X")in
> finished_kmod_load(), but there are many more aliases defined (and
> used) in the kernel. Do you plan to implement special code for "char-
> major-$X", "crypto-$X", "binfmt-$X" etc. later?

Yeah, no, that is just complete garbage.

Those module aliases already exist in the module info section. We just
don't parse the alias tags in the kernel.

So the real fix is to make find_module_all() just do that.

Doing random ad-hoc "let's prefix with 'fs-xyz'" games are completely
unacceptable. That's just pure shit. Stop this idiocy.

                Linus

[toc] | [prev] | [next] | [standalone]

#1542957 — Re: [RFC 10/10] kmod: add a sanity check on module loading

On Fri, Dec 09, 2016 at 12:56:21PM -0800, Linus Torvalds wrote:
> On Fri, Dec 9, 2016 at 12:03 PM, Martin Wilck <mwilck@suse.com> wrote:
> > On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
> >>
> >> Although this does get us in the business of keeping alias maps in
> >> kernel, the the work to support and maintain this is trivial.
> >
> > You've implemented a special treatment for request_module("fs-$X")in
> > finished_kmod_load(), but there are many more aliases defined (and
> > used) in the kernel. Do you plan to implement special code for "char-
> > major-$X", "crypto-$X", "binfmt-$X" etc. later?
> 
> Yeah, no, that is just complete garbage.
> 
> Those module aliases already exist in the module info section. We just
> don't parse the alias tags in the kernel.
> 
> So the real fix is to make find_module_all() just do that.

Ah yes, that is much sexier, this is now done and it works nicely, thanks
for the suggestion.

> Doing random ad-hoc "let's prefix with 'fs-xyz'" games are completely
> unacceptable. That's just pure shit. Stop this idiocy.

Look at that fin DNA in action :)

  Luis

[toc] | [prev] | [next] | [standalone]

#1542426 — Re: [RFC 10/10] kmod: add a sanity check on module loading

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> kmod has an optimization in place whereby if a some kernel code
> uses request_module() on a module already loaded we never bother
> userspace as the module already is loaded. This is not true for
> get_fs_type() though as it uses aliases.

Well, the obvious thing to do here is block kmod if we're currently
loading the same module.  Otherwise it has to do some weird spinning
thing in userspace anyway.

We already have module_wq for this, we just need a bit more code to
share the return value; and there's a weird corner case there where we
have "modprobe foo param=invalid" then "modprobe foo param=valid" and we
fail both with -EINVAL, but it's probably not worth fixing.

Cheers,
Rusty.

[toc] | [prev] | [next] | [standalone]

#1543245 — Re: [RFC 10/10] kmod: add a sanity check on module loading

On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> > kmod has an optimization in place whereby if a some kernel code
> > uses request_module() on a module already loaded we never bother
> > userspace as the module already is loaded. This is not true for
> > get_fs_type() though as it uses aliases.
> 
> Well, the obvious thing to do here is block kmod if we're currently
> loading the same module.

OK thanks, I've now added this, it sure helps. Test cases 0008 and 0009 require
hammering on the test over and over to see a failure on vanilla kernels,
an upper bound I found was about 150 times each test. Running test 0008
150 times with this enhancement you mentioned shaves off ~4 seconds.
For test 0009 it shaves off ~16 seconds, but as I note below the alias support
was needed as well.

> Otherwise it has to do some weird spinning
> thing in userspace anyway.

Right, but note that the get_fs_type() tests would still fail given
module.c was not alias-aware yet. I have the patches to add support
for the aliases now though and this is part of what helped shave
off time from the tests.

> We already have module_wq for this, we just need a bit more code to
> share the return value; and there's a weird corner case there where we
> have "modprobe foo param=invalid" then "modprobe foo param=valid" and we
> fail both with -EINVAL, but it's probably not worth fixing.

Hm OK. Although the set of patches I have fix and optimize now some
of these corner cases one issue that I still didn't quite yet figure
out was that a failure propagates secondary failures. That is,
say a module fails and you have loaded 4 request for the same module,
if the first request failed the last 3 *could* also fail. You can
trigger and see this with the latest script:

http://drvbp1.linux-foundation.org/~mcgrof/2016/12/16/kmod.sh

The latest version of the test_kmod driver:

http://drvbp1.linux-foundation.org/~mcgrof/2016/12/16/test_kmod.patch

./kmod.sh -t 0008
./kmod.sh -t 0009

When either of these fail you'll on dmesg that either a few NULL or
errors were found. It may not be worth fixing this race... given
that after apply all of my patches I no longer see this at all,
but I'm pretty sure a test case can be created to replicate more
easily.

FWIW a few things did occur to me:

a) list_add_rcu() is used so new modules get added first
b) find_module_all() returns the last module which was added as it traverses
   the module list

Because of a) and b) if two modules for the same driver can be on
the list at the same time then we'll get very likely a module which
is unformed or going than a live module. Changing module addition
to use list_add_tail_rcu() should mean we typically get the first
module added to the list for the module name I think, but other
than that I could not think clearly of the root case to allowing
multiple errors.

BTW should find_module_all() use rcu to traverse?

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -594,7 +594,7 @@ static struct module *find_module_all(const char *name, size_t len,
 
 	module_assert_mutex_or_preempt();
 
-	list_for_each_entry(mod, &modules, list) {
+	list_for_each_entry_rcu(mod, &modules, list) {
 		if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
 			continue;
 		if (strlen(mod->name) == len && !memcmp(mod->name, name, len))
@@ -3532,7 +3532,7 @@ static int add_unformed_module(struct module *mod)
 		goto out;
 	}
 	mod_update_bounds(mod);
-	list_add_rcu(&mod->list, &modules);
+	list_add_tail_rcu(&mod->list, &modules);
 	mod_tree_insert(mod);
 	err = 0;
 

[toc] | [prev] | [next] | [standalone]

#1543918 — Re: [RFC 10/10] kmod: add a sanity check on module loading

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
>> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>> > kmod has an optimization in place whereby if a some kernel code
>> > uses request_module() on a module already loaded we never bother
>> > userspace as the module already is loaded. This is not true for
>> > get_fs_type() though as it uses aliases.
>> 
>> Well, the obvious thing to do here is block kmod if we're currently
>> loading the same module.
>
> OK thanks, I've now added this, it sure helps. Test cases 0008 and 0009 require
> hammering on the test over and over to see a failure on vanilla kernels,
> an upper bound I found was about 150 times each test. Running test 0008
> 150 times with this enhancement you mentioned shaves off ~4 seconds.
> For test 0009 it shaves off ~16 seconds, but as I note below the alias support
> was needed as well.
>
>> Otherwise it has to do some weird spinning
>> thing in userspace anyway.
>
> Right, but note that the get_fs_type() tests would still fail given
> module.c was not alias-aware yet.

AFAICT the mistake here is that kmod is returning "done, OK" when the
module it is trying to load is already loading (but not finished
loading).  That's the root problem; it's an attempt at optimization by
kmod which goes awry.

Looking at the code in the kernel, we *already* get this right: block if
a module is still loading anyway.  Once it succeeds we return -EBUSY; if
it fails we'll proceed to try to load it again.

I don't understand what you're trying to fix with adding aliases
in-kernel?

> FWIW a few things did occur to me:
>
> a) list_add_rcu() is used so new modules get added first

Only after we're sure that there are no duplicates.

> b) find_module_all() returns the last module which was added as it traverses
>    the module list

> BTW should find_module_all() use rcu to traverse?

Yes; the kallsyms code does this on Oops.  Not really a big issue in
practice, but a nice fix.

Thanks,
Rusty.

>
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -594,7 +594,7 @@ static struct module *find_module_all(const char *name, size_t len,
>  
>  	module_assert_mutex_or_preempt();
>  
> -	list_for_each_entry(mod, &modules, list) {
> +	list_for_each_entry_rcu(mod, &modules, list) {
>  		if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
>  			continue;
>  		if (strlen(mod->name) == len && !memcmp(mod->name, name, len))
> @@ -3532,7 +3532,7 @@ static int add_unformed_module(struct module *mod)
>  		goto out;
>  	}
>  	mod_update_bounds(mod);
> -	list_add_rcu(&mod->list, &modules);
> +	list_add_tail_rcu(&mod->list, &modules);
>  	mod_tree_insert(mod);
>  	err = 0;
>  

[toc] | [prev] | [next] | [standalone]

#1544881 — Re: [RFC 10/10] kmod: add a sanity check on module loading

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> On Dec 16, 2016 9:54 PM, "Rusty Russell" <rusty@rustcorp.com.au> wrote:
> > AFAICT the mistake here is that kmod is returning "done, OK" when the
> > module it is trying to load is already loading (but not finished
> > loading).  That's the root problem; it's an attempt at optimization by
> > kmod which goes awry.
>
> This is true! To be precise though the truth of the matter is that kmod'd
> respective usermode helper: modprobe can be buggy and may lie to us. It may
> allow request_module() to return 0 but since we don't validate it, any
> assumption we make can be deadly. In the case of get_fs_type() its a null
> dereference.

Wait, what??  I can't see that in get_fs_type, which hasn't changed
since 2013.  If a caller is assuming get_fs_type() doesn't return NULL,
they're broken and need fixing of course:

        struct file_system_type *get_fs_type(const char *name)
        {
        	struct file_system_type *fs;
        	const char *dot = strchr(name, '.');
        	int len = dot ? dot - name : strlen(name);

        	fs = __get_fs_type(name, len);
        	if (!fs && (request_module("fs-%.*s", len, name) == 0))
        		fs = __get_fs_type(name, len);

        	if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
        		put_filesystem(fs);
        		fs = NULL;
        	}
        	return fs;
        }

Where does this NULL-deref is the module isn't correctly loaded?

> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
> need to verify after 0 was returned that it was not lying to us. Since kmod
> accepts aliases but find_modules_all() only works on the real module name a
> validation check cannot happen when all you have are aliases.

request_module() should block until resolution, but that's fundamentally
a userspace problem.  Let's not paper over it in kernelspace.

> *Iff* we are sure we don't want a validation (or another earlier
> optimization to avoid calling out to modrobe if the alias requested is
> already present, which does the time shaving I mentioned on the tests) then
> naturally no request_module() calls returning 0 can assert information
> about the requested module. I think we might need to change more code if we
> accept we cannot trust request_module() calls, or we accept userspace
> telling the kernel something may mean we sometimes crash. This later
> predicament seems rather odd so hence the patch.
>
> Perhaps in some cases validation of work from a umh is not critical in
> kernel but for request_module() I can tell you that today get_fs_type code
> currently asserts the module found can never be NULL.

OK, what am I missing in the code above?  

> > Looking at the code in the kernel, we *already* get this right: block if
> > a module is still loading anyway.  Once it succeeds we return -EBUSY if
> >
> > it fails we'll proceed to try to load it again.
> >
> > I don't understand what you're trying to fix with adding aliases
> > in-kernel?
>
> Two fold now:
>
> a) validation on request_module() work when an alias is used

But why?

> b) since kmod accepts aliaes, if we get aliases support, it means we could
> *also* preemptively avoid calling out to userspace for modules already
> present.

No, because once we have a module we don't request it: requesting is the
fallback case.

> >> FWIW a few things did occur to me:
> >>
> >> a) list_add_rcu() is used so new modules get added first
> >
> > Only after we're sure that there are no duplicates.
> >
> >
> OK! This is a very critical assertion. I should be able to add a debug
> WARN_ON() should two modules be on the modules list for the same module
> then ?

Yes, names must be unique.

>> b) find_module_all() returns the last module which was added as it
> traverses
>>    the module list
>
>> BTW should find_module_all() use rcu to traverse?
>
> Yes; the kallsyms code does this on Oops.  Not really a big issue in
> practice, but a nice fix.
>
> Ok, will bundle into my queue.

Please submit to Jessica for her module queue, as it's orthogonal
AFAICT.

> I will note though that I still think there's a bug in this code --
> upon a failure other "spinning" requests can fail, I believe this may
> be due to not having another state or informing pending modules too
> early of a failure but I haven't been able to prove this conjecture
> yet.

That's possible, but I can't see it from quickly re-checking the code.

The module should be fully usable at this point; the module's init has
been called successfully, so in the case of __get_fs_type() it should
now succeed.  The module cleans up its init section, but that should be
independent.

If there is a race, it's likely to be when some other caller wakes the
queue.  Moving the wakeup as soon as possible should make it easier to
trigger:

diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..78bd89d41a22 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3397,6 +3397,7 @@ static noinline int do_init_module(struct module *mod)
 
 	/* Now it's a first class citizen! */
 	mod->state = MODULE_STATE_LIVE;
+	wake_up_all(&module_wq);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_LIVE, mod);
 
@@ -3445,7 +3446,6 @@ static noinline int do_init_module(struct module *mod)
 	 */
 	call_rcu_sched(&freeinit->rcu, do_free_init);
 	mutex_unlock(&module_mutex);
-	wake_up_all(&module_wq);
 
 	return 0;
 

Thanks,
Rusty.

[toc] | [prev] | [next] | [standalone]

#1545366 — Re: [RFC 10/10] kmod: add a sanity check on module loading

On Mon, Dec 19, 2016 at 6:53 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Where does this NULL-deref is the module isn't correctly loaded?

No you are right, sorry -- I had confused a failure to mount over null
deref, my mistake.

>> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
>> need to verify after 0 was returned that it was not lying to us. Since kmod
>> accepts aliases but find_modules_all() only works on the real module name a
>> validation check cannot happen when all you have are aliases.
>
> request_module() should block until resolution, but that's fundamentally
> a userspace problem.  Let's not paper over it in kernelspace.

OK -- if userspace messes up again it may be a bit hard to prove
unless we have a validation debug thing in place, would such a thing
in debug form be reasonable ?

>> Yes; the kallsyms code does this on Oops.  Not really a big issue in
>> practice, but a nice fix.
>>
>> Ok, will bundle into my queue.
>
> Please submit to Jessica for her module queue, as it's orthogonal
> AFAICT.

Will do.

>> I will note though that I still think there's a bug in this code --
>> upon a failure other "spinning" requests can fail, I believe this may
>> be due to not having another state or informing pending modules too
>> early of a failure but I haven't been able to prove this conjecture
>> yet.
>
> That's possible, but I can't see it from quickly re-checking the code.
>
> The module should be fully usable at this point; the module's init has
> been called successfully, so in the case of __get_fs_type() it should
> now succeed.  The module cleans up its init section, but that should be
> independent.
>
> If there is a race, it's likely to be when some other caller wakes the
> queue.  Moving the wakeup as soon as possible should make it easier to
> trigger:
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63186e6..78bd89d41a22 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3397,6 +3397,7 @@ static noinline int do_init_module(struct module *mod)
>
>         /* Now it's a first class citizen! */
>         mod->state = MODULE_STATE_LIVE;
> +       wake_up_all(&module_wq);
>         blocking_notifier_call_chain(&module_notify_list,
>                                      MODULE_STATE_LIVE, mod);
>
> @@ -3445,7 +3446,6 @@ static noinline int do_init_module(struct module *mod)
>          */
>         call_rcu_sched(&freeinit->rcu, do_free_init);
>         mutex_unlock(&module_mutex);
> -       wake_up_all(&module_wq);
>
>         return 0;
>

Will give this a shot, thanks!

  Luis

[toc] | [prev] | [next] | [standalone]

#1545586 — Re: [RFC 10/10] kmod: add a sanity check on module loading

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:
> OK -- if userspace messes up again it may be a bit hard to prove
> unless we have a validation debug thing in place, would such a thing
> in debug form be reasonable ?

That makes perfect sense.  Untested hack:

diff --git a/fs/filesystems.c b/fs/filesystems.c
index c5618db110be..e5c90e80c7d3 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
 	int len = dot ? dot - name : strlen(name);
 
 	fs = __get_fs_type(name, len);
-	if (!fs && (request_module("fs-%.*s", len, name) == 0))
+	if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
 		fs = __get_fs_type(name, len);
-
+		WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name);
+	}
 	if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
 		put_filesystem(fs);
 		fs = NULL;

Maybe a similar hack for try_then_request_module(), but many places seem
to open-code request_module() so it's not as trivial...

Cheers,
Rusty.

[toc] | [prev] | [next] | [standalone]

#1545751 — Re: [RFC 10/10] kmod: add a sanity check on module loading

On Tue, Dec 20, 2016 at 8:21 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>> OK -- if userspace messes up again it may be a bit hard to prove
>> unless we have a validation debug thing in place, would such a thing
>> in debug form be reasonable ?
>
> That makes perfect sense.  Untested hack:
>
> diff --git a/fs/filesystems.c b/fs/filesystems.c
> index c5618db110be..e5c90e80c7d3 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
>         int len = dot ? dot - name : strlen(name);
>
>         fs = __get_fs_type(name, len);
> -       if (!fs && (request_module("fs-%.*s", len, name) == 0))
> +       if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
>                 fs = __get_fs_type(name, len);
> -
> +               WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name);
> +       }
>         if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
>                 put_filesystem(fs);
>                 fs = NULL;

This is precisely a type of debug patch we had added first to verify "WTF".

> Maybe a similar hack for try_then_request_module(), but many places seem
> to open-code request_module() so it's not as trivial...

Right, out of ~350 request_module() calls (not included try requests)
only ~46 check the return value. Hence a validation check, and come to
think of it, *this* was the issue that originally had me believing
that in some places we might end up in a null deref --if those open
coded request_module() calls assume the driver is loaded there could
be many places where a NULL is inevitable. Granted, I agree they
should be fixed, we could add a grammar rule to start nagging at
driver developers for started, but it does beg the question also of
what a tightly knit validation for modprobe might look like, and hence
this patch and now the completed not-yet-posted alias work.

Would it be worthy as a kconfig kmod debugging aide for now? I can
follow up with a semantic patch to nag about checking the return value
of request_module(), and we can  have 0-day then also complain about
new invalid uses.

  Luis

[toc] | [prev] | [next] | [standalone]

#1549497 — Re: [RFC 10/10] kmod: add a sanity check on module loading

"Luis R. Rodriguez" <mcgrof@kernel.org> writes:
>> Maybe a similar hack for try_then_request_module(), but many places seem
>> to open-code request_module() so it's not as trivial...

Hi Luis, Jessica (who is the main module maintainer now),

        Back from break, sorry about delay.

> Right, out of ~350 request_module() calls (not included try requests)
> only ~46 check the return value. Hence a validation check, and come to
> think of it, *this* was the issue that originally had me believing
> that in some places we might end up in a null deref --if those open
> coded request_module() calls assume the driver is loaded there could
> be many places where a NULL is inevitable.

Yes, assuming success == module loade is simply a bug.  I wrote
try_then_request_module() to attempt to encapsulate the correct logic
into a single place; maybe we need other helpers to cover (most of?) the
remaining cases?

> Granted, I agree they
> should be fixed, we could add a grammar rule to start nagging at
> driver developers for started, but it does beg the question also of
> what a tightly knit validation for modprobe might look like, and hence
> this patch and now the completed not-yet-posted alias work.

I really think aliases-in-kernel is too heavy a hammer, but a warning
when modprobe "succeeds" and the module still isn't found would be
a Good Thing.

> Would it be worthy as a kconfig kmod debugging aide for now? I can
> follow up with a semantic patch to nag about checking the return value
> of request_module(), and we can  have 0-day then also complain about
> new invalid uses.

Yeah, a warning about this would be win for sure.

BTW, I wrote the original "check-for-module-before-loading" in
module-init-tools, but I'm starting to wonder if it was a premature
optimization.  Have you thought about simply removing it and always
trying to load the module?  If it doesn't slow things down, perhaps
simplicity FTW?

Thanks,
Rusty.

[toc] | [prev] | [next] | [standalone]

#1538840 — [RFC 03/10] kmod: add dynamic max concurrent thread count

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 <mcgrof@kernel.org>
---
 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 <trace/events/module.h>
 
 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)
+{
+	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

[toc] | [prev] | [next] | [standalone]

#1538860 — Re: [RFC 03/10] kmod: add dynamic max concurrent thread count

On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@kernel.org> 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 <mcgrof@kernel.org>
> ---
>  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 <trace/events/module.h>
>
>  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

[toc] | [prev] | [next] | [standalone]

#1538884 — Re: [RFC 03/10] kmod: add dynamic max concurrent thread count

On Thu, Dec 08, 2016 at 12:28:07PM -0800, Kees Cook wrote:
> On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > 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 @@
> > @@ -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?

umh is user mode helper. kmod.c actually implements the kernel's umh code.
A subsequent series I will want to move all that to umh.c and keep module
loading separate in kmod.c But that's for later as a cleanup.

BTW any chance I can have you trim replies to file name and hunk for changes
you reply to ? As an example I did that here :)

  Luis

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web