[LTP] [PATCH v3 0/2] Rewrite exit_group01 test

[LTP] [PATCH v3 0/2] Rewrite exit_group01 test

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

exit_group01 test has been completely rewritten and tst_gettid() has been added
to support missing gettid() syscall from GLIBC

Andrea Cervesato (2):
  Add tst_gettid wrapper around gettid syscall
  Refactor exit_group01 using new API

 include/tst_pid.h                             |   8 +
 lib/tst_pid.c                                 |   5 +
 testcases/kernel/syscalls/exit_group/Makefile |   2 +
 .../kernel/syscalls/exit_group/exit_group01.c | 160 ++++++++++++------
 4 files changed, 124 insertions(+), 51 deletions(-)

-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 1/2] Add tst_gettid wrapper around gettid syscall

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/tst_pid.h | 8 ++++++++
 lib/tst_pid.c     | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/include/tst_pid.h b/include/tst_pid.h
index 774c845ce..6958d5d23 100644
--- a/include/tst_pid.h
+++ b/include/tst_pid.h
@@ -50,4 +50,12 @@ static inline int tst_get_free_pids(void (*cleanup_fn)(void))
  */
 pid_t tst_getpid(void);
 
+/*
+ * Direct gettid() syscall. Some glibc versions cache gettid() return value
+ * which can cause confusing issues for example in processes created by
+ * direct clone() syscall (without using the glibc wrapper). Use this function
+ * whenever the current process may be a child of the main test process.
+ */
+pid_t tst_gettid(void);
+
 #endif /* TST_PID_H__ */
diff --git a/lib/tst_pid.c b/lib/tst_pid.c
index cfaa5db36..4e9dc7a52 100644
--- a/lib/tst_pid.c
+++ b/lib/tst_pid.c
@@ -166,3 +166,8 @@ pid_t tst_getpid(void)
 {
 	return syscall(SYS_getpid);
 }
+
+pid_t tst_gettid(void)
+{
+	return syscall(SYS_gettid);
+}
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/2] Add tst_gettid wrapper around gettid syscall

[Cyril Hrubis]

Hi!
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  include/tst_pid.h | 8 ++++++++
>  lib/tst_pid.c     | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/tst_pid.h b/include/tst_pid.h
> index 774c845ce..6958d5d23 100644
> --- a/include/tst_pid.h
> +++ b/include/tst_pid.h
> @@ -50,4 +50,12 @@ static inline int tst_get_free_pids(void (*cleanup_fn)(void))
>   */
>  pid_t tst_getpid(void);
>  
> +/*
> + * Direct gettid() syscall. Some glibc versions cache gettid() return value
> + * which can cause confusing issues for example in processes created by
> + * direct clone() syscall (without using the glibc wrapper). Use this function
> + * whenever the current process may be a child of the main test process.
                                              ^
					      thread spawned from the main ...
> + */
> +pid_t tst_gettid(void);
> +
>  #endif /* TST_PID_H__ */
> diff --git a/lib/tst_pid.c b/lib/tst_pid.c
> index cfaa5db36..4e9dc7a52 100644
> --- a/lib/tst_pid.c
> +++ b/lib/tst_pid.c
> @@ -166,3 +166,8 @@ pid_t tst_getpid(void)
>  {
>  	return syscall(SYS_getpid);
>  }
> +
> +pid_t tst_gettid(void)
> +{
> +	return syscall(SYS_gettid);
> +}
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 2/2] Refactor exit_group01 using new API

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

We provided a different approach to exit_group() testing, spawning
multiple threads inside the child and checking if they get killed with
the parent process.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
Added thread_alive() function that checks existance of running threads, instead
of using kill(tid, 0) method.
tst_gettid() usage

 testcases/kernel/syscalls/exit_group/Makefile |   2 +
 .../kernel/syscalls/exit_group/exit_group01.c | 160 ++++++++++++------
 2 files changed, 111 insertions(+), 51 deletions(-)

