[PATCH v2 0/4] module autoloading fixes and cleanups

[PATCH v2 0/4] module autoloading fixes and cleanups

[Eric Biggers]

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/20200310223731.126894-1-ebiggers@kernel.org/T/#u)
by documenting the modprobe sysctl, adding a self-test for the empty
path case, and downgrading a user-reachable WARN_ONCE().

Eric Biggers (4):
  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: 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

[PATCH v2 1/4] kmod: make request_module() return an error when autoloading is disabled

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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.

Cc: stable@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 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

[PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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().

Cc: stable@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 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

[PATCH v2 3/4] docs: admin-guide: document the kernel.modprobe sysctl

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Document the kernel.modprobe sysctl in the same place that all the other
kernel.* sysctls are documented.  Make sure to mention how to use this
sysctl to completely disable module autoloading, and how this sysctl
relates to CONFIG_STATIC_USERMODEHELPER.

Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 Documentation/admin-guide/sysctl/kernel.rst | 25 ++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index def074807cee9..454f3402ed321 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -49,7 +49,7 @@ show up in /proc/sys/kernel:
 - kexec_load_disabled
 - kptr_restrict
 - l2cr                        [ PPC only ]
-- modprobe                    ==> Documentation/debugging-modules.txt
+- modprobe
 - modules_disabled
 - msg_next_id		      [ sysv ipc ]
 - msgmax
@@ -444,6 +444,29 @@ l2cr: (PPC only)
 This flag controls the L2 cache of G3 processor boards. If
 0, the cache is disabled. Enabled if nonzero.
 
+modprobe:
+=========
+
+The path to the usermode helper for autoloading kernel modules, by
+default "/sbin/modprobe".  This binary is executed when the kernel
+requests a module.  For example, if userspace passes an unknown
+filesystem type "foo" to mount(), then the kernel will automatically
+request the module "fs-foo.ko" by executing this usermode helper.
+This usermode helper should insert the needed module into the kernel.
+
+This sysctl only affects module autoloading.  It has no effect on the
+ability to explicitly insert modules.
+
+If this sysctl is set to the empty string, then module autoloading is
+completely disabled.  The kernel will not try to execute a usermode
+helper at all, nor will it call the kernel_module_request LSM hook.
+
+If CONFIG_STATIC_USERMODEHELPER=y is set in the kernel configuration,
+then the configured static usermode helper overrides this sysctl,
+except that the empty string is still accepted to completely disable
+module autoloading as described above.
+
+Also see Documentation/debugging-modules.txt.
 
 modules_disabled:
 =================
-- 
2.25.1

[PATCH v2 4/4] selftests: kmod: test disabling module autoloading

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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.

Note: 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.  So I fixed that too.

Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: NeilBrown <neilb@suse.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 tools/testing/selftests/kmod/kmod.sh | 43 +++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
index 8b944cf042f6c..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()
@@ -505,18 +534,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#*:*:}
 }
 
@@ -611,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

Re: [PATCH v2 3/4] docs: admin-guide: document the kernel.modprobe sysctl

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3230 bytes --]

On Thu, Mar 12 2020, Eric Biggers wrote:

> From: Eric Biggers <ebiggers@google.com>
>
> Document the kernel.modprobe sysctl in the same place that all the other
> kernel.* sysctls are documented.  Make sure to mention how to use this
> sysctl to completely disable module autoloading, and how this sysctl
> relates to CONFIG_STATIC_USERMODEHELPER.
>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jeff Vander Stoep <jeffv@google.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  Documentation/admin-guide/sysctl/kernel.rst | 25 ++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index def074807cee9..454f3402ed321 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -49,7 +49,7 @@ show up in /proc/sys/kernel:
>  - kexec_load_disabled
>  - kptr_restrict
>  - l2cr                        [ PPC only ]
> -- modprobe                    ==> Documentation/debugging-modules.txt
> +- modprobe
>  - modules_disabled
>  - msg_next_id		      [ sysv ipc ]
>  - msgmax
> @@ -444,6 +444,29 @@ l2cr: (PPC only)
>  This flag controls the L2 cache of G3 processor boards. If
>  0, the cache is disabled. Enabled if nonzero.
>  
> +modprobe:
> +=========
> +
> +The path to the usermode helper for autoloading kernel modules, by
> +default "/sbin/modprobe".  This binary is executed when the kernel
> +requests a module.  For example, if userspace passes an unknown
> +filesystem type "foo" to mount(), then the kernel will automatically
> +request the module "fs-foo.ko" by executing this usermode helper.

