Luis R. Rodriguez
2020-Jun-10 15:49 UTC
[Bridge] [PATCH 3/5] test_kmod: Avoid potential double free in trigger_config_run_type()
From: Tiezhu Yang <yangtiezhu at loongson.cn>
Reset the member "test_fs" of the test configuration after a call
of the function "kfree_const" to a null pointer so that a double
memory release will not be performed.
Fixes: d9c6a72d6fa2 ("kmod: add test driver to stress test the module
loader")
Acked-by: Luis Chamberlain <mcgrof at kernel.org>
Signed-off-by: Tiezhu Yang <yangtiezhu at loongson.cn>
Signed-off-by: Luis Chamberlain <mcgrof at kernel.org>
---
lib/test_kmod.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index e651c37d56db..eab52770070d 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -745,7 +745,7 @@ static int trigger_config_run_type(struct kmod_test_device
*test_dev,
break;
case TEST_KMOD_FS_TYPE:
kfree_const(config->test_fs);
- config->test_driver = NULL;
+ config->test_fs = NULL;
copied = config_copy_test_fs(config, test_str,
strlen(test_str));
break;
--
2.26.2
Luis R. Rodriguez
2020-Jun-10 15:49 UTC
[Bridge] [PATCH 1/5] selftests: kmod: Use variable NAME in kmod_test_0001()
From: Tiezhu Yang <yangtiezhu at loongson.cn>
Use the variable NAME instead of "\000" directly in kmod_test_0001().
Acked-by: Luis Chamberlain <mcgrof at kernel.org>
Signed-off-by: Tiezhu Yang <yangtiezhu at loongson.cn>
Signed-off-by: Luis Chamberlain <mcgrof at 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 3702dbcc90a7..da60c3bd4f23 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -341,7 +341,7 @@ kmod_test_0001_driver()
kmod_defaults_driver
config_num_threads 1
- printf '\000' >"$DIR"/config_test_driver
+ printf $NAME >"$DIR"/config_test_driver
config_trigger ${FUNCNAME[0]}
config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
}
@@ -352,7 +352,7 @@ kmod_test_0001_fs()
kmod_defaults_fs
config_num_threads 1
- printf '\000' >"$DIR"/config_test_fs
+ printf $NAME >"$DIR"/config_test_fs
config_trigger ${FUNCNAME[0]}
config_expect_result ${FUNCNAME[0]} -EINVAL
}
--
2.26.2
Luis R. Rodriguez
2020-Jun-10 15:49 UTC
[Bridge] [PATCH 2/5] kmod: Remove redundant "be an" in the comment
From: Tiezhu Yang <yangtiezhu at loongson.cn> There exists redundant "be an" in the comment, remove it. Acked-by: Luis Chamberlain <mcgrof at kernel.org> Signed-off-by: Tiezhu Yang <yangtiezhu at loongson.cn> Signed-off-by: Luis Chamberlain <mcgrof at kernel.org> --- kernel/kmod.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/kernel/kmod.c b/kernel/kmod.c index 37c3c4b97b8e..3cd075ce2a1e 100644 --- a/kernel/kmod.c +++ b/kernel/kmod.c @@ -36,9 +36,8 @@ * * If you need less than 50 threads would mean we're dealing with systems * smaller than 3200 pages. This assumes you are capable of having ~13M memory, - * and this would only be an be an upper limit, after which the OOM killer - * would take effect. Systems like these are very unlikely if modules are - * enabled. + * and this would only be an upper limit, after which the OOM killer would take + * effect. Systems like these are very unlikely if modules are enabled. */ #define MAX_KMOD_CONCURRENT 50 static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT); -- 2.26.2
From: Luis Chamberlain <mcgrof at kernel.org> Tiezhu Yang had sent out a patch set with a slew of kmod selftest fixes, and one patch which modified kmod to return 254 when a module was not found. This opened up pandora's box about why that was being used for and low and behold its because when UMH_WAIT_PROC is used we call a kernel_wait4() call but have never unwrapped the error code. The commit log for that fix details the rationale for the approach taken. I'd appreciate some review on that, in particular nfs folks as it seems a case was never really hit before. This goes boot tested, selftested with kmod, and 0-day gives its build blessings. Luis Chamberlain (2): umh: fix processed error when UMH_WAIT_PROC is used selftests: simplify kmod failure value Tiezhu Yang (3): selftests: kmod: Use variable NAME in kmod_test_0001() kmod: Remove redundant "be an" in the comment test_kmod: Avoid potential double free in trigger_config_run_type() drivers/block/drbd/drbd_nl.c | 20 +++++------ fs/nfsd/nfs4recover.c | 2 +- include/linux/sched/task.h | 13 ++++++++ kernel/kmod.c | 5 ++- kernel/umh.c | 4 +-- lib/test_kmod.c | 2 +- net/bridge/br_stp_if.c | 10 ++---- security/keys/request_key.c | 2 +- tools/testing/selftests/kmod/kmod.sh | 50 +++++++++++++++++++++++----- 9 files changed, 71 insertions(+), 37 deletions(-) -- 2.26.2
Luis R. Rodriguez
2020-Jun-10 15:49 UTC
[Bridge] [PATCH 4/5] umh: fix processed error when UMH_WAIT_PROC is used
From: Luis Chamberlain <mcgrof at kernel.org>
When UMH_WAIT_PROC is used we call kernel_wait4(). This is
the *only* place in the kernel where we actually inspect the
error code. Prior to this patch we returned the value from the
wait call, and that technically requires us to use wrappers
such as WEXITSTATUS(). We either fix all callers to start
using WEXITSTATUS() and friends *or* we do address this within
the umh code and let the callers get the actual error code.
The way we use kernel_wait4() on the umh is with the options
set to 0, and when this is done the wait call only waits for
terminated children. Because of this, there is no point to
complicate checks for the umh with W*() calls. That would
make the checks complex, redundant, and simply not needed.
By making the umh do the checks for us we keep users
kernel_wait4() at bay, and promote avoiding introduction of
further W*() macros and the complexities this can bring.
There were only a few callers which properly checked for
the error status using open-coded solutions. We remove
them as they are no longer needed, and also remove open
coded implicit uses of W*() uses which should never
trigger given that the options passed to wait is 0.
The only helpers we really need are for termination, so we
just include those, and we prefix our W*() helpers with K.
Since all this does is *correct* an error code, if one
was found, this change only fixes reporting the *correct*
error, and there are two places where this matters, and
which this patch fixes:
* request_module() used to fail with an error code of
256 when a module was not found. Now it properly
returns 1.
* fs/nfsd/nfs4recover.c: we never were disabling the
upcall as the error code of -ENOENT or -EACCES was
*never* properly checked for.
Reported-by: Tiezhu Yang <yangtiezhu at loongson.cn>
Signed-off-by: Luis Chamberlain <mcgrof at kernel.org>
---
drivers/block/drbd/drbd_nl.c | 20 ++++++++------------
fs/nfsd/nfs4recover.c | 2 +-
include/linux/sched/task.h | 13 +++++++++++++
kernel/umh.c | 4 ++--
net/bridge/br_stp_if.c | 10 ++--------
security/keys/request_key.c | 2 +-
6 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index da4a3ebe04ef..aee272e620b9 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -382,13 +382,11 @@ int drbd_khelper(struct drbd_device *device, char *cmd)
notify_helper(NOTIFY_CALL, device, connection, cmd, 0);
ret = call_usermodehelper(drbd_usermode_helper, argv, envp, UMH_WAIT_PROC);
if (ret)
- drbd_warn(device, "helper command: %s %s %s exit code %u (0x%x)\n",
- drbd_usermode_helper, cmd, mb,
- (ret >> 8) & 0xff, ret);
+ drbd_warn(device, "helper command: %s %s %s failed with exit code %u
(0x%x)\n",
+ drbd_usermode_helper, cmd, mb, ret, ret);
else
- drbd_info(device, "helper command: %s %s %s exit code %u (0x%x)\n",
- drbd_usermode_helper, cmd, mb,
- (ret >> 8) & 0xff, ret);
+ drbd_info(device, "helper command: %s %s %s completed
successfully\n",
+ drbd_usermode_helper, cmd, mb);
sib.sib_reason = SIB_HELPER_POST;
sib.helper_exit_code = ret;
drbd_bcast_event(device, &sib);
@@ -424,13 +422,11 @@ enum drbd_peer_state conn_khelper(struct drbd_connection
*connection, char *cmd)
ret = call_usermodehelper(drbd_usermode_helper, argv, envp, UMH_WAIT_PROC);
if (ret)
- drbd_warn(connection, "helper command: %s %s %s exit code %u
(0x%x)\n",
- drbd_usermode_helper, cmd, resource_name,
- (ret >> 8) & 0xff, ret);
+ drbd_warn(connection, "helper command: %s %s %s failed with exit code %u
(0x%x)\n",
+ drbd_usermode_helper, cmd, resource_name, ret, ret);
else
- drbd_info(connection, "helper command: %s %s %s exit code %u
(0x%x)\n",
- drbd_usermode_helper, cmd, resource_name,
- (ret >> 8) & 0xff, ret);
+ drbd_info(connection, "helper command: %s %s %s completed
successfully\n",
+ drbd_usermode_helper, cmd, resource_name);
/* TODO: conn_bcast_event() ?? */
notify_helper(NOTIFY_RESPONSE, NULL, connection, cmd, ret);
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 9e40dfecf1b1..33e6a7fd7961 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1820,7 +1820,7 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0,
char *env1)
ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
/*
- * Disable the upcall mechanism if we're getting an ENOENT or EACCES
+ * Disable the upcall mechanism if we're getting an -ENOENT or -EACCES
* error. The admin can re-enable it on the fly by using sysfs
* once the problem has been fixed.
*/
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index 38359071236a..bba06befbff5 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -103,6 +103,19 @@ struct mm_struct *copy_init_mm(void);
extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
extern long kernel_wait4(pid_t, int __user *, int, struct rusage *);
+/* Only add helpers for actual use cases in the kernel */
+#define KWEXITSTATUS(status) (__KWEXITSTATUS(status))
+#define KWIFEXITED(status) (__KWIFEXITED(status))
+
+/* Nonzero if STATUS indicates normal termination. */
+#define __KWIFEXITED(status) (__KWTERMSIG(status) == 0)
+
+/* If KWIFEXITED(STATUS), the low-order 8 bits of the status. */
+#define __KWEXITSTATUS(status) (((status) & 0xff00) >> 8)
+
+/* If KWIFSIGNALED(STATUS), the terminating signal. */
+#define __KWTERMSIG(status) ((status) & 0x7f)
+
extern void free_task(struct task_struct *tsk);
/* sched_exec is called by processes performing an exec */
diff --git a/kernel/umh.c b/kernel/umh.c
index 79f139a7ca03..f81e8698e36e 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -154,8 +154,8 @@ static void call_usermodehelper_exec_sync(struct
subprocess_info *sub_info)
* the real error code is already in sub_info->retval or
* sub_info->retval is 0 anyway, so don't mess with it then.
*/
- if (ret)
- sub_info->retval = ret;
+ if (KWIFEXITED(ret))
+ sub_info->retval = KWEXITSTATUS(ret);
}
/* Restore default kernel sig handler */
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index ba55851fe132..bdd94b45396b 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -133,14 +133,8 @@ static int br_stp_call_user(struct net_bridge *br, char
*arg)
/* call userspace STP and report program errors */
rc = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
- if (rc > 0) {
- if (rc & 0xff)
- br_debug(br, BR_STP_PROG " received signal %d\n",
- rc & 0x7f);
- else
- br_debug(br, BR_STP_PROG " exited with code %d\n",
- (rc >> 8) & 0xff);
- }
+ if (rc != 0)
+ br_debug(br, BR_STP_PROG " failed with exit code %d\n", rc);
return rc;
}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index e1b9f1a80676..ff462f3d46ca 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -193,7 +193,7 @@ static int call_sbin_request_key(struct key *authkey, void
*aux)
ret = call_usermodehelper_keys(request_key, argv, envp, keyring,
UMH_WAIT_PROC);
kdebug("usermode -> 0x%x", ret);
- if (ret >= 0) {
+ if (ret != 0) {
/* ret is the exit/wait code */
if (test_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags) ||
key_validate(key) < 0)
--
2.26.2
Luis R. Rodriguez
2020-Jun-10 15:49 UTC
[Bridge] [PATCH 5/5] selftests: simplify kmod failure value
From: Luis Chamberlain <mcgrof at kernel.org>
The "odd" 256 value was just an issue with the umh never
wrapping it around with WEXITSTATUS() for us. Now that it
does that, we can use a sane value / name for the selftest,
and this is no longer a oddity.
We add a way to detect this for older kernels, and support
the old return value for kernel code where it was given.
This never affected userspace.
Reported-by: Tiezhu Yang <yangtiezhu at loongson.cn>
Signed-off-by: Luis Chamberlain <mcgrof at kernel.org>
---
tools/testing/selftests/kmod/kmod.sh | 46 +++++++++++++++++++++++-----
1 file changed, 39 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kmod/kmod.sh
b/tools/testing/selftests/kmod/kmod.sh
index da60c3bd4f23..df7b21d8561c 100755
--- a/tools/testing/selftests/kmod/kmod.sh
+++ b/tools/testing/selftests/kmod/kmod.sh
@@ -64,6 +64,8 @@ ALL_TESTS="$ALL_TESTS 0009:150:1"
ALL_TESTS="$ALL_TESTS 0010:1:1"
ALL_TESTS="$ALL_TESTS 0011:1:1"
+MODULE_NOT_FOUND="FAILURE"
+
# Kselftest framework requirement - SKIP code is 4.
ksft_skip=4
@@ -155,14 +157,19 @@ test_finish()
echo "Test completed"
}
+# OLD_FAILURE is just because the old kernel umh never wrapped
+# the error with WEXITSTATUS(). Now that it does it, we get the
+# appropriate actual value from userspace observed in-kernel.
+
+# We keep the old mapping to ensure this script keeps working
+# with older kernels.
errno_name_to_val()
{
case "$1" in
- # kmod calls modprobe and upon of a module not found
- # modprobe returns just 1... However in the kernel we
- # *sometimes* see 256...
- MODULE_NOT_FOUND)
+ OLD_FAILURE)
echo 256;;
+ FAILURE)
+ echo 1;;
SUCCESS)
echo 0;;
-EPERM)
@@ -181,7 +188,9 @@ errno_name_to_val()
errno_val_to_name()
case "$1" in
256)
- echo MODULE_NOT_FOUND;;
+ echo OLD_FAILURE;;
+ 1)
+ echo FAILURE;;
0)
echo SUCCESS;;
-1)
@@ -335,6 +344,28 @@ kmod_defaults_fs()
config_set_test_case_fs
}
+check_umh()
+{
+ NAME=''
+
+ kmod_defaults_driver
+ config_num_threads 1
+ printf '\0' >"$DIR"/config_test_driver
+ config_trigger ${FUNCNAME[0]}
+ RC=$(config_get_test_result)
+ if [[ "$RC" == "256" ]]; then
+ MODULE_NOT_FOUND="OLD_FAILURE"
+ echo "check_umh: you have and old umh which didn't wrap errors"
+ echo " with WEXITSTATUS(). This is OK!"
+ elif [[ "$RC" != "1" ]]; then
+ echo "check_umh: Unexpected return value with no modprobe argument:
$RC"
+ exit
+ else
+ echo "check_umh: You have a new umh which wraps erros with"
+ echo " WEXITSTATUS(). This is OK!"
+ fi
+}
+
kmod_test_0001_driver()
{
NAME='\000'
@@ -343,7 +374,7 @@ kmod_test_0001_driver()
config_num_threads 1
printf $NAME >"$DIR"/config_test_driver
config_trigger ${FUNCNAME[0]}
- config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
+ config_expect_result ${FUNCNAME[0]} $MODULE_NOT_FOUND
}
kmod_test_0001_fs()
@@ -371,7 +402,7 @@ kmod_test_0002_driver()
config_set_driver $NAME
config_num_threads 1
config_trigger ${FUNCNAME[0]}
- config_expect_result ${FUNCNAME[0]} MODULE_NOT_FOUND
+ config_expect_result ${FUNCNAME[0]} $MODULE_NOT_FOUND
}
kmod_test_0002_fs()
@@ -648,6 +679,7 @@ load_req_mod
MODPROBE=$(</proc/sys/kernel/modprobe)
trap "test_finish" EXIT
+check_umh
parse_args $@
exit 0
--
2.26.2
On Wed, 10 Jun 2020 15:49:18 +0000 "Luis R. Rodriguez" <mcgrof at kernel.org> wrote:> Tiezhu Yang had sent out a patch set with a slew of kmod selftest > fixes, and one patch which modified kmod to return 254 when a module > was not found. This opened up pandora's box about why that was being > used for and low and behold its because when UMH_WAIT_PROC is used > we call a kernel_wait4() call but have never unwrapped the error code. > The commit log for that fix details the rationale for the approach > taken. I'd appreciate some review on that, in particular nfs folks > as it seems a case was never really hit before. > > This goes boot tested, selftested with kmod, and 0-day gives its > build blessings.Any thoughts on which kernel version(s) need some/all of these fixes?> drivers/block/drbd/drbd_nl.c | 20 +++++------ > fs/nfsd/nfs4recover.c | 2 +- > include/linux/sched/task.h | 13 ++++++++ > kernel/kmod.c | 5 ++- > kernel/umh.c | 4 +-- > lib/test_kmod.c | 2 +- > net/bridge/br_stp_if.c | 10 ++---- > security/keys/request_key.c | 2 +- > tools/testing/selftests/kmod/kmod.sh | 50 +++++++++++++++++++++++-----I'm not really sure who takes kmod changes - I'll grab these unless someone shouts at me.