[PATCH v2 1/2] tests: move common definitions and functions into pidfd.h

[PATCH v2 1/2] tests: move common definitions and functions into pidfd.h

[Suren Baghdasaryan]

Move definitions and functions used across different pidfd tests into
pidfd.h header.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 tools/testing/selftests/pidfd/pidfd.h          | 18 ++++++++++++++++++
 .../testing/selftests/pidfd/pidfd_open_test.c  |  5 -----
 tools/testing/selftests/pidfd/pidfd_test.c     | 10 ----------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 8452e910463f..db4377af6be7 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -16,6 +16,14 @@
 
 #include "../kselftest.h"
 
+#ifndef __NR_pidfd_open
+#define __NR_pidfd_open -1
+#endif
+
+#ifndef __NR_pidfd_send_signal
+#define __NR_pidfd_send_signal -1
+#endif
+
 /*
  * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
  * That means, when it wraps around any pid < 300 will be skipped.
@@ -53,5 +61,15 @@ int wait_for_pid(pid_t pid)
 	return WEXITSTATUS(status);
 }
 
+static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
+{
+	return syscall(__NR_pidfd_open, pid, flags);
+}
+
+static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
+					unsigned int flags)
+{
+	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
+}
 
 #endif /* __PIDFD_H */
diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
index 0377133dd6dc..b9fe75fc3e51 100644
--- a/tools/testing/selftests/pidfd/pidfd_open_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
@@ -22,11 +22,6 @@
 #include "pidfd.h"
 #include "../kselftest.h"
 
-static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
-{
-	return syscall(__NR_pidfd_open, pid, flags);
-}
-
 static int safe_int(const char *numstr, int *converted)
 {
 	char *err = NULL;
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index 7eaa8a3de262..17b2fd621726 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -21,10 +21,6 @@
 #include "pidfd.h"
 #include "../kselftest.h"
 
-#ifndef __NR_pidfd_send_signal
-#define __NR_pidfd_send_signal -1
-#endif
-
 #define str(s) _str(s)
 #define _str(s) #s
 #define CHILD_THREAD_MIN_WAIT 3 /* seconds */
@@ -47,12 +43,6 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
 #endif
 }
 
-static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
-					unsigned int flags)
-{
-	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
-}
-
 static int signal_received;
 
 static void set_signal_received_on_sigusr1(int sig)
-- 
2.22.0.709.g102302147b-goog

[PATCH v2 2/2] tests: add pidfd poll tests

[Suren Baghdasaryan]

This adds testing for polling on pidfd of a process being killed. Test runs
10000 iterations by default to stress test pidfd polling functionality.
It accepts an optional command-line parameter to override the number or
iterations to run.
Specifically, it tests for:
- pidfd_open on a child process succeeds
- pidfd_send_signal on a child process succeeds
- polling on pidfd succeeds and returns exactly one event
- returned event is POLLIN
- event is received within 3 secs of the process being killed

10000 iterations was chosen because of the race condition being tested
which is not consistently reproducible but usually is revealed after less
than 2000 iterations.
Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state")

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 tools/testing/selftests/pidfd/.gitignore      |   1 +
 tools/testing/selftests/pidfd/Makefile        |   2 +-
 .../testing/selftests/pidfd/pidfd_poll_test.c | 112 ++++++++++++++++++
 3 files changed, 114 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c

diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
index 16d84d117bc0..a67896347d34 100644
--- a/tools/testing/selftests/pidfd/.gitignore
+++ b/tools/testing/selftests/pidfd/.gitignore
@@ -1,2 +1,3 @@
 pidfd_open_test
+pidfd_poll_test
 pidfd_test
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index 720b2d884b3c..ed58b7108d18 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 CFLAGS += -g -I../../../../usr/include/ -lpthread
 
-TEST_GEN_PROGS := pidfd_test pidfd_open_test
+TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c
new file mode 100644
index 000000000000..d45c612a0fe5
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <poll.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "pidfd.h"
+#include "../kselftest.h"
+
+static bool timeout;
+
+static void handle_alarm(int sig)
+{
+	timeout = true;
+}
+
+int main(int argc, char **argv)
+{
+	struct pollfd fds;
+	int iter, nevents;
+	int nr_iterations = 10000;
+
+	fds.events = POLLIN;
+
+	if (argc > 2)
+		ksft_exit_fail_msg("Unexpected command line argument\n");
+
+	if (argc == 2) {
+		nr_iterations = atoi(argv[1]);
+		if (nr_iterations <= 0)
+			ksft_exit_fail_msg("invalid input parameter %s\n",
+					argv[1]);
+	}
+
+	ksft_print_msg("running pidfd poll test for %d iterations\n",
+		nr_iterations);
+
+	for (iter = 0; iter < nr_iterations; iter++) {
+		int pidfd;
+		int child_pid = fork();
+
+		if (child_pid < 0) {
+			if (errno == EAGAIN) {
+				iter--;
+				continue;
+			}
+			ksft_exit_fail_msg(
+				"%s - failed to fork a child process\n",
+				strerror(errno));
+		}
+
+		if (!child_pid) {
+			/* Child process just sleeps for a min and exits */
+			sleep(60);
+			exit(EXIT_SUCCESS);
+		}
+
+		/* Parent kills the child and waits for its death */
+		pidfd = sys_pidfd_open(child_pid, 0);
+		if (pidfd < 0)
+			ksft_exit_fail_msg("%s - pidfd_open failed\n",
+					strerror(errno));
+
+		/* Setup 3 sec alarm - plenty of time */
+		if (signal(SIGALRM, handle_alarm) == SIG_ERR)
+			ksft_exit_fail_msg("%s - signal failed\n",
+					strerror(errno));
+		alarm(3);
+
+		/* Send SIGKILL to the child */
+		if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0))
+			ksft_exit_fail_msg("%s - pidfd_send_signal failed\n",
+					strerror(errno));
+
+		/* Wait for the death notification */
+		fds.fd = pidfd;
+		nevents = poll(&fds, 1, -1);
+
+		/* Check for error conditions */
+		if (nevents < 0)
+			ksft_exit_fail_msg("%s - poll failed\n",
+					strerror(errno));
+
+		if (nevents != 1)
+			ksft_exit_fail_msg("unexpected poll result: %d\n",
+					nevents);
+
+		if (!(fds.revents & POLLIN))
+			ksft_exit_fail_msg(
+				"unexpected event type received: 0x%x\n",
+				fds.revents);
+
+		if (timeout)
+			ksft_exit_fail_msg(
+				"death notification wait timeout\n");
+
+		close(pidfd);
+	}
+
+	ksft_test_result_pass("pidfd poll test: pass\n");
+	return ksft_exit_pass();
+}
-- 
2.22.0.709.g102302147b-goog

Re: [PATCH v2 1/2] tests: move common definitions and functions into pidfd.h