I don't think it is right to add the ".ko" there.  The string "fs-foo"
is what is passed to the named executable, and it make well end up
loading "bar.ko", depending what aliases are set up.
I would probably write  '... request the module named 'fs-foo" by executing..'
(The "name" for a module can come from the file that stores it, and
alias inside it, or configuration in modprobe.d).

Thanks,
NeilBrown


> +This usermode helper should insert the needed module into the kernel.
> +
> +This sysctl only affects module autoloading.  It has no effect on the
> +ability to explicitly insert modules.
> +
> +If this sysctl is set to the empty string, then module autoloading is
> +completely disabled.  The kernel will not try to execute a usermode
> +helper at all, nor will it call the kernel_module_request LSM hook.
> +
> +If CONFIG_STATIC_USERMODEHELPER=y is set in the kernel configuration,
> +then the configured static usermode helper overrides this sysctl,
> +except that the empty string is still accepted to completely disable
> +module autoloading as described above.
> +
> +Also see Documentation/debugging-modules.txt.
>  
>  modules_disabled:
>  =================
> -- 
> 2.25.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1915 bytes --]

On Thu, Mar 12 2020, Eric Biggers wrote:

> From: Eric Biggers <ebiggers@google.com>
>
> 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().
>
> Cc: stable@vger.kernel.org
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jeff Vander Stoep <jeffv@google.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  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);

I strongly support the replacement of "WARN" by "pr_warn".
I wonder if we really want the "once" now.  Possibly using rate_limited
would be justified, but I think that in general we should see a warning
every time this event happens.

Thanks,
NeilBrown


>  	}
>  
>  	if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
> -- 
> 2.25.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 1/4] kmod: make request_module() return an error when autoloading is disabled

[Luis Chamberlain]

On Thu, Mar 12, 2020 at 01:25:49PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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.
> 
> Cc: stable@vger.kernel.org
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jeff Vander Stoep <jeffv@google.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Luis Chamberlain]

On Thu, Mar 12, 2020 at 01:25:50PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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().
> 
> Cc: stable@vger.kernel.org
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jeff Vander Stoep <jeffv@google.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v2 2/4] fs/filesystems.c: downgrade user-reachable WARN_ONCE() to pr_warn_once()

[Luis Chamberlain]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

On Fri, Mar 13, 2020 at 09:06:46AM +1100, NeilBrown wrote:
> On Thu, Mar 12 2020, Eric Biggers wrote:
> 
> > From: Eric Biggers <ebiggers@google.com>
> >
> > 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().
> >
> > Cc: stable@vger.kernel.org
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jeff Vander Stoep <jeffv@google.com>
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > Cc: NeilBrown <neilb@suse.com>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  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);
> 
> I strongly support the replacement of "WARN" by "pr_warn".
> I wonder if we really want the "once" now.  Possibly using rate_limited
> would be justified, but I think that in general we should see a warning
> every time this event happens.

Since the usefulness of the print is at boot, I think pr_warn_once() is
good right now but just because I cannot think of a case where multiple
prints are currently desirable, or where this should be possible
post-boot. Can you?

  Luis

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] docs: admin-guide: document the kernel.modprobe sysctl

[Luis Chamberlain]

[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]

On Fri, Mar 13, 2020 at 09:04:01AM +1100, NeilBrown wrote:
> On Thu, Mar 12 2020, Eric Biggers wrote:
> 
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Document the kernel.modprobe sysctl in the same place that all the other
> > kernel.* sysctls are documented.  Make sure to mention how to use this
> > sysctl to completely disable module autoloading, and how this sysctl
> > relates to CONFIG_STATIC_USERMODEHELPER.
> >
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jeff Vander Stoep <jeffv@google.com>
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > Cc: NeilBrown <neilb@suse.com>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  Documentation/admin-guide/sysctl/kernel.rst | 25 ++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > index def074807cee9..454f3402ed321 100644
> > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > @@ -49,7 +49,7 @@ show up in /proc/sys/kernel:
> >  - kexec_load_disabled
> >  - kptr_restrict
> >  - l2cr                        [ PPC only ]
> > -- modprobe                    ==> Documentation/debugging-modules.txt
> > +- modprobe
> >  - modules_disabled
> >  - msg_next_id		      [ sysv ipc ]
> >  - msgmax
> > @@ -444,6 +444,29 @@ l2cr: (PPC only)
> >  This flag controls the L2 cache of G3 processor boards. If
> >  0, the cache is disabled. Enabled if nonzero.
> >  
> > +modprobe:
> > +=========
> > +
> > +The path to the usermode helper for autoloading kernel modules, by
> > +default "/sbin/modprobe".  This binary is executed when the kernel
> > +requests a module.  For example, if userspace passes an unknown
> > +filesystem type "foo" to mount(), then the kernel will automatically
> > +request the module "fs-foo.ko" by executing this usermode helper.
> 
> I don't think it is right to add the ".ko" there.  The string "fs-foo"
> is what is passed to the named executable, and it make well end up
> loading "bar.ko", depending what aliases are set up.
> I would probably write  '... request the module named 'fs-foo" by executing..'

