[PATCH] selftests: add tests for clone3()

[PATCH] selftests: add tests for clone3()

[Adrian Reber]

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

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();
+}
-- 
2.23.0

Re: [PATCH] selftests: add tests for clone3()

[Christian Brauner]

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