diff --git a/testcases/kernel/syscalls/exit_group/Makefile b/testcases/kernel/syscalls/exit_group/Makefile
index 1273a4e9c..adbac3c51 100644
--- a/testcases/kernel/syscalls/exit_group/Makefile
+++ b/testcases/kernel/syscalls/exit_group/Makefile
@@ -3,6 +3,8 @@
 
 top_srcdir		?= ../../../..
 
+exit_group01: CFLAGS+=-pthread
+
 include $(top_srcdir)/include/mk/testcases.mk
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/exit_group/exit_group01.c b/testcases/kernel/syscalls/exit_group/exit_group01.c
index 5bf5b0218..2fcfff2f2 100644
--- a/testcases/kernel/syscalls/exit_group/exit_group01.c
+++ b/testcases/kernel/syscalls/exit_group/exit_group01.c
@@ -1,68 +1,126 @@
-/******************************************************************************
- * Copyright (c) Crackerjack Project., 2007                                   *
- * Ported to LTP by Manas Kumar Nayak <maknayak@in.ibm.com>                   *
- * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>                          *
- *                                                                            *
- * This program is free software;  you can redistribute it and/or modify      *
- * it under the terms of the GNU General Public License as published by       *
- * the Free Software Foundation; either version 2 of the License, or          *
- * (at your option) any later version.                                        *
- *                                                                            *
- * This program is distributed in the hope that it will be useful,            *
- * but WITHOUT ANY WARRANTY;  without even the implied warranty of            *
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See                  *
- * the GNU General Public License for more details.                           *
- *                                                                            *
- * You should have received a copy of the GNU General Public License          *
- * along with this program;  if not, write to the Free Software Foundation,   *
- * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA           *
- *                                                                            *
- ******************************************************************************/
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) Crackerjack Project., 2007
+ * Ported to LTP by Manas Kumar Nayak <maknayak@in.ibm.com>
+ * Copyright (c) 2015 Linux Test Project
+ * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>
+ * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
 
-#include <stdio.h>
-#include <errno.h>
-#include <linux/unistd.h>
-#include <sys/wait.h>
+/*\
+ * [Description]
+ *
+ * This test checks if exit_group() correctly ends a spawned child and all its
+ * running threads.
+ */
 
-#include "test.h"
-#include "safe_macros.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include "tst_safe_pthread.h"
+#include "tst_test.h"
 #include "lapi/syscalls.h"
 
-char *TCID = "exit_group01";
-int testno;
-int TST_TOTAL = 1;
+#define THREADS_NUM 10
+
+static pid_t *tids;
 