[Suren Baghdasaryan]

I'm terribly sorry. I forgot to add a link to the original version of
this patch with Christian's comments. It's at:
https://lore.kernel.org/linux-kselftest/20190723173907.196488-1-surenb@google.com
and I think I addressed all comments there.
The patch should apply cleanly to the latest Linus' ToT (v5.3-rc1).
Thanks,
Suren.

On Wed, Jul 24, 2019 at 5:22 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> Move definitions and functions used across different pidfd tests into
> pidfd.h header.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  tools/testing/selftests/pidfd/pidfd.h          | 18 ++++++++++++++++++
>  .../testing/selftests/pidfd/pidfd_open_test.c  |  5 -----
>  tools/testing/selftests/pidfd/pidfd_test.c     | 10 ----------
>  3 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
> index 8452e910463f..db4377af6be7 100644
> --- a/tools/testing/selftests/pidfd/pidfd.h
> +++ b/tools/testing/selftests/pidfd/pidfd.h
> @@ -16,6 +16,14 @@
>
>  #include "../kselftest.h"
>
> +#ifndef __NR_pidfd_open
> +#define __NR_pidfd_open -1
> +#endif
> +
> +#ifndef __NR_pidfd_send_signal
> +#define __NR_pidfd_send_signal -1
> +#endif
> +
>  /*
>   * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
>   * That means, when it wraps around any pid < 300 will be skipped.
> @@ -53,5 +61,15 @@ int wait_for_pid(pid_t pid)
>         return WEXITSTATUS(status);
>  }
>
> +static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
> +{
> +       return syscall(__NR_pidfd_open, pid, flags);
> +}
> +
> +static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> +                                       unsigned int flags)
> +{
> +       return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> +}
>
>  #endif /* __PIDFD_H */
> diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
> index 0377133dd6dc..b9fe75fc3e51 100644
> --- a/tools/testing/selftests/pidfd/pidfd_open_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
> @@ -22,11 +22,6 @@
>  #include "pidfd.h"
>  #include "../kselftest.h"
>
> -static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
> -{
> -       return syscall(__NR_pidfd_open, pid, flags);
> -}
> -
>  static int safe_int(const char *numstr, int *converted)
>  {
>         char *err = NULL;
> diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
> index 7eaa8a3de262..17b2fd621726 100644
> --- a/tools/testing/selftests/pidfd/pidfd_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_test.c
> @@ -21,10 +21,6 @@
>  #include "pidfd.h"
>  #include "../kselftest.h"
>
> -#ifndef __NR_pidfd_send_signal
> -#define __NR_pidfd_send_signal -1
> -#endif
> -
>  #define str(s) _str(s)
>  #define _str(s) #s
>  #define CHILD_THREAD_MIN_WAIT 3 /* seconds */
> @@ -47,12 +43,6 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
>  #endif
>  }
>
> -static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> -                                       unsigned int flags)
> -{
> -       return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> -}
> -
>  static int signal_received;
>
>  static void set_signal_received_on_sigusr1(int sig)
> --
> 2.22.0.709.g102302147b-goog
>

Re: [PATCH v2 1/2] tests: move common definitions and functions into pidfd.h

[Christian Brauner]

On Wed, Jul 24, 2019 at 05:22:03PM -0700, Suren Baghdasaryan wrote:
> Move definitions and functions used across different pidfd tests into
> pidfd.h header.
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Christian Brauner <christian@brauner.io>

> ---
>  tools/testing/selftests/pidfd/pidfd.h          | 18 ++++++++++++++++++
>  .../testing/selftests/pidfd/pidfd_open_test.c  |  5 -----
>  tools/testing/selftests/pidfd/pidfd_test.c     | 10 ----------
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
> index 8452e910463f..db4377af6be7 100644
> --- a/tools/testing/selftests/pidfd/pidfd.h
> +++ b/tools/testing/selftests/pidfd/pidfd.h
> @@ -16,6 +16,14 @@
>  
>  #include "../kselftest.h"
>  
> +#ifndef __NR_pidfd_open
> +#define __NR_pidfd_open -1
> +#endif
> +
> +#ifndef __NR_pidfd_send_signal
> +#define __NR_pidfd_send_signal -1
> +#endif
> +
>  /*
>   * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
>   * That means, when it wraps around any pid < 300 will be skipped.
> @@ -53,5 +61,15 @@ int wait_for_pid(pid_t pid)
>  	return WEXITSTATUS(status);
>  }
>  
> +static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
> +{
> +	return syscall(__NR_pidfd_open, pid, flags);
> +}
> +
> +static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> +					unsigned int flags)
> +{
> +	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> +}
>  
>  #endif /* __PIDFD_H */
> diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
> index 0377133dd6dc..b9fe75fc3e51 100644
> --- a/tools/testing/selftests/pidfd/pidfd_open_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
> @@ -22,11 +22,6 @@
>  #include "pidfd.h"
>  #include "../kselftest.h"
>  
> -static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
> -{
> -	return syscall(__NR_pidfd_open, pid, flags);
> -}
> -
>  static int safe_int(const char *numstr, int *converted)
>  {
>  	char *err = NULL;
> diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
> index 7eaa8a3de262..17b2fd621726 100644
> --- a/tools/testing/selftests/pidfd/pidfd_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_test.c
> @@ -21,10 +21,6 @@
>  #include "pidfd.h"
>  #include "../kselftest.h"
>  
> -#ifndef __NR_pidfd_send_signal
> -#define __NR_pidfd_send_signal -1
> -#endif
> -
>  #define str(s) _str(s)
>  #define _str(s) #s
>  #define CHILD_THREAD_MIN_WAIT 3 /* seconds */
> @@ -47,12 +43,6 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
>  #endif
>  }
>  
> -static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> -					unsigned int flags)
> -{
> -	return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> -}
> -
>  static int signal_received;
>  
>  static void set_signal_received_on_sigusr1(int sig)
> -- 
> 2.22.0.709.g102302147b-goog
> 

Re: [PATCH v2 2/2] tests: add pidfd poll tests

[Christian Brauner]

On Wed, Jul 24, 2019 at 05:22:04PM -0700, Suren Baghdasaryan wrote:
> This adds testing for polling on pidfd of a process being killed. Test runs
> 10000 iterations by default to stress test pidfd polling functionality.
> It accepts an optional command-line parameter to override the number or
> iterations to run.
> Specifically, it tests for:
> - pidfd_open on a child process succeeds
> - pidfd_send_signal on a child process succeeds
> - polling on pidfd succeeds and returns exactly one event
> - returned event is POLLIN
> - event is received within 3 secs of the process being killed
> 
> 10000 iterations was chosen because of the race condition being tested
> which is not consistently reproducible but usually is revealed after less
> than 2000 iterations.
> Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state")
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Christian Brauner <christian@brauner.io>

