[PATCH v3 0/2] pidfd: waiting on processes through pidfds

[PATCH v3 0/2] pidfd: waiting on processes through pidfds

[Christian Brauner]

Hey everyone,

/* v3 */
This adds the ability to wait on processes using pidfds. This is one of
the few missing pieces to make it possible to manage processes using
only pidfds.

Now major changes have occured since v2. The only thing that was changed
has been to move the translation of a pidfd into a struct pid to a
dedicated helper that also does a get_pid() on it to keep the exit code
identical for all switch cases.
I've also added a test to verify that we correctly fail when an invalid
file descriptor is passed.

The core patch for waitid is pleasantly small. The largest change is
caused by adding proper tests for waitid(P_PIDFD).

/* v2 */
Link: https://lore.kernel.org/lkml/20190727085201.11743-1-christian@brauner.io

/* v1 */
Link: https://lore.kernel.org/lkml/20190726093934.13557-1-christian@brauner.io

/* v0 */
Link: https://lore.kernel.org/lkml/20190724144651.28272-1-christian@brauner.io

Christian

Christian Brauner (2):
  pidfd: add P_PIDFD to waitid()
  pidfd: add pidfd_wait tests

 include/linux/pid.h                        |   4 +
 include/uapi/linux/wait.h                  |   1 +
 kernel/exit.c                              |  33 ++-
 kernel/fork.c                              |   8 +
 kernel/signal.c                            |   7 +-
 tools/testing/selftests/pidfd/pidfd.h      |  25 ++
 tools/testing/selftests/pidfd/pidfd_test.c |  14 --
 tools/testing/selftests/pidfd/pidfd_wait.c | 258 +++++++++++++++++++++
 8 files changed, 331 insertions(+), 19 deletions(-)
 create mode 100644 tools/testing/selftests/pidfd/pidfd_wait.c

-- 
2.22.0

[PATCH v3 1/2] pidfd: add P_PIDFD to waitid()

[Christian Brauner]

This adds the P_PIDFD type to waitid().
One of the last remaining bits for the pidfd api is to make it possible
to wait on pidfds. With P_PIDFD added to waitid() the parts of userspace
that want to use the pidfd api to exclusively manage processes can do so
now.

One of the things this will unblock in the future is the ability to make
it possible to retrieve the exit status via waitid(P_PIDFD) for
non-parent processes if handed a _suitable_ pidfd that has this feature
set. This is similar to what you can do on FreeBSD with kqueue(). It
might even end up being possible to wait on a process as a non-parent if
an appropriate property is enabled on the pidfd.

With P_PIDFD no scoping of the process identified by the pidfd is
possible, i.e. it explicitly blocks things such as wait4(-1), wait4(0),
waitid(P_ALL), waitid(P_PGID) etc. It only allows for semantics
equivalent to wait4(pid), waitid(P_PID). Users that need scoping should
rely on pid-based wait*() syscalls for now.

Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Fernandes (Google) <joel@joelfernandes.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
v1:
- Linus Torvalds <torvalds@linux-foundation.org>:
  - use flag as discussed before, not a dedicated pidfd_wait() syscall
- Oleg Nesterov <oleg@redhat.com>:
  - use flag as discussed before, not a dedicated pidfd_wait() syscall

v2:
- Linus Torvalds <torvalds@linux-foundation.org>:
  - move find_get_pid() calls into switch statements to avoid checking
    the type argument twice

v3:
- Linus Torvalds <torvalds@linux-foundation.org>:
  - add a dedicated helper which does the transformation from pidfd to
    struct pid to avoid open coding this
  - take an additional reference to struct pid to keep the exit code
    unified
---
 include/linux/pid.h       |  4 ++++
 include/uapi/linux/wait.h |  1 +
 kernel/exit.c             | 33 ++++++++++++++++++++++++++++++---
 kernel/fork.c             |  8 ++++++++
 kernel/signal.c           |  7 +++++--
 5 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index 2a83e434db9d..9645b1194c98 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -72,6 +72,10 @@ extern struct pid init_struct_pid;
 
 extern const struct file_operations pidfd_fops;
 
