Hello.
This patch set updates clone3 selftest in several aspects: - adding checks for exit_signal invalid values handling; - adding clone3 to selftests targets; - enabling clone3 tests on all architectures; - minor cleanups of the clone3 test.
Applied on top of brauer/linux.git/for-next.
Changes since v1[1]: - exit_signal check extended to cover more cases of invalid exit_signal value.
[1] https://lkml.org/lkml/2019/9/10/416
Eugene Syromiatnikov (6): selftests/clone3: convert test modes into an enum selftests/clone3: add a check for invalid exit_signal selftests/clone3: use uint64_t for flags parameter selftests/clone3: fix up format strings selftests/clone3: enable clone3 self-tests on all architectures selftests: add clone3 to TARGETS
tools/testing/selftests/Makefile | 1 + tools/testing/selftests/clone3/Makefile | 4 +-- tools/testing/selftests/clone3/clone3.c | 64 ++++++++++++++++++++++++++++----- 3 files changed, 57 insertions(+), 12 deletions(-)
* tools/testing/selftests/clone3/clone3.c (CLONE3_ARGS_NO_TEST, CLONE3_ARGS_ALL_0, CLONE3_ARGS_ALL_1): Change into an enum. (call_clone3): Change test_mode parameter type to enum test_mode; use switch statement for actions that dependent on test_mode selection. (test_clone3): Change test_mode parameter type to enum test_mode.
Signed-off-by: Eugene Syromiatnikov esyr@redhat.com --- tools/testing/selftests/clone3/clone3.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index a0f1989..7b65ee5 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -24,16 +24,18 @@ /* V1 includes set_tid */ #define CLONE3_ARGS_SIZE_V1 72
-#define CLONE3_ARGS_NO_TEST 0 -#define CLONE3_ARGS_ALL_0 1 -#define CLONE3_ARGS_ALL_1 2 +enum test_mode { + CLONE3_ARGS_NO_TEST, + CLONE3_ARGS_ALL_0, + CLONE3_ARGS_ALL_1, +};
static pid_t raw_clone(struct clone_args *args, size_t size) { return syscall(__NR_clone3, args, size); }
-static int call_clone3(int flags, size_t size, int test_mode) +static int call_clone3(int flags, size_t size, enum test_mode test_mode) { struct clone_args args = {0}; pid_t ppid = -1; @@ -46,7 +48,8 @@ static int call_clone3(int flags, size_t size, int test_mode) if (size == 0) size = sizeof(struct clone_args);
- if (test_mode == CLONE3_ARGS_ALL_0) { + switch (test_mode) { + case CLONE3_ARGS_ALL_0: args.flags = 0; args.pidfd = 0; args.child_tid = 0; @@ -56,7 +59,9 @@ static int call_clone3(int flags, size_t size, int test_mode) args. stack_size = 0; args.tls = 0; args.set_tid = 0; - } else if (test_mode == CLONE3_ARGS_ALL_1) { + break; + + case CLONE3_ARGS_ALL_1: args.flags = 1; args.pidfd = 1; args.child_tid = 1; @@ -66,6 +71,7 @@ static int call_clone3(int flags, size_t size, int test_mode) args. stack_size = 1; args.tls = 1; args.set_tid = 1; + break; }
pid = raw_clone(&args, size); @@ -91,7 +97,8 @@ static int call_clone3(int flags, size_t size, int test_mode) return 0; }
-static int test_clone3(int flags, size_t size, int expected, int test_mode) +static int test_clone3(int flags, size_t size, int expected, + enum test_mode test_mode) { int ret;
Check that the kernel fails calls with exit_signal with non-zero highest 32 bits, negative 32-bit exit_signal, and not failing on passing invalid exit_signal withing CSIGNAL mask, like legacy clone syscalls do.
* tools/testing/selftests/clone3/clone3.c (enum test_mode): Add CLONE3_ARGS_INVAL_EXIT_SIGNAL_BIG, CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG, CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG, CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG. (call_clone3): Add args.exit_signal initialisation in case test_mode is equal to one of the added enum test_mode values. (main): Add test_clone3 clone check with test_mode equal to the added enum test_mode values.
Signed-off-by: Eugene Syromiatnikov esyr@redhat.com --- tools/testing/selftests/clone3/clone3.c | 36 +++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 7b65ee5..318189f9 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -28,6 +28,10 @@ enum test_mode { CLONE3_ARGS_NO_TEST, CLONE3_ARGS_ALL_0, CLONE3_ARGS_ALL_1, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_BIG, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG, };
static pid_t raw_clone(struct clone_args *args, size_t size) @@ -72,6 +76,22 @@ static int call_clone3(int flags, size_t size, enum test_mode test_mode) args.tls = 1; args.set_tid = 1; break; + + case CLONE3_ARGS_INVAL_EXIT_SIGNAL_BIG: + args.exit_signal = 0xbadc0ded00000000ULL; + break; + + case CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG: + args.exit_signal = 0x0000000080000000ULL; + break; + + case CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG: + args.exit_signal = 0x0000000000000100ULL; + break; + + case CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG: + args.exit_signal = 0x00000000000000f0ULL; + break; }
pid = raw_clone(&args, size); @@ -146,6 +166,22 @@ int main(int argc, char *argv[]) /* Do a clone3() with all members set to 1 */ if (test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, CLONE3_ARGS_ALL_1)) goto on_error; + /* Do a clone3() with exit_signal having highest 32 bits non-zero */ + if (test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_BIG)) + goto on_error; + /* Do a clone3() with negative 32-bit exit_signal */ + if (test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG)) + goto on_error; + /* Do a clone3() with exit_signal not fitting into CSIGNAL mask */ + if (test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG)) + goto on_error; + /* Do a clone3() with NSIG < exit_signal < CSIG */ + if (test_clone3(0, CLONE3_ARGS_SIZE_V0, 0, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG)) + goto on_error; /* * Do a clone3() with sizeof(struct clone_args) + 8 * and all members set to 0.
Flags parameter in both userspace and kernel clone args is 64-bit wide, there's little reason to have it signed and 32-bit in tests.
* tools/testing/selftests/clone3/clone3.c: Include <inttypes.h> and <stdint.h>. (call_clone3): Change flags parameter type from int to uint64_t. (test_clone3): Change flags parameter type from int to uint64_t; change the format string that prints it accordingly.
Signed-off-by: Eugene Syromiatnikov esyr@redhat.com --- tools/testing/selftests/clone3/clone3.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 318189f9..613544b 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -4,8 +4,10 @@
#define _GNU_SOURCE #include <errno.h> +#include <inttypes.h> #include <linux/types.h> #include <linux/sched.h> +#include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <sys/syscall.h> @@ -39,7 +41,7 @@ static pid_t raw_clone(struct clone_args *args, size_t size) return syscall(__NR_clone3, args, size); }
-static int call_clone3(int flags, size_t size, enum test_mode test_mode) +static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode) { struct clone_args args = {0}; pid_t ppid = -1; @@ -117,12 +119,13 @@ static int call_clone3(int flags, size_t size, enum test_mode test_mode) return 0; }
-static int test_clone3(int flags, size_t size, int expected, +static int test_clone3(uint64_t flags, size_t size, int expected, enum test_mode test_mode) { int ret;
- ksft_print_msg("[%d] Trying clone3() with flags 0x%x (size %d)\n", + ksft_print_msg("[%d] Trying clone3() with flags %#" PRIx64 " (size %d)" + "\n", getpid(), flags, size); ret = call_clone3(flags, size, test_mode); ksft_print_msg("[%d] clone3() with flags says :%d expected %d\n",
* tools/testing/selftests/clone3/clone3.c (test_clone3): Change format qualifier for printing size field from %d to %zu; place colon right after the word "says".
Signed-off-by: Eugene Syromiatnikov esyr@redhat.com --- tools/testing/selftests/clone3/clone3.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 613544b..eb705f6 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -124,11 +124,11 @@ static int test_clone3(uint64_t flags, size_t size, int expected, { int ret;
- ksft_print_msg("[%d] Trying clone3() with flags %#" PRIx64 " (size %d)" + ksft_print_msg("[%d] Trying clone3() with flags %#" PRIx64 " (size %zu)" "\n", getpid(), flags, size); ret = call_clone3(flags, size, test_mode); - ksft_print_msg("[%d] clone3() with flags says :%d expected %d\n", + ksft_print_msg("[%d] clone3() with flags says: %d expected %d\n", getpid(), ret, expected); if (ret != expected) ksft_exit_fail_msg(
clone3() is available on most architectures, so there's no reason to restrict the respective self-tests to x86_64.
* tools/testing/selftests/clone3/Makefile (TEST_GEN_PROGS): Set always, not only ifeq ($(ARCH),x86_64).
Signed-off-by: Eugene Syromiatnikov esyr@redhat.com --- tools/testing/selftests/clone3/Makefile | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile index 4efcf45..faa069c 100644 --- a/tools/testing/selftests/clone3/Makefile +++ b/tools/testing/selftests/clone3/Makefile @@ -4,8 +4,6 @@ ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/)
CFLAGS += -I../../../../usr/include/
-ifeq ($(ARCH),x86_64) - TEST_GEN_PROGS := clone3 clone3_set_tid -endif +TEST_GEN_PROGS := clone3 clone3_set_tid
include ../lib.mk
* tools/testing/selftests/Makefile (TARGETS): Add clone3.
Signed-off-by: Eugene Syromiatnikov esyr@redhat.com --- tools/testing/selftests/Makefile | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c..05163e4 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -4,6 +4,7 @@ TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities TARGETS += cgroup +TARGETS += clone3 TARGETS += cpufreq TARGETS += cpu-hotplug TARGETS += drivers/dma-buf
On Tue, Sep 10, 2019 at 07:01:30PM +0100, Eugene Syromiatnikov wrote:
Hello.
This patch set updates clone3 selftest in several aspects:
- adding checks for exit_signal invalid values handling;
- adding clone3 to selftests targets;
- enabling clone3 tests on all architectures;
- minor cleanups of the clone3 test.
Applied on top of brauer/linux.git/for-next.
So I like this series a lot! Testing is very important. And thanks for catching the clone3() exit_signal problem. This way we got to release a non-broken kernel. :)
Some notes: I dropped the set_tid extension from the core process updates for 5.4 because we ended up in a discussion that made it clear we potentially need the ability to restore pids in multiple pid namespaces. This means we need some more discussion and the patchset is delayed for at least one release. Unfortunately, this also means the test that you have based yours upon does not exist anymore. However, the tests should not be blocked on this. I'd encourage you to talk to Adrian (who is Cced here anyway) and come up with a clone3() test suite I can merge. You can very likely do a Co-Developed-by so no-ones work gets dropped. :)
Ideally I'd like to see: - verifying passing different struct sizes works correctly - verify that flag combinations work correctly - verify that struct members have correct values etc. pp.
We definitely want the exit_signal test as a regression test so it doesn't bite us again!
(Oh, please also add tool/test/selftests/clone3/ to the pidfd/core process MAINTAINERS entry.)
Thanks! Christian
On Mon, Sep 16, 2019 at 09:49:34AM +0200, Christian Brauner wrote:
On Tue, Sep 10, 2019 at 07:01:30PM +0100, Eugene Syromiatnikov wrote:
Hello.
This patch set updates clone3 selftest in several aspects:
- adding checks for exit_signal invalid values handling;
- adding clone3 to selftests targets;
- enabling clone3 tests on all architectures;
- minor cleanups of the clone3 test.
Applied on top of brauer/linux.git/for-next.
So I like this series a lot! Testing is very important. And thanks for catching the clone3() exit_signal problem. This way we got to release a non-broken kernel. :)
Some notes: I dropped the set_tid extension from the core process updates for 5.4 because we ended up in a discussion that made it clear we potentially need the ability to restore pids in multiple pid namespaces. This means we need some more discussion and the patchset is delayed for at least one release. Unfortunately, this also means the test that you have based yours upon does not exist anymore. However, the tests should not be blocked on this. I'd encourage you to talk to Adrian (who is Cced here anyway) and come up with a clone3() test suite I can merge. You can very likely do a Co-Developed-by so no-ones work gets dropped. :)
Ideally I'd like to see:
- verifying passing different struct sizes works correctly
- verify that flag combinations work correctly
- verify that struct members have correct values etc. pp.
We definitely want the exit_signal test as a regression test so it doesn't bite us again!
(Oh, please also add tool/test/selftests/clone3/ to the pidfd/core process MAINTAINERS entry.)
Eugene and I have already discussed this. We will resubmit the clone3() selftests in the next few days with all our changes combined.
Adrian
On Mon, Sep 16, 2019 at 10:12:47AM +0200, Adrian Reber wrote:
On Mon, Sep 16, 2019 at 09:49:34AM +0200, Christian Brauner wrote:
On Tue, Sep 10, 2019 at 07:01:30PM +0100, Eugene Syromiatnikov wrote:
Hello.
This patch set updates clone3 selftest in several aspects:
- adding checks for exit_signal invalid values handling;
- adding clone3 to selftests targets;
- enabling clone3 tests on all architectures;
- minor cleanups of the clone3 test.
Applied on top of brauer/linux.git/for-next.
So I like this series a lot! Testing is very important. And thanks for catching the clone3() exit_signal problem. This way we got to release a non-broken kernel. :)
Some notes: I dropped the set_tid extension from the core process updates for 5.4 because we ended up in a discussion that made it clear we potentially need the ability to restore pids in multiple pid namespaces. This means we need some more discussion and the patchset is delayed for at least one release. Unfortunately, this also means the test that you have based yours upon does not exist anymore. However, the tests should not be blocked on this. I'd encourage you to talk to Adrian (who is Cced here anyway) and come up with a clone3() test suite I can merge. You can very likely do a Co-Developed-by so no-ones work gets dropped. :)
Ideally I'd like to see:
- verifying passing different struct sizes works correctly
- verify that flag combinations work correctly
- verify that struct members have correct values etc. pp.
We definitely want the exit_signal test as a regression test so it doesn't bite us again!
(Oh, please also add tool/test/selftests/clone3/ to the pidfd/core process MAINTAINERS entry.)
Eugene and I have already discussed this. We will resubmit the clone3() selftests in the next few days with all our changes combined.
Excellent! Very happy to hear this! :) Christian
linux-kselftest-mirror@lists.linaro.org
-
Adrian Reber -
Christian Brauner -
Eugene Syromiatnikov