And that is just because filesystems, in this case a mount call, will
use the fs- prefix for aliases. This is tribal knowledge in the context
above, and so someone not familiar with this won't easily grasp this.

Is there an easier autoloading example other than filesystems we can use that
doesn't require you to explain the aliasing thing?

What is module autoloading? Where is this documented ? If that
can be slightly clarified this would be even easier to understand as
well.

  Luis

> (The "name" for a module can come from the file that stores it, and
> alias inside it, or configuration in modprobe.d).
> 
> Thanks,
> NeilBrown
> 
> 
> > +This usermode helper should insert the needed module into the kernel.
> > +
> > +This sysctl only affects module autoloading.  It has no effect on the
> > +ability to explicitly insert modules.
> > +
> > +If this sysctl is set to the empty string, then module autoloading is
> > +completely disabled.  The kernel will not try to execute a usermode
> > +helper at all, nor will it call the kernel_module_request LSM hook.
> > +
> > +If CONFIG_STATIC_USERMODEHELPER=y is set in the kernel configuration,
> > +then the configured static usermode helper overrides this sysctl,
> > +except that the empty string is still accepted to completely disable
> > +module autoloading as described above.
> > +
> > +Also see Documentation/debugging-modules.txt.
> >  
> >  modules_disabled:
> >  =================
> > -- 
> > 2.25.1



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/4] selftests: kmod: test disabling module autoloading

[Luis Chamberlain]

On Thu, Mar 12, 2020 at 01:25:52PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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.
> 
> Note: 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.  So I fixed that too.
> 
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jeff Vander Stoep <jeffv@google.com>
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks!

Can you split up the get_test_count()/get_test_enabled() fix into
another patch though? 

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v2 3/4] docs: admin-guide: document the kernel.modprobe sysctl

[Eric Biggers]

On Fri, Mar 13, 2020 at 01:07:27AM +0000, Luis Chamberlain wrote:
> > > +modprobe:
> > > +=========
> > > +
> > > +The path to the usermode helper for autoloading kernel modules, by
> > > +default "/sbin/modprobe".  This binary is executed when the kernel
> > > +requests a module.  For example, if userspace passes an unknown
> > > +filesystem type "foo" to mount(), then the kernel will automatically
> > > +request the module "fs-foo.ko" by executing this usermode helper.
> > 
> > I don't think it is right to add the ".ko" there.  The string "fs-foo"
> > is what is passed to the named executable, and it make well end up
> > loading "bar.ko", depending what aliases are set up.
> > I would probably write  '... request the module named 'fs-foo" by executing..'
> 
> And that is just because filesystems, in this case a mount call, will
> use the fs- prefix for aliases. This is tribal knowledge in the context
> above, and so someone not familiar with this won't easily grasp this.
> 
> Is there an easier autoloading example other than filesystems we can use that
> doesn't require you to explain the aliasing thing?
> 
> What is module autoloading? Where is this documented ? If that
> can be slightly clarified this would be even easier to understand as
> well.
> 

I think we're getting too down into the weeds here.  The purpose of this patch
is just to document the modprobe sysctl, not to to give a full explanation of
how module autoloading works including modaliases and everything.  And this
sysctl isn't needed to enable module autoloading; it's enabled by default.
Most users already use module autoloading without ever touching this sysctl.

Let's just write instead:

	For example, if userspace passes an unknown filesystem type to mount(),
	then the kernel will automatically request the corresponding filesystem
	module by executing this usermode helper.  This usermode helper should
	insert the needed module into the kernel.

If someone wants to write a new documentation file that fully explains kernel
modules (I don't see any yet), they should should certainly do so.  It's more
than I set out to do, though.  IMO, just documenting this sysctl is already a
nice improvement by itself.

- Eric

Re: [PATCH v2 3/4] docs: admin-guide: document the kernel.modprobe sysctl

[Luis Chamberlain]

On Fri, Mar 13, 2020 at 12:05:29PM -0700, Eric Biggers wrote:
> Let's just write instead:
> 
> 	For example, if userspace passes an unknown filesystem type to mount(),
> 	then the kernel will automatically request the corresponding filesystem
> 	module by executing this usermode helper.  This usermode helper should
> 	insert the needed module into the kernel.

Works with me.

  Luis