+struct file;
+
+extern struct pid *pidfd_pid(const struct file *file);
+
 static inline struct pid *get_pid(struct pid *pid)
 {
 	if (pid)
diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
index ac49a220cf2a..85b809fc9f11 100644
--- a/include/uapi/linux/wait.h
+++ b/include/uapi/linux/wait.h
@@ -17,6 +17,7 @@
 #define P_ALL		0
 #define P_PID		1
 #define P_PGID		2
+#define P_PIDFD		3
 
 
 #endif /* _UAPI_LINUX_WAIT_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index a75b6a7f458a..64bb6893a37d 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1552,6 +1552,23 @@ static long do_wait(struct wait_opts *wo)
 	return retval;
 }
 
+static struct pid *pidfd_get_pid(unsigned int fd)
+{
+	struct fd f;
+	struct pid *pid;
+
+	f = fdget(fd);
+	if (!f.file)
+		return ERR_PTR(-EBADF);
+
+	pid = pidfd_pid(f.file);
+	if (!IS_ERR(pid))
+		get_pid(pid);
+
+	fdput(f);
+	return pid;
+}
+
 static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 			  int options, struct rusage *ru)
 {
@@ -1574,19 +1591,29 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 		type = PIDTYPE_PID;
 		if (upid <= 0)
 			return -EINVAL;
+
+		pid = find_get_pid(upid);
 		break;
 	case P_PGID:
 		type = PIDTYPE_PGID;
 		if (upid <= 0)
 			return -EINVAL;
+
+		pid = find_get_pid(upid);
+		break;
+	case P_PIDFD:
+		type = PIDTYPE_PID;
+		if (upid < 0)
+			return -EINVAL;
+
+		pid = pidfd_get_pid(upid);
+		if (IS_ERR(pid))
+			return PTR_ERR(pid);
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	if (type < PIDTYPE_MAX)
-		pid = find_get_pid(upid);
-
 	wo.wo_type	= type;
 	wo.wo_pid	= pid;
 	wo.wo_flags	= options;
diff --git a/kernel/fork.c b/kernel/fork.c
index d8ae0f1b4148..b169e2ca2d84 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1690,6 +1690,14 @@ static inline void rcu_copy_process(struct task_struct *p)
 #endif /* #ifdef CONFIG_TASKS_RCU */
 }
 
+struct pid *pidfd_pid(const struct file *file)
+{
+	if (file->f_op == &pidfd_fops)
+		return file->private_data;
+
+	return ERR_PTR(-EBADF);
+}
+
 static int pidfd_release(struct inode *inode, struct file *file)
 {
 	struct pid *pid = file->private_data;
diff --git a/kernel/signal.c b/kernel/signal.c
index 91b789dd6e72..2e567f64812f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3672,8 +3672,11 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, siginfo_t *info)
 
 static struct pid *pidfd_to_pid(const struct file *file)
 {
-	if (file->f_op == &pidfd_fops)
-		return file->private_data;
+	struct pid *pid;
+
+	pid = pidfd_pid(file);
+	if (!IS_ERR(pid))
+		return pid;
 
 	return tgid_pidfd_to_pid(file);
 }
-- 
2.22.0

[PATCH v3 2/2] pidfd: add pidfd_wait tests

[Christian Brauner]

Add tests for pidfd_wait() and CLONE_WAIT_PID:
- test that waitid(P_PIDFD) can wait on a pidfd
- test that waitid(P_PIDFD) can wait on a pidfd and return siginfo_t
- test that waitid(P_PIDFD) works with WEXITED
- test that waitid(P_PIDFD) works with WSTOPPED
- test that waitid(P_PIDFD) works with WUNTRACED
- test that waitid(P_PIDFD) works with WCONTINUED
- test that waitid(P_PIDFD) works with WNOWAIT
- test that waitid(P_PIDFD)works with WNOHANG

Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Fernandes (Google) <joel@joelfernandes.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
v1:
- Christian Brauner <christian@brauner.io>:
  - adapt tests to new P_PIDFD concept

