[LTP] [PATCH V2 0/2] syscalls/clone3: New tests

[LTP] [PATCH V2 0/2] syscalls/clone3: New tests

[Viresh Kumar]

Hello,

Here is the second attempt at adding clone3() related tests.

--
viresh

V1->V2:
- Relocated pidfd_send_signal.h, as clone3() needs to use the syscall.
- clone301.c is now sending and verifying signals between parent/child
  processes.
- More failure tests to validate remaining fields.

Viresh Kumar (2):
  syscalls/pidfd_send_signal: Move pidfd_send_signal.h to include/lapi/
  syscalls/clone3: New tests

 configure.ac                                  |   1 +
 include/lapi/clone.h                          |  49 ++++++
 .../lapi}/pidfd_send_signal.h                 |   8 +-
 runtest/syscalls                              |   3 +
 testcases/kernel/syscalls/clone3/.gitignore   |   2 +
 testcases/kernel/syscalls/clone3/Makefile     |   7 +
 testcases/kernel/syscalls/clone3/clone301.c   | 152 ++++++++++++++++++
 testcases/kernel/syscalls/clone3/clone302.c   | 101 ++++++++++++
 .../pidfd_send_signal/pidfd_send_signal01.c   |   2 +-
 .../pidfd_send_signal/pidfd_send_signal02.c   |   2 +-
 .../pidfd_send_signal/pidfd_send_signal03.c   |   2 +-
 11 files changed, 320 insertions(+), 9 deletions(-)
 create mode 100644 include/lapi/clone.h
 rename {testcases/kernel/syscalls/pidfd_send_signal => include/lapi}/pidfd_send_signal.h (73%)
 create mode 100644 testcases/kernel/syscalls/clone3/.gitignore
 create mode 100644 testcases/kernel/syscalls/clone3/Makefile
 create mode 100644 testcases/kernel/syscalls/clone3/clone301.c
 create mode 100644 testcases/kernel/syscalls/clone3/clone302.c

-- 
2.21.0.rc0.269.g1a574e7a288b

[LTP] [PATCH V2 1/2] syscalls/pidfd_send_signal: Move pidfd_send_signal.h to include/lapi/

[Viresh Kumar]

Move pidfd_send_signal.h to include/lapi/ to make it available for other
syscall tests.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 .../lapi}/pidfd_send_signal.h                             | 8 ++------
 .../syscalls/pidfd_send_signal/pidfd_send_signal01.c      | 2 +-
 .../syscalls/pidfd_send_signal/pidfd_send_signal02.c      | 2 +-
 .../syscalls/pidfd_send_signal/pidfd_send_signal03.c      | 2 +-
 4 files changed, 5 insertions(+), 9 deletions(-)
 rename {testcases/kernel/syscalls/pidfd_send_signal => include/lapi}/pidfd_send_signal.h (73%)

diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal.h b/include/lapi/pidfd_send_signal.h
similarity index 73%
rename from testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal.h
rename to include/lapi/pidfd_send_signal.h
index dc17fe058672..37de7ab401d0 100644
--- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal.h
+++ b/include/lapi/pidfd_send_signal.h
@@ -10,17 +10,13 @@
 #include "tst_test.h"
 #include "lapi/syscalls.h"
 
-static void check_syscall_support(void)
-{
-	/* allow the tests to fail early */
-	tst_syscall(__NR_pidfd_send_signal);
-}
+/* allow the tests to fail early */
+#define check_syscall_support()		tst_syscall(__NR_pidfd_send_signal)
 
 #ifndef HAVE_PIDFD_SEND_SIGNAL
 static int pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
 				 unsigned int flags)
 {
-	tst_res(TINFO, "Testing syscall(__NR_pidfd_send_signal)");
 	return tst_syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
 }
 #endif /* HAVE_PIDFD_SEND_SIGNAL */
diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c
index 03a4ae9bea41..3137b6967371 100644
--- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c
+++ b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c
@@ -12,8 +12,8 @@
 #define _GNU_SOURCE
 #include <signal.h>
 #include <stdlib.h>
+#include "lapi/pidfd_send_signal.h"
 #include "tst_safe_pthread.h"
-#include "pidfd_send_signal.h"
 
 #define SIGNAL  SIGUSR1
 #define DATA	777
diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c
index 74914523f0b8..610c67120a7a 100644
--- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c
+++ b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c
@@ -24,8 +24,8 @@
 #define _GNU_SOURCE
 #include <pwd.h>
 #include <signal.h>
+#include "lapi/pidfd_send_signal.h"
 #include "tst_safe_pthread.h"
-#include "pidfd_send_signal.h"
 
 #define CORRECT_SIGNAL		SIGUSR1
 #define DIFFERENT_SIGNAL	SIGUSR2
diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c
index 3420afbb9526..7d65e6ddc543 100644
--- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c
+++ b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c
@@ -19,7 +19,7 @@
 #include <signal.h>
 #include <stdio.h>
 #include <unistd.h>
-#include "pidfd_send_signal.h"
+#include "lapi/pidfd_send_signal.h"
 #include "tst_safe_pthread.h"
 
 #define PIDTRIES	3
-- 
2.21.0.rc0.269.g1a574e7a288b

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Viresh Kumar]

Add tests to check working of clone3() syscall.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 configure.ac                                |   1 +
 include/lapi/clone.h                        |  49 +++++++
 runtest/syscalls                            |   3 +
 testcases/kernel/syscalls/clone3/.gitignore |   2 +
 testcases/kernel/syscalls/clone3/Makefile   |   7 +
 testcases/kernel/syscalls/clone3/clone301.c | 152 ++++++++++++++++++++
 testcases/kernel/syscalls/clone3/clone302.c | 101 +++++++++++++
 7 files changed, 315 insertions(+)
 create mode 100644 include/lapi/clone.h
 create mode 100644 testcases/kernel/syscalls/clone3/.gitignore
 create mode 100644 testcases/kernel/syscalls/clone3/Makefile
 create mode 100644 testcases/kernel/syscalls/clone3/clone301.c
 create mode 100644 testcases/kernel/syscalls/clone3/clone302.c

