This series fixes a bug where request_module() was reporting success to
kernel code when module autoloading had been completely disabled via
'echo > /proc/sys/kernel/modprobe'.
It also addresses the issues raised on the original thread
(https://lkml.kernel.org/lkml/[email protected]/T/#u)
by documenting the modprobe sysctl, adding a self-test for the empty
path case, and downgrading a user-reachable WARN_ONCE().
Changed since v2:
- Adjusted the new documentation to avoid implicitly bringing up
module aliases, which are a more complex topic.
- Split the selftest patch into two patches, one to fix the test
numbering bug and one to add the new tests.
Changed since v1:
- Added patches to address the other issues raised on the thread.
Eric Biggers (5):
kmod: make request_module() return an error when autoloading is
disabled
fs/filesystems.c: downgrade user-reachable WARN_ONCE() to
pr_warn_once()
docs: admin-guide: document the kernel.modprobe sysctl
selftests: kmod: fix handling test numbers above 9
selftests: kmod: test disabling module autoloading
Documentation/admin-guide/sysctl/kernel.rst | 25 +++++++++++-
fs/filesystems.c | 4 +-
kernel/kmod.c | 4 +-
tools/testing/selftests/kmod/kmod.sh | 43 +++++++++++++++++++--
4 files changed, 68 insertions(+), 8 deletions(-)
--
2.25.1
From: Eric Biggers <[email protected]>
It's long been possible to disable kernel module autoloading completely
(while still allowing manual module insertion) by setting
/proc/sys/kernel/modprobe to the empty string. This can be preferable
to setting it to a nonexistent file since it avoids the overhead of an
attempted execve(), avoids potential deadlocks, and avoids the call to
security_kernel_module_request() and thus on SELinux-based systems
eliminates the need to write SELinux rules to dontaudit module_request.
However, when module autoloading is disabled in this way,
request_module() returns 0. This is broken because callers expect 0 to
mean that the module was successfully loaded.
Apparently this was never noticed because this method of disabling
module autoloading isn't used much, and also most callers don't use the
return value of request_module() since it's always necessary to check
whether the module registered its functionality or not anyway. But
improperly returning 0 can indeed confuse a few callers, for example
get_fs_type() in fs/filesystems.c where it causes a WARNING to be hit:
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);
}
This is easily reproduced with:
echo > /proc/sys/kernel/modprobe
mount -t NONEXISTENT none /
It causes:
request_module fs-NONEXISTENT succeeded, but still no fs?
WARNING: CPU: 1 PID: 1106 at fs/filesystems.c:275 get_fs_type+0xd6/0xf0
[...]
This should actually use pr_warn_once() rather than WARN_ONCE(), since
it's also user-reachable if userspace immediately unloads the module.
Regardless, request_module() should correctly return an error when it
fails. So let's make it return -ENOENT, which matches the error when
the modprobe binary doesn't exist.
I've also sent patches to document and test this case.
Acked-by: Luis Chamberlain <[email protected]>
Cc: [email protected]
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jeff Vander Stoep <[email protected]>
Cc: Jessica Yu <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: NeilBrown <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
kernel/kmod.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/kmod.c b/kernel/kmod.c
index bc6addd9152b4..a2de58de6ab62 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -120,7 +120,7 @@ static int call_modprobe(char *module_name, int wait)
* invoke it.
*
* If module auto-loading support is disabled then this function
- * becomes a no-operation.
+ * simply returns -ENOENT.
*/
int __request_module(bool wait, const char *fmt, ...)
{
@@ -137,7 +137,7 @@ int __request_module(bool wait, const char *fmt, ...)
WARN_ON_ONCE(wait && current_is_async());
if (!modprobe_path[0])
- return 0;
+ return -ENOENT;
va_start(args, fmt);
ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
--
2.25.1
From: Eric Biggers <[email protected]>
Test that request_module() fails with -ENOENT when
/proc/sys/kernel/modprobe contains (a) a nonexistent path, and (b) an
empty path.
Case (b) is a regression test for the patch "kmod: make request_module()
return an error when autoloading is disabled".
Tested with 'kmod.sh -t 0010 && kmod.sh -t 0011', and also simply with
'kmod.sh' to run all kmod tests.
Acked-by: Luis Chamberlain <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jeff Vander Stoep <[email protected]>
Cc: Jessica Yu <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: NeilBrown <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
tools/testing/selftests/kmod/kmod.sh | 30 ++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index 315a43111e046..3702dbcc90a77 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -61,6 +61,8 @@ ALL_TESTS="$ALL_TESTS 0006:10:1"
ALL_TESTS="$ALL_TESTS 0007:5:1"
ALL_TESTS="$ALL_TESTS 0008:150:1"
ALL_TESTS="$ALL_TESTS 0009:150:1"
+ALL_TESTS="$ALL_TESTS 0010:1:1"
+ALL_TESTS="$ALL_TESTS 0011:1:1"
# Kselftest framework requirement - SKIP code is 4.
ksft_skip=4
@@ -149,6 +151,7 @@ function load_req_mod()
test_finish()
{
+ echo "$MODPROBE" > /proc/sys/kernel/modprobe
echo "Test completed"
}
@@ -443,6 +446,30 @@ kmod_test_0009()
config_expect_result ${FUNCNAME[0]} SUCCESS
}
+kmod_test_0010()
+{
+ kmod_defaults_driver
+ config_num_threads 1
+ echo "/KMOD_TEST_NONEXISTENT" > /proc/sys/kernel/modprobe
+ config_trigger ${FUNCNAME[0]}
+ config_expect_result ${FUNCNAME[0]} -ENOENT
+ echo "$MODPROBE" > /proc/sys/kernel/modprobe
+}
+
+kmod_test_0011()
+{
+ kmod_defaults_driver
+ config_num_threads 1
+ # This causes the kernel to not even try executing modprobe. The error
+ # code is still -ENOENT like when modprobe doesn't exist, so we can't
+ # easily test for the exact difference. But this still is a useful test
+ # since there was a bug where request_module() returned 0 in this case.
+ echo > /proc/sys/kernel/modprobe
+ config_trigger ${FUNCNAME[0]}
+ config_expect_result ${FUNCNAME[0]} -ENOENT
+ echo "$MODPROBE" > /proc/sys/kernel/modprobe
+}
+
list_tests()
{
echo "Test ID list:"
@@ -460,6 +487,8 @@ list_tests()
echo "0007 x $(get_test_count 0007) - multithreaded tests with default setup test request_module() and get_fs_type()"
echo "0008 x $(get_test_count 0008) - multithreaded - push kmod_concurrent over max_modprobes for request_module()"
echo "0009 x $(get_test_count 0009) - multithreaded - push kmod_concurrent over max_modprobes for get_fs_type()"
+ echo "0010 x $(get_test_count 0010) - test nonexistent modprobe path"
+ echo "0011 x $(get_test_count 0011) - test completely disabling module autoloading"
}
usage()
@@ -616,6 +645,7 @@ test_reqs
allow_user_defaults
load_req_mod
+MODPROBE=$(</proc/sys/kernel/modprobe)
trap "test_finish" EXIT
parse_args $@
--
2.25.1
From: Eric Biggers <[email protected]>
get_test_count() and get_test_enabled() were broken for test numbers
above 9 due to awk interpreting a field specification like '$0010' as
octal rather than decimal. Fix it by stripping the leading zeroes.
Acked-by: Luis Chamberlain <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jeff Vander Stoep <[email protected]>
Cc: Jessica Yu <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: NeilBrown <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
tools/testing/selftests/kmod/kmod.sh | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index 8b944cf042f6c..315a43111e046 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -505,18 +505,23 @@ function test_num()
fi
}
-function get_test_count()
+function get_test_data()
{
test_num $1
- TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+ local field_num=$(echo $1 | sed 's/^0*//')
+ echo $ALL_TESTS | awk '{print $'$field_num'}'
+}
+
+function get_test_count()
+{
+ TEST_DATA=$(get_test_data $1)
LAST_TWO=${TEST_DATA#*:*}
echo ${LAST_TWO%:*}
}
function get_test_enabled()
{
- test_num $1
- TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+ TEST_DATA=$(get_test_data $1)
echo ${TEST_DATA#*:*:}
}
--
2.25.1
From: Eric Biggers <[email protected]>
After request_module(), nothing is stopping the module from being
unloaded until someone takes a reference to it via try_get_module().
The WARN_ONCE() in get_fs_type() is thus user-reachable, via userspace
running 'rmmod' concurrently.
Since WARN_ONCE() is for kernel bugs only, not for user-reachable
situations, downgrade this warning to pr_warn_once().
Acked-by: Luis Chamberlain <[email protected]>
Cc: [email protected]
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jeff Vander Stoep <[email protected]>
Cc: Jessica Yu <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: NeilBrown <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
fs/filesystems.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/filesystems.c b/fs/filesystems.c
index 77bf5f95362da..90b8d879fbaf3 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -272,7 +272,9 @@ struct file_system_type *get_fs_type(const char *name)
fs = __get_fs_type(name, len);
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 (!fs)
+ pr_warn_once("request_module fs-%.*s succeeded, but still no fs?\n",
+ len, name);
}
if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
--
2.25.1
Hi
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.5.9, v5.4.25, v4.19.109, v4.14.173, v4.9.216, v4.4.216.
v5.5.9: Build OK!
v5.4.25: Build OK!
v4.19.109: Build OK!
v4.14.173: Build OK!
v4.9.216: Failed to apply! Possible dependencies:
41124db869b7 ("fs: warn in case userspace lied about modprobe return")
v4.4.216: Failed to apply! Possible dependencies:
41124db869b7 ("fs: warn in case userspace lied about modprobe return")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
+++ Sasha Levin [17/03/20 22:30 +0000]:
>Hi
>
>[This is an automated email]
>
>This commit has been processed because it contains a -stable tag.
>The stable tag indicates that it's relevant for the following trees: all
>
>The bot has tested the following trees: v5.5.9, v5.4.25, v4.19.109, v4.14.173, v4.9.216, v4.4.216.
>
>v5.5.9: Build OK!
>v5.4.25: Build OK!
>v4.19.109: Build OK!
>v4.14.173: Build OK!
>v4.9.216: Failed to apply! Possible dependencies:
> 41124db869b7 ("fs: warn in case userspace lied about modprobe return")
>
>v4.4.216: Failed to apply! Possible dependencies:
> 41124db869b7 ("fs: warn in case userspace lied about modprobe return")
>
>
>NOTE: The patch will not be queued to stable trees until it is upstream.
>
>How should we proceed with this patch?
Since commit 41124db869b7 was introduced v4.13, I guess we should
change the stable tag to:
Cc: [email protected] # v4.13+
+++ Eric Biggers [14/03/20 14:34 -0700]:
>From: Eric Biggers <[email protected]>
>
>After request_module(), nothing is stopping the module from being
>unloaded until someone takes a reference to it via try_get_module().
>
>The WARN_ONCE() in get_fs_type() is thus user-reachable, via userspace
>running 'rmmod' concurrently.
>
>Since WARN_ONCE() is for kernel bugs only, not for user-reachable
>situations, downgrade this warning to pr_warn_once().
>
>Acked-by: Luis Chamberlain <[email protected]>
>Cc: [email protected]
>Cc: Alexei Starovoitov <[email protected]>
>Cc: Andrew Morton <[email protected]>
>Cc: Greg Kroah-Hartman <[email protected]>
>Cc: Jeff Vander Stoep <[email protected]>
>Cc: Jessica Yu <[email protected]>
>Cc: Kees Cook <[email protected]>
>Cc: NeilBrown <[email protected]>
>Signed-off-by: Eric Biggers <[email protected]>
>---
> fs/filesystems.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/fs/filesystems.c b/fs/filesystems.c
>index 77bf5f95362da..90b8d879fbaf3 100644
>--- a/fs/filesystems.c
>+++ b/fs/filesystems.c
>@@ -272,7 +272,9 @@ struct file_system_type *get_fs_type(const char *name)
> fs = __get_fs_type(name, len);
> 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 (!fs)
>+ pr_warn_once("request_module fs-%.*s succeeded, but still no fs?\n",
>+ len, name);
Hm, what was the rationale for warning only once again? It might be useful
for debugging issues to see each instance of request_module() failure
(and with which fs). However, I don't have a concrete use case to
support this argument, so:
Reviewed-by: Jessica Yu <[email protected]>
+++ Eric Biggers [14/03/20 14:34 -0700]:
>From: Eric Biggers <[email protected]>
>
>It's long been possible to disable kernel module autoloading completely
>(while still allowing manual module insertion) by setting
>/proc/sys/kernel/modprobe to the empty string. This can be preferable
>to setting it to a nonexistent file since it avoids the overhead of an
>attempted execve(), avoids potential deadlocks, and avoids the call to
>security_kernel_module_request() and thus on SELinux-based systems
>eliminates the need to write SELinux rules to dontaudit module_request.
>
>However, when module autoloading is disabled in this way,
>request_module() returns 0. This is broken because callers expect 0 to
>mean that the module was successfully loaded.
>
>Apparently this was never noticed because this method of disabling
>module autoloading isn't used much, and also most callers don't use the
>return value of request_module() since it's always necessary to check
>whether the module registered its functionality or not anyway. But
>improperly returning 0 can indeed confuse a few callers, for example
>get_fs_type() in fs/filesystems.c where it causes a WARNING to be hit:
>
> 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);
> }
>
>This is easily reproduced with:
>
> echo > /proc/sys/kernel/modprobe
> mount -t NONEXISTENT none /
>
>It causes:
>
> request_module fs-NONEXISTENT succeeded, but still no fs?
> WARNING: CPU: 1 PID: 1106 at fs/filesystems.c:275 get_fs_type+0xd6/0xf0
> [...]
>
>This should actually use pr_warn_once() rather than WARN_ONCE(), since
>it's also user-reachable if userspace immediately unloads the module.
>Regardless, request_module() should correctly return an error when it
>fails. So let's make it return -ENOENT, which matches the error when
>the modprobe binary doesn't exist.
>
>I've also sent patches to document and test this case.
>
>Acked-by: Luis Chamberlain <[email protected]>
>Cc: [email protected]
>Cc: Alexei Starovoitov <[email protected]>
>Cc: Andrew Morton <[email protected]>
>Cc: Greg Kroah-Hartman <[email protected]>
>Cc: Jeff Vander Stoep <[email protected]>
>Cc: Jessica Yu <[email protected]>
>Cc: Kees Cook <[email protected]>
>Cc: NeilBrown <[email protected]>
>Signed-off-by: Eric Biggers <[email protected]>
Reviewed-by: Jessica Yu <[email protected]>
On Wed, Mar 18, 2020 at 04:43:15PM +0100, Jessica Yu wrote:
> +++ Eric Biggers [14/03/20 14:34 -0700]:
> > From: Eric Biggers <[email protected]>
> >
> > After request_module(), nothing is stopping the module from being
> > unloaded until someone takes a reference to it via try_get_module().
> >
> > The WARN_ONCE() in get_fs_type() is thus user-reachable, via userspace
> > running 'rmmod' concurrently.
> >
> > Since WARN_ONCE() is for kernel bugs only, not for user-reachable
> > situations, downgrade this warning to pr_warn_once().
> >
> > Acked-by: Luis Chamberlain <[email protected]>
> > Cc: [email protected]
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Jeff Vander Stoep <[email protected]>
> > Cc: Jessica Yu <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: NeilBrown <[email protected]>
> > Signed-off-by: Eric Biggers <[email protected]>
> > ---
> > fs/filesystems.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/filesystems.c b/fs/filesystems.c
> > index 77bf5f95362da..90b8d879fbaf3 100644
> > --- a/fs/filesystems.c
> > +++ b/fs/filesystems.c
> > @@ -272,7 +272,9 @@ struct file_system_type *get_fs_type(const char *name)
> > fs = __get_fs_type(name, len);
> > 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 (!fs)
> > + pr_warn_once("request_module fs-%.*s succeeded, but still no fs?\n",
> > + len, name);
>
> Hm, what was the rationale for warning only once again? It might be useful
> for debugging issues to see each instance of request_module() failure
> (and with which fs). However, I don't have a concrete use case to
> support this argument, so:
>
> Reviewed-by: Jessica Yu <[email protected]>
This was discussed on v2, see
https://lkml.kernel.org/lkml/[email protected]/.
If the warning triggers, then it indicates a broken modprobe program.
Printing the warning multiple times wouldn't really add any new information.
And in any case, it's printed once both before and after this patch.
- Eric
On Wed, Mar 18, 2020 at 04:09:26PM +0100, Jessica Yu wrote:
> +++ Sasha Levin [17/03/20 22:30 +0000]:
> > Hi
> >
> > [This is an automated email]
> >
> > This commit has been processed because it contains a -stable tag.
> > The stable tag indicates that it's relevant for the following trees: all
> >
> > The bot has tested the following trees: v5.5.9, v5.4.25, v4.19.109, v4.14.173, v4.9.216, v4.4.216.
> >
> > v5.5.9: Build OK!
> > v5.4.25: Build OK!
> > v4.19.109: Build OK!
> > v4.14.173: Build OK!
> > v4.9.216: Failed to apply! Possible dependencies:
> > 41124db869b7 ("fs: warn in case userspace lied about modprobe return")
> >
> > v4.4.216: Failed to apply! Possible dependencies:
> > 41124db869b7 ("fs: warn in case userspace lied about modprobe return")
> >
> >
> > NOTE: The patch will not be queued to stable trees until it is upstream.
> >
> > How should we proceed with this patch?
>
> Since commit 41124db869b7 was introduced v4.13, I guess we should
> change the stable tag to:
>
> Cc: [email protected] # v4.13+
>
It should use:
Fixes: 41124db869b7 ("fs: warn in case userspace lied about modprobe return")
Cc: <[email protected]> # v4.13+
I'll add it.