> ---
>  tools/testing/selftests/pidfd/.gitignore      |   1 +
>  tools/testing/selftests/pidfd/Makefile        |   2 +-
>  .../testing/selftests/pidfd/pidfd_poll_test.c | 112 ++++++++++++++++++
>  3 files changed, 114 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c
> 
> diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
> index 16d84d117bc0..a67896347d34 100644
> --- a/tools/testing/selftests/pidfd/.gitignore
> +++ b/tools/testing/selftests/pidfd/.gitignore
> @@ -1,2 +1,3 @@
>  pidfd_open_test
> +pidfd_poll_test
>  pidfd_test
> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> index 720b2d884b3c..ed58b7108d18 100644
> --- a/tools/testing/selftests/pidfd/Makefile
> +++ b/tools/testing/selftests/pidfd/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  CFLAGS += -g -I../../../../usr/include/ -lpthread
>  
> -TEST_GEN_PROGS := pidfd_test pidfd_open_test
> +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test
>  
>  include ../lib.mk
>  
> diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> new file mode 100644
> index 000000000000..d45c612a0fe5
> --- /dev/null
> +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
> +#include <poll.h>
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <syscall.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "pidfd.h"
> +#include "../kselftest.h"
> +
> +static bool timeout;
> +
> +static void handle_alarm(int sig)
> +{
> +	timeout = true;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	struct pollfd fds;
> +	int iter, nevents;
> +	int nr_iterations = 10000;
> +
> +	fds.events = POLLIN;
> +
> +	if (argc > 2)
> +		ksft_exit_fail_msg("Unexpected command line argument\n");
> +
> +	if (argc == 2) {
> +		nr_iterations = atoi(argv[1]);
> +		if (nr_iterations <= 0)
> +			ksft_exit_fail_msg("invalid input parameter %s\n",
> +					argv[1]);
> +	}
> +
> +	ksft_print_msg("running pidfd poll test for %d iterations\n",
> +		nr_iterations);
> +
> +	for (iter = 0; iter < nr_iterations; iter++) {
> +		int pidfd;
> +		int child_pid = fork();
> +
> +		if (child_pid < 0) {
> +			if (errno == EAGAIN) {
> +				iter--;
> +				continue;
> +			}
> +			ksft_exit_fail_msg(
> +				"%s - failed to fork a child process\n",
> +				strerror(errno));
> +		}
> +
> +		if (!child_pid) {
> +			/* Child process just sleeps for a min and exits */
> +			sleep(60);
> +			exit(EXIT_SUCCESS);
> +		}
> +
> +		/* Parent kills the child and waits for its death */
> +		pidfd = sys_pidfd_open(child_pid, 0);
> +		if (pidfd < 0)
> +			ksft_exit_fail_msg("%s - pidfd_open failed\n",
> +					strerror(errno));
> +
> +		/* Setup 3 sec alarm - plenty of time */
> +		if (signal(SIGALRM, handle_alarm) == SIG_ERR)
> +			ksft_exit_fail_msg("%s - signal failed\n",
> +					strerror(errno));
> +		alarm(3);
> +
> +		/* Send SIGKILL to the child */
> +		if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0))
> +			ksft_exit_fail_msg("%s - pidfd_send_signal failed\n",
> +					strerror(errno));
> +
> +		/* Wait for the death notification */
> +		fds.fd = pidfd;
> +		nevents = poll(&fds, 1, -1);
> +
> +		/* Check for error conditions */
> +		if (nevents < 0)
> +			ksft_exit_fail_msg("%s - poll failed\n",
> +					strerror(errno));
> +
> +		if (nevents != 1)
> +			ksft_exit_fail_msg("unexpected poll result: %d\n",
> +					nevents);
> +
> +		if (!(fds.revents & POLLIN))
> +			ksft_exit_fail_msg(
> +				"unexpected event type received: 0x%x\n",
> +				fds.revents);
> +
> +		if (timeout)
> +			ksft_exit_fail_msg(
> +				"death notification wait timeout\n");
> +
> +		close(pidfd);
> +	}
> +
> +	ksft_test_result_pass("pidfd poll test: pass\n");
> +	return ksft_exit_pass();
> +}
> -- 
> 2.22.0.709.g102302147b-goog
> 

Re: [PATCH v2 1/2] tests: move common definitions and functions into pidfd.h

[Christian Brauner]

On Wed, Jul 24, 2019 at 05:34:04PM -0700, Suren Baghdasaryan wrote:
> I'm terribly sorry. I forgot to add a link to the original version of

No worries!

> this patch with Christian's comments. It's at:
> https://lore.kernel.org/linux-kselftest/20190723173907.196488-1-surenb@google.com
> and I think I addressed all comments there.
> The patch should apply cleanly to the latest Linus' ToT (v5.3-rc1).
> Thanks,
> Suren.

I'll pick this up.

Thanks!
Christian

> 
> On Wed, Jul 24, 2019 at 5:22 PM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > Move definitions and functions used across different pidfd tests into
> > pidfd.h header.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  tools/testing/selftests/pidfd/pidfd.h          | 18 ++++++++++++++++++
> >  .../testing/selftests/pidfd/pidfd_open_test.c  |  5 -----
> >  tools/testing/selftests/pidfd/pidfd_test.c     | 10 ----------
> >  3 files changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
> > index 8452e910463f..db4377af6be7 100644
> > --- a/tools/testing/selftests/pidfd/pidfd.h
> > +++ b/tools/testing/selftests/pidfd/pidfd.h
> > @@ -16,6 +16,14 @@
> >
> >  #include "../kselftest.h"
> >
> > +#ifndef __NR_pidfd_open
> > +#define __NR_pidfd_open -1
> > +#endif
> > +
> > +#ifndef __NR_pidfd_send_signal
> > +#define __NR_pidfd_send_signal -1
> > +#endif
> > +
> >  /*
> >   * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
> >   * That means, when it wraps around any pid < 300 will be skipped.
> > @@ -53,5 +61,15 @@ int wait_for_pid(pid_t pid)
> >         return WEXITSTATUS(status);
> >  }
> >
> > +static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
> > +{
> > +       return syscall(__NR_pidfd_open, pid, flags);
> > +}
> > +
> > +static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> > +                                       unsigned int flags)
> > +{
> > +       return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> > +}
> >
> >  #endif /* __PIDFD_H */
> > diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
> > index 0377133dd6dc..b9fe75fc3e51 100644
> > --- a/tools/testing/selftests/pidfd/pidfd_open_test.c
> > +++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
> > @@ -22,11 +22,6 @@
> >  #include "pidfd.h"
> >  #include "../kselftest.h"
> >
> > -static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
> > -{
> > -       return syscall(__NR_pidfd_open, pid, flags);
> > -}
> > -
> >  static int safe_int(const char *numstr, int *converted)
> >  {
> >         char *err = NULL;
> > diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
> > index 7eaa8a3de262..17b2fd621726 100644
> > --- a/tools/testing/selftests/pidfd/pidfd_test.c
> > +++ b/tools/testing/selftests/pidfd/pidfd_test.c
> > @@ -21,10 +21,6 @@
> >  #include "pidfd.h"
> >  #include "../kselftest.h"
> >
> > -#ifndef __NR_pidfd_send_signal
> > -#define __NR_pidfd_send_signal -1
> > -#endif
> > -
> >  #define str(s) _str(s)
> >  #define _str(s) #s
> >  #define CHILD_THREAD_MIN_WAIT 3 /* seconds */
> > @@ -47,12 +43,6 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
> >  #endif
> >  }
> >
> > -static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> > -                                       unsigned int flags)
> > -{
> > -       return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> > -}
> > -
> >  static int signal_received;
> >
> >  static void set_signal_received_on_sigusr1(int sig)
> > --
> > 2.22.0.709.g102302147b-goog
> >