diff --git a/configure.ac b/configure.ac
index 238d1cde85f2..cf89bd8c351e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -75,6 +75,7 @@ AC_CHECK_HEADERS(fts.h, [have_fts=1])
 AC_SUBST(HAVE_FTS_H, $have_fts)
 
 AC_CHECK_FUNCS([ \
+    clone3 \
     copy_file_range \
     epoll_pwait \
     execveat \
diff --git a/include/lapi/clone.h b/include/lapi/clone.h
new file mode 100644
index 000000000000..2b8cbdbe08e0
--- /dev/null
+++ b/include/lapi/clone.h
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Linaro Limited. All rights reserved.
+ * Author: Viresh Kumar <viresh.kumar@linaro.org>
+ */
+
+#ifndef LAPI_CLONE_H__
+#define LAPI_CLONE_H__
+
+#include <sys/syscall.h>
+#include <linux/types.h>
+#include <sched.h>
+
+#include "config.h"
+#include "lapi/syscalls.h"
+
+#ifndef HAVE_CLONE3
+struct clone_args {
+	uint64_t __attribute__((aligned(8))) flags;
+	uint64_t __attribute__((aligned(8))) pidfd;
+	uint64_t __attribute__((aligned(8))) child_tid;
+	uint64_t __attribute__((aligned(8))) parent_tid;
+	uint64_t __attribute__((aligned(8))) exit_signal;
+	uint64_t __attribute__((aligned(8))) stack;
+	uint64_t __attribute__((aligned(8))) stack_size;
+	uint64_t __attribute__((aligned(8))) tls;
+};
+
+int clone3(struct clone_args *args, size_t size)
+{
+	return tst_syscall(__NR_clone3, args, size);
+}
+#endif
+
+#ifndef CLONE_PIDFD
+#define CLONE_PIDFD	0x00001000	/* set if a pidfd should be placed in parent */
+#endif
+
+void clone3_supported_by_kernel(void)
+{
+	if ((tst_kvercmp(5, 3, 0)) < 0) {
+		/* Check if the syscall is backported on an older kernel */
+		TEST(syscall(__NR_clone3, NULL, 0));
+		if (TST_RET == -1 && TST_ERR == ENOSYS)
+			tst_brk(TCONF, "Test not supported on kernel version < v5.3");
+	}
+}
+
+#endif /* LAPI_CLONE_H__ */
diff --git a/runtest/syscalls b/runtest/syscalls
index 6f2dcd82acf6..65ef53f33e0b 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -105,6 +105,9 @@ clone07 clone07
 clone08 clone08
 clone09 clone09
 
+clone301 clone301
+clone302 clone302
+
 close01 close01
 close02 close02
 close08 close08
diff --git a/testcases/kernel/syscalls/clone3/.gitignore b/testcases/kernel/syscalls/clone3/.gitignore
new file mode 100644
index 000000000000..604cb903e33d
--- /dev/null
+++ b/testcases/kernel/syscalls/clone3/.gitignore
@@ -0,0 +1,2 @@
+clone301
+clone302
diff --git a/testcases/kernel/syscalls/clone3/Makefile b/testcases/kernel/syscalls/clone3/Makefile
new file mode 100644
index 000000000000..18896b6f28c0
--- /dev/null
+++ b/testcases/kernel/syscalls/clone3/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/clone3/clone301.c b/testcases/kernel/syscalls/clone3/clone301.c
new file mode 100644
index 000000000000..babf8464108c
--- /dev/null
+++ b/testcases/kernel/syscalls/clone3/clone301.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * Basic clone3() test.
+ */
+#define _GNU_SOURCE
+
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "lapi/clone.h"
+#include "lapi/pidfd_send_signal.h"
+
+#define CHILD_SIGNAL	SIGUSR1
+
+static int pidfd, child_tid, parent_tid, count, exit_signal;
+static struct sigaction *psig_action, *csig_action;
+static struct clone_args *args;
+static siginfo_t *uinfo;
+
+static struct tcase {
+	uint64_t flags;
+	int exit_signal;
+} tcases[] = {
+	{0, SIGCHLD},
+	{0, SIGUSR2},
+	{CLONE_FS, SIGCHLD},
+	{CLONE_NEWPID, SIGCHLD},
+	{CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, SIGCHLD},
+};
+
+static void parent_rx_signal(int sig, siginfo_t *info, void *ucontext)
+{
+	if (sig == exit_signal)
+		tst_res(TPASS, "clone3() passed: Parent received correct signal (index %d)", count);
+	else
+		tst_res(TFAIL, "clone3() failed: Parent received incorrect signal (index %d)", count);
+}
+
+static void child_rx_signal(int sig, siginfo_t *info, void *ucontext)
+{
+	if (info) {
+		int n = info->si_value.sival_int;
+
+		if (sig == CHILD_SIGNAL)
+			tst_res(TPASS, "clone3() passed: Child received correct signal (index %d)", n);
+		else
+			tst_res(TFAIL, "clone3() failed: Child received incorrect signal (index %d)", n);
+	} else {
+		tst_res(TFAIL, "clone3() failed: Invalid info");
+	}
+}
+
+static void do_child(int clone_pidfd)
+{
+	if (clone_pidfd) {
+		SAFE_SIGACTION(CHILD_SIGNAL, csig_action, NULL);
+		TST_CHECKPOINT_WAKE_AND_WAIT(0);
+	}
+
+	exit(0);
+}
+
+static void run(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+	int status, clone_pidfd = tc->flags & CLONE_PIDFD;
+	pid_t pid;
+
+	args->flags = tc->flags;
+	args->pidfd = (uint64_t)(&pidfd);
+	args->child_tid = (uint64_t)(&child_tid);
+	args->parent_tid = (uint64_t)(&parent_tid);
+	args->exit_signal = tc->exit_signal;
+	args->stack = 0;
+	args->stack_size = 0;
+	args->tls = 0;
+
+	TEST(pid = clone3(args, sizeof(*args)));
+	if (pid < 0) {
+		tst_res(TFAIL | TTERRNO, "clone3() failed (%d)", n);
+		return;
+	}
+
+	if (!pid)
+		do_child(clone_pidfd);
+
+	count = n;
+	exit_signal = tc->exit_signal;
+	SAFE_SIGACTION(exit_signal, psig_action, NULL);
+
+	/* Need to send signal to child process */
+	if (clone_pidfd) {
+		TST_CHECKPOINT_WAIT(0);
+
+		uinfo->si_value.sival_int = n;
+
+		TEST(pidfd_send_signal(pidfd, CHILD_SIGNAL, uinfo, 0));
+		if (TST_RET != 0) {
+			tst_res(TFAIL | TTERRNO, "pidfd_send_signal() failed");
+			return;
+		}
+
+		TST_CHECKPOINT_WAKE(0);
+	}
+
+	SAFE_WAITPID(pid, &status, __WALL);
+}
+
+static void setup(void)
+{
+	clone3_supported_by_kernel();
+
+	psig_action = SAFE_MALLOC(sizeof(*psig_action));
+	memset(psig_action, 0, sizeof(*psig_action));
+	psig_action->sa_sigaction = parent_rx_signal;
+	psig_action->sa_flags = SA_SIGINFO;
+
+	csig_action = SAFE_MALLOC(sizeof(*csig_action));
+	memset(csig_action, 0, sizeof(*csig_action));
+	csig_action->sa_sigaction = child_rx_signal;
+	csig_action->sa_flags = SA_SIGINFO;
+
+	uinfo = SAFE_MALLOC(sizeof(*uinfo));
+	memset(uinfo, 0, sizeof(*uinfo));
+	uinfo->si_signo = CHILD_SIGNAL;
+	uinfo->si_code = SI_QUEUE;
+	uinfo->si_pid = getpid();
+	uinfo->si_uid = getuid();
+}
+
+static void cleanup(void)
+{
+	free(uinfo);
+	free(csig_action);
+	free(psig_action);
+}
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.needs_checkpoints = 1,
+	.bufs = (struct tst_buffers []) {
+		{&args, .size = sizeof(*args)},
+		{},
+	}
+};
diff --git a/testcases/kernel/syscalls/clone3/clone302.c b/testcases/kernel/syscalls/clone3/clone302.c
new file mode 100644
index 000000000000..1355a5c4a07f
--- /dev/null
+++ b/testcases/kernel/syscalls/clone3/clone302.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * Basic clone3() test to check various failures.
+ */
+#define _GNU_SOURCE
+
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "lapi/clone.h"
+
+static struct clone_args *valid_args, *invalid_args;
+unsigned long stack;
+static int *invalid_address;
+
+static struct tcase {
+	const char *name;
+	struct clone_args **args;
+	size_t size;
+	uint64_t flags;
+	int **pidfd;
+	int **child_tid;
+	int **parent_tid;
+	int exit_signal;
+	unsigned long stack;
+	unsigned long stack_size;
+	unsigned long tls;
+	int exp_errno;
+} tcases[] = {
+	{"invalid args", &invalid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
+	{"zero size", &valid_args, 0, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
+	{"short size", &valid_args, sizeof(*valid_args) - 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
+	{"extra size", &valid_args, sizeof(*valid_args) + 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
+	{"sighand-no-VM", &valid_args, sizeof(*valid_args), CLONE_SIGHAND, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
+	{"thread-no-sighand", &valid_args, sizeof(*valid_args), CLONE_THREAD, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
+	{"fs-newns", &valid_args, sizeof(*valid_args), CLONE_FS | CLONE_NEWNS, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
+	{"invalid pidfd", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, &invalid_address, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
+	{"invalid childtid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, &invalid_address, NULL, SIGCHLD, 0, 0, 0, EFAULT},
+	{"invalid parenttid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, NULL, &invalid_address, SIGCHLD, 0, 0, 0, EFAULT},
+	{"invalid signal", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, CSIGNAL + 1, 0, 0, 0, EINVAL},
+	{"zero-stack-size", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, (unsigned long)&stack, 0, 0, EINVAL},
+	{"invalid-stack", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 4, 0, EINVAL},
+};
+
+static void setup(void)
+{
+	clone3_supported_by_kernel();
+
+	invalid_address = tst_get_bad_addr(NULL);
+}
+
+static void run(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+	struct clone_args *args = *tc->args;
+
+	if (args) {
+		args->flags = tc->flags;
+		if (tc->pidfd)
+			args->pidfd = (uint64_t)(*tc->pidfd);
+		if (tc->child_tid)
+			args->child_tid = (uint64_t)(*tc->child_tid);
+		if (tc->parent_tid)
+			args->parent_tid = (uint64_t)(*tc->parent_tid);
+		args->exit_signal = tc->exit_signal;
+		args->stack = tc->stack;
+		args->stack_size = tc->stack_size;
+		args->tls = tc->tls;
+	}
+
+	TEST(clone3(args, tc->size));
+
+	if (!TST_RET)
+		exit(EXIT_SUCCESS);
+
+	if (TST_RET >= 0) {
+		tst_res(TFAIL, "%s: clone3() passed unexpectedly", tc->name);
+		return;
+	}
+
+	if (tc->exp_errno != TST_ERR) {
+		tst_res(TFAIL | TTERRNO, "%s: clone3() should fail with %s",
+			tc->name, tst_strerrno(tc->exp_errno));
+		return;
+	}
+
+	tst_res(TPASS | TTERRNO, "%s: clone3() failed as expected", tc->name);
+}
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = run,
+	.setup = setup,
+	.needs_tmpdir = 1,
+	.bufs = (struct tst_buffers []) {
+		{&valid_args, .size = sizeof(*valid_args)},
+		{},
+	}
+};
-- 
2.21.0.rc0.269.g1a574e7a288b

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Viresh Kumar]

On 20-03-20, 00:01, Cyril Hrubis wrote:
> > diff --git a/testcases/kernel/syscalls/clone3/clone301.c b/testcases/kernel/syscalls/clone3/clone301.c
> > new file mode 100644
> > index 000000000000..babf8464108c
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/clone3/clone301.c
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
> > + *
> > + * Basic clone3() test.
> > + */
> > +#define _GNU_SOURCE
> > +
> > +#include <stdlib.h>
> > +
> > +#include "tst_test.h"
> > +#include "lapi/clone.h"
> > +#include "lapi/pidfd_send_signal.h"
> > +
> > +#define CHILD_SIGNAL	SIGUSR1
> > +
> > +static int pidfd, child_tid, parent_tid, count, exit_signal;
> > +static struct sigaction *psig_action, *csig_action;
> > +static struct clone_args *args;
> > +static siginfo_t *uinfo;
> > +
> > +static struct tcase {
> > +	uint64_t flags;
> > +	int exit_signal;
> > +} tcases[] = {
> > +	{0, SIGCHLD},
> > +	{0, SIGUSR2},
> > +	{CLONE_FS, SIGCHLD},
> > +	{CLONE_NEWPID, SIGCHLD},
> > +	{CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, SIGCHLD},
> > +};
> > +
> > +static void parent_rx_signal(int sig, siginfo_t *info, void *ucontext)
> > +{
> > +	if (sig == exit_signal)
> > +		tst_res(TPASS, "clone3() passed: Parent received correct signal (index %d)", count);
> > +	else
> > +		tst_res(TFAIL, "clone3() failed: Parent received incorrect signal (index %d)", count);
> > +}
> > +
> > +static void child_rx_signal(int sig, siginfo_t *info, void *ucontext)
> > +{
> > +	if (info) {
> > +		int n = info->si_value.sival_int;
> > +
> > +		if (sig == CHILD_SIGNAL)
> > +			tst_res(TPASS, "clone3() passed: Child received correct signal (index %d)", n);
> > +		else
> > +			tst_res(TFAIL, "clone3() failed: Child received incorrect signal (index %d)", n);
> > +	} else {
> > +		tst_res(TFAIL, "clone3() failed: Invalid info");
> > +	}
> > +}
> 
> Calling tst_res() is not safe from a signal handler context.
> 
> So what we should do here is store the sig and info->si_value.sival_int
> to a global variables and check them the do_child function instead.
> 
> And the same applies for the parent handler as well.

Lemme start by excepting that I am bad with userspace programming and
I mostly do kernel stuff :(

With clone, parent and child process don't space address space and so
the variables aren't shared. I tried the CLONE_VM thing but with the
first write, the program gets killed. Not sure how to fix that.

> > +static void setup(void)
> > +{
> > +	clone3_supported_by_kernel();
> > +
> > +	psig_action = SAFE_MALLOC(sizeof(*psig_action));
> > +	memset(psig_action, 0, sizeof(*psig_action));
> > +	psig_action->sa_sigaction = parent_rx_signal;
> > +	psig_action->sa_flags = SA_SIGINFO;
> > +
> > +	csig_action = SAFE_MALLOC(sizeof(*csig_action));
> > +	memset(csig_action, 0, sizeof(*csig_action));
> > +	csig_action->sa_sigaction = child_rx_signal;
> > +	csig_action->sa_flags = SA_SIGINFO;
> 
> There is no need to allocate these, we can just define them as a static
> global variables with:
> 
> static struct sigaction psig_action = {
> 	.sa_sigaction = parent_rx_signal,
> 	.sa_flags = SA_SIGINFO,
> };

I thought about that but then followed what pidfd_send_signal() did.

> > +static struct clone_args *valid_args, *invalid_args;
> > +unsigned long stack;
> > +static int *invalid_address;
> > +
> > +static struct tcase {
> > +	const char *name;
> > +	struct clone_args **args;
> > +	size_t size;
> > +	uint64_t flags;
> > +	int **pidfd;
> > +	int **child_tid;
> > +	int **parent_tid;
> > +	int exit_signal;
> > +	unsigned long stack;
> > +	unsigned long stack_size;
> > +	unsigned long tls;
> > +	int exp_errno;
> > +} tcases[] = {
> > +	{"invalid args", &invalid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > +	{"zero size", &valid_args, 0, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > +	{"short size", &valid_args, sizeof(*valid_args) - 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > +	{"extra size", &valid_args, sizeof(*valid_args) + 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > +	{"sighand-no-VM", &valid_args, sizeof(*valid_args), CLONE_SIGHAND, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > +	{"thread-no-sighand", &valid_args, sizeof(*valid_args), CLONE_THREAD, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > +	{"fs-newns", &valid_args, sizeof(*valid_args), CLONE_FS | CLONE_NEWNS, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > +	{"invalid pidfd", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, &invalid_address, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > +	{"invalid childtid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, &invalid_address, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > +	{"invalid parenttid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, NULL, &invalid_address, SIGCHLD, 0, 0, 0, EFAULT},
> > +	{"invalid signal", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, CSIGNAL + 1, 0, 0, 0, EINVAL},
> > +	{"zero-stack-size", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, (unsigned long)&stack, 0, 0, EINVAL},
> > +	{"invalid-stack", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 4, 0, EINVAL},
> > +};
> > +
> > +static void setup(void)
> > +{
> > +	clone3_supported_by_kernel();
> > +
> > +	invalid_address = tst_get_bad_addr(NULL);
> > +}
> > +
> > +static void run(unsigned int n)
> > +{
> > +	struct tcase *tc = &tcases[n];
> > +	struct clone_args *args = *tc->args;
> > +
> > +	if (args) {
> > +		args->flags = tc->flags;
> > +		if (tc->pidfd)
> > +			args->pidfd = (uint64_t)(*tc->pidfd);
> > +		if (tc->child_tid)
> > +			args->child_tid = (uint64_t)(*tc->child_tid);
> > +		if (tc->parent_tid)
> > +			args->parent_tid = (uint64_t)(*tc->parent_tid);
> > +		args->exit_signal = tc->exit_signal;
> > +		args->stack = tc->stack;
> > +		args->stack_size = tc->stack_size;
> > +		args->tls = tc->tls;
> > +	}
> 
> Isn't this wrong now that invalid_args != NULL?
> 
> Shouldn't this rather be if (args == valid_args) ?

invalid_args is still NULL, invalid_address isn't though.

-- 
viresh

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Viresh Kumar]

On 19-03-20, 20:49, Viresh Kumar wrote:
> Lemme start by excepting that I am bad with userspace programming and

                 accepting***

> I mostly do kernel stuff :(



-- 
viresh

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Viresh Kumar]

On 20-03-20, 00:24, Cyril Hrubis wrote:
> Huh? All that we have to is to move the code from the child_rx_signal()
> to the do_child() here, the child would setup a handler and call
> pause(), then it checks if correct values have been stored to a global
> varibles. And the same for the parent, the point is that we should do a
> minimal amount of work in the handler itself.
> 
> The problem here is that tst_res() writes to std streams, that have
> locks, so if we happen to get a signal while something writes there as
> well, we deadlock. Also printf()-like functions may call malloc, which
> has locks and may deadlock in the same way. It's unlikely that it will
> ever happen in this test, but that does not excuse us...

Ahh, I somehow read that as you are asking me to do all TPASS, TFAIL
thing in the run() call instead and so went into sharing between
parent child. Sorry about that.

-- 
viresh

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Viresh Kumar]

On 20-03-20, 00:24, Cyril Hrubis wrote:
> Huh? All that we have to is to move the code from the child_rx_signal()
> to the do_child() here, the child would setup a handler and call
> pause(), then it checks if correct values have been stored to a global
> varibles. And the same for the parent, the point is that we should do a
> minimal amount of work in the handler itself.

There is a problem with using pause() here.

Child is doing this:

static void do_child(int clone_pidfd, int n)
{
	SAFE_SIGACTION(CHILD_SIGNAL, &csig_action, NULL);

	TST_CHECKPOINT_WAKE(0);
	pause();
	TST_CHECKPOINT_WAIT(1);

	if (child_received_signal)
		tst_res(TPASS, "clone3() passed: Child received correct signal (index %d)", n);

        exit(0);
}

and parent:

        TST_CHECKPOINT_WAIT(0);

        TEST(pidfd_send_signal(pidfd, CHILD_SIGNAL, &uinfo, 0));
        if (TST_RET != 0) {
                tst_res(TFAIL | TTERRNO, "pidfd_send_signal() failed");
                return;
        }

        TST_CHECKPOINT_WAKE(1);


What's happening is that parent is able to send the signal before the
child calls pause() and so it hangs. If I simply remove pause() it all
works fine for me, but is pidfd_send_signal() synchronous ? Does it
wait until the time child signal is executed ? If yes, then we don't
have a problem, else we may run into timing issue.

We can add a delay in parent before sending the signal, but that is
still racy in worst cases.

-- 
viresh

[LTP] [PATCH V2 1/2] syscalls/pidfd_send_signal: Move pidfd_send_signal.h to include/lapi/

[Cyril Hrubis]

Hi!
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  .../lapi}/pidfd_send_signal.h                             | 8 ++------
>  .../syscalls/pidfd_send_signal/pidfd_send_signal01.c      | 2 +-
>  .../syscalls/pidfd_send_signal/pidfd_send_signal02.c      | 2 +-
>  .../syscalls/pidfd_send_signal/pidfd_send_signal03.c      | 2 +-
>  4 files changed, 5 insertions(+), 9 deletions(-)
>  rename {testcases/kernel/syscalls/pidfd_send_signal => include/lapi}/pidfd_send_signal.h (73%)
> 
> diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal.h b/include/lapi/pidfd_send_signal.h
> similarity index 73%
> rename from testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal.h
> rename to include/lapi/pidfd_send_signal.h
> index dc17fe058672..37de7ab401d0 100644
> --- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal.h
> +++ b/include/lapi/pidfd_send_signal.h
> @@ -10,17 +10,13 @@
>  #include "tst_test.h"
>  #include "lapi/syscalls.h"
>  
> -static void check_syscall_support(void)
> -{
> -	/* allow the tests to fail early */
> -	tst_syscall(__NR_pidfd_send_signal);
> -}
> +/* allow the tests to fail early */
> +#define check_syscall_support()		tst_syscall(__NR_pidfd_send_signal)

Huh, why do we change this to a macro?

I guess that you got unused warnings. The canonical way how to implement
functions in C headers is to make them static inline instead of this
macro hackery.

Also as we are moving it to a public header it should probably be
renamed to pidfd_send_signal_supported() or something that starts with
the syscall name.

>  #ifndef HAVE_PIDFD_SEND_SIGNAL
>  static int pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
>  				 unsigned int flags)
>  {
> -	tst_res(TINFO, "Testing syscall(__NR_pidfd_send_signal)");
>  	return tst_syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
>  }
>  #endif /* HAVE_PIDFD_SEND_SIGNAL */
> diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c
> index 03a4ae9bea41..3137b6967371 100644
> --- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c
> +++ b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal01.c
> @@ -12,8 +12,8 @@
>  #define _GNU_SOURCE
>  #include <signal.h>
>  #include <stdlib.h>
> +#include "lapi/pidfd_send_signal.h"
>  #include "tst_safe_pthread.h"
> -#include "pidfd_send_signal.h"
>  
>  #define SIGNAL  SIGUSR1
>  #define DATA	777
> diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c
> index 74914523f0b8..610c67120a7a 100644
> --- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c
> +++ b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal02.c
> @@ -24,8 +24,8 @@
>  #define _GNU_SOURCE
>  #include <pwd.h>
>  #include <signal.h>
> +#include "lapi/pidfd_send_signal.h"
>  #include "tst_safe_pthread.h"
> -#include "pidfd_send_signal.h"
>  
>  #define CORRECT_SIGNAL		SIGUSR1
>  #define DIFFERENT_SIGNAL	SIGUSR2
> diff --git a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c
> index 3420afbb9526..7d65e6ddc543 100644
> --- a/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c
> +++ b/testcases/kernel/syscalls/pidfd_send_signal/pidfd_send_signal03.c
> @@ -19,7 +19,7 @@
>  #include <signal.h>
>  #include <stdio.h>
>  #include <unistd.h>
> -#include "pidfd_send_signal.h"
> +#include "lapi/pidfd_send_signal.h"
>  #include "tst_safe_pthread.h"
>  
>  #define PIDTRIES	3
> -- 
> 2.21.0.rc0.269.g1a574e7a288b
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Cyril Hrubis]

Hi!
> Add tests to check working of clone3() syscall.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  configure.ac                                |   1 +
>  include/lapi/clone.h                        |  49 +++++++
>  runtest/syscalls                            |   3 +
>  testcases/kernel/syscalls/clone3/.gitignore |   2 +
>  testcases/kernel/syscalls/clone3/Makefile   |   7 +
>  testcases/kernel/syscalls/clone3/clone301.c | 152 ++++++++++++++++++++
>  testcases/kernel/syscalls/clone3/clone302.c | 101 +++++++++++++
>  7 files changed, 315 insertions(+)
>  create mode 100644 include/lapi/clone.h
>  create mode 100644 testcases/kernel/syscalls/clone3/.gitignore
>  create mode 100644 testcases/kernel/syscalls/clone3/Makefile
>  create mode 100644 testcases/kernel/syscalls/clone3/clone301.c
>  create mode 100644 testcases/kernel/syscalls/clone3/clone302.c
> 
> diff --git a/configure.ac b/configure.ac
> index 238d1cde85f2..cf89bd8c351e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -75,6 +75,7 @@ AC_CHECK_HEADERS(fts.h, [have_fts=1])
>  AC_SUBST(HAVE_FTS_H, $have_fts)
>  
>  AC_CHECK_FUNCS([ \
> +    clone3 \
>      copy_file_range \
>      epoll_pwait \
>      execveat \
> diff --git a/include/lapi/clone.h b/include/lapi/clone.h
> new file mode 100644
> index 000000000000..2b8cbdbe08e0
> --- /dev/null
> +++ b/include/lapi/clone.h
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 Linaro Limited. All rights reserved.
> + * Author: Viresh Kumar <viresh.kumar@linaro.org>
> + */
> +
> +#ifndef LAPI_CLONE_H__
> +#define LAPI_CLONE_H__
> +
> +#include <sys/syscall.h>
> +#include <linux/types.h>
> +#include <sched.h>
> +
> +#include "config.h"
> +#include "lapi/syscalls.h"
> +
> +#ifndef HAVE_CLONE3
> +struct clone_args {
> +	uint64_t __attribute__((aligned(8))) flags;
> +	uint64_t __attribute__((aligned(8))) pidfd;
> +	uint64_t __attribute__((aligned(8))) child_tid;
> +	uint64_t __attribute__((aligned(8))) parent_tid;
> +	uint64_t __attribute__((aligned(8))) exit_signal;
> +	uint64_t __attribute__((aligned(8))) stack;
> +	uint64_t __attribute__((aligned(8))) stack_size;
> +	uint64_t __attribute__((aligned(8))) tls;
> +};
> +
> +int clone3(struct clone_args *args, size_t size)
> +{
> +	return tst_syscall(__NR_clone3, args, size);
> +}
> +#endif
> +
> +#ifndef CLONE_PIDFD
> +#define CLONE_PIDFD	0x00001000	/* set if a pidfd should be placed in parent */
> +#endif
> +
> +void clone3_supported_by_kernel(void)
> +{
> +	if ((tst_kvercmp(5, 3, 0)) < 0) {
> +		/* Check if the syscall is backported on an older kernel */
> +		TEST(syscall(__NR_clone3, NULL, 0));
> +		if (TST_RET == -1 && TST_ERR == ENOSYS)
> +			tst_brk(TCONF, "Test not supported on kernel version < v5.3");
> +	}
> +}
> +
> +#endif /* LAPI_CLONE_H__ */
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 6f2dcd82acf6..65ef53f33e0b 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -105,6 +105,9 @@ clone07 clone07
>  clone08 clone08
>  clone09 clone09
>  
> +clone301 clone301
> +clone302 clone302
> +
>  close01 close01
>  close02 close02
>  close08 close08
> diff --git a/testcases/kernel/syscalls/clone3/.gitignore b/testcases/kernel/syscalls/clone3/.gitignore
> new file mode 100644
> index 000000000000..604cb903e33d
> --- /dev/null
> +++ b/testcases/kernel/syscalls/clone3/.gitignore
> @@ -0,0 +1,2 @@
> +clone301
> +clone302
> diff --git a/testcases/kernel/syscalls/clone3/Makefile b/testcases/kernel/syscalls/clone3/Makefile
> new file mode 100644
> index 000000000000..18896b6f28c0
> --- /dev/null
> +++ b/testcases/kernel/syscalls/clone3/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/clone3/clone301.c b/testcases/kernel/syscalls/clone3/clone301.c
> new file mode 100644
> index 000000000000..babf8464108c
> --- /dev/null
> +++ b/testcases/kernel/syscalls/clone3/clone301.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
> + *
> + * Basic clone3() test.
> + */
> +#define _GNU_SOURCE
> +
> +#include <stdlib.h>
> +
> +#include "tst_test.h"
> +#include "lapi/clone.h"
> +#include "lapi/pidfd_send_signal.h"
> +
> +#define CHILD_SIGNAL	SIGUSR1
> +
> +static int pidfd, child_tid, parent_tid, count, exit_signal;
> +static struct sigaction *psig_action, *csig_action;
> +static struct clone_args *args;
> +static siginfo_t *uinfo;
> +
> +static struct tcase {
> +	uint64_t flags;
> +	int exit_signal;
> +} tcases[] = {
> +	{0, SIGCHLD},
> +	{0, SIGUSR2},
> +	{CLONE_FS, SIGCHLD},
> +	{CLONE_NEWPID, SIGCHLD},
> +	{CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, SIGCHLD},
> +};
> +
> +static void parent_rx_signal(int sig, siginfo_t *info, void *ucontext)
> +{
> +	if (sig == exit_signal)
> +		tst_res(TPASS, "clone3() passed: Parent received correct signal (index %d)", count);
> +	else
> +		tst_res(TFAIL, "clone3() failed: Parent received incorrect signal (index %d)", count);
> +}
> +
> +static void child_rx_signal(int sig, siginfo_t *info, void *ucontext)
> +{
> +	if (info) {
> +		int n = info->si_value.sival_int;
> +
> +		if (sig == CHILD_SIGNAL)
> +			tst_res(TPASS, "clone3() passed: Child received correct signal (index %d)", n);
> +		else
> +			tst_res(TFAIL, "clone3() failed: Child received incorrect signal (index %d)", n);
> +	} else {
> +		tst_res(TFAIL, "clone3() failed: Invalid info");
> +	}
> +}

Calling tst_res() is not safe from a signal handler context.

So what we should do here is store the sig and info->si_value.sival_int
to a global variables and check them the do_child function instead.

And the same applies for the parent handler as well.

> +static void do_child(int clone_pidfd)
> +{
> +	if (clone_pidfd) {
> +		SAFE_SIGACTION(CHILD_SIGNAL, csig_action, NULL);
> +		TST_CHECKPOINT_WAKE_AND_WAIT(0);
> +	}
> +
> +	exit(0);
> +}
> +
> +static void run(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	int status, clone_pidfd = tc->flags & CLONE_PIDFD;
> +	pid_t pid;
> +
> +	args->flags = tc->flags;
> +	args->pidfd = (uint64_t)(&pidfd);
> +	args->child_tid = (uint64_t)(&child_tid);
> +	args->parent_tid = (uint64_t)(&parent_tid);
> +	args->exit_signal = tc->exit_signal;
> +	args->stack = 0;
> +	args->stack_size = 0;
> +	args->tls = 0;
> +
> +	TEST(pid = clone3(args, sizeof(*args)));
> +	if (pid < 0) {
> +		tst_res(TFAIL | TTERRNO, "clone3() failed (%d)", n);
> +		return;
> +	}
> +
> +	if (!pid)
> +		do_child(clone_pidfd);
> +
> +	count = n;
> +	exit_signal = tc->exit_signal;
> +	SAFE_SIGACTION(exit_signal, psig_action, NULL);
> +
> +	/* Need to send signal to child process */
> +	if (clone_pidfd) {
> +		TST_CHECKPOINT_WAIT(0);
> +
> +		uinfo->si_value.sival_int = n;
> +
> +		TEST(pidfd_send_signal(pidfd, CHILD_SIGNAL, uinfo, 0));
> +		if (TST_RET != 0) {
> +			tst_res(TFAIL | TTERRNO, "pidfd_send_signal() failed");
> +			return;
> +		}
> +
> +		TST_CHECKPOINT_WAKE(0);
> +	}
> +
> +	SAFE_WAITPID(pid, &status, __WALL);
> +}
> +
> +static void setup(void)
> +{
> +	clone3_supported_by_kernel();
> +
> +	psig_action = SAFE_MALLOC(sizeof(*psig_action));
> +	memset(psig_action, 0, sizeof(*psig_action));
> +	psig_action->sa_sigaction = parent_rx_signal;
> +	psig_action->sa_flags = SA_SIGINFO;
> +
> +	csig_action = SAFE_MALLOC(sizeof(*csig_action));
> +	memset(csig_action, 0, sizeof(*csig_action));
> +	csig_action->sa_sigaction = child_rx_signal;
> +	csig_action->sa_flags = SA_SIGINFO;

There is no need to allocate these, we can just define them as a static
global variables with:

static struct sigaction psig_action = {
	.sa_sigaction = parent_rx_signal,
	.sa_flags = SA_SIGINFO,
};

> +	uinfo = SAFE_MALLOC(sizeof(*uinfo));
> +	memset(uinfo, 0, sizeof(*uinfo));
> +	uinfo->si_signo = CHILD_SIGNAL;
> +	uinfo->si_code = SI_QUEUE;
> +	uinfo->si_pid = getpid();
> +	uinfo->si_uid = getuid();

Here as well, the only thing that has to be initialized on runtime are
the pid and uid.

> +}
> +
> +static void cleanup(void)
> +{
> +	free(uinfo);
> +	free(csig_action);
> +	free(psig_action);
> +}
> +
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.needs_checkpoints = 1,
> +	.bufs = (struct tst_buffers []) {
> +		{&args, .size = sizeof(*args)},
> +		{},
> +	}
> +};
> diff --git a/testcases/kernel/syscalls/clone3/clone302.c b/testcases/kernel/syscalls/clone3/clone302.c
> new file mode 100644
> index 000000000000..1355a5c4a07f
> --- /dev/null
> +++ b/testcases/kernel/syscalls/clone3/clone302.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 Viresh Kumar <viresh.kumar@linaro.org>
> + *
> + * Basic clone3() test to check various failures.
> + */
> +#define _GNU_SOURCE
> +
> +#include <stdlib.h>
> +
> +#include "tst_test.h"
> +#include "lapi/clone.h"
> +
> +static struct clone_args *valid_args, *invalid_args;
> +unsigned long stack;
> +static int *invalid_address;
> +
> +static struct tcase {
> +	const char *name;
> +	struct clone_args **args;
> +	size_t size;
> +	uint64_t flags;
> +	int **pidfd;
> +	int **child_tid;
> +	int **parent_tid;
> +	int exit_signal;
> +	unsigned long stack;
> +	unsigned long stack_size;
> +	unsigned long tls;
> +	int exp_errno;
> +} tcases[] = {
> +	{"invalid args", &invalid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> +	{"zero size", &valid_args, 0, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> +	{"short size", &valid_args, sizeof(*valid_args) - 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> +	{"extra size", &valid_args, sizeof(*valid_args) + 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> +	{"sighand-no-VM", &valid_args, sizeof(*valid_args), CLONE_SIGHAND, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> +	{"thread-no-sighand", &valid_args, sizeof(*valid_args), CLONE_THREAD, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> +	{"fs-newns", &valid_args, sizeof(*valid_args), CLONE_FS | CLONE_NEWNS, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> +	{"invalid pidfd", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, &invalid_address, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> +	{"invalid childtid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, &invalid_address, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> +	{"invalid parenttid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, NULL, &invalid_address, SIGCHLD, 0, 0, 0, EFAULT},
> +	{"invalid signal", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, CSIGNAL + 1, 0, 0, 0, EINVAL},
> +	{"zero-stack-size", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, (unsigned long)&stack, 0, 0, EINVAL},
> +	{"invalid-stack", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 4, 0, EINVAL},
> +};
> +
> +static void setup(void)
> +{
> +	clone3_supported_by_kernel();
> +
> +	invalid_address = tst_get_bad_addr(NULL);
> +}
> +
> +static void run(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	struct clone_args *args = *tc->args;
> +
> +	if (args) {
> +		args->flags = tc->flags;
> +		if (tc->pidfd)
> +			args->pidfd = (uint64_t)(*tc->pidfd);
> +		if (tc->child_tid)
> +			args->child_tid = (uint64_t)(*tc->child_tid);
> +		if (tc->parent_tid)
> +			args->parent_tid = (uint64_t)(*tc->parent_tid);
> +		args->exit_signal = tc->exit_signal;
> +		args->stack = tc->stack;
> +		args->stack_size = tc->stack_size;
> +		args->tls = tc->tls;
> +	}

Isn't this wrong now that invalid_args != NULL?

Shouldn't this rather be if (args == valid_args) ?

> +	TEST(clone3(args, tc->size));
> +
> +	if (!TST_RET)
> +		exit(EXIT_SUCCESS);
> +
> +	if (TST_RET >= 0) {
> +		tst_res(TFAIL, "%s: clone3() passed unexpectedly", tc->name);
> +		return;
> +	}
> +
> +	if (tc->exp_errno != TST_ERR) {
> +		tst_res(TFAIL | TTERRNO, "%s: clone3() should fail with %s",
> +			tc->name, tst_strerrno(tc->exp_errno));
> +		return;
> +	}
> +
> +	tst_res(TPASS | TTERRNO, "%s: clone3() failed as expected", tc->name);
> +}
> +
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +	.setup = setup,
> +	.needs_tmpdir = 1,
> +	.bufs = (struct tst_buffers []) {
> +		{&valid_args, .size = sizeof(*valid_args)},
> +		{},
> +	}
> +};
> -- 
> 2.21.0.rc0.269.g1a574e7a288b
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Cyril Hrubis]

Hi!
> > > +static void child_rx_signal(int sig, siginfo_t *info, void *ucontext)
> > > +{
> > > +	if (info) {
> > > +		int n = info->si_value.sival_int;
> > > +
> > > +		if (sig == CHILD_SIGNAL)
> > > +			tst_res(TPASS, "clone3() passed: Child received correct signal (index %d)", n);
> > > +		else
> > > +			tst_res(TFAIL, "clone3() failed: Child received incorrect signal (index %d)", n);
> > > +	} else {
> > > +		tst_res(TFAIL, "clone3() failed: Invalid info");
> > > +	}
> > > +}
> > 
> > Calling tst_res() is not safe from a signal handler context.
> > 
> > So what we should do here is store the sig and info->si_value.sival_int
> > to a global variables and check them the do_child function instead.
> > 
> > And the same applies for the parent handler as well.
> 
> Lemme start by excepting that I am bad with userspace programming and
> I mostly do kernel stuff :(

You are doing a great job :-).

> With clone, parent and child process don't space address space and so
> the variables aren't shared. I tried the CLONE_VM thing but with the
> first write, the program gets killed. Not sure how to fix that.

Huh? All that we have to is to move the code from the child_rx_signal()
to the do_child() here, the child would setup a handler and call
pause(), then it checks if correct values have been stored to a global
varibles. And the same for the parent, the point is that we should do a
minimal amount of work in the handler itself.

The problem here is that tst_res() writes to std streams, that have
locks, so if we happen to get a signal while something writes there as
well, we deadlock. Also printf()-like functions may call malloc, which
has locks and may deadlock in the same way. It's unlikely that it will
ever happen in this test, but that does not excuse us...

> > > +static void setup(void)
> > > +{
> > > +	clone3_supported_by_kernel();
> > > +
> > > +	psig_action = SAFE_MALLOC(sizeof(*psig_action));
> > > +	memset(psig_action, 0, sizeof(*psig_action));
> > > +	psig_action->sa_sigaction = parent_rx_signal;
> > > +	psig_action->sa_flags = SA_SIGINFO;
> > > +
> > > +	csig_action = SAFE_MALLOC(sizeof(*csig_action));
> > > +	memset(csig_action, 0, sizeof(*csig_action));
> > > +	csig_action->sa_sigaction = child_rx_signal;
> > > +	csig_action->sa_flags = SA_SIGINFO;
> > 
> > There is no need to allocate these, we can just define them as a static
> > global variables with:
> > 
> > static struct sigaction psig_action = {
> > 	.sa_sigaction = parent_rx_signal,
> > 	.sa_flags = SA_SIGINFO,
> > };
> 
> I thought about that but then followed what pidfd_send_signal() did.
> 
> > > +static struct clone_args *valid_args, *invalid_args;
> > > +unsigned long stack;
> > > +static int *invalid_address;
> > > +
> > > +static struct tcase {
> > > +	const char *name;
> > > +	struct clone_args **args;
> > > +	size_t size;
> > > +	uint64_t flags;
> > > +	int **pidfd;
> > > +	int **child_tid;
> > > +	int **parent_tid;
> > > +	int exit_signal;
> > > +	unsigned long stack;
> > > +	unsigned long stack_size;
> > > +	unsigned long tls;
> > > +	int exp_errno;
> > > +} tcases[] = {
> > > +	{"invalid args", &invalid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > > +	{"zero size", &valid_args, 0, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > > +	{"short size", &valid_args, sizeof(*valid_args) - 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > > +	{"extra size", &valid_args, sizeof(*valid_args) + 1, 0, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > > +	{"sighand-no-VM", &valid_args, sizeof(*valid_args), CLONE_SIGHAND, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > > +	{"thread-no-sighand", &valid_args, sizeof(*valid_args), CLONE_THREAD, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > > +	{"fs-newns", &valid_args, sizeof(*valid_args), CLONE_FS | CLONE_NEWNS, NULL, NULL, NULL, SIGCHLD, 0, 0, 0, EINVAL},
> > > +	{"invalid pidfd", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, &invalid_address, NULL, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > > +	{"invalid childtid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, &invalid_address, NULL, SIGCHLD, 0, 0, 0, EFAULT},
> > > +	{"invalid parenttid", &valid_args, sizeof(*valid_args), CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_PIDFD, NULL, NULL, &invalid_address, SIGCHLD, 0, 0, 0, EFAULT},
> > > +	{"invalid signal", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, CSIGNAL + 1, 0, 0, 0, EINVAL},
> > > +	{"zero-stack-size", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, (unsigned long)&stack, 0, 0, EINVAL},
> > > +	{"invalid-stack", &valid_args, sizeof(*valid_args), 0, NULL, NULL, NULL, SIGCHLD, 0, 4, 0, EINVAL},
> > > +};
> > > +
> > > +static void setup(void)
> > > +{
> > > +	clone3_supported_by_kernel();
> > > +
> > > +	invalid_address = tst_get_bad_addr(NULL);
> > > +}
> > > +
> > > +static void run(unsigned int n)
> > > +{
> > > +	struct tcase *tc = &tcases[n];
> > > +	struct clone_args *args = *tc->args;
> > > +
> > > +	if (args) {
> > > +		args->flags = tc->flags;
> > > +		if (tc->pidfd)
> > > +			args->pidfd = (uint64_t)(*tc->pidfd);
> > > +		if (tc->child_tid)
> > > +			args->child_tid = (uint64_t)(*tc->child_tid);
> > > +		if (tc->parent_tid)
> > > +			args->parent_tid = (uint64_t)(*tc->parent_tid);
> > > +		args->exit_signal = tc->exit_signal;
> > > +		args->stack = tc->stack;
> > > +		args->stack_size = tc->stack_size;
> > > +		args->tls = tc->tls;
> > > +	}
> > 
> > Isn't this wrong now that invalid_args != NULL?
> > 
> > Shouldn't this rather be if (args == valid_args) ?
> 
> invalid_args is still NULL, invalid_address isn't though.

Ah, my bad. I wonder if we should set the invalid_args to the result of
tst_get_bad_address() as well, just for a consistency.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH V2 2/2] syscalls/clone3: New tests

[Cyril Hrubis]

Hi!
> What's happening is that parent is able to send the signal before the
> child calls pause() and so it hangs. If I simply remove pause() it all
> works fine for me, but is pidfd_send_signal() synchronous ? Does it
> wait until the time child signal is executed ? If yes, then we don't
> have a problem, else we may run into timing issue.
> 
> We can add a delay in parent before sending the signal, but that is
> still racy in worst cases.

In that case we can add a loop over a volatile varible changed from the
signal handler with a short usleep. Something as:

static volatile int wait_for_signal;

The do_child() would do:

	wait_for_signal = 1;

	TST_CHECKPOINT_WAKE(..);

	while (wait_for_signal)
		usleep(100);


And the handler would do:

	wait_for_signal = 0;

-- 
Cyril Hrubis
chrubis@suse.cz