Date:   Wed, 11 Jan 2017 19:27:56 +0100
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Tom Gundersen <teg@...m.no>
Cc:     Petr Mladek <pmladek@...e.com>, shuah@...nel.org, jeyu@...hat.com,
        rusty@...tcorp.com.au, ebiederm@...ssion.com,
        dmitry.torokhov@...il.com, acme@...hat.com, corbet@....net,
        martin.wilck@...e.com, mmarek@...e.com, hare@...e.com,
        rwright@....com, jeffm@...e.com, DSterba@...e.com,
        fdmanana@...e.com, neilb@...e.com, linux@...ck-us.net,
        rgoldwyn@...e.com, subashab@...eaurora.org, xypron.glpk@....de,
        keescook@...omium.org, atomlin@...hat.com, mbenes@...e.cz,
        paulmck@...ux.vnet.ibm.com, dan.j.williams@...el.com,
        jpoimboe@...hat.com, davem@...emloft.net, mingo@...hat.com,
        akpm@...ux-foundation.org, torvalds@...ux-foundation.org,
        linux-kselftest@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC 09/10] kmod: add helpers for getting kmod count and limit

On Fri, Dec 16, 2016 at 08:57:26AM +0100, Luis R. Rodriguez wrote:
> On Thu, Dec 15, 2016 at 05:56:19PM +0100, Petr Mladek wrote:
> > On Thu 2016-12-08 11:49:20, Luis R. Rodriguez wrote:
> > > 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@...nel.org>
> > > ---
> > >  include/linux/kmod.h |  8 +++++
> > >  kernel/kmod.c        | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  kernel/sysctl.c      | 14 +++++++++
> > >  3 files changed, 103 insertions(+), 2 deletions(-)
> > 
> > I am not sure if it is worth it. As you say in the 3rd patch,
> > there was rather low limit for 16 years and nobody probably had
> > problems with it.
> 
> Note, *probably* - ie, this could have gone unreported for a while, and
> to be frank how can we know for sure a pesky module just did not load due
> to this? In the case of get_fs_type() issue this can be fatal for a partition
> mount, not a good example to wait to look forward to before we take this
> serious.
> 
> I added the sysctl value mostly for read purposes, the count is probably
> useless for any accounting to be done in userspace due to delays this
> reading and making this value useful in userspace can have, I can nuke
> that. The kmod-limit however seems very useful so that userspace knows
> how to properly thread *safely* modprobe calls more deterministically.
> 
> Adding write support to let one bump the limit was just an easy convenience
> possible given the read support was being added, but its use should
> really only be useful for testing purposes post bootup given that the
> real value in the limit will be important at boot time prior to the sysctl
> parsing. The real know tweak which should be used in case of issues is
> the module parameter added earlier.
> 
> So I could drop the kmod-count, and just make the kmod-limit read-only.
> Thoughts?

OK I've done this and also since there was confusion about dependencies
possibly affecting kmod_concurrent I've added a note about this on the
Documentation/sysctl/kernel.txt documentation. This documentation also
clarifies the intent behind exposing this interface, which is to help
enable userspace make using modprobe more deterministic (its why I've
Cc'd Tom). The following changes have been made, and I'll fold this into this
patch and rename the title.

> > Anyway, it seems that such know should also get documented in
> > Documentation/sysctl/kernel.txt
> 
> Will do if we keep them, thanks.

Below are the changes I've made:

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index a32b4b748644..c82aeaf60ca7 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -370,6 +370,26 @@ with the "modules_disabled" sysctl.
 
 ==============================================================
 
+kmod-limit:
+
+Get the max amount of concurrent requests (kmod_concurrent) the kernel can
+make out to userspace to call 'modprobe'. This limit is known internally to the
+kernel as max_modprobes. This interface is designed to enable userspace to
+query the kernel for the max_modprobes limit so userspace can more
+deterministically handle module loading by only enabling max_modprobes
+'modprobe' calls at a time.
+
+Dependencies are resolved in userspace through depmod, so one modprobe
+call only bumps the number of concurrent threads (kmod_concurrent) by one.
+Dependencies for a module then are loaded directly in userspace using
+init_module() / finit_module() skipping bumping kmod_concurrent or being
+affected by max_modprobes.
+
+The max_modprobes value is set at build time with CONFIG_MAX_KMOD_CONCURRENT.
+You can override at initialization with the module parameter max_modprobes.
+
+==============================================================
+
 kptr_restrict:
 
 This toggle indicates whether restrictions are placed on
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index c30d797fe4d3..1ee833e5896d 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -40,8 +40,6 @@ int __request_module(bool wait, const char *name, ...);
 	((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
diff --git a/kernel/kmod.c b/kernel/kmod.c
index f2fd9f088278..0303bce326b8 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -158,16 +158,6 @@ unsigned int get_kmod_umh_limit(void)
 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
@@ -226,11 +216,6 @@ 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
@@ -247,40 +232,12 @@ static void __set_max_modprobes(unsigned int suggested)
  * 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)
-		__set_max_modprobes(1 << 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;
+		max_modprobes = min(max_threads/2,
+				    1 << CONFIG_MAX_KMOD_CONCURRENT);
 }
 
 int sysctl_kmod_limit(struct ctl_table *table, int write,
@@ -297,15 +254,12 @@ int sysctl_kmod_limit(struct ctl_table *table, int write,
 	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;
+	if (write)
+		return -EPERM;
 
-	__set_max_modprobes((unsigned int) local_max_modprobes);
+	ret = proc_douintvec_minmax(&t, write, buffer, lenp, ppos);
 
-	return 0;
+	return ret;
 }
 
 #endif /* CONFIG_MODULES */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index d59cca78417a..52cf84131f74 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -661,17 +661,10 @@ static struct ctl_table kern_table[] = {
 		.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,
+		.mode		= 0444,
 		.proc_handler	= sysctl_kmod_limit,
 	},
 #endif

Powered by blists - more mailing lists