2019-11-08 19:24:58

by Adrian Reber

[permalink] [raw]
Subject: [PATCH v3] selftests: add tests for clone3()

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 <[email protected]>
Signed-off-by: Adrian Reber <[email protected]>
---
v2:
- Applied Christian's suggestions
- Skip root-only tests when running as non-root
v3:
- Removed unnecessary test case (everything set to 1)
- Correctly handle children without SIGCHLD
---
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 | 203 ++++++++++++++++++++++
5 files changed, 213 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 cba1095547fd..0040b7a6410b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12829,6 +12829,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..ca9ac31abbe6
--- /dev/null
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -0,0 +1,203 @@
+// 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
+ */
+#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_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 = {
+ .flags = flags,
+ .exit_signal = SIGCHLD,
+ };
+
+ struct clone_args_extended {
+ struct clone_args args;
+ __aligned_u64 excess_space[2];
+ } args_ext;
+
+ pid_t pid = -1;
+ int status;
+
+ memset(&args_ext, 0, sizeof(args_ext));
+ if (size > sizeof(struct clone_args))
+ args_ext.excess_space[1] = 1;
+
+ if (size == 0)
+ size = sizeof(struct clone_args);
+
+ switch (test_mode) {
+ case CLONE3_ARGS_ALL_0:
+ args.flags = 0;
+ args.exit_signal = 0;
+ 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;
+ }
+
+ memcpy(&args_ext.args, &args, sizeof(struct clone_args));
+
+ pid = raw_clone((struct clone_args *)&args_ext, 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);
+ }
+
+ ksft_print_msg("I am the parent (%d). My child's pid is %d\n",
+ getpid(), pid);
+
+ if (waitpid(-1, &status, __WALL) < 0) {
+ ksft_print_msg("Child returned %s\n", strerror(errno));
+ return -errno;
+ }
+ 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;
+
+ uid_t uid = getuid();
+
+ ksft_print_header();
+ ksft_set_plan(15);
+
+ /* Just a simple clone3() should return 0.*/
+ test_clone3(0, 0, 0, CLONE3_ARGS_NO_TEST);
+
+ /* Do a clone3() in a new PID NS.*/
+ if (uid == 0)
+ test_clone3(CLONE_NEWPID, 0, 0, CLONE3_ARGS_NO_TEST);
+ else
+ ksft_test_result_skip("Skipping clone3() with CLONE_NEWPID\n");
+
+ /* 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 exit_signal having highest 32 bits non-zero */
+ test_clone3(0, 0, -EINVAL, CLONE3_ARGS_INVAL_EXIT_SIGNAL_BIG);
+
+ /* Do a clone3() with negative 32-bit exit_signal */
+ test_clone3(0, 0, -EINVAL, CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG);
+
+ /* Do a clone3() with exit_signal not fitting into CSIGNAL mask */
+ test_clone3(0, 0, -EINVAL, CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG);
+
+ /* Do a clone3() with NSIG < exit_signal < CSIG */
+ test_clone3(0, 0, -EINVAL, CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG);
+
+ /*
+ * Do a clone3() with sizeof(struct clone_args) + 8
+ * and all members set to 0. This resets exit_signal and wait()
+ * will not get a result.
+ */
+ test_clone3(0, sizeof(struct clone_args) + 8, 0, CLONE3_ARGS_ALL_0);
+
+ /* 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. */
+ if (uid == 0)
+ test_clone3(CLONE_NEWPID, CLONE3_ARGS_SIZE_V0, 0,
+ CLONE3_ARGS_NO_TEST);
+ else
+ ksft_test_result_skip("Skipping clone3() with CLONE_NEWPID\n");
+
+ /* 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 */
+ if (uid == 0)
+ test_clone3(CLONE_NEWPID, sizeof(struct clone_args) + 8, 0,
+ CLONE3_ARGS_NO_TEST);
+ else
+ ksft_test_result_skip("Skipping clone3() with CLONE_NEWPID\n");
+
+ /* 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


2019-11-11 13:10:16

by Christian Brauner

[permalink] [raw]
Subject: Re: [PATCH v3] selftests: add tests for clone3()

On Fri, Nov 08, 2019 at 07:56:29PM +0100, Adrian Reber wrote:
> +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[])

Nit: missing \n between } and int main().
I'll just fix that up myself.

> + /*
> + * Do a clone3() with sizeof(struct clone_args) + 8
> + * and all members set to 0. This resets exit_signal and wait()
> + * will not get a result.

That comment is not true and now also misleading since you now pass
_WALL to waitpid() above. I'll just remove it when applying.

> + */
> + test_clone3(0, sizeof(struct clone_args) + 8, 0, CLONE3_ARGS_ALL_0);
> +
> + /* 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. */
> + if (uid == 0)
> + test_clone3(CLONE_NEWPID, CLONE3_ARGS_SIZE_V0, 0,
> + CLONE3_ARGS_NO_TEST);
> + else
> + ksft_test_result_skip("Skipping clone3() with CLONE_NEWPID\n");
> +
> + /* 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 */
> + if (uid == 0)
> + test_clone3(CLONE_NEWPID, sizeof(struct clone_args) + 8, 0,
> + CLONE3_ARGS_NO_TEST);

So there's a missing test condition here, no? I've just realized you're
passing in sizeof(struct clone_args) + 8 which hits the first excess
space 64 bit value which is 0. That's good and the reason why this test
passes.
But I don't see any test for sizoef(struct clone_args_extended) or at
least sizeof(struct clone_args) + 16 such that you actually hit the
second 64 bit integer which is initialized to 1 and thus clone3() should
fail with -E2BIG.If I haven't overlooked this test, can you please add
it? It's quite important since it's a different codepath than the
sizeof(PAGE_SIZE) + 8 codepath.

Thanks!
Christian