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
    [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
    [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
    [RFC 06/10] kmod: provide sanity check on kmod_concurrent access "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +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
    [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
    [RFC 09/10] kmod: add helpers for getting kmod count and limit "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-08 20:50 +0100

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

Upon running into an old kmod v19 issue with mount (get_fs_type()) a few of us
hunted for the cause of the issue. Although the issue ended up being a
userspace issue, a stress test driver was written to help reproduce the issue,
and along the way a few other fixes and sanity checks were implemented.

I've taken the time to generalize the stress test driver as a kselftest driver
with a 9 test cases. The last two test cases reveal an existing issue which
is not yet addressed upstream, even if you have kmod v19 present. A fix is
proposed in the last patch. Orignally we had discarded this patch as too
complex due to the alias handling, but upon further analysis of test cases
and memory pressure issues, it seems worth considering. Other than the
last patch I don't think much of the other patches are controversial, but
sending as RFC first just in case.

If its not clear, an end goal here is to make module loading a bit more
deterministic with stronger sanity checks and stress tests. Please note,
the stress test diver requires 4 GiB of RAM to run all tests without running
out of memory. A lot of this has to do with the memory requirements needed
for a dynamic test for multiple threads, but note that the final memory
pressure and OOMs actually don't come from this allocation, but instead
from many finit_module() calls, this consumes quite a bit of memory, specially
if you have a lot of dependencies which also need to be loaded prior to
your needed module -- as is the case for filesystem drivers.

These patches are available on my linux-next git-tree on my branch
20161208-kmod-test-driver-try2 [0], which is based on linux-next tag
next-20161208. Patches are also available based on v4.9-rc8 [1] for
those looking for a bit more stable tree given x86_64 on linux-next is
hosed at the moment.

Since kmod.c doesn't seem to get much love, and since I've been digging
quite a bit into it for other users (firmware) I suppose I could volunteer
myself to maintain this code as well, unless there are oppositions to this.

[0] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20161208-kmod-test-driver-try2
[1] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux.git/log/?h=20161208-kmod-test-driver

Luis R. Rodriguez (10):
  kmod: add test driver to stress test the module loader
  module: fix memory leak on early load_module() failures
  kmod: add dynamic max concurrent thread count
  kmod: provide wrappers for kmod_concurrent inc/dec
  kmod: return -EBUSY if modprobe limit is reached
  kmod: provide sanity check on kmod_concurrent access
  kmod: use simplified rate limit printk
  sysctl: add support for unsigned int properly
  kmod: add helpers for getting kmod count and limit
  kmod: add a sanity check on module loading

 Documentation/admin-guide/kernel-parameters.txt |    7 +
 include/linux/kmod.h                            |    9 +
 include/linux/sysctl.h                          |    3 +
 init/Kconfig                                    |   23 +
 init/main.c                                     |    1 +
 kernel/kmod.c                                   |  244 ++++-
 kernel/module.c                                 |   12 +-
 kernel/sysctl.c                                 |  198 +++-
 lib/Kconfig.debug                               |   25 +
 lib/Makefile                                    |    1 +
 lib/test_kmod.c                                 | 1248 +++++++++++++++++++++++
 tools/testing/selftests/kmod/Makefile           |   11 +
 tools/testing/selftests/kmod/config             |    7 +
 tools/testing/selftests/kmod/kmod.sh            |  448 ++++++++
 14 files changed, 2199 insertions(+), 38 deletions(-)
 create mode 100644 lib/test_kmod.c
 create mode 100644 tools/testing/selftests/kmod/Makefile
 create mode 100644 tools/testing/selftests/kmod/config
 create mode 100755 tools/testing/selftests/kmod/kmod.sh

-- 
2.10.1

[toc] | [next] | [standalone]

#1538832 — [RFC 02/10] module: fix memory leak on early load_module() failures

While looking for early possible module loading failures I was
able to reproduce a memory leak possible with kmemleak. There
are a few rare ways to trigger a failure:

  o we've run into a failure while processing kernel parameters
    (parse_args() returns an error)
  o mod_sysfs_setup() fails
  o we're a live patch module and copy_module_elf() fails

Chances of running into this issue is really low.

kmemleak splat:

unreferenced object 0xffff9f2c4ada1b00 (size 32):
  comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s)
  hex dump (first 32 bytes):
    6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
    [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
    [<ffffffff8c1bc581>] kstrdup+0x31/0x60
    [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
    [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
    [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
    [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
    [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
    [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
    [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
    [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
    [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
    [<ffffffffffffffff>] 0xffffffffffffffff

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/module.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/module.c b/kernel/module.c
index f7482db0f843..e420ed67e533 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3722,6 +3722,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	mod_sysfs_teardown(mod);
  coming_cleanup:
 	mod->state = MODULE_STATE_GOING;
+	destroy_params(mod->kp, mod->num_kp);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);
-- 
2.10.1

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

#1538863 — Re: [RFC 02/10] module: fix memory leak on early load_module() failures

On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> While looking for early possible module loading failures I was
> able to reproduce a memory leak possible with kmemleak. There
> are a few rare ways to trigger a failure:
>
>   o we've run into a failure while processing kernel parameters
>     (parse_args() returns an error)
>   o mod_sysfs_setup() fails
>   o we're a live patch module and copy_module_elf() fails
>
> Chances of running into this issue is really low.
>
> kmemleak splat:
>
> unreferenced object 0xffff9f2c4ada1b00 (size 32):
>   comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s)
>   hex dump (first 32 bytes):
>     6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
>     [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
>     [<ffffffff8c1bc581>] kstrdup+0x31/0x60
>     [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
>     [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
>     [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
>     [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
>     [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
>     [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
>     [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
>     [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
>     [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
>     [<ffffffffffffffff>] 0xffffffffffffffff
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Acked-by: Kees Cook <keescook@chromium.org>

Is this worth sending through -stable too?

-Kees

> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f7482db0f843..e420ed67e533 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3722,6 +3722,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>         mod_sysfs_teardown(mod);
>   coming_cleanup:
>         mod->state = MODULE_STATE_GOING;
> +       destroy_params(mod->kp, mod->num_kp);
>         blocking_notifier_call_chain(&module_notify_list,
>                                      MODULE_STATE_GOING, mod);
>         klp_module_going(mod);
> --
> 2.10.1
>



-- 
Kees Cook
Nexus Security

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

#1538888 — Re: [RFC 02/10] module: fix memory leak on early load_module() failures

On Thu, Dec 8, 2016 at 2:30 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> While looking for early possible module loading failures I was
>> able to reproduce a memory leak possible with kmemleak. There
>> are a few rare ways to trigger a failure:
>>
>>   o we've run into a failure while processing kernel parameters
>>     (parse_args() returns an error)
>>   o mod_sysfs_setup() fails
>>   o we're a live patch module and copy_module_elf() fails
>>
>> Chances of running into this issue is really low.
>>
>> kmemleak splat:
>>
>> unreferenced object 0xffff9f2c4ada1b00 (size 32):
>>   comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s)
>>   hex dump (first 32 bytes):
>>     6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>   backtrace:
>>     [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
>>     [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
>>     [<ffffffff8c1bc581>] kstrdup+0x31/0x60
>>     [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
>>     [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
>>     [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
>>     [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
>>     [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
>>     [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
>>     [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
>>     [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
>>     [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
>>     [<ffffffffffffffff>] 0xffffffffffffffff
>>
>> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>
> Acked-by: Kees Cook <keescook@chromium.org>
>
> Is this worth sending through -stable too?

Yes, for some reason git-send e-mail complained to me about
stable@kernel.org not being a valid local address, so I had to remove
it, but indeed. I'll try to fix this e-mail issue later and add your
tag.

 Luis

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

#1538889 — Re: [RFC 02/10] module: fix memory leak on early load_module() failures

On Thu, Dec 8, 2016 at 1:10 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Dec 8, 2016 at 2:30 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>> While looking for early possible module loading failures I was
>>> able to reproduce a memory leak possible with kmemleak. There
>>> are a few rare ways to trigger a failure:
>>>
>>>   o we've run into a failure while processing kernel parameters
>>>     (parse_args() returns an error)
>>>   o mod_sysfs_setup() fails
>>>   o we're a live patch module and copy_module_elf() fails
>>>
>>> Chances of running into this issue is really low.
>>>
>>> kmemleak splat:
>>>
>>> unreferenced object 0xffff9f2c4ada1b00 (size 32):
>>>   comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s)
>>>   hex dump (first 32 bytes):
>>>     6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
>>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>>   backtrace:
>>>     [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
>>>     [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
>>>     [<ffffffff8c1bc581>] kstrdup+0x31/0x60
>>>     [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
>>>     [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
>>>     [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
>>>     [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
>>>     [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
>>>     [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
>>>     [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
>>>     [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
>>>     [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
>>>     [<ffffffffffffffff>] 0xffffffffffffffff
>>>
>>> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>>
>> Acked-by: Kees Cook <keescook@chromium.org>
>>
>> Is this worth sending through -stable too?
>
> Yes, for some reason git-send e-mail complained to me about
> stable@kernel.org not being a valid local address, so I had to remove
> it, but indeed. I'll try to fix this e-mail issue later and add your
> tag.

Yup, you want stable@vger.kernel.org. :)

-Kees

-- 
Kees Cook
Nexus Security

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

#1539558 — Re: [RFC 02/10] module: fix memory leak on early load_module() failures

On Thu, 8 Dec 2016, Luis R. Rodriguez wrote:

> While looking for early possible module loading failures I was
> able to reproduce a memory leak possible with kmemleak. There
> are a few rare ways to trigger a failure:
> 
>   o we've run into a failure while processing kernel parameters
>     (parse_args() returns an error)
>   o mod_sysfs_setup() fails
>   o we're a live patch module and copy_module_elf() fails
> 
> Chances of running into this issue is really low.
> 
> kmemleak splat:
> 
> unreferenced object 0xffff9f2c4ada1b00 (size 32):
>   comm "kworker/u16:4", pid 82, jiffies 4294897636 (age 681.816s)
>   hex dump (first 32 bytes):
>     6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff8c6cfeba>] kmemleak_alloc+0x4a/0xa0
>     [<ffffffff8c200046>] __kmalloc_track_caller+0x126/0x230
>     [<ffffffff8c1bc581>] kstrdup+0x31/0x60
>     [<ffffffff8c1bc5d4>] kstrdup_const+0x24/0x30
>     [<ffffffff8c3c23aa>] kvasprintf_const+0x7a/0x90
>     [<ffffffff8c3b5481>] kobject_set_name_vargs+0x21/0x90
>     [<ffffffff8c4fbdd7>] dev_set_name+0x47/0x50
>     [<ffffffffc07819e5>] memstick_check+0x95/0x33c [memstick]
>     [<ffffffff8c09c893>] process_one_work+0x1f3/0x4b0
>     [<ffffffff8c09cb98>] worker_thread+0x48/0x4e0
>     [<ffffffff8c0a2b79>] kthread+0xc9/0xe0
>     [<ffffffff8c6dab5f>] ret_from_fork+0x1f/0x40
>     [<ffffffffffffffff>] 0xffffffffffffffff
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

What about

Fixes: e180a6b7759a ("param: fix charp parameters set via sysfs")

?

Miroslav

> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index f7482db0f843..e420ed67e533 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3722,6 +3722,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	mod_sysfs_teardown(mod);
>   coming_cleanup:
>  	mod->state = MODULE_STATE_GOING;
> +	destroy_params(mod->kp, mod->num_kp);
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
>  	klp_module_going(mod);
> -- 
> 2.10.1
> 

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

#1538833 — [RFC 08/10] sysctl: add support for unsigned int properly

Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32 fields")
added proc_douintvec() to start help adding support for unsigned int,
this however was only half the work needed, all these issues are present
with the current implementation:

  o Printing the values shows a negative value, this happens
    since do_proc_dointvec() and this uses proc_put_long()
  o We can easily wrap around the int values: UINT_MAX is
    4294967295, if we echo in 4294967295 + 1 we end up with 0,
    using 4294967295 + 2 we end up with 1.
 o We echo negative values in and they are accepted

Fix all these issues by adding our own do_proc_douintvec(). Likewise to
keep parity provide the other typically useful proc_douintvec_minmax().
Adding proc_douintvec_minmax_sysadmin() is easy but we wait for an actual
user for that.

Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 include/linux/sysctl.h |   3 +
 kernel/sysctl.c        | 184 +++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 181 insertions(+), 6 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index adf4e51cf597..a35d40ecc211 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -47,6 +47,9 @@ extern int proc_douintvec(struct ctl_table *, int,
 			 void __user *, size_t *, loff_t *);
 extern int proc_dointvec_minmax(struct ctl_table *, int,
 				void __user *, size_t *, loff_t *);
+extern int proc_douintvec_minmax(struct ctl_table *table, int write,
+				 void __user *buffer, size_t *lenp,
+				 loff_t *ppos);
 extern int proc_dointvec_jiffies(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
 extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 1a292ebcbbb6..06711e648fa3 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
 	return 0;
 }
 
-static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
-				 int *valp,
-				 int write, void *data)
+static int do_proc_douintvec_conv(unsigned long *lvalp,
+				  unsigned int *valp,
+				  int write, void *data)
 {
 	if (write) {
-		if (*negp)
+		if (*lvalp > (unsigned long) UINT_MAX)
 			return -EINVAL;
 		*valp = *lvalp;
 	} else {
@@ -2243,6 +2243,115 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
 			buffer, lenp, ppos, conv, data);
 }
 
+static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
+			       int write, void __user *buffer,
+			       size_t *lenp, loff_t *ppos,
+			       int (*conv)(unsigned long *lvalp,
+					   unsigned int *valp,
+					   int write, void *data),
+			       void *data)
+{
+	unsigned int *i, vleft;
+	bool first = true;
+	int err = 0;
+	size_t left;
+	char *kbuf = NULL, *p;
+
+	if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
+		*lenp = 0;
+		return 0;
+	}
+
+	i = (unsigned int *) tbl_data;
+	vleft = table->maxlen / sizeof(*i);
+	left = *lenp;
+
+	if (!conv)
+		conv = do_proc_douintvec_conv;
+
+	if (write) {
+		if (*ppos) {
+			switch (sysctl_writes_strict) {
+			case SYSCTL_WRITES_STRICT:
+				goto out;
+			case SYSCTL_WRITES_WARN:
+				warn_sysctl_write(table);
+				break;
+			default:
+				break;
+			}
+		}
+
+		if (left > PAGE_SIZE - 1)
+			left = PAGE_SIZE - 1;
+		p = kbuf = memdup_user_nul(buffer, left);
+		if (IS_ERR(kbuf))
+			return PTR_ERR(kbuf);
+	}
+
+	for (; left && vleft--; i++, first=false) {
+		unsigned long lval;
+		bool neg;
+
+		if (write) {
+			left -= proc_skip_spaces(&p);
+
+			if (!left)
+				break;
+			err = proc_get_long(&p, &left, &lval, &neg,
+					     proc_wspace_sep,
+					     sizeof(proc_wspace_sep), NULL);
+			if (neg) {
+				err = -EINVAL;
+				break;
+			}
+			if (err)
+				break;
+			if (conv(&lval, i, 1, data)) {
+				err = -EINVAL;
+				break;
+			}
+		} else {
+			if (conv(&lval, i, 0, data)) {
+				err = -EINVAL;
+				break;
+			}
+			if (!first)
+				err = proc_put_char(&buffer, &left, '\t');
+			if (err)
+				break;
+			err = proc_put_long(&buffer, &left, lval, false);
+			if (err)
+				break;
+		}
+	}
+
+	if (!write && !first && left && !err)
+		err = proc_put_char(&buffer, &left, '\n');
+	if (write && !err && left)
+		left -= proc_skip_spaces(&p);
+	if (write) {
+		kfree(kbuf);
+		if (first)
+			return err ? : -EINVAL;
+	}
+	*lenp -= left;
+out:
+	*ppos += *lenp;
+	return err;
+}
+
+static int do_proc_douintvec(struct ctl_table *table, int write,
+			     void __user *buffer, size_t *lenp, loff_t *ppos,
+			     int (*conv)(unsigned long *lvalp,
+					 unsigned int *valp,
+					 int write, void *data),
+			     void *data)
+{
+	return __do_proc_douintvec(table->data, table, write,
+				   buffer, lenp, ppos, conv, data);
+}
+
 /**
  * proc_dointvec - read a vector of integers
  * @table: the sysctl table
@@ -2278,8 +2387,8 @@ int proc_dointvec(struct ctl_table *table, int write,
 int proc_douintvec(struct ctl_table *table, int write,
 		     void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	return do_proc_dointvec(table, write, buffer, lenp, ppos,
-				do_proc_douintvec_conv, NULL);
+	return do_proc_douintvec(table, write, buffer, lenp, ppos,
+				 do_proc_douintvec_conv, NULL);
 }
 
 /*
@@ -2384,6 +2493,62 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 				do_proc_dointvec_minmax_conv, &param);
 }
 
+struct do_proc_douintvec_minmax_conv_param {
+	unsigned int *min;
+	unsigned int *max;
+};
+
+static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
+					 unsigned int *valp,
+					 int write, void *data)
+{
+	struct do_proc_douintvec_minmax_conv_param *param = data;
+	if (write) {
+		unsigned int val = *lvalp;
+		if ((param->min && *param->min > val) ||
+		    (param->max && *param->max < val))
+			return -ERANGE;
+
+		if (*lvalp > (unsigned long) UINT_MAX)
+			return -EINVAL;
+		*valp = val;
+	} else {
+		unsigned int val = *valp;
+		*lvalp = (unsigned long) val;
+	}
+	return 0;
+}
+
+/**
+ * proc_douintvec_minmax - read a vector of unsigned ints with min/max values
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * Reads/writes up to table->maxlen/sizeof(unsigned int) unsigned integer
+ * values from/to the user buffer, treated as an ASCII string. Negative
+ * strings are not allowed.
+ *
+ * This routine will ensure the values are within the range specified by
+ * table->extra1 (min) and table->extra2 (max). There is a final sanity
+ * check for UINT_MAX to avoid having to support wrap around uses from
+ * userspace.
+ *
+ * Returns 0 on success.
+ */
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+			  void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct do_proc_douintvec_minmax_conv_param param = {
+		.min = (unsigned int *) table->extra1,
+		.max = (unsigned int *) table->extra2,
+	};
+	return do_proc_douintvec(table, write, buffer, lenp, ppos,
+				 do_proc_douintvec_minmax_conv, &param);
+}
+
 static void validate_coredump_safety(void)
 {
 #ifdef CONFIG_COREDUMP
@@ -2891,6 +3056,12 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 	return -ENOSYS;
 }
 
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+			  void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	return -ENOSYS;
+}
+
 int proc_dointvec_jiffies(struct ctl_table *table, int write,
 		    void __user *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -2933,6 +3104,7 @@ EXPORT_SYMBOL(proc_dointvec);
 EXPORT_SYMBOL(proc_douintvec);
 EXPORT_SYMBOL(proc_dointvec_jiffies);
 EXPORT_SYMBOL(proc_dointvec_minmax);
+EXPORT_SYMBOL_GPL(proc_douintvec_minmax);
 EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
 EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
 EXPORT_SYMBOL(proc_dostring);
-- 
2.10.1

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

#1538834 — [RFC 05/10] kmod: return -EBUSY if modprobe limit is reached

Running out of our modprobe limit is not a memory limit but
a system specific established limitation set to avoid a possible
recursive issue with modprobe. This gives userspace a better idea
of what happened if we can't load a module, it could use this to
wait and try again.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/kmod.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 049d7eabda38..ab38539f7e91 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -117,7 +117,7 @@ static int kmod_umh_threads_get(void)
 	if (atomic_read(&kmod_concurrent) < max_modprobes)
 		return 0;
 	atomic_dec(&kmod_concurrent);
-	return -ENOMEM;
+	return -EBUSY;
 }
 
 static void kmod_umh_threads_put(void)
-- 
2.10.1

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

#1538835 — [RFC 07/10] kmod: use simplified rate limit printk

Just use the simplified rate limit printk when the max modprobe
limit is reached, while at it throw out a bone should the error
be triggered.

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

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 09cf35a2075a..ef65f4c3578a 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -158,7 +158,6 @@ int __request_module(bool wait, const char *fmt, ...)
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
 	int ret;
-	static int kmod_loop_msg;
 
 	/*
 	 * We don't allow synchronous module loading from async.  Module
@@ -183,13 +182,8 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	ret = kmod_umh_threads_get();
 	if (ret) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg < 5) {
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-			kmod_loop_msg++;
-		}
+		pr_err_ratelimited("request_module: modprobe limit (%u) reached with module %s\n",
+				   max_modprobes, module_name);
 		return ret;
 	}
 
-- 
2.10.1

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

#1538837 — [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

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)
+		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]

#1538862 — Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> 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)
> +               return 0;
> +       atomic_dec(&kmod_concurrent);
> +       return -ENOMEM;
> +}
> +
> +static void kmod_umh_threads_put(void)
> +{
> +       atomic_dec(&kmod_concurrent);
> +}

Can you use a kref here instead? We're trying to kill raw use of
atomic_t for reference counting...

> +
>  /**
>   * __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
>



-- 
Kees Cook
Nexus Security

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

#1538882 — Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

On Thu, Dec 08, 2016 at 12:29:42PM -0800, Kees Cook wrote:
> On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > 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)
> > +               return 0;
> > +       atomic_dec(&kmod_concurrent);
> > +       return -ENOMEM;
> > +}
> > +
> > +static void kmod_umh_threads_put(void)
> > +{
> > +       atomic_dec(&kmod_concurrent);
> > +}
> 
> Can you use a kref here instead? We're trying to kill raw use of
> atomic_t for reference counting...

That's a much broader functional change than I was looking for, but I am up for
it. Can you describe the benefit of using kref you expect or why this is an
ongoing crusade? Since its a larger functional change how about doing this
change later, and we can test impact with the tress test driver. In theory if
there are benefits can't we add a test case to prove the gains?

  Luis

[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]

#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]

#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]

#1538841 — [RFC 09/10] kmod: add helpers for getting kmod count and limit

This adds helpers for getting access to the kmod count and limit from
userspace. While at it, this also lets userspace fine tune the kmod
limit after boot, it uses the shiny new proc_douintvec_minmax().

These knobs should help userspace more gracefully and deterministically
handle module loading.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 include/linux/kmod.h |  8 +++++
 kernel/kmod.c        | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 kernel/sysctl.c      | 14 +++++++++
 3 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 15783cd7f056..94c7379cff94 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -39,13 +39,21 @@ int __request_module(bool wait, const char *name, ...);
 #define try_then_request_module(x, mod...) \
 	((x) ?: (__request_module(true, mod), (x)))
 void init_kmod_umh(void);
+unsigned int get_kmod_umh_limit(void);
+int sysctl_kmod_count(struct ctl_table *table, int write,
+		      void __user *buffer, size_t *lenp, loff_t *ppos);
+int sysctl_kmod_limit(struct ctl_table *table, int write,
+		      void __user *buffer, size_t *lenp, loff_t *ppos);
 #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) { }
+static unsigned int get_kmod_umh_limit(void) { return 0; }
 #define try_then_request_module(x, mod...) (x)
 #endif
 
+#define get_kmod_umh_limit get_kmod_umh_limit
+
 struct cred;
 struct file;
 
diff --git a/kernel/kmod.c b/kernel/kmod.c
index ef65f4c3578a..a0f449f77ed7 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -138,6 +138,27 @@ static void kmod_umh_threads_put(void)
 }
 
 /**
+ * get_kmod_umh_limit - get concurrent modprobe thread limit
+ *
+ * Returns the number of allowed concurrent modprobe calls.
+ */
+unsigned int get_kmod_umh_limit(void)
+{
+	return max_modprobes;
+}
+EXPORT_SYMBOL_GPL(get_kmod_umh_limit);
+
+/**
+ * get_kmod_umh_count - get number of concurrent modprobe calls running
+ *
+ * Returns the number of concurrent modprobe calls currently running.
+ */
+int get_kmod_umh_count(void)
+{
+	return atomic_read(&kmod_concurrent);
+}
+
+/**
  * __request_module - try to load a kernel module
  * @wait: wait (or not) for the operation to complete
  * @fmt: printf style format string for the name of the module
@@ -196,6 +217,11 @@ int __request_module(bool wait, const char *fmt, ...)
 }
 EXPORT_SYMBOL(__request_module);
 
+static void __set_max_modprobes(unsigned int suggested)
+{
+	max_modprobes = min((unsigned int) max_threads/2, suggested);
+}
+
 /*
  * 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
@@ -212,12 +238,65 @@ EXPORT_SYMBOL(__request_module);
  * 4096 concurrent modprobe instances:
  *
  *	kmod.max_modprobes=4096
+ *
+ * You can also set the limit via sysctl:
+ *
+ * echo 4096 > /proc/sys/kernel/kmod-limit
+ *
+ * You can also set the query the current thread count:
+ *
+ * cat /proc/sys/kernel/kmod-count
+ *
+ * These knobs should enable userspace to more gracefully and
+ * deterministically handle module loading.
  */
 void __init init_kmod_umh(void)
 {
 	if (!max_modprobes)
-		max_modprobes = min(max_threads/2,
-				    2 << CONFIG_MAX_KMOD_CONCURRENT);
+		__set_max_modprobes(2 << CONFIG_MAX_KMOD_CONCURRENT);
+}
+
+int sysctl_kmod_count(struct ctl_table *table, int write,
+		      void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table t;
+	int ret = 0;
+	int count = get_kmod_umh_count();
+
+	t = *table;
+	t.data = &count;
+
+	if (write)
+		return -EPERM;
+
+	ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
+
+	return ret;
+}
+
+int sysctl_kmod_limit(struct ctl_table *table, int write,
+		      void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table t;
+	int ret;
+	unsigned int local_max_modprobes = max_modprobes;
+	unsigned int min = 0;
+	unsigned int max = max_threads/2;
+
+	t = *table;
+	t.data = &local_max_modprobes;
+	t.extra1 = &min;
+	t.extra2 = &max;
+
+	ret = proc_douintvec_minmax(&t, write, buffer, lenp, ppos);
+	if (ret == -ERANGE)
+		pr_err("modprobe thread valid range: %u - %u\n", min, max);
+	if (ret || !write)
+		return ret;
+
+	__set_max_modprobes((unsigned int) local_max_modprobes);
+
+	return 0;
 }
 
 #endif /* CONFIG_MODULES */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 06711e648fa3..0ba56001e49b 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -660,6 +660,20 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &one,
 		.extra2		= &one,
 	},
+	{
+		.procname	= "kmod-count",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(int),
+		.mode		= 0444,
+		.proc_handler	= sysctl_kmod_count,
+	},
+	{
+		.procname	= "kmod-limit",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= sysctl_kmod_limit,
+	},
 #endif
 #ifdef CONFIG_UEVENT_HELPER
 	{
-- 
2.10.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web