v2: unchanged

v3:
- Christian Brauner <christian@brauner.io>:
- add a test for passing an invalid pidfd
---
 tools/testing/selftests/pidfd/pidfd.h      |  25 ++
 tools/testing/selftests/pidfd/pidfd_test.c |  14 --
 tools/testing/selftests/pidfd/pidfd_wait.c | 258 +++++++++++++++++++++
 3 files changed, 283 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/pidfd/pidfd_wait.c

diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 8452e910463f..7d7d0ca05e0b 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -16,6 +16,26 @@
 
 #include "../kselftest.h"
 
+#ifndef P_PIDFD
+#define P_PIDFD 3
+#endif
+
+#ifndef CLONE_PIDFD
+#define CLONE_PIDFD 0x00001000
+#endif
+
+#ifndef __NR_pidfd_open
+#define __NR_pidfd_open -1
+#endif
+
+#ifndef __NR_pidfd_send_signal
+#define __NR_pidfd_send_signal -1
+#endif
+
+#ifndef __NR_clone3
+#define __NR_clone3 -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 +73,10 @@ int wait_for_pid(pid_t pid)
 	return WEXITSTATUS(status);
 }
 
+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_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index 7eaa8a3de262..42e3eb494d72 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -21,20 +21,12 @@
 #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 */
 
 #define MAX_EVENTS 5
 