-static void verify_exit_group(void)
+static void thread_alive(const int tid)
 {
-	pid_t cpid, w;
-	int status;
+	char folder[128];
+    struct stat sb;
+	char state;
 
-	cpid = fork();
-	if (cpid == -1)
-		tst_brkm(TFAIL | TERRNO, NULL, "fork failed");
+	snprintf(folder, sizeof(folder), "/proc/%i/stat", tid);
 
-	if (cpid == 0) {
-		TEST(tst_syscall(__NR_exit_group, 4));
-	} else {
-		w = SAFE_WAIT(NULL, &status);
-
-		if (WIFEXITED(status) && (WEXITSTATUS(status) == 4)) {
-			tst_resm(TPASS, "exit_group() succeeded");
-		} else {
-			tst_resm(TFAIL | TERRNO,
-				 "exit_group() failed (wait status = %d)", w);
+	for (;;) {
+		if (stat(folder, &sb) == -1) {
+			if (errno != ENOENT)
+				tst_brk(TBROK | TERRNO, "stat() error");
+
+			break;
+		}
+
+		SAFE_FILE_SCANF(folder, "%*i %*s %c", &state);
+
+		if (state != 'S') {
+			tst_brk(TBROK,
+				"Thread %d is supposed to sleep but it's in '%c' state",
+				tid, state);
 		}
+
+		usleep(1000);
 	}
 }
 
-int main(int ac, char **av)
+static void *worker(void *arg)
 {
-	int lc;
+	int i = *((int *)arg);
 
-	tst_parse_opts(ac, av, NULL, NULL);
+	tids[i] = tst_gettid();
 
-	for (lc = 0; TEST_LOOPING(lc); lc++)
-		verify_exit_group();
+	TST_CHECKPOINT_WAKE(0);
+	pause();
 
-	tst_exit();
+	return arg;
 }
+
+static void spawn_threads(void)
+{
+	pthread_t threads[THREADS_NUM];
+
+	for (int i = 0; i < THREADS_NUM; i++) {
+		SAFE_PTHREAD_CREATE(&threads[i], NULL, worker, (void *)&i);
+		TST_CHECKPOINT_WAIT(0);
+
+		/* wait for paused thread */
+		TST_PROCESS_STATE_WAIT(tids[i], 'S', 0);
+	}
+}
+
+static void run(void)
+{
+	pid_t pid;
+	int status;
+
+	pid = SAFE_FORK();
+	if (!pid) {
+		spawn_threads();
+
+		TEST(tst_syscall(__NR_exit_group, 4));
+		if (TST_RET == -1)
+			tst_brk(TBROK | TERRNO, "exit_group() error");
+
+		return;
+	}
+
+	SAFE_WAITPID(pid, &status, 0);
+
+	for (int i = 0; i < THREADS_NUM; i++)
+		thread_alive(tids[i]);
+
+	TST_EXP_EXPR(WIFEXITED(status) && WEXITSTATUS(status) == 4,
+		"exit_group() succeeded");
+}
+
+static void setup(void)
+{
+	tids = SAFE_MMAP(
+		NULL,
+		sizeof(pid_t) * THREADS_NUM,
+		PROT_READ | PROT_WRITE,
+		MAP_SHARED | MAP_ANONYMOUS,
+		-1, 0);
+}
+
+static void cleanup(void)
+{
+	SAFE_MUNMAP(tids, sizeof(pid_t) * THREADS_NUM);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run,
+	.forks_child = 1,
+	.needs_checkpoints = 1,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/2] Refactor exit_group01 using new API

[Richard Palethorpe]

Hello,

Andrea Cervesato <andrea.cervesato@suse.de> writes:

> From: Andrea Cervesato <andrea.cervesato@suse.com>
>
> We provided a different approach to exit_group() testing, spawning
> multiple threads inside the child and checking if they get killed with
> the parent process.
>
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
> Added thread_alive() function that checks existance of running threads, instead
> of using kill(tid, 0) method.
> tst_gettid() usage
>
>  testcases/kernel/syscalls/exit_group/Makefile |   2 +
>  .../kernel/syscalls/exit_group/exit_group01.c | 160 ++++++++++++------
>  2 files changed, 111 insertions(+), 51 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/exit_group/Makefile b/testcases/kernel/syscalls/exit_group/Makefile
> index 1273a4e9c..adbac3c51 100644
> --- a/testcases/kernel/syscalls/exit_group/Makefile
> +++ b/testcases/kernel/syscalls/exit_group/Makefile
> @@ -3,6 +3,8 @@
>  
>  top_srcdir		?= ../../../..
>  
> +exit_group01: CFLAGS+=-pthread
> +
>  include $(top_srcdir)/include/mk/testcases.mk
>  
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/exit_group/exit_group01.c b/testcases/kernel/syscalls/exit_group/exit_group01.c
> index 5bf5b0218..2fcfff2f2 100644
> --- a/testcases/kernel/syscalls/exit_group/exit_group01.c
> +++ b/testcases/kernel/syscalls/exit_group/exit_group01.c
> @@ -1,68 +1,126 @@
> -/******************************************************************************
> - * Copyright (c) Crackerjack Project., 2007                                   *
> - * Ported to LTP by Manas Kumar Nayak <maknayak@in.ibm.com>                   *
> - * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>                          *
> - *                                                                            *
> - * This program is free software;  you can redistribute it and/or modify      *
> - * it under the terms of the GNU General Public License as published by       *
> - * the Free Software Foundation; either version 2 of the License, or          *
> - * (at your option) any later version.                                        *
> - *                                                                            *
> - * This program is distributed in the hope that it will be useful,            *
> - * but WITHOUT ANY WARRANTY;  without even the implied warranty of            *
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See                  *
> - * the GNU General Public License for more details.                           *
> - *                                                                            *
> - * You should have received a copy of the GNU General Public License          *
> - * along with this program;  if not, write to the Free Software Foundation,   *
> - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA           *
> - *                                                                            *
> - ******************************************************************************/
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) Crackerjack Project., 2007
> + * Ported to LTP by Manas Kumar Nayak <maknayak@in.ibm.com>
> + * Copyright (c) 2015 Linux Test Project
> + * Copyright (C) 2015 Cyril Hrubis <chrubis@suse.cz>
> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
>  
> -#include <stdio.h>
> -#include <errno.h>
> -#include <linux/unistd.h>
> -#include <sys/wait.h>
> +/*\
> + * [Description]
> + *
> + * This test checks if exit_group() correctly ends a spawned child and all its
> + * running threads.
> + */
>  
> -#include "test.h"
> -#include "safe_macros.h"
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include "tst_safe_pthread.h"
> +#include "tst_test.h"
>  #include "lapi/syscalls.h"
>  
> -char *TCID = "exit_group01";
> -int testno;
> -int TST_TOTAL = 1;
> +#define THREADS_NUM 10
> +
> +static pid_t *tids;
>  
> -static void verify_exit_group(void)
> +static void thread_alive(const int tid)
>  {
> -	pid_t cpid, w;
> -	int status;
> +	char folder[128];
> +    struct stat sb;
> +	char state;
>  
> -	cpid = fork();
> -	if (cpid == -1)
> -		tst_brkm(TFAIL | TERRNO, NULL, "fork failed");
> +	snprintf(folder, sizeof(folder), "/proc/%i/stat", tid);
>  
> -	if (cpid == 0) {
> -		TEST(tst_syscall(__NR_exit_group, 4));
> -	} else {
> -		w = SAFE_WAIT(NULL, &status);
> -
> -		if (WIFEXITED(status) && (WEXITSTATUS(status) == 4)) {
> -			tst_resm(TPASS, "exit_group() succeeded");
> -		} else {
> -			tst_resm(TFAIL | TERRNO,
> -				 "exit_group() failed (wait status = %d)", w);
> +	for (;;) {
> +		if (stat(folder, &sb) == -1) {
> +			if (errno != ENOENT)
> +				tst_brk(TBROK | TERRNO, "stat() error");
> +
> +			break;
> +		}
> +
> +		SAFE_FILE_SCANF(folder, "%*i %*s %c", &state);
> +
> +		if (state != 'S') {
> +			tst_brk(TBROK,
> +				"Thread %d is supposed to sleep but it's in '%c' state",
> +				tid, state);
>  		}

This could fail due to PID reuse or maybe because state == 'Z'.

Perhaps instead we could check the threads are put into the Z state?

I guess that the child threads are reparented to init and it reaps
them. So you will have to prevent that by setting PR_SET_CHILD_SUBREAPER
on the main test process with prctl.

The man pages are not clear on this though. I guess this is how it works
from reading the kernel code.

> +
> +		usleep(1000);

This probably should be sched_yield. It's hard to know what a sensible
sleep time would be for any given system.

>  	}
>  }
>  
> -int main(int ac, char **av)
> +static void *worker(void *arg)
>  {
> -	int lc;
> +	int i = *((int *)arg);
>  
> -	tst_parse_opts(ac, av, NULL, NULL);
> +	tids[i] = tst_gettid();
>  
> -	for (lc = 0; TEST_LOOPING(lc); lc++)
> -		verify_exit_group();
> +	TST_CHECKPOINT_WAKE(0);
> +	pause();
>  
> -	tst_exit();
> +	return arg;
>  }
> +
> +static void spawn_threads(void)
> +{
> +	pthread_t threads[THREADS_NUM];
> +
> +	for (int i = 0; i < THREADS_NUM; i++) {
> +		SAFE_PTHREAD_CREATE(&threads[i], NULL, worker, (void *)&i);
> +		TST_CHECKPOINT_WAIT(0);
> +
> +		/* wait for paused thread */
> +		TST_PROCESS_STATE_WAIT(tids[i], 'S', 0);
> +	}
> +}
> +
> +static void run(void)
> +{
> +	pid_t pid;
> +	int status;
> +
> +	pid = SAFE_FORK();
> +	if (!pid) {
> +		spawn_threads();
> +
> +		TEST(tst_syscall(__NR_exit_group, 4));
> +		if (TST_RET == -1)
> +			tst_brk(TBROK | TERRNO, "exit_group() error");
> +
> +		return;
> +	}
> +
> +	SAFE_WAITPID(pid, &status, 0);

You could move this after thread_alive and check that the child threads
all eventually transition to Z.

You maybe need to wait on all the child threads as well if this process
is set as a sub-reaper. I don't see anything in the kernel to suggest
waiting on a normal PID will wait for its child threads.

Also exit_group appears to signal all the child threads as we would have
to do in userland if the syscall didn't exist.

> +
> +	for (int i = 0; i < THREADS_NUM; i++)
> +		thread_alive(tids[i]);
> +
> +	TST_EXP_EXPR(WIFEXITED(status) && WEXITSTATUS(status) == 4,
> +		"exit_group() succeeded");
> +}
> +
> +static void setup(void)
> +{
> +	tids = SAFE_MMAP(
> +		NULL,
> +		sizeof(pid_t) * THREADS_NUM,
> +		PROT_READ | PROT_WRITE,
> +		MAP_SHARED | MAP_ANONYMOUS,
> +		-1, 0);
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_MUNMAP(tids, sizeof(pid_t) * THREADS_NUM);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.forks_child = 1,
> +	.needs_checkpoints = 1,
> +};
> -- 
> 2.35.3


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/2] Refactor exit_group01 using new API

[Cyril Hrubis]

Hi!
> This could fail due to PID reuse or maybe because state == 'Z'.
> 
> Perhaps instead we could check the threads are put into the Z state?
> 
> I guess that the child threads are reparented to init and it reaps
> them. So you will have to prevent that by setting PR_SET_CHILD_SUBREAPER
> on the main test process with prctl.
> 
> The man pages are not clear on this though. I guess this is how it works
> from reading the kernel code.

What about we:

1) allocate a piece of shared memory
2) each thread would run a loop that would increment a counter in that
   memory