Re: [PATCH v2 2/2] tests: add pidfd poll tests

[Yann Droneaud]

Hi,

Le mercredi 24 juillet 2019 à 17:22 -0700, Suren Baghdasaryan a écrit :
> This adds testing for polling on pidfd of a process being killed. Test runs
> 10000 iterations by default to stress test pidfd polling functionality.
> It accepts an optional command-line parameter to override the number or
> iterations to run.
> Specifically, it tests for:
> - pidfd_open on a child process succeeds
> - pidfd_send_signal on a child process succeeds
> - polling on pidfd succeeds and returns exactly one event
> - returned event is POLLIN
> - event is received within 3 secs of the process being killed
> 
> 10000 iterations was chosen because of the race condition being tested
> which is not consistently reproducible but usually is revealed after less
> than 2000 iterations.
> Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state")
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  tools/testing/selftests/pidfd/.gitignore      |   1 +
>  tools/testing/selftests/pidfd/Makefile        |   2 +-
>  .../testing/selftests/pidfd/pidfd_poll_test.c | 112 ++++++++++++++++++
>  3 files changed, 114 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c
> 
> diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
> index 16d84d117bc0..a67896347d34 100644
> --- a/tools/testing/selftests/pidfd/.gitignore
> +++ b/tools/testing/selftests/pidfd/.gitignore
> @@ -1,2 +1,3 @@
>  pidfd_open_test
> +pidfd_poll_test
>  pidfd_test
> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> index 720b2d884b3c..ed58b7108d18 100644
> --- a/tools/testing/selftests/pidfd/Makefile
> +++ b/tools/testing/selftests/pidfd/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  CFLAGS += -g -I../../../../usr/include/ -lpthread
>  
> -TEST_GEN_PROGS := pidfd_test pidfd_open_test
> +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test
>  
>  include ../lib.mk
>  
> diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> new file mode 100644
> index 000000000000..d45c612a0fe5
> --- /dev/null
> +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>
> +#include <poll.h>
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <syscall.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "pidfd.h"
> +#include "../kselftest.h"
> +
> +static bool timeout;
> +
> +static void handle_alarm(int sig)
> +{
> +	timeout = true;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	struct pollfd fds;
> +	int iter, nevents;
> +	int nr_iterations = 10000;
> +
> +	fds.events = POLLIN;
> +
> +	if (argc > 2)
> +		ksft_exit_fail_msg("Unexpected command line argument\n");
> +
> +	if (argc == 2) {
> +		nr_iterations = atoi(argv[1]);
> +		if (nr_iterations <= 0)
> +			ksft_exit_fail_msg("invalid input parameter %s\n",
> +					argv[1]);
> +	}
> +
> +	ksft_print_msg("running pidfd poll test for %d iterations\n",
> +		nr_iterations);
> +
> +	for (iter = 0; iter < nr_iterations; iter++) {
> +		int pidfd;
> +		int child_pid = fork();
> +
> +		if (child_pid < 0) {
> +			if (errno == EAGAIN) {
> +				iter--;
> +				continue;
> +			}
> +			ksft_exit_fail_msg(
> +				"%s - failed to fork a child process\n",
> +				strerror(errno));
> +		}
> +
> +		if (!child_pid) {
> +			/* Child process just sleeps for a min and exits */
> +			sleep(60);
> +			exit(EXIT_SUCCESS);
> +		}
> +
> +		/* Parent kills the child and waits for its death */
> +		pidfd = sys_pidfd_open(child_pid, 0);
> +		if (pidfd < 0)
> +			ksft_exit_fail_msg("%s - pidfd_open failed\n",
> +					strerror(errno));
> +
> +		/* Setup 3 sec alarm - plenty of time */
> +		if (signal(SIGALRM, handle_alarm) == SIG_ERR)
> +			ksft_exit_fail_msg("%s - signal failed\n",
> +					strerror(errno));
> +		alarm(3);
> +
> +		/* Send SIGKILL to the child */
> +		if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0))
> +			ksft_exit_fail_msg("%s - pidfd_send_signal failed\n",
> +					strerror(errno));
> +
> +		/* Wait for the death notification */
> +		fds.fd = pidfd;
> +		nevents = poll(&fds, 1, -1);
> +
> +		/* Check for error conditions */
> +		if (nevents < 0)
> +			ksft_exit_fail_msg("%s - poll failed\n",
> +					strerror(errno));
> +
> +		if (nevents != 1)
> +			ksft_exit_fail_msg("unexpected poll result: %d\n",
> +					nevents);
> +
> +		if (!(fds.revents & POLLIN))
> +			ksft_exit_fail_msg(
> +				"unexpected event type received: 0x%x\n",
> +				fds.revents);
> +
> +		if (timeout)
> +			ksft_exit_fail_msg(
> +				"death notification wait timeout\n");
> +
> +		close(pidfd);

There's no call to wait(), or alike function. Is it required for the
test to left zombies ?

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH v2 2/2] tests: add pidfd poll tests

[Suren Baghdasaryan]