-#ifndef CLONE_PIDFD
-#define CLONE_PIDFD 0x00001000
-#endif
-
 static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
 {
 	size_t stack_size = 1024;
@@ -47,12 +39,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)
diff --git a/tools/testing/selftests/pidfd/pidfd_wait.c b/tools/testing/selftests/pidfd/pidfd_wait.c
new file mode 100644
index 000000000000..887c086d98ab
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_wait.c
@@ -0,0 +1,258 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <signal.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sched.h>
+#include <string.h>
+#include <sys/resource.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "pidfd.h"
+#include "../kselftest.h"
+
+#define ptr_to_u64(ptr) ((__u64)((uintptr_t)(ptr)))
+
+static pid_t sys_clone3(struct clone_args *args)
+{
+	return syscall(__NR_clone3, args, sizeof(struct clone_args));
+}
+
+static int sys_waitid(int which, pid_t pid, siginfo_t *info, int options,
+		      struct rusage *ru)
+{
+	return syscall(__NR_waitid, which, pid, info, options, ru);
+}
+
+static int test_pidfd_wait_simple(void)
+{
+	const char *test_name = "pidfd wait simple";
+	int pidfd = -1, status = 0;
+	pid_t parent_tid = -1;
+	struct clone_args args = {
+		.parent_tid = ptr_to_u64(&parent_tid),
+		.pidfd = ptr_to_u64(&pidfd),
+		.flags = CLONE_PIDFD | CLONE_PARENT_SETTID,
+		.exit_signal = SIGCHLD,
+	};
+	int ret;
+	pid_t pid;
+	siginfo_t info = {
+		.si_signo = 0,
+	};
+
+	pidfd = open("/proc/self", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
+	if (pidfd < 0)
+		ksft_exit_fail_msg("%s test: failed to open /proc/self %s\n",
+				   test_name, strerror(errno));
+
+	pid = sys_waitid(P_PIDFD, pidfd, &info, WEXITED, NULL);
+	if (pid == 0)
+		ksft_exit_fail_msg(
+			"%s test: succeeded to wait on invalid pidfd %s\n",
+			test_name, strerror(errno));
+	close(pidfd);
+	pidfd = -1;
+
+	pid = sys_clone3(&args);
+	if (pid < 0)
+		ksft_exit_fail_msg("%s test: failed to create new process %s\n",
+				   test_name, strerror(errno));
+
+	if (pid == 0)
+		exit(EXIT_SUCCESS);
+
+	pid = sys_waitid(P_PIDFD, pidfd, &info, WEXITED, NULL);
+	if (pid < 0)
+		ksft_exit_fail_msg(
+			"%s test: failed to wait on process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+
+	if (!WIFEXITED(info.si_status) || WEXITSTATUS(info.si_status))
+		ksft_exit_fail_msg(
+			"%s test: unexpected status received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+	close(pidfd);
+
+	if (info.si_signo != SIGCHLD)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_signo value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_signo, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_code != CLD_EXITED)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_code value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_code, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_pid != parent_tid)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_pid value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_pid, parent_tid, pidfd,
+			strerror(errno));
+
+	ksft_test_result_pass("%s test: Passed\n", test_name);
+	return 0;
+}
+
+static int test_pidfd_wait_states(void)
+{
+	const char *test_name = "pidfd wait states";
+	int pidfd = -1, status = 0;
+	pid_t parent_tid = -1;
+	struct clone_args args = {
+		.parent_tid = ptr_to_u64(&parent_tid),
+		.pidfd = ptr_to_u64(&pidfd),
+		.flags = CLONE_PIDFD | CLONE_PARENT_SETTID,
+		.exit_signal = SIGCHLD,
+	};
+	int ret;
+	pid_t pid;
+	siginfo_t info = {
+		.si_signo = 0,
+	};
+
+	pid = sys_clone3(&args);
+	if (pid < 0)
+		ksft_exit_fail_msg("%s test: failed to create new process %s\n",
+				   test_name, strerror(errno));
+
+	if (pid == 0) {
+		kill(getpid(), SIGSTOP);
+		kill(getpid(), SIGSTOP);
+		exit(EXIT_SUCCESS);
+	}
+
+	ret = sys_waitid(P_PIDFD, pidfd, &info, WSTOPPED, NULL);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s test: failed to wait on WSTOPPED process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+
+	if (info.si_signo != SIGCHLD)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_signo value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_signo, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_code != CLD_STOPPED)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_code value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_code, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_pid != parent_tid)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_pid value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_pid, parent_tid, pidfd,
+			strerror(errno));
+
+	ret = sys_pidfd_send_signal(pidfd, SIGCONT, NULL, 0);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s test: failed to send signal to process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+
+	ret = sys_waitid(P_PIDFD, pidfd, &info, WCONTINUED, NULL);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s test: failed to wait WCONTINUED on process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+
+	if (info.si_signo != SIGCHLD)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_signo value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_signo, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_code != CLD_CONTINUED)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_code value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_code, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_pid != parent_tid)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_pid value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_pid, parent_tid, pidfd,
+			strerror(errno));
+
+	ret = sys_waitid(P_PIDFD, pidfd, &info, WUNTRACED, NULL);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s test: failed to wait on WUNTRACED process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+
+	if (info.si_signo != SIGCHLD)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_signo value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_signo, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_code != CLD_STOPPED)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_code value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_code, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_pid != parent_tid)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_pid value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_pid, parent_tid, pidfd,
+			strerror(errno));
+
+	ret = sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s test: failed to send SIGKILL to process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+
+	ret = sys_waitid(P_PIDFD, pidfd, &info, WEXITED, NULL);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s test: failed to wait on WEXITED process with pid %d and pidfd %d: %s\n",
+			test_name, parent_tid, pidfd, strerror(errno));
+
+	if (info.si_signo != SIGCHLD)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_signo value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_signo, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_code != CLD_KILLED)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_code value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_code, parent_tid, pidfd,
+			strerror(errno));
+
+	if (info.si_pid != parent_tid)
+		ksft_exit_fail_msg(
+			"%s test: unexpected si_pid value %d received after waiting on process with pid %d and pidfd %d: %s\n",
+			test_name, info.si_pid, parent_tid, pidfd,
+			strerror(errno));
+
+	close(pidfd);
+
+	ksft_test_result_pass("%s test: Passed\n", test_name);
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	ksft_print_header();
+	ksft_set_plan(2);
+
+	test_pidfd_wait_simple();
+	test_pidfd_wait_states();
+
+	return ksft_exit_pass();
+}
-- 
2.22.0

Re: [PATCH v3 1/2] pidfd: add P_PIDFD to waitid()