3) the test process will wait() the main child
4) then monitor the shared memory to make sure that counters are not
   incremented anymore

Also child threads shouldn't be reparented to init, as long as at least
one of the threads runs the child process is technically alive and I
guess that wait() will not return until last thread in the group is
dead.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/2] Refactor exit_group01 using new API

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> This could fail due to PID reuse or maybe because state == 'Z'.
>> 
>> Perhaps instead we could check the threads are put into the Z state?
>> 
>> I guess that the child threads are reparented to init and it reaps
>> them. So you will have to prevent that by setting PR_SET_CHILD_SUBREAPER
>> on the main test process with prctl.
>> 
>> The man pages are not clear on this though. I guess this is how it works
>> from reading the kernel code.
>
> What about we:
>
> 1) allocate a piece of shared memory
> 2) each thread would run a loop that would increment a counter in that
>    memory
> 3) the test process will wait() the main child
> 4) then monitor the shared memory to make sure that counters are not
>    incremented anymore

+1

>
> Also child threads shouldn't be reparented to init, as long as at least
> one of the threads runs the child process is technically alive and I
> guess that wait() will not return until last thread in the group is
> dead.

OK, I looked again and it appears that indeed the thread group leader is
not reaped if there are threads in the group that are still alive
(wait_consider_task). Also it is only the thread group leader that can
enter a zombie state (exit_notify).

So this just leaves PID recycling as a potential issue IIRC.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp