* [PATCH v1 2/2] tests: add pidfd_open() tests
2019-05-16 13:59 [PATCH v1 1/2] pid: add pidfd_open() Christian Brauner
@ 2019-05-16 13:59 ` Christian Brauner
2019-05-16 14:27 ` [PATCH v1 1/2] pid: add pidfd_open() Oleg Nesterov
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2019-05-16 13:59 UTC (permalink / raw)
To: jannh, oleg, viro, torvalds, linux-kernel, arnd
Cc: akpm, cyphar, dhowells, ebiederm, elena.reshetova, keescook,
luto, luto, tglx, linux-alpha, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-xtensa, linux-api, linux-arch,
linux-kselftest, joel, dancol, serge, Christian Brauner,
Michael Kerrisk (man-pages)
This adds testing for the new pidfd_open() syscalls. Specifically, we test:
- that no invalid flags can be passed to pidfd_open()
- that no invalid pid can be passed to pidfd_open()
- that a pidfd can be retrieved with pidfd_open()
- that the retrieved pidfd references the correct pid
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: Thomas Gleixner <tglx@linutronix.de>
Cc: Jann Horn <jannh@google.com>
Cc: David Howells <dhowells@redhat.com>
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.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>
Cc: linux-api@vger.kernel.org
---
v1: unchanged
---
tools/testing/selftests/pidfd/Makefile | 2 +-
tools/testing/selftests/pidfd/pidfd.h | 57 ++++++
.../testing/selftests/pidfd/pidfd_open_test.c | 170 ++++++++++++++++++
tools/testing/selftests/pidfd/pidfd_test.c | 41 +----
4 files changed, 229 insertions(+), 41 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd.h
create mode 100644 tools/testing/selftests/pidfd/pidfd_open_test.c
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index deaf8073bc06..b36c0be70848 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,6 +1,6 @@
CFLAGS += -g -I../../../../usr/include/
-TEST_GEN_PROGS := pidfd_test
+TEST_GEN_PROGS := pidfd_test pidfd_open_test
include ../lib.mk
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
new file mode 100644
index 000000000000..8452e910463f
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __PIDFD_H
+#define __PIDFD_H
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/mount.h>
+
+#include "../kselftest.h"
+
+/*
+ * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
+ * That means, when it wraps around any pid < 300 will be skipped.
+ * So we need to use a pid > 300 in order to test recycling.
+ */
+#define PID_RECYCLE 1000
+
+/*
+ * Define a few custom error codes for the child process to clearly indicate
+ * what is happening. This way we can tell the difference between a system
+ * error, a test error, etc.
+ */
+#define PIDFD_PASS 0
+#define PIDFD_FAIL 1
+#define PIDFD_ERROR 2
+#define PIDFD_SKIP 3
+#define PIDFD_XFAIL 4
+
+int wait_for_pid(pid_t pid)
+{
+ int status, ret;
+
+again:
+ ret = waitpid(pid, &status, 0);
+ if (ret == -1) {
+ if (errno == EINTR)
+ goto again;
+
+ return -1;
+ }
+
+ if (!WIFEXITED(status))
+ return -1;
+
+ return WEXITSTATUS(status);
+}
+
+
+#endif /* __PIDFD_H */
diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
new file mode 100644
index 000000000000..9b073c1ac618
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/mount.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#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;
+ long sli;
+
+ errno = 0;
+ sli = strtol(numstr, &err, 0);
+ if (errno == ERANGE && (sli == LONG_MAX || sli == LONG_MIN))
+ return -ERANGE;
+
+ if (errno != 0 && sli == 0)
+ return -EINVAL;
+
+ if (err == numstr || *err != '\0')
+ return -EINVAL;
+
+ if (sli > INT_MAX || sli < INT_MIN)
+ return -ERANGE;
+
+ *converted = (int)sli;
+ return 0;
+}
+
+static int char_left_gc(const char *buffer, size_t len)
+{
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ if (buffer[i] == ' ' ||
+ buffer[i] == '\t')
+ continue;
+
+ return i;
+ }
+
+ return 0;
+}
+
+static int char_right_gc(const char *buffer, size_t len)
+{
+ int i;
+
+ for (i = len - 1; i >= 0; i--) {
+ if (buffer[i] == ' ' ||
+ buffer[i] == '\t' ||
+ buffer[i] == '\n' ||
+ buffer[i] == '\0')
+ continue;
+
+ return i + 1;
+ }
+
+ return 0;
+}
+
+static char *trim_whitespace_in_place(char *buffer)
+{
+ buffer += char_left_gc(buffer, strlen(buffer));
+ buffer[char_right_gc(buffer, strlen(buffer))] = '\0';
+ return buffer;
+}
+
+static pid_t get_pid_from_fdinfo_file(int pidfd, const char *key, size_t keylen)
+{
+ int ret;
+ char path[512];
+ FILE *f;
+ size_t n = 0;
+ pid_t result = -1;
+ char *line = NULL;
+
+ snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", pidfd);
+
+ f = fopen(path, "re");
+ if (!f)
+ return -1;
+
+ while (getline(&line, &n, f) != -1) {
+ char *numstr;
+
+ if (strncmp(line, key, keylen))
+ continue;
+
+ numstr = trim_whitespace_in_place(line + 4);
+ ret = safe_int(numstr, &result);
+ if (ret < 0)
+ goto out;
+
+ break;
+ }
+
+out:
+ free(line);
+ fclose(f);
+ return result;
+}
+
+int main(int argc, char **argv)
+{
+ int pidfd = -1, ret = 1;
+ pid_t pid;
+
+ pidfd = sys_pidfd_open(-1, 0);
+ if (pidfd >= 0) {
+ ksft_print_msg(
+ "%s - succeeded to open pidfd for invalid pid -1\n",
+ strerror(errno));
+ goto on_error;
+ }
+ ksft_test_result_pass("do not allow invalid pid test: passed\n");
+ ksft_inc_pass_cnt();
+
+ pidfd = sys_pidfd_open(getpid(), 1);
+ if (pidfd >= 0) {
+ ksft_print_msg(
+ "%s - succeeded to open pidfd with invalid flag value specified\n",
+ strerror(errno));
+ goto on_error;
+ }
+ ksft_test_result_pass("do not allow invalid flag test: passed\n");
+ ksft_inc_pass_cnt();
+
+ pidfd = sys_pidfd_open(getpid(), 0);
+ if (pidfd < 0) {
+ ksft_print_msg("%s - failed to open pidfd\n", strerror(errno));
+ goto on_error;
+ }
+ ksft_test_result_pass("open a new pidfd test: passed\n");
+ ksft_inc_pass_cnt();
+
+ pid = get_pid_from_fdinfo_file(pidfd, "Pid:", sizeof("Pid:") - 1);
+ ksft_print_msg("pidfd %d refers to process with pid %d\n", pidfd, pid);
+
+ ret = 0;
+
+on_error:
+ if (pidfd >= 0)
+ close(pidfd);
+
+ return !ret ? ksft_exit_pass() : ksft_exit_fail();
+}
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index d59378a93782..f01de87249c9 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -14,6 +14,7 @@
#include <sys/wait.h>
#include <unistd.h>
+#include "pidfd.h"
#include "../kselftest.h"
static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
@@ -62,28 +63,6 @@ static int test_pidfd_send_signal_simple_success(void)
return 0;
}
-static int wait_for_pid(pid_t pid)
-{
- int status, ret;
-
-again:
- ret = waitpid(pid, &status, 0);
- if (ret == -1) {
- if (errno == EINTR)
- goto again;
-
- return -1;
- }
-
- if (ret != pid)
- goto again;
-
- if (!WIFEXITED(status))
- return -1;
-
- return WEXITSTATUS(status);
-}
-
static int test_pidfd_send_signal_exited_fail(void)
{
int pidfd, ret, saved_errno;
@@ -128,13 +107,6 @@ static int test_pidfd_send_signal_exited_fail(void)
return 0;
}
-/*
- * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
- * That means, when it wraps around any pid < 300 will be skipped.
- * So we need to use a pid > 300 in order to test recycling.
- */
-#define PID_RECYCLE 1000
-
/*
* Maximum number of cycles we allow. This is equivalent to PID_MAX_DEFAULT.
* If users set a higher limit or we have cycled PIDFD_MAX_DEFAULT number of
@@ -143,17 +115,6 @@ static int test_pidfd_send_signal_exited_fail(void)
*/
#define PIDFD_MAX_DEFAULT 0x8000
-/*
- * Define a few custom error codes for the child process to clearly indicate
- * what is happening. This way we can tell the difference between a system
- * error, a test error, etc.
- */
-#define PIDFD_PASS 0
-#define PIDFD_FAIL 1
-#define PIDFD_ERROR 2
-#define PIDFD_SKIP 3
-#define PIDFD_XFAIL 4
-
static int test_pidfd_send_signal_recycled_pid_fail(void)
{
int i, ret;
--
2.21.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 13:59 [PATCH v1 1/2] pid: add pidfd_open() Christian Brauner
2019-05-16 13:59 ` [PATCH v1 2/2] tests: add pidfd_open() tests Christian Brauner
@ 2019-05-16 14:27 ` Oleg Nesterov
2019-05-16 14:56 ` Aleksa Sarai
2019-05-16 14:57 ` Christian Brauner
2019-05-16 14:56 ` Geert Uytterhoeven
2019-05-18 9:48 ` Joel Fernandes
3 siblings, 2 replies; 13+ messages in thread
From: Oleg Nesterov @ 2019-05-16 14:27 UTC (permalink / raw)
To: Christian Brauner
Cc: jannh, viro, torvalds, linux-kernel, arnd, akpm, cyphar,
dhowells, ebiederm, elena.reshetova, keescook, luto, luto, tglx,
linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
joel, dancol, serge, Geert Uytterhoeven
On 05/16, Christian Brauner wrote:
>
> With the introduction of pidfds through CLONE_PIDFD it is possible to
> created pidfds at process creation time.
Now I am wondering why do we need CLONE_PIDFD, you can just do
pid = fork();
pidfd_open(pid);
> +SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> +{
> + int fd, ret;
> + struct pid *p;
> + struct task_struct *tsk;
> +
> + if (flags)
> + return -EINVAL;
> +
> + if (pid <= 0)
> + return -EINVAL;
> +
> + p = find_get_pid(pid);
> + if (!p)
> + return -ESRCH;
> +
> + ret = 0;
> + rcu_read_lock();
> + /*
> + * If this returns non-NULL the pid was used as a thread-group
> + * leader. Note, we race with exec here: If it changes the
> + * thread-group leader we might return the old leader.
> + */
> + tsk = pid_task(p, PIDTYPE_TGID);
> + if (!tsk)
> + ret = -ESRCH;
> + rcu_read_unlock();
> +
> + fd = ret ?: pidfd_create(p);
> + put_pid(p);
> + return fd;
> +}
Looks correct, feel free to add Reviewed-by: Oleg Nesterov <oleg@redhat.com>
But why do we need task_struct *tsk?
rcu_read_lock();
if (!pid_task(PIDTYPE_TGID))
ret = -ESRCH;
rcu_read_unlock();
and in fact we do not even need rcu_read_lock(), we could do
// shut up rcu_dereference_check()
rcu_lock_acquire(&rcu_lock_map);
if (!pid_task(PIDTYPE_TGID))
ret = -ESRCH;
rcu_lock_release(&rcu_lock_map);
Well... I won't insist, but the comment about the race with exec looks a bit
confusing to me. It is true, but we do not care at all, we are not going to
use the task_struct returned by pid_task().
Oleg.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 14:27 ` [PATCH v1 1/2] pid: add pidfd_open() Oleg Nesterov
@ 2019-05-16 14:56 ` Aleksa Sarai
2019-05-16 15:06 ` Oleg Nesterov
2019-05-16 14:57 ` Christian Brauner
1 sibling, 1 reply; 13+ messages in thread
From: Aleksa Sarai @ 2019-05-16 14:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, jannh, viro, torvalds, linux-kernel, arnd,
akpm, dhowells, ebiederm, elena.reshetova, keescook, luto, luto,
tglx, linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
joel, dancol, serge, Geert Uytterhoeven
[-- Attachment #1: Type: text/plain, Size: 642 bytes --]
On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> On 05/16, Christian Brauner wrote:
> >
> > With the introduction of pidfds through CLONE_PIDFD it is possible to
> > created pidfds at process creation time.
>
> Now I am wondering why do we need CLONE_PIDFD, you can just do
>
> pid = fork();
> pidfd_open(pid);
While the race window would be exceptionally short, there is the
possibility that the child will die and their pid will be recycled
before you do pidfd_open(). CLONE_PIDFD removes the race completely.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 14:56 ` Aleksa Sarai
@ 2019-05-16 15:06 ` Oleg Nesterov
2019-05-16 15:12 ` Aleksa Sarai
0 siblings, 1 reply; 13+ messages in thread
From: Oleg Nesterov @ 2019-05-16 15:06 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, jannh, viro, torvalds, linux-kernel, arnd,
akpm, dhowells, ebiederm, elena.reshetova, keescook, luto, luto,
tglx, linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
joel, dancol, serge, Geert Uytterhoeven
On 05/17, Aleksa Sarai wrote:
>
> On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 05/16, Christian Brauner wrote:
> > >
> > > With the introduction of pidfds through CLONE_PIDFD it is possible to
> > > created pidfds at process creation time.
> >
> > Now I am wondering why do we need CLONE_PIDFD, you can just do
> >
> > pid = fork();
> > pidfd_open(pid);
>
> While the race window would be exceptionally short, there is the
> possibility that the child will die
Yes,
> and their pid will be recycled
> before you do pidfd_open().
No.
Unless the caller's sub-thread does wait() before pidfd_open(), of course.
Or unless you do signal(SIGCHILD, SIG_IGN).
Oleg.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 15:06 ` Oleg Nesterov
@ 2019-05-16 15:12 ` Aleksa Sarai
2019-05-16 15:22 ` Oleg Nesterov
0 siblings, 1 reply; 13+ messages in thread
From: Aleksa Sarai @ 2019-05-16 15:12 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, jannh, viro, torvalds, linux-kernel, arnd,
akpm, dhowells, ebiederm, elena.reshetova, keescook, luto, luto,
tglx, linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
joel, dancol, serge, Geert Uytterhoeven
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> On 05/17, Aleksa Sarai wrote:
> > On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> > > On 05/16, Christian Brauner wrote:
> > > > With the introduction of pidfds through CLONE_PIDFD it is possible to
> > > > created pidfds at process creation time.
> > >
> > > Now I am wondering why do we need CLONE_PIDFD, you can just do
> > >
> > > pid = fork();
> > > pidfd_open(pid);
> >
> > While the race window would be exceptionally short, there is the
> > possibility that the child will die
>
> Yes,
>
> > and their pid will be recycled
> > before you do pidfd_open().
>
> No.
>
> Unless the caller's sub-thread does wait() before pidfd_open(), of course.
> Or unless you do signal(SIGCHILD, SIG_IGN).
What about CLONE_PARENT?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 15:12 ` Aleksa Sarai
@ 2019-05-16 15:22 ` Oleg Nesterov
2019-05-16 15:29 ` Christian Brauner
0 siblings, 1 reply; 13+ messages in thread
From: Oleg Nesterov @ 2019-05-16 15:22 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, jannh, viro, torvalds, linux-kernel, arnd,
akpm, dhowells, ebiederm, elena.reshetova, keescook, luto, luto,
tglx, linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
joel, dancol, serge, Geert Uytterhoeven
On 05/17, Aleksa Sarai wrote:
>
> On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 05/17, Aleksa Sarai wrote:
> > > On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> > > > On 05/16, Christian Brauner wrote:
> > > > > With the introduction of pidfds through CLONE_PIDFD it is possible to
> > > > > created pidfds at process creation time.
> > > >
> > > > Now I am wondering why do we need CLONE_PIDFD, you can just do
> > > >
> > > > pid = fork();
> > > > pidfd_open(pid);
> > >
> > > While the race window would be exceptionally short, there is the
> > > possibility that the child will die
> >
> > Yes,
> >
> > > and their pid will be recycled
> > > before you do pidfd_open().
> >
> > No.
> >
> > Unless the caller's sub-thread does wait() before pidfd_open(), of course.
> > Or unless you do signal(SIGCHILD, SIG_IGN).
>
> What about CLONE_PARENT?
I should have mentioned CLONE_PARENT ;)
Of course in this case the child can be reaped before pidfd_open(). But how often
do you or other people use clone(CLONE_PARENT) ? not to mention you can trivially
eliminate/detect this race if you really need this.
Don't get me wrong, I am not trying to say that CLONE_PIDFD is a bad idea.
But to me pidfd_open() is much more useful. Say, as a perl programmer I can easily
use pidfd_open(), but not CLONE_PIDFD.
Oleg.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 15:22 ` Oleg Nesterov
@ 2019-05-16 15:29 ` Christian Brauner
0 siblings, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2019-05-16 15:29 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Aleksa Sarai, jannh, viro, torvalds, linux-kernel, arnd, akpm,
dhowells, ebiederm, elena.reshetova, keescook, luto, luto, tglx,
linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
joel, dancol, serge, Geert Uytterhoeven
On Thu, May 16, 2019 at 05:22:53PM +0200, Oleg Nesterov wrote:
> On 05/17, Aleksa Sarai wrote:
> >
> > On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> > > On 05/17, Aleksa Sarai wrote:
> > > > On 2019-05-16, Oleg Nesterov <oleg@redhat.com> wrote:
> > > > > On 05/16, Christian Brauner wrote:
> > > > > > With the introduction of pidfds through CLONE_PIDFD it is possible to
> > > > > > created pidfds at process creation time.
> > > > >
> > > > > Now I am wondering why do we need CLONE_PIDFD, you can just do
> > > > >
> > > > > pid = fork();
> > > > > pidfd_open(pid);
> > > >
> > > > While the race window would be exceptionally short, there is the
> > > > possibility that the child will die
> > >
> > > Yes,
> > >
> > > > and their pid will be recycled
> > > > before you do pidfd_open().
> > >
> > > No.
> > >
> > > Unless the caller's sub-thread does wait() before pidfd_open(), of course.
> > > Or unless you do signal(SIGCHILD, SIG_IGN).
> >
> > What about CLONE_PARENT?
>
> I should have mentioned CLONE_PARENT ;)
>
> Of course in this case the child can be reaped before pidfd_open(). But how often
> do you or other people use clone(CLONE_PARENT) ? not to mention you can trivially
> eliminate/detect this race if you really need this.
>
> Don't get me wrong, I am not trying to say that CLONE_PIDFD is a bad idea.
>
> But to me pidfd_open() is much more useful. Say, as a perl programmer I can easily
> use pidfd_open(), but not CLONE_PIDFD.
Right, but for a libc, service- or container manager CLONE_PIDFD is much
nicer when spawning processes quickly. :) I think both are very good to
have.
Thanks, Oleg. As always super helpful reviews. :)
Christian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 14:27 ` [PATCH v1 1/2] pid: add pidfd_open() Oleg Nesterov
2019-05-16 14:56 ` Aleksa Sarai
@ 2019-05-16 14:57 ` Christian Brauner
1 sibling, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2019-05-16 14:57 UTC (permalink / raw)
To: Oleg Nesterov
Cc: jannh, viro, torvalds, linux-kernel, arnd, akpm, cyphar,
dhowells, ebiederm, elena.reshetova, keescook, luto, luto, tglx,
linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
joel, dancol, serge, Geert Uytterhoeven
On Thu, May 16, 2019 at 04:27:00PM +0200, Oleg Nesterov wrote:
> On 05/16, Christian Brauner wrote:
> >
> > With the introduction of pidfds through CLONE_PIDFD it is possible to
> > created pidfds at process creation time.
>
> Now I am wondering why do we need CLONE_PIDFD, you can just do
>
> pid = fork();
> pidfd_open(pid);
CLONE_PIDFD eliminates the race at the source and let's us avoid two
syscalls for the sake of one. That'll obviously matter even more when we
enable CLONE_THREAD | CLONE_PIDFD.
pidfd_open() is really just a necessity for anyone who does non-parent
process management aka LMK or service managers.
I also would like to reserve the ability at some point (e.g. with cloneX
or sm) to be able to specify specific additional flags at process
creation time that modify pidfd behavior.
>
> > +SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> > +{
> > + int fd, ret;
> > + struct pid *p;
> > + struct task_struct *tsk;
> > +
> > + if (flags)
> > + return -EINVAL;
> > +
> > + if (pid <= 0)
> > + return -EINVAL;
> > +
> > + p = find_get_pid(pid);
> > + if (!p)
> > + return -ESRCH;
> > +
> > + ret = 0;
> > + rcu_read_lock();
> > + /*
> > + * If this returns non-NULL the pid was used as a thread-group
> > + * leader. Note, we race with exec here: If it changes the
> > + * thread-group leader we might return the old leader.
> > + */
> > + tsk = pid_task(p, PIDTYPE_TGID);
> > + if (!tsk)
> > + ret = -ESRCH;
> > + rcu_read_unlock();
> > +
> > + fd = ret ?: pidfd_create(p);
> > + put_pid(p);
> > + return fd;
> > +}
>
> Looks correct, feel free to add Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>
> But why do we need task_struct *tsk?
>
> rcu_read_lock();
> if (!pid_task(PIDTYPE_TGID))
> ret = -ESRCH;
> rcu_read_unlock();
Sure, that's simpler. I'll rework and add your Reviewed-by.
>
> and in fact we do not even need rcu_read_lock(), we could do
>
> // shut up rcu_dereference_check()
> rcu_lock_acquire(&rcu_lock_map);
> if (!pid_task(PIDTYPE_TGID))
> ret = -ESRCH;
> rcu_lock_release(&rcu_lock_map);
>
> Well... I won't insist, but the comment about the race with exec looks a bit
> confusing to me. It is true, but we do not care at all, we are not going to
> use the task_struct returned by pid_task().
Yeah, I can remove it.
Thanks!
Christian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 13:59 [PATCH v1 1/2] pid: add pidfd_open() Christian Brauner
2019-05-16 13:59 ` [PATCH v1 2/2] tests: add pidfd_open() tests Christian Brauner
2019-05-16 14:27 ` [PATCH v1 1/2] pid: add pidfd_open() Oleg Nesterov
@ 2019-05-16 14:56 ` Geert Uytterhoeven
2019-05-16 14:58 ` Christian Brauner
2019-05-18 9:48 ` Joel Fernandes
3 siblings, 1 reply; 13+ messages in thread
From: Geert Uytterhoeven @ 2019-05-16 14:56 UTC (permalink / raw)
To: Christian Brauner, David Howells
Cc: Jann Horn, Oleg Nesterov, Al Viro, torvalds,
Linux Kernel Mailing List, Arnd Bergmann, linux-ia64,
Linux-sh list, linux-mips, Joel Fernandes,
open list:KERNEL SELFTEST FRAMEWORK, sparclinux, elena.reshetova,
Linux-Arch, linux-s390, Daniel Colascione, Serge E. Hallyn,
linux-xtensa, Kees Cook, linux-m68k, Andy Lutomirski,
Thomas Gleixner, Linux ARM, Parisc List, Linux API, cyphar,
Andy Lutomirski, Eric W. Biederman, alpha, Andrew Morton,
linuxppc-dev
Hi Christian, David,
On Thu, May 16, 2019 at 4:00 PM Christian Brauner <christian@brauner.io> wrote:
> This adds the pidfd_open() syscall. It allows a caller to retrieve pollable
> pidfds for a process which did not get created via CLONE_PIDFD, i.e. for a
> process that is created via traditional fork()/clone() calls that is only
> referenced by a PID:
>
> int pidfd = pidfd_open(1234, 0);
> ret = pidfd_send_signal(pidfd, SIGSTOP, NULL, 0);
>
> With the introduction of pidfds through CLONE_PIDFD it is possible to
> created pidfds at process creation time.
> However, a lot of processes get created with traditional PID-based calls
> such as fork() or clone() (without CLONE_PIDFD). For these processes a
> caller can currently not create a pollable pidfd. This is a huge problem
> for Android's low memory killer (LMK) and service managers such as systemd.
> Both are examples of tools that want to make use of pidfds to get reliable
> notification of process exit for non-parents (pidfd polling) and race-free
> signal sending (pidfd_send_signal()). They intend to switch to this API for
> process supervision/management as soon as possible. Having no way to get
> pollable pidfds from PID-only processes is one of the biggest blockers for
> them in adopting this api. With pidfd_open() making it possible to retrieve
> pidfd for PID-based processes we enable them to adopt this api.
>
> In line with Arnd's recent changes to consolidate syscall numbers across
> architectures, I have added the pidfd_open() syscall to all architectures
> at the same time.
> +428 common pidfd_open sys_pidfd_open
This number conflicts with "[PATCH 4/4] uapi: Wire up the mount API
syscalls on non-x86 arches", which is requested to be included before
rc1.
Note that none of this is part of linux-next.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 14:56 ` Geert Uytterhoeven
@ 2019-05-16 14:58 ` Christian Brauner
0 siblings, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2019-05-16 14:58 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: David Howells, Jann Horn, Oleg Nesterov, Al Viro, torvalds,
Linux Kernel Mailing List, Arnd Bergmann, linux-ia64,
Linux-sh list, linux-mips, Joel Fernandes,
open list:KERNEL SELFTEST FRAMEWORK, sparclinux, elena.reshetova,
Linux-Arch, linux-s390, Daniel Colascione, Serge E. Hallyn,
linux-xtensa, Kees Cook, linux-m68k, Andy Lutomirski,
Thomas Gleixner, Linux ARM, Parisc List, Linux API, cyphar,
Andy Lutomirski, Eric W. Biederman, alpha, Andrew Morton,
linuxppc-dev
On Thu, May 16, 2019 at 04:56:08PM +0200, Geert Uytterhoeven wrote:
> Hi Christian, David,
>
> On Thu, May 16, 2019 at 4:00 PM Christian Brauner <christian@brauner.io> wrote:
> > This adds the pidfd_open() syscall. It allows a caller to retrieve pollable
> > pidfds for a process which did not get created via CLONE_PIDFD, i.e. for a
> > process that is created via traditional fork()/clone() calls that is only
> > referenced by a PID:
> >
> > int pidfd = pidfd_open(1234, 0);
> > ret = pidfd_send_signal(pidfd, SIGSTOP, NULL, 0);
> >
> > With the introduction of pidfds through CLONE_PIDFD it is possible to
> > created pidfds at process creation time.
> > However, a lot of processes get created with traditional PID-based calls
> > such as fork() or clone() (without CLONE_PIDFD). For these processes a
> > caller can currently not create a pollable pidfd. This is a huge problem
> > for Android's low memory killer (LMK) and service managers such as systemd.
> > Both are examples of tools that want to make use of pidfds to get reliable
> > notification of process exit for non-parents (pidfd polling) and race-free
> > signal sending (pidfd_send_signal()). They intend to switch to this API for
> > process supervision/management as soon as possible. Having no way to get
> > pollable pidfds from PID-only processes is one of the biggest blockers for
> > them in adopting this api. With pidfd_open() making it possible to retrieve
> > pidfd for PID-based processes we enable them to adopt this api.
> >
> > In line with Arnd's recent changes to consolidate syscall numbers across
> > architectures, I have added the pidfd_open() syscall to all architectures
> > at the same time.
>
> > +428 common pidfd_open sys_pidfd_open
>
> This number conflicts with "[PATCH 4/4] uapi: Wire up the mount API
> syscalls on non-x86 arches", which is requested to be included before
> rc1.
Yep, already spotted this thanks to Arnd! Will change the syscall
numbers.
Thanks!
Christian
>
> Note that none of this is part of linux-next.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-16 13:59 [PATCH v1 1/2] pid: add pidfd_open() Christian Brauner
` (2 preceding siblings ...)
2019-05-16 14:56 ` Geert Uytterhoeven
@ 2019-05-18 9:48 ` Joel Fernandes
2019-05-18 10:04 ` Christian Brauner
3 siblings, 1 reply; 13+ messages in thread
From: Joel Fernandes @ 2019-05-18 9:48 UTC (permalink / raw)
To: Christian Brauner
Cc: jannh, oleg, viro, torvalds, linux-kernel, arnd, akpm, cyphar,
dhowells, ebiederm, elena.reshetova, keescook, luto, luto, tglx,
linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
dancol, serge, surenb, Geert Uytterhoeven
Hi Christian,
For next revision, could you also CC surenb@google.com as well? He is also
working on the low memory killer. And also suggest CC to
kernel-team@android.com. And mentioned some comments below, thanks.
On Thu, May 16, 2019 at 03:59:42PM +0200, Christian Brauner wrote:
[snip]
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 20881598bdfa..4afca3d6dcb8 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -38,6 +38,7 @@
> #include <linux/syscalls.h>
> #include <linux/proc_ns.h>
> #include <linux/proc_fs.h>
> +#include <linux/sched/signal.h>
> #include <linux/sched/task.h>
> #include <linux/idr.h>
>
> @@ -451,6 +452,55 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> return idr_get_next(&ns->idr, &nr);
> }
>
> +/**
> + * pidfd_open() - Open new pid file descriptor.
> + *
> + * @pid: pid for which to retrieve a pidfd
> + * @flags: flags to pass
> + *
> + * This creates a new pid file descriptor with the O_CLOEXEC flag set for
> + * the process identified by @pid. Currently, the process identified by
> + * @pid must be a thread-group leader. This restriction currently exists
> + * for all aspects of pidfds including pidfd creation (CLONE_PIDFD cannot
> + * be used with CLONE_THREAD) and pidfd polling (only supports thread group
> + * leaders).
> + *
> + * Return: On success, a cloexec pidfd is returned.
> + * On error, a negative errno number will be returned.
> + */
> +SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> +{
> + int fd, ret;
> + struct pid *p;
> + struct task_struct *tsk;
> +
> + if (flags)
> + return -EINVAL;
> +
> + if (pid <= 0)
> + return -EINVAL;
> +
> + p = find_get_pid(pid);
> + if (!p)
> + return -ESRCH;
> +
> + ret = 0;
> + rcu_read_lock();
> + /*
> + * If this returns non-NULL the pid was used as a thread-group
> + * leader. Note, we race with exec here: If it changes the
> + * thread-group leader we might return the old leader.
> + */
> + tsk = pid_task(p, PIDTYPE_TGID);
Just trying to understand the comment here. The issue is that we might either
return the new leader, or the old leader depending on the overlap with
concurrent de_thread right? In either case, we don't care though.
I suggest to remove the "Note..." part of the comment since it doesn't seem the
race is relevant here unless we are doing something else with tsk in the
function, but if you want to keep it that's also fine. Comment text should
probably should be 'return the new leader' though.
> + if (!tsk)
> + ret = -ESRCH;
Perhaps -EINVAL? AFAICS, this can only happen if a CLONE_THREAD pid was
passed as argument to pidfd_open which is invalid. But let me know what you
had in mind..
thanks,
- Joel
> + rcu_read_unlock();
> +
> + fd = ret ?: pidfd_create(p);
> + put_pid(p);
> + return fd;
> +}
> +
> void __init pid_idr_init(void)
> {
> /* Verify no one has done anything silly: */
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] pid: add pidfd_open()
2019-05-18 9:48 ` Joel Fernandes
@ 2019-05-18 10:04 ` Christian Brauner
0 siblings, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2019-05-18 10:04 UTC (permalink / raw)
To: Joel Fernandes
Cc: jannh, oleg, viro, torvalds, linux-kernel, arnd, akpm, cyphar,
dhowells, ebiederm, elena.reshetova, keescook, luto, luto, tglx,
linux-alpha, linux-arm-kernel, linux-ia64, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-xtensa, linux-api, linux-arch, linux-kselftest,
dancol, serge, surenb, Geert Uytterhoeven, kernel-team
On Sat, May 18, 2019 at 05:48:03AM -0400, Joel Fernandes wrote:
> Hi Christian,
>
> For next revision, could you also CC surenb@google.com as well? He is also
> working on the low memory killer. And also suggest CC to
> kernel-team@android.com. And mentioned some comments below, thanks.
Yip, totally. Just added them both to my Cc list. :)
(I saw you added Suren manually. I added the Android kernel team now too.)
>
> On Thu, May 16, 2019 at 03:59:42PM +0200, Christian Brauner wrote:
> [snip]
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 20881598bdfa..4afca3d6dcb8 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -38,6 +38,7 @@
> > #include <linux/syscalls.h>
> > #include <linux/proc_ns.h>
> > #include <linux/proc_fs.h>
> > +#include <linux/sched/signal.h>
> > #include <linux/sched/task.h>
> > #include <linux/idr.h>
> >
> > @@ -451,6 +452,55 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> > return idr_get_next(&ns->idr, &nr);
> > }
> >
> > +/**
> > + * pidfd_open() - Open new pid file descriptor.
> > + *
> > + * @pid: pid for which to retrieve a pidfd
> > + * @flags: flags to pass
> > + *
> > + * This creates a new pid file descriptor with the O_CLOEXEC flag set for
> > + * the process identified by @pid. Currently, the process identified by
> > + * @pid must be a thread-group leader. This restriction currently exists
> > + * for all aspects of pidfds including pidfd creation (CLONE_PIDFD cannot
> > + * be used with CLONE_THREAD) and pidfd polling (only supports thread group
> > + * leaders).
> > + *
> > + * Return: On success, a cloexec pidfd is returned.
> > + * On error, a negative errno number will be returned.
> > + */
> > +SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> > +{
> > + int fd, ret;
> > + struct pid *p;
> > + struct task_struct *tsk;
> > +
> > + if (flags)
> > + return -EINVAL;
> > +
> > + if (pid <= 0)
> > + return -EINVAL;
> > +
> > + p = find_get_pid(pid);
> > + if (!p)
> > + return -ESRCH;
> > +
> > + ret = 0;
> > + rcu_read_lock();
> > + /*
> > + * If this returns non-NULL the pid was used as a thread-group
> > + * leader. Note, we race with exec here: If it changes the
> > + * thread-group leader we might return the old leader.
> > + */
> > + tsk = pid_task(p, PIDTYPE_TGID);
>
> Just trying to understand the comment here. The issue is that we might either
> return the new leader, or the old leader depending on the overlap with
> concurrent de_thread right? In either case, we don't care though.
>
> I suggest to remove the "Note..." part of the comment since it doesn't seem the
> race is relevant here unless we are doing something else with tsk in the
> function, but if you want to keep it that's also fine. Comment text should
> probably should be 'return the new leader' though.
Nah, I actually removed the comment already independently (cf. see [1]).
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/commit/?h=pidfd_open&id=dcfc98c2d957bf3ac14b06414cb2cf4c673fc297
>
> > + if (!tsk)
> > + ret = -ESRCH;
>
> Perhaps -EINVAL? AFAICS, this can only happen if a CLONE_THREAD pid was
> passed as argument to pidfd_open which is invalid. But let me know what you
> had in mind..
Hm, from the kernel's perspective ESRCH is correct but I guess EINVAL is
nicer for userspace. So I don't have objections to using EINVAL. My
first version did too I think.
Thanks!
Christian
^ permalink raw reply [flat|nested] 13+ messages in thread