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@xxxxxxxxxx>
> Signed-off-by: Adrian Reber <areber@xxxxxxxxxx>
> ---
> 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
