* [PATCH 1/3] wait: add wait_event_killable_timeout()
2017-08-09 23:46 [PATCH 0/3] kmod: pending fixes for v4.13-final Luis R. Rodriguez
@ 2017-08-09 23:46 ` Luis R. Rodriguez
2017-08-10 10:33 ` Peter Zijlstra
2017-08-09 23:46 ` [PATCH 2/3] kmod: fix wait on recursive loop Luis R. Rodriguez
2017-08-09 23:46 ` [PATCH 3/3] test_kmod: fix description for -s -and -c parameters Luis R. Rodriguez
2 siblings, 1 reply; 5+ messages in thread
From: Luis R. Rodriguez @ 2017-08-09 23:46 UTC (permalink / raw)
To: akpm
Cc: mingo, peterz, keescook, dmitry.torokhov, jeyu, rusty, mmarek,
pmladek, mbenes, jpoimboe, ebiederm, shuah, matt.redfearn,
dan.carpenter, colin.king, danielmentz, dcb314, linux-kselftest,
linux-kernel, Luis R. Rodriguez
This wait is similar to wait_event_interruptable_timeout() but only accepts
SIGKILL interrupt signal. Other signals are ignored.
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
include/linux/wait.h | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 5b74e36c0ca8..dc19880c02f5 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -757,6 +757,43 @@ extern int do_wait_intr_irq(wait_queue_head_t *, wait_queue_entry_t *);
__ret; \
})
+#define __wait_event_killable_timeout(wq_head, condition, timeout) \
+ ___wait_event(wq_head, ___wait_cond_timeout(condition), \
+ TASK_KILLABLE, 0, timeout, \
+ __ret = schedule_timeout(__ret))
+
+/**
+ * wait_event_killable_timeout - sleep until a condition gets true or a timeout elapses
+ * @wq_head: the waitqueue to wait on
+ * @condition: a C expression for the event to wait for
+ * @timeout: timeout, in jiffies
+ *
+ * The process is put to sleep (TASK_KILLABLE) until the
+ * @condition evaluates to true or a kill signal is received.
+ * The @condition is checked each time the waitqueue @wq_head is woken up.
+ *
+ * wake_up() has to be called after changing any variable that could
+ * change the result of the wait condition.
+ *
+ * Returns:
+ * 0 if the @condition evaluated to %false after the @timeout elapsed,
+ * 1 if the @condition evaluated to %true after the @timeout elapsed,
+ * the remaining jiffies (at least 1) if the @condition evaluated
+ * to %true before the @timeout elapsed, or -%ERESTARTSYS if it was
+ * interrupted by a kill signal.
+ *
+ * Only kill signals interrupt this process.
+ */
+#define wait_event_killable_timeout(wq_head, condition, timeout) \
+({ \
+ long __ret = timeout; \
+ might_sleep(); \
+ if (!___wait_cond_timeout(condition)) \
+ __ret = __wait_event_killable_timeout(wq_head, \
+ condition, timeout); \
+ __ret; \
+})
+
#define __wait_event_lock_irq(wq_head, condition, lock, cmd) \
(void)___wait_event(wq_head, condition, TASK_UNINTERRUPTIBLE, 0, 0, \
--
2.14.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] wait: add wait_event_killable_timeout()
2017-08-09 23:46 ` [PATCH 1/3] wait: add wait_event_killable_timeout() Luis R. Rodriguez
@ 2017-08-10 10:33 ` Peter Zijlstra
0 siblings, 0 replies; 5+ messages in thread
From: Peter Zijlstra @ 2017-08-10 10:33 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: akpm, mingo, keescook, dmitry.torokhov, jeyu, rusty, mmarek,
pmladek, mbenes, jpoimboe, ebiederm, shuah, matt.redfearn,
dan.carpenter, colin.king, danielmentz, dcb314, linux-kselftest,
linux-kernel
On Wed, Aug 09, 2017 at 04:46:33PM -0700, Luis R. Rodriguez wrote:
> This wait is similar to wait_event_interruptable_timeout() but only accepts
> SIGKILL interrupt signal. Other signals are ignored.
>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
Assuming someone else is going to merge all these:
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/3] kmod: fix wait on recursive loop
2017-08-09 23:46 [PATCH 0/3] kmod: pending fixes for v4.13-final Luis R. Rodriguez
2017-08-09 23:46 ` [PATCH 1/3] wait: add wait_event_killable_timeout() Luis R. Rodriguez
@ 2017-08-09 23:46 ` Luis R. Rodriguez
2017-08-09 23:46 ` [PATCH 3/3] test_kmod: fix description for -s -and -c parameters Luis R. Rodriguez
2 siblings, 0 replies; 5+ messages in thread
From: Luis R. Rodriguez @ 2017-08-09 23:46 UTC (permalink / raw)
To: akpm
Cc: mingo, peterz, keescook, dmitry.torokhov, jeyu, rusty, mmarek,
pmladek, mbenes, jpoimboe, ebiederm, shuah, matt.redfearn,
dan.carpenter, colin.king, danielmentz, dcb314, linux-kselftest,
linux-kernel, Luis R. Rodriguez
Recursive loops with module loading were previously handled in kmod
by restricting the number of modprobe calls to 50 and if that limit
was breached request_module() would return an error and a user would
see the following on their kernel dmesg:
request_module: runaway loop modprobe binfmt-464c
Starting init:/sbin/init exists but couldn't execute it (error -8)
This issue could happen for instance when a 64-bit kernel boots a 32-bit
userspace on some architectures and has no 32-bit binary format hanlders.
This is visible, for instance, when a CONFIG_MODULES enabled 64-bit MIPS
kernel boots a into o32 root filesystem and the binfmt handler for o32
binaries is not built-in.
After commit 6d7964a722af ("kmod: throttle kmod thread limit") we now
don't have any visible signs of an error and the kernel just waits for
the loop to end somehow.
Although this *particular* recursive loop could also be addressed
by doing a sanity check on search_binary_handler() and disallowing
a modular binfmt to be required for modprobe, a generic solution for
any recursive kernel kmod issues is still needed.
This should catch these loops. We can investigate each loop and address
each one separately as they come in, this however puts a stop gap for
them as before.
Fixes: 6d7964a722af ("kmod: throttle kmod thread limit")
Reported-by: Matt Redfearn <matt.redfearn@imgtec.com>
Tested-by: Matt Redfearn <matt.redfearn@imgetc.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
kernel/kmod.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 6d016c5d97c8..2f37acde640b 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -70,6 +70,18 @@ static DECLARE_RWSEM(umhelper_sem);
static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT);
static DECLARE_WAIT_QUEUE_HEAD(kmod_wq);
+/*
+ * This is a restriction on having *all* MAX_KMOD_CONCURRENT threads
+ * running at the same time without returning. When this happens we
+ * believe you've somehow ended up with a recursive module dependency
+ * creating a loop.
+ *
+ * We have no option but to fail.
+ *
+ * Userspace should proactively try to detect and prevent these.
+ */
+#define MAX_KMOD_ALL_BUSY_TIMEOUT 5
+
/*
modprobe_path is set via /proc/sys.
*/
@@ -167,8 +179,17 @@ int __request_module(bool wait, const char *fmt, ...)
pr_warn_ratelimited("request_module: kmod_concurrent_max (%u) close to 0 (max_modprobes: %u), for module %s, throttling...",
atomic_read(&kmod_concurrent_max),
MAX_KMOD_CONCURRENT, module_name);
- wait_event_interruptible(kmod_wq,
- atomic_dec_if_positive(&kmod_concurrent_max) >= 0);
+ ret = wait_event_killable_timeout(kmod_wq,
+ atomic_dec_if_positive(&kmod_concurrent_max) >= 0,
+ MAX_KMOD_ALL_BUSY_TIMEOUT * HZ);
+ if (!ret) {
+ pr_warn_ratelimited("request_module: modprobe %s cannot be processed, kmod busy with %d threads for more than %d seconds now",
+ module_name, MAX_KMOD_CONCURRENT, MAX_KMOD_ALL_BUSY_TIMEOUT);
+ return -ETIME;
+ } else if (ret == -ERESTARTSYS) {
+ pr_warn_ratelimited("request_module: sigkill sent for modprobe %s, giving up", module_name);
+ return ret;
+ }
}
trace_module_request(module_name, wait, _RET_IP_);
--
2.14.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] test_kmod: fix description for -s -and -c parameters
2017-08-09 23:46 [PATCH 0/3] kmod: pending fixes for v4.13-final Luis R. Rodriguez
2017-08-09 23:46 ` [PATCH 1/3] wait: add wait_event_killable_timeout() Luis R. Rodriguez
2017-08-09 23:46 ` [PATCH 2/3] kmod: fix wait on recursive loop Luis R. Rodriguez
@ 2017-08-09 23:46 ` Luis R. Rodriguez
2 siblings, 0 replies; 5+ messages in thread
From: Luis R. Rodriguez @ 2017-08-09 23:46 UTC (permalink / raw)
To: akpm
Cc: mingo, peterz, keescook, dmitry.torokhov, jeyu, rusty, mmarek,
pmladek, mbenes, jpoimboe, ebiederm, shuah, matt.redfearn,
dan.carpenter, colin.king, danielmentz, dcb314, linux-kselftest,
linux-kernel, Luis R. Rodriguez
The descriptions were reversed, correct this.
Reported-by: Daniel Mentz <danielmentz@google.com>
Fixes: 64b671204afd71 ("test_sysctl: add generic script to expand on tests")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
tools/testing/selftests/kmod/kmod.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index 8cecae9a8bca..7956ea3be667 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -473,8 +473,8 @@ usage()
echo " all Runs all tests (default)"
echo " -t Run test ID the number amount of times is recommended"
echo " -w Watch test ID run until it runs into an error"
- echo " -c Run test ID once"
- echo " -s Run test ID x test-count number of times"
+ echo " -s Run test ID once"
+ echo " -c Run test ID x test-count number of times"
echo " -l List all test ID list"
echo " -h|--help Help"
echo
--
2.14.0
^ permalink raw reply related [flat|nested] 5+ messages in thread