On Thu, Jul 25, 2019 at 4:58 AM Yann Droneaud <ydroneaud@opteya.com> wrote:
>
> Hi,
>
> Le mercredi 24 juillet 2019 à 17:22 -0700, Suren Baghdasaryan a écrit :
> > This adds testing for polling on pidfd of a process being killed. Test runs
> > 10000 iterations by default to stress test pidfd polling functionality.
> > It accepts an optional command-line parameter to override the number or
> > iterations to run.
> > Specifically, it tests for:
> > - pidfd_open on a child process succeeds
> > - pidfd_send_signal on a child process succeeds
> > - polling on pidfd succeeds and returns exactly one event
> > - returned event is POLLIN
> > - event is received within 3 secs of the process being killed
> >
> > 10000 iterations was chosen because of the race condition being tested
> > which is not consistently reproducible but usually is revealed after less
> > than 2000 iterations.
> > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state")
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  tools/testing/selftests/pidfd/.gitignore      |   1 +
> >  tools/testing/selftests/pidfd/Makefile        |   2 +-
> >  .../testing/selftests/pidfd/pidfd_poll_test.c | 112 ++++++++++++++++++
> >  3 files changed, 114 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c
> >
> > diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
> > index 16d84d117bc0..a67896347d34 100644
> > --- a/tools/testing/selftests/pidfd/.gitignore
> > +++ b/tools/testing/selftests/pidfd/.gitignore
> > @@ -1,2 +1,3 @@
> >  pidfd_open_test
> > +pidfd_poll_test
> >  pidfd_test
> > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > index 720b2d884b3c..ed58b7108d18 100644
> > --- a/tools/testing/selftests/pidfd/Makefile
> > +++ b/tools/testing/selftests/pidfd/Makefile
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  CFLAGS += -g -I../../../../usr/include/ -lpthread
> >
> > -TEST_GEN_PROGS := pidfd_test pidfd_open_test
> > +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test
> >
> >  include ../lib.mk
> >
> > diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > new file mode 100644
> > index 000000000000..d45c612a0fe5
> > --- /dev/null
> > +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > @@ -0,0 +1,112 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define _GNU_SOURCE
> > +#include <errno.h>
> > +#include <linux/types.h>
> > +#include <linux/wait.h>
> > +#include <poll.h>
> > +#include <signal.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <syscall.h>
> > +#include <sys/wait.h>
> > +#include <unistd.h>
> > +
> > +#include "pidfd.h"
> > +#include "../kselftest.h"
> > +
> > +static bool timeout;
> > +
> > +static void handle_alarm(int sig)
> > +{
> > +     timeout = true;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +     struct pollfd fds;
> > +     int iter, nevents;
> > +     int nr_iterations = 10000;
> > +
> > +     fds.events = POLLIN;
> > +
> > +     if (argc > 2)
> > +             ksft_exit_fail_msg("Unexpected command line argument\n");
> > +
> > +     if (argc == 2) {
> > +             nr_iterations = atoi(argv[1]);
> > +             if (nr_iterations <= 0)
> > +                     ksft_exit_fail_msg("invalid input parameter %s\n",
> > +                                     argv[1]);
> > +     }
> > +
> > +     ksft_print_msg("running pidfd poll test for %d iterations\n",
> > +             nr_iterations);
> > +
> > +     for (iter = 0; iter < nr_iterations; iter++) {
> > +             int pidfd;
> > +             int child_pid = fork();
> > +
> > +             if (child_pid < 0) {
> > +                     if (errno == EAGAIN) {
> > +                             iter--;
> > +                             continue;
> > +                     }
> > +                     ksft_exit_fail_msg(
> > +                             "%s - failed to fork a child process\n",
> > +                             strerror(errno));
> > +             }
> > +
> > +             if (!child_pid) {
> > +                     /* Child process just sleeps for a min and exits */
> > +                     sleep(60);
> > +                     exit(EXIT_SUCCESS);
> > +             }
> > +
> > +             /* Parent kills the child and waits for its death */
> > +             pidfd = sys_pidfd_open(child_pid, 0);
> > +             if (pidfd < 0)
> > +                     ksft_exit_fail_msg("%s - pidfd_open failed\n",
> > +                                     strerror(errno));
> > +
> > +             /* Setup 3 sec alarm - plenty of time */
> > +             if (signal(SIGALRM, handle_alarm) == SIG_ERR)
> > +                     ksft_exit_fail_msg("%s - signal failed\n",
> > +                                     strerror(errno));
> > +             alarm(3);
> > +
> > +             /* Send SIGKILL to the child */
> > +             if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0))
> > +                     ksft_exit_fail_msg("%s - pidfd_send_signal failed\n",
> > +                                     strerror(errno));
> > +
> > +             /* Wait for the death notification */
> > +             fds.fd = pidfd;
> > +             nevents = poll(&fds, 1, -1);
> > +
> > +             /* Check for error conditions */
> > +             if (nevents < 0)
> > +                     ksft_exit_fail_msg("%s - poll failed\n",
> > +                                     strerror(errno));
> > +
> > +             if (nevents != 1)
> > +                     ksft_exit_fail_msg("unexpected poll result: %d\n",
> > +                                     nevents);
> > +
> > +             if (!(fds.revents & POLLIN))
> > +                     ksft_exit_fail_msg(
> > +                             "unexpected event type received: 0x%x\n",
> > +                             fds.revents);
> > +
> > +             if (timeout)
> > +                     ksft_exit_fail_msg(
> > +                             "death notification wait timeout\n");
> > +
> > +             close(pidfd);
>
> There's no call to wait(), or alike function. Is it required for the
> test to left zombies ?

The test checks that death notification gets sent from kernel, which
by design should happen for any process that has non-zero
task->exit_state and that includes zombies (see
https://elixir.bootlin.com/linux/v5.3-rc1/source/kernel/fork.c#L1731
for that condition). So IIUC this code, the poll() should succeed no
matter if the task is zombie or dead. Or do I misunderstand your
question?
Thanks,
Suren.

>
> Regards.
>
> --
> Yann Droneaud
> OPTEYA
>
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH v2 2/2] tests: add pidfd poll tests

[Yann Droneaud]

Hi,