[Oleg Nesterov]

On 07/28, Christian Brauner wrote:
>
> +static struct pid *pidfd_get_pid(unsigned int fd)
> +{
> +	struct fd f;
> +	struct pid *pid;
> +
> +	f = fdget(fd);
> +	if (!f.file)
> +		return ERR_PTR(-EBADF);
> +
> +	pid = pidfd_pid(f.file);
> +	if (!IS_ERR(pid))
> +		get_pid(pid);
> +
> +	fdput(f);
> +	return pid;
> +}

Agreed, this looks better than the previous version.

FWIW,

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH v3 1/2] pidfd: add P_PIDFD to waitid()

[Christian Brauner]

On Mon, Jul 29, 2019 at 04:27:44PM +0200, Oleg Nesterov wrote:
> On 07/28, Christian Brauner wrote:
> >
> > +static struct pid *pidfd_get_pid(unsigned int fd)
> > +{
> > +	struct fd f;
> > +	struct pid *pid;
> > +
> > +	f = fdget(fd);
> > +	if (!f.file)
> > +		return ERR_PTR(-EBADF);
> > +
> > +	pid = pidfd_pid(f.file);
> > +	if (!IS_ERR(pid))
> > +		get_pid(pid);
> > +
> > +	fdput(f);
> > +	return pid;
> > +}
> 
> Agreed, this looks better than the previous version.
> 
> FWIW,
> 
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Thanks Oleg!
Christian

Re: [PATCH v3 1/2] pidfd: add P_PIDFD to waitid()

[Kees Cook]

On Sun, Jul 28, 2019 at 12:22:29AM +0200, Christian Brauner wrote:
> diff --git a/kernel/exit.c b/kernel/exit.c
> index a75b6a7f458a..64bb6893a37d 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1552,6 +1552,23 @@ static long do_wait(struct wait_opts *wo)
>  	return retval;
>  }
>  
> +static struct pid *pidfd_get_pid(unsigned int fd)
> +{
> +	struct fd f;
> +	struct pid *pid;
> +
> +	f = fdget(fd);
> +	if (!f.file)
> +		return ERR_PTR(-EBADF);
> +
> +	pid = pidfd_pid(f.file);
> +	if (!IS_ERR(pid))
> +		get_pid(pid);
> +
> +	fdput(f);
> +	return pid;
> +}
> +
>  static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
>  			  int options, struct rusage *ru)
>  {
> @@ -1574,19 +1591,29 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
>  		type = PIDTYPE_PID;
>  		if (upid <= 0)
>  			return -EINVAL;
> +
> +		pid = find_get_pid(upid);
>  		break;
>  	case P_PGID:
>  		type = PIDTYPE_PGID;
>  		if (upid <= 0)
>  			return -EINVAL;
> +
> +		pid = find_get_pid(upid);
> +		break;
> +	case P_PIDFD:
> +		type = PIDTYPE_PID;
> +		if (upid < 0)
> +			return -EINVAL;
> +
> +		pid = pidfd_get_pid(upid);
> +		if (IS_ERR(pid))
> +			return PTR_ERR(pid);

I spent some time convincing myself that this early bail out was
correct. It seems this path is only reachable in the EBADF case, so that
makes sense. The other failure modes in this switch all give a NULL pid
so that the final do_wait() returns ECHILD. So, as long as that's
intentional (which I think it is), this all looks fine. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v3 2/2] pidfd: add pidfd_wait tests

[Kees Cook]

On Sun, Jul 28, 2019 at 12:22:30AM +0200, Christian Brauner wrote:
> Add tests for pidfd_wait() and CLONE_WAIT_PID:
> - test that waitid(P_PIDFD) can wait on a pidfd
> - test that waitid(P_PIDFD) can wait on a pidfd and return siginfo_t
> - test that waitid(P_PIDFD) works with WEXITED
> - test that waitid(P_PIDFD) works with WSTOPPED
> - test that waitid(P_PIDFD) works with WUNTRACED
> - test that waitid(P_PIDFD) works with WCONTINUED
> - test that waitid(P_PIDFD) works with WNOWAIT
> - test that waitid(P_PIDFD)works with WNOHANG

