Re: [PATCH v11 2/2] selftests: add tests for clone3() with *set_tid

From: Andrei Vagin <avagin@gmail.com>
To: Adrian Reber <areber@redhat.com>
Cc: Christian Brauner <christian.brauner@ubuntu.com>,
	Eric Biederman <ebiederm@xmission.com>,
	Pavel Emelyanov <ovzxemul@gmail.com>,
	Jann Horn <jannh@google.com>, Oleg Nesterov <oleg@redhat.com>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	linux-kernel@vger.kernel.org, Mike Rapoport <rppt@linux.ibm.com>,
	Radostin Stoyanov <rstoyanov1@gmail.com>
Subject: Re: [PATCH v11 2/2] selftests: add tests for clone3() with *set_tid
Date: Fri, 15 Nov 2019 14:20:18 -0800	[thread overview]
Message-ID: <20191115222018.GB353836@gmail.com> (raw)
In-Reply-To: <20191115123621.142252-2-areber@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

On Fri, Nov 15, 2019 at 01:36:21PM +0100, Adrian Reber wrote:
> This tests clone3() with *set_tid to see if all desired PIDs are working
> as expected. The tests are trying multiple invalid input parameters as
> well as creating processes while specifying a certain PID in multiple
> PID namespaces at the same time.
> 
> Additionally this moves common clone3() test code into clone3_selftests.h.
> 
> Signed-off-by: Adrian Reber <areber@redhat.com>
> ---
> v9:
>  - applied all changes from Christian's review (except using the
>    NSpid: parsing code from selftests/pidfd/pidfd_fdinfo_test.c)
> 
> v10:
>  - added even more '\n' and include file fixes (Christian)
> 
> v11:
>  - added more return code checking at multiple places (Andrei)
>  - also add set_tid/set_tid_size to internal struct (Andrei)

I think we can add a test case to trigger the issue what I found in the
previous version of the kernel patch. You can find my version of this
test case in the attached patch.

nit: we need to flush stdout and stderr buffers before calling the raw
clone3 syscall and _exit(). Otherwise, some log messages can be lost and
some of them can be printed twice.

To trigger this issue, you can run the test and redirect its output to
file or pipe:

$ ./clone3_set_tid | cat

I have attached the patch to address both these problems. It is a draft
version and may require some work.

Adrian and Christian, it is up to you to decide whether we want to
update the current patch or to fix this on top by a separate patch.

Thanks,
Andrei


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2586 bytes --]

diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile
index cf976c732906..a7c7d8d15e1c 100644
--- a/tools/testing/selftests/clone3/Makefile
+++ b/tools/testing/selftests/clone3/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-CFLAGS += -g -I../../../../usr/include/
+CFLAGS += -Wall -g -I../../../../usr/include/
 
 TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid
 
diff --git a/tools/testing/selftests/clone3/clone3_set_tid.c b/tools/testing/selftests/clone3/clone3_set_tid.c
index 3480e1c46983..ab1df5ce201f 100644
--- a/tools/testing/selftests/clone3/clone3_set_tid.c
+++ b/tools/testing/selftests/clone3/clone3_set_tid.c
@@ -30,6 +30,12 @@
 static int pipe_1[2];
 static int pipe_2[2];
 
+static void flush()
+{
+	fflush(stdout);
+	fflush(stderr);
+}
+
 static int call_clone3_set_tid(pid_t *set_tid,
 			       size_t set_tid_size,
 			       int flags,
@@ -46,6 +52,7 @@ static int call_clone3_set_tid(pid_t *set_tid,
 		.set_tid_size = set_tid_size,
 	};
 
+	flush();
 	pid = sys_clone3(&args, sizeof(struct clone_args));
 	if (pid < 0) {
 		ksft_print_msg("%s - Failed to create new process\n",
@@ -83,6 +90,7 @@ static int call_clone3_set_tid(pid_t *set_tid,
 			close(pipe_2[0]);
 		}
 
+		flush();
 		if (set_tid[0] != getpid())
 			_exit(EXIT_FAILURE);
 		_exit(exit_code);
@@ -153,7 +161,7 @@ int main(int argc, char *argv[])
 		ksft_exit_fail_msg("pipe() failed\n");
 
 	ksft_print_header();
-	ksft_set_plan(27);
+	ksft_set_plan(29);
 
 	f = fopen("/proc/sys/kernel/pid_max", "r");
 	if (f == NULL)
@@ -249,6 +257,7 @@ int main(int argc, char *argv[])
 	pid = fork();
 	if (pid == 0) {
 		ksft_print_msg("Child has PID %d\n", getpid());
+		flush();
 		_exit(EXIT_SUCCESS);
 	}
 	if (waitpid(pid, &status, 0) < 0)
@@ -283,6 +292,19 @@ int main(int argc, char *argv[])
 	/* Let's create a PID 1 */
 	ns_pid = fork();
 	if (ns_pid == 0) {
+		set_tid[0] = 43;
+		set_tid[1] = -1;
+		/*
+		 * This should fail as there are not enough active PID
+		 * namespaces. Again assuming this is running in the host's
+		 * PID namespace. Not yet nested.
+		 */
+		test_clone3_set_tid(set_tid, 2, 0, -EINVAL, 0, 0);
+
+		set_tid[0] = 43;
+		set_tid[1] = pid;
+		test_clone3_set_tid(set_tid, 2, 0, 0, 43, 0);
+
 		ksft_print_msg("Child in PID namespace has PID %d\n", getpid());
 		set_tid[0] = 2;
 		test_clone3_set_tid(set_tid, 1, 0, 0, 2, 0);
@@ -309,6 +331,8 @@ int main(int argc, char *argv[])
 		 */
 		test_clone3_set_tid(set_tid, 3, CLONE_NEWPID, 0, 42, true);
 
+
+		flush();
 		_exit(ksft_cnt.ksft_pass);
 	}
 