Le jeudi 25 juillet 2019 à 08:48 -0700, Suren Baghdasaryan a écrit :
> On Thu, Jul 25, 2019 at 4:58 AM Yann Droneaud <ydroneaud@opteya.com> wrote:
> > Le mercredi 24 juillet 2019 à 17:22 -0700, Suren Baghdasaryan a écrit :
> > > This adds testing for polling on pidfd of a process being killed. Test runs
> > > 10000 iterations by default to stress test pidfd polling functionality.
> > > It accepts an optional command-line parameter to override the number or
> > > iterations to run.
> > > Specifically, it tests for:
> > > - pidfd_open on a child process succeeds
> > > - pidfd_send_signal on a child process succeeds
> > > - polling on pidfd succeeds and returns exactly one event
> > > - returned event is POLLIN
> > > - event is received within 3 secs of the process being killed
> > > 
> > > 10000 iterations was chosen because of the race condition being tested
> > > which is not consistently reproducible but usually is revealed after less
> > > than 2000 iterations.
> > > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state")
> > > 
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >  tools/testing/selftests/pidfd/.gitignore      |   1 +
> > >  tools/testing/selftests/pidfd/Makefile        |   2 +-
> > >  .../testing/selftests/pidfd/pidfd_poll_test.c | 112 ++++++++++++++++++
> > >  3 files changed, 114 insertions(+), 1 deletion(-)
> > >  create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > 
> > > diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
> > > index 16d84d117bc0..a67896347d34 100644
> > > --- a/tools/testing/selftests/pidfd/.gitignore
> > > +++ b/tools/testing/selftests/pidfd/.gitignore
> > > @@ -1,2 +1,3 @@
> > >  pidfd_open_test
> > > +pidfd_poll_test
> > >  pidfd_test
> > > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > > index 720b2d884b3c..ed58b7108d18 100644
> > > --- a/tools/testing/selftests/pidfd/Makefile
> > > +++ b/tools/testing/selftests/pidfd/Makefile
> > > @@ -1,7 +1,7 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > >  CFLAGS += -g -I../../../../usr/include/ -lpthread
> > > 
> > > -TEST_GEN_PROGS := pidfd_test pidfd_open_test
> > > +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test
> > > 
> > >  include ../lib.mk
> > > 
> > > diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > new file mode 100644
> > > index 000000000000..d45c612a0fe5
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > @@ -0,0 +1,112 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#define _GNU_SOURCE
> > > +#include <errno.h>
> > > +#include <linux/types.h>
> > > +#include <linux/wait.h>
> > > +#include <poll.h>
> > > +#include <signal.h>
> > > +#include <stdbool.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <syscall.h>
> > > +#include <sys/wait.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "pidfd.h"
> > > +#include "../kselftest.h"
> > > +
> > > +static bool timeout;
> > > +
> > > +static void handle_alarm(int sig)
> > > +{
> > > +     timeout = true;
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +     struct pollfd fds;
> > > +     int iter, nevents;
> > > +     int nr_iterations = 10000;
> > > +
> > > +     fds.events = POLLIN;
> > > +
> > > +     if (argc > 2)
> > > +             ksft_exit_fail_msg("Unexpected command line argument\n");
> > > +
> > > +     if (argc == 2) {
> > > +             nr_iterations = atoi(argv[1]);
> > > +             if (nr_iterations <= 0)
> > > +                     ksft_exit_fail_msg("invalid input parameter %s\n",
> > > +                                     argv[1]);
> > > +     }
> > > +
> > > +     ksft_print_msg("running pidfd poll test for %d iterations\n",
> > > +             nr_iterations);
> > > +
> > > +     for (iter = 0; iter < nr_iterations; iter++) {
> > > +             int pidfd;
> > > +             int child_pid = fork();
> > > +
> > > +             if (child_pid < 0) {
> > > +                     if (errno == EAGAIN) {
> > > +                             iter--;
> > > +                             continue;
> > > +                     }
> > > +                     ksft_exit_fail_msg(
> > > +                             "%s - failed to fork a child process\n",
> > > +                             strerror(errno));
> > > +             }
> > > +
> > > +             if (!child_pid) {
> > > +                     /* Child process just sleeps for a min and exits */
> > > +                     sleep(60);
> > > +                     exit(EXIT_SUCCESS);
> > > +             }
> > > +
> > > +             /* Parent kills the child and waits for its death */
> > > +             pidfd = sys_pidfd_open(child_pid, 0);
> > > +             if (pidfd < 0)
> > > +                     ksft_exit_fail_msg("%s - pidfd_open failed\n",
> > > +                                     strerror(errno));
> > > +
> > > +             /* Setup 3 sec alarm - plenty of time */
> > > +             if (signal(SIGALRM, handle_alarm) == SIG_ERR)
> > > +                     ksft_exit_fail_msg("%s - signal failed\n",
> > > +                                     strerror(errno));
> > > +             alarm(3);
> > > +
> > > +             /* Send SIGKILL to the child */
> > > +             if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0))
> > > +                     ksft_exit_fail_msg("%s - pidfd_send_signal failed\n",
> > > +                                     strerror(errno));
> > > +
> > > +             /* Wait for the death notification */
> > > +             fds.fd = pidfd;
> > > +             nevents = poll(&fds, 1, -1);
> > > +
> > > +             /* Check for error conditions */
> > > +             if (nevents < 0)
> > > +                     ksft_exit_fail_msg("%s - poll failed\n",
> > > +                                     strerror(errno));
> > > +
> > > +             if (nevents != 1)
> > > +                     ksft_exit_fail_msg("unexpected poll result: %d\n",
> > > +                                     nevents);
> > > +
> > > +             if (!(fds.revents & POLLIN))
> > > +                     ksft_exit_fail_msg(
> > > +                             "unexpected event type received: 0x%x\n",
> > > +                             fds.revents);
> > > +
> > > +             if (timeout)
> > > +                     ksft_exit_fail_msg(
> > > +                             "death notification wait timeout\n");
> > > +
> > > +             close(pidfd);
> > 
> > There's no call to wait(), or alike function. Is it required for the
> > test to left zombies ?
> 
> The test checks that death notification gets sent from kernel, which
> by design should happen for any process that has non-zero
> task->exit_state and that includes zombies (see
> https://elixir.bootlin.com/linux/v5.3-rc1/source/kernel/fork.c#L1731
> for that condition). So IIUC this code, the poll() should succeed no
> matter if the task is zombie or dead. Or do I misunderstand your
> question?

OK, so I think it would be better to call wait() (in the future
pidfd_wait()) to reap processes as they die.

I'm not afraid by one zombie, but herding an army of 10000 zombie
processes is another matter :)

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH v2 2/2] tests: add pidfd poll tests

[Suren Baghdasaryan]