Reviewed-by: Kees Cook <keescook@chromium.org>

This all looks good to me! :) One note that doesn't apply to this patch
in particular, but might be nice to add (as I didn't see in the existing
tests) was testing for pathological conditions: passing in /dev/zero for
the pidfd, etc. (Maybe I missed those?)

-- 
Kees Cook

Re: [PATCH v3 2/2] pidfd: add pidfd_wait tests

[Christian Brauner]

On Mon, Jul 29, 2019 at 09:31:23AM -0700, Kees Cook wrote:
> On Sun, Jul 28, 2019 at 12:22:30AM +0200, Christian Brauner wrote:
> > Add tests for pidfd_wait() and CLONE_WAIT_PID:
> > - test that waitid(P_PIDFD) can wait on a pidfd
> > - test that waitid(P_PIDFD) can wait on a pidfd and return siginfo_t
> > - test that waitid(P_PIDFD) works with WEXITED
> > - test that waitid(P_PIDFD) works with WSTOPPED
> > - test that waitid(P_PIDFD) works with WUNTRACED
> > - test that waitid(P_PIDFD) works with WCONTINUED
> > - test that waitid(P_PIDFD) works with WNOWAIT
> > - test that waitid(P_PIDFD)works with WNOHANG
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> This all looks good to me! :) One note that doesn't apply to this patch
> in particular, but might be nice to add (as I didn't see in the existing
> tests) was testing for pathological conditions: passing in /dev/zero for
> the pidfd, etc. (Maybe I missed those?)

Yeah, I can probably just add one for this scenario. :)

Thanks!
Christian

Re: [PATCH v3 1/2] pidfd: add P_PIDFD to waitid()

[Christian Brauner]

On Mon, Jul 29, 2019 at 09:24:38AM -0700, Kees Cook wrote:
> On Sun, Jul 28, 2019 at 12:22:29AM +0200, Christian Brauner wrote:
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index a75b6a7f458a..64bb6893a37d 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -1552,6 +1552,23 @@ static long do_wait(struct wait_opts *wo)
> >  	return retval;
> >  }
> >  
> > +static struct pid *pidfd_get_pid(unsigned int fd)
> > +{
> > +	struct fd f;
> > +	struct pid *pid;
> > +
> > +	f = fdget(fd);
> > +	if (!f.file)
> > +		return ERR_PTR(-EBADF);
> > +
> > +	pid = pidfd_pid(f.file);
> > +	if (!IS_ERR(pid))
> > +		get_pid(pid);
> > +
> > +	fdput(f);
> > +	return pid;
> > +}
> > +
> >  static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
> >  			  int options, struct rusage *ru)
> >  {
> > @@ -1574,19 +1591,29 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
> >  		type = PIDTYPE_PID;
> >  		if (upid <= 0)
> >  			return -EINVAL;
> > +
> > +		pid = find_get_pid(upid);
> >  		break;
> >  	case P_PGID:
> >  		type = PIDTYPE_PGID;
> >  		if (upid <= 0)
> >  			return -EINVAL;
> > +
> > +		pid = find_get_pid(upid);
> > +		break;
> > +	case P_PIDFD:
> > +		type = PIDTYPE_PID;
> > +		if (upid < 0)
> > +			return -EINVAL;
> > +
> > +		pid = pidfd_get_pid(upid);
> > +		if (IS_ERR(pid))
> > +			return PTR_ERR(pid);
> 
> I spent some time convincing myself that this early bail out was
> correct. It seems this path is only reachable in the EBADF case, so that
> makes sense. The other failure modes in this switch all give a NULL pid
> so that the final do_wait() returns ECHILD. So, as long as that's
> intentional (which I think it is), this all looks fine. :)

Yep, it is. I didn't want to refactor that out of do_wait(). :)

Thanks!
Christian

> 
> Reviewed-by: Kees Cook <keescook@chromium.org>