| Message ID | 20191030112100.700087-1-areber@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: add tests for clone3() | expand |
On Wed, Oct 30, 2019 at 12:21:00PM +0100, Adrian Reber wrote: > This adds tests for clone3() with different values and sizes > of struct clone_args. > > This selftest was initially part of of the clone3() with PID selftest. > After that patch was almost merged Eugene sent out a couple of patches > to fix problems with these test. > > This commit now only contains the clone3() selftest after the LPC > decision to rework clone3() with PID to allow setting the PID in > multiple PID namespaces including all of Eugene's patches. > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > Signed-off-by: Adrian Reber <areber@redhat.com> > --- > MAINTAINERS | 1 + > tools/testing/selftests/Makefile | 1 + > tools/testing/selftests/clone3/.gitignore | 1 + > tools/testing/selftests/clone3/Makefile | 7 + > tools/testing/selftests/clone3/clone3.c | 195 ++++++++++++++++++++++ > 5 files changed, 205 insertions(+) > create mode 100644 tools/testing/selftests/clone3/.gitignore > create mode 100644 tools/testing/selftests/clone3/Makefile > create mode 100644 tools/testing/selftests/clone3/clone3.c Thanks for the patch. A few comments below. > > diff --git a/MAINTAINERS b/MAINTAINERS > index c6c34d04ce95..d9426de8fb38 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12830,6 +12830,7 @@ S: Maintained > T: git git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git > F: samples/pidfd/ > F: tools/testing/selftests/pidfd/ > +F: tools/testing/selftests/clone3/ > K: (?i)pidfd > K: (?i)clone3 > K: \b(clone_args|kernel_clone_args)\b > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile > index 4cdbae6f4e61..ad442364218a 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 > diff --git a/tools/testing/selftests/clone3/.gitignore b/tools/testing/selftests/clone3/.gitignore > new file mode 100644 > index 000000000000..85d9d3ba2524 > --- /dev/null > +++ b/tools/testing/selftests/clone3/.gitignore > @@ -0,0 +1 @@ > +clone3 > diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile > new file mode 100644 > index 000000000000..ea922c014ae4 > --- /dev/null > +++ b/tools/testing/selftests/clone3/Makefile > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +CFLAGS += -I../../../../usr/include/ > + > +TEST_GEN_PROGS := clone3 > + > +include ../lib.mk > diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c > new file mode 100644 > index 000000000000..a5a1ed91e550 > --- /dev/null > +++ b/tools/testing/selftests/clone3/clone3.c > @@ -0,0 +1,195 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* Based on Christian Brauner's clone3() example */ > + > +#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> > +#include <sys/types.h> > +#include <sys/un.h> > +#include <sys/wait.h> > +#include <unistd.h> > +#include <sched.h> > + > +#include "../kselftest.h" > + > +/* > + * Different sizes of struct clone_args > + */ > +#define CLONE3_ARGS_SIZE_V0 64 This is defined in include/uapi/sched.h and so should probably be protected by an ifdef: #ifndef CLONE3_ARGS_SIZE_V0 #define CLONE3_ARGS_SIZE_V0 64 #endif > + > +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) > +{ > + return syscall(__NR_clone3, args, size); > +} > + > +static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode) > +{ > + struct clone_args args = {0}; Nit: If you want to, you could do struct clone_args args = { .flags = flags, .exit_signal = SIGCHLD, }; The rest of the structure (includ padding iirc) will then be zeroed. > + pid_t ppid = -1; > + pid_t pid = -1; > + int status; > + > + args.flags = flags; > + args.exit_signal = SIGCHLD; > + > + if (size == 0) > + size = sizeof(struct clone_args); > + > + switch (test_mode) { > + case CLONE3_ARGS_ALL_0: > + args.flags = 0; > + args.pidfd = 0; > + args.child_tid = 0; > + args.parent_tid = 0; > + args.exit_signal = 0; > + args.stack = 0; > + args. stack_size = 0; > + args.tls = 0; Hm, the explicit zeroing for _all_ fields here is not needed, right? Would probably make sense to just do: case CLONE3_ARGS_ALL_0: args.flags = 0; args.exit_signal = 0; break; since the rest will have been zeroed when you initialized the struct above. > + break; Nit: Can you please remove the \n between the break and case keywords. > + > + case CLONE3_ARGS_ALL_1: > + args.flags = 1; > + args.pidfd = 1; > + args.child_tid = 1; > + args.parent_tid = 1; > + args.exit_signal = 1; > + args.stack = 1; > + args. stack_size = 1; > + args.tls = 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); > + if (pid < 0) { > + ksft_print_msg("%s - Failed to create new process\n", > + strerror(errno)); > + return -errno; > + } > + > + if (pid == 0) { > + ksft_print_msg("I am the child, my PID is %d\n", getpid()); > + _exit(EXIT_SUCCESS); > + } > + > + ppid = getpid(); Nit: You don't need the additional ppid variable since you're only using it once. You an just call getpid() and be done I think. > + ksft_print_msg("I am the parent (%d). My child's pid is %d\n", > + ppid, pid); > + > + (void)wait(&status); > + if (WEXITSTATUS(status)) > + return WEXITSTATUS(status); This should probably be at least: if (wait(&status) < 0) return <some-errno-value-you-find-reasonable>; if (WEXITSTATUS(status)) return WEXITSTATUS(status); otherwise status is not meaningful. > + > + return 0; > +} > + > +static void 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 %#" 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", > + getpid(), ret, expected); > + if (ret != expected) > + ksft_test_result_fail( > + "[%d] Result (%d) is different than expected (%d)\n", > + getpid(), ret, expected); > + else > + ksft_test_result_pass( > + "[%d] Result (%d) matches expectation (%d)\n", > + getpid(), ret, expected); > +} > +int main(int argc, char *argv[]) > +{ > + pid_t pid; > + > + ksft_print_header(); > + ksft_set_plan(17); > + > + /* Just a simple clone3() should return 0.*/ > + test_clone3(0, 0, 0, CLONE3_ARGS_NO_TEST); Nit: Can you place \n between all of those? I think that makes it more readable here because all of the tests have comments. > + /* Do a clone3() in a new PID NS.*/ > + test_clone3(CLONE_NEWPID, 0, 0, CLONE3_ARGS_NO_TEST); > + /* Do a clone3() with CLONE3_ARGS_SIZE_V0. */ > + test_clone3(0, CLONE3_ARGS_SIZE_V0, 0, CLONE3_ARGS_NO_TEST); > + /* Do a clone3() with CLONE3_ARGS_SIZE_V0 - 8 */ > + test_clone3(0, CLONE3_ARGS_SIZE_V0 - 8, -EINVAL, CLONE3_ARGS_NO_TEST); > + /* Do a clone3() with sizeof(struct clone_args) + 8 */ > + test_clone3(0, sizeof(struct clone_args) + 8, 0, CLONE3_ARGS_NO_TEST); > + /* Do a clone3() with all members set to 1 */ > + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, CLONE3_ARGS_ALL_1); > + /* Do a clone3() with exit_signal having highest 32 bits non-zero */ > + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, > + CLONE3_ARGS_INVAL_EXIT_SIGNAL_BIG); > + /* Do a clone3() with negative 32-bit exit_signal */ > + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, > + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG); > + /* Do a clone3() with exit_signal not fitting into CSIGNAL mask */ > + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, > + CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG); > + /* Do a clone3() with NSIG < exit_signal < CSIG */ > + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, > + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG); > + /* > + * Do a clone3() with sizeof(struct clone_args) + 8 > + * and all members set to 0. > + */ > + test_clone3(0, sizeof(struct clone_args) + 8, 0, CLONE3_ARGS_ALL_0); > + /* > + * Do a clone3() with sizeof(struct clone_args) + 8 > + * and all members set to 0. > + */ > + test_clone3(0, sizeof(struct clone_args) + 8, -EINVAL, > + CLONE3_ARGS_ALL_1); Hm... All of those size checks do not change the actual struct size, right? Only the size _argument_. Wouldn't it make more sense to also perform size tests where you e.g. do struct clone_args_extended { struct clone_args args; __aligned_u64 execess_space[2]; }; and then you can perform tests such as: struct clone_args_extended args_ext; memset(&args_exit, 0, sizeof(args_exit)); args_ext.excess_space[1] = 1; pid = raw_clone(&args_ext.args, sizeof(clone_args_extended)); Christian
diff --git a/MAINTAINERS b/MAINTAINERS index c6c34d04ce95..d9426de8fb38 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12830,6 +12830,7 @@ S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git F: samples/pidfd/ F: tools/testing/selftests/pidfd/ +F: tools/testing/selftests/clone3/ K: (?i)pidfd K: (?i)clone3 K: \b(clone_args|kernel_clone_args)\b diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 4cdbae6f4e61..ad442364218a 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 diff --git a/tools/testing/selftests/clone3/.gitignore b/tools/testing/selftests/clone3/.gitignore new file mode 100644 index 000000000000..85d9d3ba2524 --- /dev/null +++ b/tools/testing/selftests/clone3/.gitignore @@ -0,0 +1 @@ +clone3 diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile new file mode 100644 index 000000000000..ea922c014ae4 --- /dev/null +++ b/tools/testing/selftests/clone3/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0 + +CFLAGS += -I../../../../usr/include/ + +TEST_GEN_PROGS := clone3 + +include ../lib.mk diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c new file mode 100644 index 000000000000..a5a1ed91e550 --- /dev/null +++ b/tools/testing/selftests/clone3/clone3.c @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* Based on Christian Brauner's clone3() example */ + +#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> +#include <sys/types.h> +#include <sys/un.h> +#include <sys/wait.h> +#include <unistd.h> +#include <sched.h> + +#include "../kselftest.h" + +/* + * Different sizes of struct clone_args + */ +#define CLONE3_ARGS_SIZE_V0 64 + +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) +{ + return syscall(__NR_clone3, args, size); +} + +static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode) +{ + struct clone_args args = {0}; + pid_t ppid = -1; + pid_t pid = -1; + int status; + + args.flags = flags; + args.exit_signal = SIGCHLD; + + if (size == 0) + size = sizeof(struct clone_args); + + switch (test_mode) { + case CLONE3_ARGS_ALL_0: + args.flags = 0; + args.pidfd = 0; + args.child_tid = 0; + args.parent_tid = 0; + args.exit_signal = 0; + args.stack = 0; + args. stack_size = 0; + args.tls = 0; + break; + + case CLONE3_ARGS_ALL_1: + args.flags = 1; + args.pidfd = 1; + args.child_tid = 1; + args.parent_tid = 1; + args.exit_signal = 1; + args.stack = 1; + args. stack_size = 1; + args.tls = 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); + if (pid < 0) { + ksft_print_msg("%s - Failed to create new process\n", + strerror(errno)); + return -errno; + } + + if (pid == 0) { + ksft_print_msg("I am the child, my PID is %d\n", getpid()); + _exit(EXIT_SUCCESS); + } + + ppid = getpid(); + ksft_print_msg("I am the parent (%d). My child's pid is %d\n", + ppid, pid); + + (void)wait(&status); + if (WEXITSTATUS(status)) + return WEXITSTATUS(status); + + return 0; +} + +static void 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 %#" 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", + getpid(), ret, expected); + if (ret != expected) + ksft_test_result_fail( + "[%d] Result (%d) is different than expected (%d)\n", + getpid(), ret, expected); + else + ksft_test_result_pass( + "[%d] Result (%d) matches expectation (%d)\n", + getpid(), ret, expected); +} +int main(int argc, char *argv[]) +{ + pid_t pid; + + ksft_print_header(); + ksft_set_plan(17); + + /* Just a simple clone3() should return 0.*/ + test_clone3(0, 0, 0, CLONE3_ARGS_NO_TEST); + /* Do a clone3() in a new PID NS.*/ + test_clone3(CLONE_NEWPID, 0, 0, CLONE3_ARGS_NO_TEST); + /* Do a clone3() with CLONE3_ARGS_SIZE_V0. */ + test_clone3(0, CLONE3_ARGS_SIZE_V0, 0, CLONE3_ARGS_NO_TEST); + /* Do a clone3() with CLONE3_ARGS_SIZE_V0 - 8 */ + test_clone3(0, CLONE3_ARGS_SIZE_V0 - 8, -EINVAL, CLONE3_ARGS_NO_TEST); + /* Do a clone3() with sizeof(struct clone_args) + 8 */ + test_clone3(0, sizeof(struct clone_args) + 8, 0, CLONE3_ARGS_NO_TEST); + /* Do a clone3() with all members set to 1 */ + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, CLONE3_ARGS_ALL_1); + /* Do a clone3() with exit_signal having highest 32 bits non-zero */ + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_BIG); + /* Do a clone3() with negative 32-bit exit_signal */ + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG); + /* Do a clone3() with exit_signal not fitting into CSIGNAL mask */ + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG); + /* Do a clone3() with NSIG < exit_signal < CSIG */ + test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, + CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG); + /* + * Do a clone3() with sizeof(struct clone_args) + 8 + * and all members set to 0. + */ + test_clone3(0, sizeof(struct clone_args) + 8, 0, CLONE3_ARGS_ALL_0); + /* + * Do a clone3() with sizeof(struct clone_args) + 8 + * and all members set to 0. + */ + test_clone3(0, sizeof(struct clone_args) + 8, -EINVAL, + CLONE3_ARGS_ALL_1); + /* Do a clone3() with > page size */ + test_clone3(0, getpagesize() + 8, -E2BIG, CLONE3_ARGS_NO_TEST); + /* Do a clone3() with CLONE3_ARGS_SIZE_V0 in a new PID NS. */ + test_clone3(CLONE_NEWPID, CLONE3_ARGS_SIZE_V0, 0, CLONE3_ARGS_NO_TEST); + /* Do a clone3() with CLONE3_ARGS_SIZE_V0 - 8 in a new PID NS */ + test_clone3(CLONE_NEWPID, CLONE3_ARGS_SIZE_V0 - 8, -EINVAL, + CLONE3_ARGS_NO_TEST); + /* Do a clone3() with sizeof(struct clone_args) + 8 in a new PID NS */ + test_clone3(CLONE_NEWPID, sizeof(struct clone_args) + 8, 0, + CLONE3_ARGS_NO_TEST); + /* Do a clone3() with > page size in a new PID NS */ + test_clone3(CLONE_NEWPID, getpagesize() + 8, -E2BIG, + CLONE3_ARGS_NO_TEST); + + return !ksft_get_fail_cnt() ? ksft_exit_pass() : ksft_exit_fail(); +}