On Thu, Jul 25, 2019 at 1:57 PM Yann Droneaud <ydroneaud@opteya.com> wrote:
>
> Hi,
>
> Le jeudi 25 juillet 2019 à 08:48 -0700, Suren Baghdasaryan a écrit :
> > On Thu, Jul 25, 2019 at 4:58 AM Yann Droneaud <ydroneaud@opteya.com> wrote:
> > > Le mercredi 24 juillet 2019 à 17:22 -0700, Suren Baghdasaryan a écrit :
> > > > This adds testing for polling on pidfd of a process being killed. Test runs
> > > > 10000 iterations by default to stress test pidfd polling functionality.
> > > > It accepts an optional command-line parameter to override the number or
> > > > iterations to run.
> > > > Specifically, it tests for:
> > > > - pidfd_open on a child process succeeds
> > > > - pidfd_send_signal on a child process succeeds
> > > > - polling on pidfd succeeds and returns exactly one event
> > > > - returned event is POLLIN
> > > > - event is received within 3 secs of the process being killed
> > > >
> > > > 10000 iterations was chosen because of the race condition being tested
> > > > which is not consistently reproducible but usually is revealed after less
> > > > than 2000 iterations.
> > > > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state")
> > > >
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > ---
> > > >  tools/testing/selftests/pidfd/.gitignore      |   1 +
> > > >  tools/testing/selftests/pidfd/Makefile        |   2 +-
> > > >  .../testing/selftests/pidfd/pidfd_poll_test.c | 112 ++++++++++++++++++
> > > >  3 files changed, 114 insertions(+), 1 deletion(-)
> > > >  create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > >
> > > > diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
> > > > index 16d84d117bc0..a67896347d34 100644
> > > > --- a/tools/testing/selftests/pidfd/.gitignore
> > > > +++ b/tools/testing/selftests/pidfd/.gitignore
> > > > @@ -1,2 +1,3 @@
> > > >  pidfd_open_test
> > > > +pidfd_poll_test
> > > >  pidfd_test
> > > > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> > > > index 720b2d884b3c..ed58b7108d18 100644
> > > > --- a/tools/testing/selftests/pidfd/Makefile
> > > > +++ b/tools/testing/selftests/pidfd/Makefile
> > > > @@ -1,7 +1,7 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > >  CFLAGS += -g -I../../../../usr/include/ -lpthread
> > > >
> > > > -TEST_GEN_PROGS := pidfd_test pidfd_open_test
> > > > +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test
> > > >
> > > >  include ../lib.mk
> > > >
> > > > diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > > new file mode 100644
> > > > index 000000000000..d45c612a0fe5
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > > @@ -0,0 +1,112 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +#define _GNU_SOURCE
> > > > +#include <errno.h>
> > > > +#include <linux/types.h>
> > > > +#include <linux/wait.h>
> > > > +#include <poll.h>
> > > > +#include <signal.h>
> > > > +#include <stdbool.h>
> > > > +#include <stdio.h>
> > > > +#include <stdlib.h>
> > > > +#include <string.h>
> > > > +#include <syscall.h>
> > > > +#include <sys/wait.h>
> > > > +#include <unistd.h>
> > > > +
> > > > +#include "pidfd.h"
> > > > +#include "../kselftest.h"
> > > > +
> > > > +static bool timeout;
> > > > +
> > > > +static void handle_alarm(int sig)
> > > > +{
> > > > +     timeout = true;
> > > > +}
> > > > +
> > > > +int main(int argc, char **argv)
> > > > +{
> > > > +     struct pollfd fds;
> > > > +     int iter, nevents;
> > > > +     int nr_iterations = 10000;
> > > > +
> > > > +     fds.events = POLLIN;
> > > > +
> > > > +     if (argc > 2)
> > > > +             ksft_exit_fail_msg("Unexpected command line argument\n");
> > > > +
> > > > +     if (argc == 2) {
> > > > +             nr_iterations = atoi(argv[1]);
> > > > +             if (nr_iterations <= 0)
> > > > +                     ksft_exit_fail_msg("invalid input parameter %s\n",
> > > > +                                     argv[1]);
> > > > +     }
> > > > +
> > > > +     ksft_print_msg("running pidfd poll test for %d iterations\n",
> > > > +             nr_iterations);
> > > > +
> > > > +     for (iter = 0; iter < nr_iterations; iter++) {
> > > > +             int pidfd;
> > > > +             int child_pid = fork();
> > > > +
> > > > +             if (child_pid < 0) {
> > > > +                     if (errno == EAGAIN) {
> > > > +                             iter--;
> > > > +                             continue;
> > > > +                     }
> > > > +                     ksft_exit_fail_msg(
> > > > +                             "%s - failed to fork a child process\n",
> > > > +                             strerror(errno));
> > > > +             }
> > > > +
> > > > +             if (!child_pid) {
> > > > +                     /* Child process just sleeps for a min and exits */
> > > > +                     sleep(60);
> > > > +                     exit(EXIT_SUCCESS);
> > > > +             }
> > > > +
> > > > +             /* Parent kills the child and waits for its death */
> > > > +             pidfd = sys_pidfd_open(child_pid, 0);
> > > > +             if (pidfd < 0)
> > > > +                     ksft_exit_fail_msg("%s - pidfd_open failed\n",
> > > > +                                     strerror(errno));
> > > > +
> > > > +             /* Setup 3 sec alarm - plenty of time */
> > > > +             if (signal(SIGALRM, handle_alarm) == SIG_ERR)
> > > > +                     ksft_exit_fail_msg("%s - signal failed\n",
> > > > +                                     strerror(errno));
> > > > +             alarm(3);
> > > > +
> > > > +             /* Send SIGKILL to the child */
> > > > +             if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0))
> > > > +                     ksft_exit_fail_msg("%s - pidfd_send_signal failed\n",
> > > > +                                     strerror(errno));
> > > > +
> > > > +             /* Wait for the death notification */
> > > > +             fds.fd = pidfd;
> > > > +             nevents = poll(&fds, 1, -1);
> > > > +
> > > > +             /* Check for error conditions */
> > > > +             if (nevents < 0)
> > > > +                     ksft_exit_fail_msg("%s - poll failed\n",
> > > > +                                     strerror(errno));
> > > > +
> > > > +             if (nevents != 1)
> > > > +                     ksft_exit_fail_msg("unexpected poll result: %d\n",
> > > > +                                     nevents);
> > > > +
> > > > +             if (!(fds.revents & POLLIN))
> > > > +                     ksft_exit_fail_msg(
> > > > +                             "unexpected event type received: 0x%x\n",
> > > > +                             fds.revents);
> > > > +
> > > > +             if (timeout)
> > > > +                     ksft_exit_fail_msg(
> > > > +                             "death notification wait timeout\n");
> > > > +
> > > > +             close(pidfd);
> > >
> > > There's no call to wait(), or alike function. Is it required for the
> > > test to left zombies ?
> >
> > The test checks that death notification gets sent from kernel, which
> > by design should happen for any process that has non-zero
> > task->exit_state and that includes zombies (see
> > https://elixir.bootlin.com/linux/v5.3-rc1/source/kernel/fork.c#L1731
> > for that condition). So IIUC this code, the poll() should succeed no
> > matter if the task is zombie or dead. Or do I misunderstand your
> > question?
>
> OK, so I think it would be better to call wait() (in the future
> pidfd_wait()) to reap processes as they die.
>
> I'm not afraid by one zombie, but herding an army of 10000 zombie
> processes is another matter :)

Ah, I see what you mean now. I can respin this patch with a
wait(child_pid, NULL, 0) after close(pidfd) call. Would that address
your concern?

> Regards.
>
> --
> Yann Droneaud
> OPTEYA
>
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH v2 2/2] tests: add pidfd poll tests

[Yann Droneaud]

Hi,

Le jeudi 25 juillet 2019 à 14:10 -0700, Suren Baghdasaryan a écrit :
> On Thu, Jul 25, 2019 at 1:57 PM Yann Droneaud <ydroneaud@opteya.com> wrote:
> > Le jeudi 25 juillet 2019 à 08:48 -0700, Suren Baghdasaryan a écrit :
> > > On Thu, Jul 25, 2019 at 4:58 AM Yann Droneaud <ydroneaud@opteya.com> wrote:
> > > > Le mercredi 24 juillet 2019 à 17:22 -0700, Suren Baghdasaryan a écrit :
> > > > > This adds testing for polling on pidfd of a process being killed. Test runs
> > > > > 10000 iterations by default to stress test pidfd polling functionality.
> > > > > It accepts an optional command-line parameter to override the number or
> > > > > iterations to run.
> > > > > Specifically, it tests for:
> > > > > - pidfd_open on a child process succeeds
> > > > > - pidfd_send_signal on a child process succeeds
> > > > > - polling on pidfd succeeds and returns exactly one event
> > > > > - returned event is POLLIN
> > > > > - event is received within 3 secs of the process being killed
> > > > > 
> > > > > 10000 iterations was chosen because of the race condition being tested
> > > > > which is not consistently reproducible but usually is revealed after less
> > > > > than 2000 iterations.
> > > > > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state")
> > > > > 
> > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > > ---
> > > > >  tools/testing/selftests/pidfd/.gitignore      |   1 +
> > > > >  tools/testing/selftests/pidfd/Makefile        |   2 +-
> > > > >  .../testing/selftests/pidfd/pidfd_poll_test.c | 112 ++++++++++++++++++
> > > > >  3 files changed, 114 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c

[...]

> > > > > diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > > > new file mode 100644
> > > > > index 000000000000..d45c612a0fe5
> > > > > --- /dev/null
> > > > > +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c
> > > > > @@ -0,0 +1,112 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +
> > > > > +#define _GNU_SOURCE
> > > > > +#include <errno.h>
> > > > > +#include <linux/types.h>
> > > > > +#include <linux/wait.h>
> > > > > +#include <poll.h>
> > > > > +#include <signal.h>
> > > > > +#include <stdbool.h>
> > > > > +#include <stdio.h>
> > > > > +#include <stdlib.h>
> > > > > +#include <string.h>
> > > > > +#include <syscall.h>
> > > > > +#include <sys/wait.h>
> > > > > +#include <unistd.h>
> > > > > +
> > > > > +#include "pidfd.h"
> > > > > +#include "../kselftest.h"
> > > > > +
> > > > > +static bool timeout;
> > > > > +
> > > > > +static void handle_alarm(int sig)
> > > > > +{
> > > > > +     timeout = true;
> > > > > +}
> > > > > +
> > > > > +int main(int argc, char **argv)
> > > > > +{
> > > > > +     struct pollfd fds;
> > > > > +     int iter, nevents;
> > > > > +     int nr_iterations = 10000;
> > > > > +
> > > > > +     fds.events = POLLIN;
> > > > > +
> > > > > +     if (argc > 2)
> > > > > +             ksft_exit_fail_msg("Unexpected command line argument\n");
> > > > > +
> > > > > +     if (argc == 2) {
> > > > > +             nr_iterations = atoi(argv[1]);
> > > > > +             if (nr_iterations <= 0)
> > > > > +                     ksft_exit_fail_msg("invalid input parameter %s\n",
> > > > > +                                     argv[1]);
> > > > > +     }
> > > > > +
> > > > > +     ksft_print_msg("running pidfd poll test for %d iterations\n",
> > > > > +             nr_iterations);
> > > > > +
> > > > > +     for (iter = 0; iter < nr_iterations; iter++) {
> > > > > +             int pidfd;
> > > > > +             int child_pid = fork();
> > > > > +
> > > > > +             if (child_pid < 0) {
> > > > > +                     if (errno == EAGAIN) {
> > > > > +                             iter--;
> > > > > +                             continue;
> > > > > +                     }
> > > > > +                     ksft_exit_fail_msg(
> > > > > +                             "%s - failed to fork a child process\n",
> > > > > +                             strerror(errno));
> > > > > +             }
> > > > > +
> > > > > +             if (!child_pid) {
> > > > > +                     /* Child process just sleeps for a min and exits */
> > > > > +                     sleep(60);
> > > > > +                     exit(EXIT_SUCCESS);
> > > > > +             }
> > > > > +
> > > > > +             /* Parent kills the child and waits for its death */
> > > > > +             pidfd = sys_pidfd_open(child_pid, 0);
> > > > > +             if (pidfd < 0)
> > > > > +                     ksft_exit_fail_msg("%s - pidfd_open failed\n",
> > > > > +                                     strerror(errno));
> > > > > +
> > > > > +             /* Setup 3 sec alarm - plenty of time */
> > > > > +             if (signal(SIGALRM, handle_alarm) == SIG_ERR)
> > > > > +                     ksft_exit_fail_msg("%s - signal failed\n",
> > > > > +                                     strerror(errno));
> > > > > +             alarm(3);
> > > > > +
> > > > > +             /* Send SIGKILL to the child */
> > > > > +             if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0))
> > > > > +                     ksft_exit_fail_msg("%s - pidfd_send_signal failed\n",
> > > > > +                                     strerror(errno));
> > > > > +
> > > > > +             /* Wait for the death notification */
> > > > > +             fds.fd = pidfd;
> > > > > +             nevents = poll(&fds, 1, -1);
> > > > > +
> > > > > +             /* Check for error conditions */
> > > > > +             if (nevents < 0)
> > > > > +                     ksft_exit_fail_msg("%s - poll failed\n",
> > > > > +                                     strerror(errno));
> > > > > +
> > > > > +             if (nevents != 1)
> > > > > +                     ksft_exit_fail_msg("unexpected poll result: %d\n",
> > > > > +                                     nevents);
> > > > > +
> > > > > +             if (!(fds.revents & POLLIN))
> > > > > +                     ksft_exit_fail_msg(
> > > > > +                             "unexpected event type received: 0x%x\n",
> > > > > +                             fds.revents);
> > > > > +
> > > > > +             if (timeout)
> > > > > +                     ksft_exit_fail_msg(
> > > > > +                             "death notification wait timeout\n");
> > > > > +
> > > > > +             close(pidfd);
> > > > 
> > > > There's no call to wait(), or alike function. Is it required for the
> > > > test to left zombies ?
> > > 
> > > The test checks that death notification gets sent from kernel, which
> > > by design should happen for any process that has non-zero
> > > task->exit_state and that includes zombies (see
> > > https://elixir.bootlin.com/linux/v5.3-rc1/source/kernel/fork.c#L1731
> > > for that condition). So IIUC this code, the poll() should succeed no
> > > matter if the task is zombie or dead. Or do I misunderstand your
> > > question?
> > 
> > OK, so I think it would be better to call wait() (in the future
> > pidfd_wait()) to reap processes as they die.
> > 
> > I'm not afraid by one zombie, but herding an army of 10000 zombie
> > processes is another matter :)
> 
> Ah, I see what you mean now. I can respin this patch with a
> wait(child_pid, NULL, 0) after close(pidfd) call. Would that address
> your concern?
> 

Yes, thanks !

Regards.

-- 
Yann Droneaud
OPTEYA