Re: [PATCH 1/2] pid: add pidfd_open()

On Wed, May 15, 2019 at 3:04 AM 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:

Thanks for doing this work. I'm really looking forward to this new approach to process management.

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.

I'm glad it's easier now.

arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm64/include/asm/unistd32.h | 2 + arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 +

It'd be nice to arrange the system call tables so that we need to change only one file when adding a new system call.

[Snip system call wiring]

--- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -67,6 +67,7 @@ struct pid extern struct pid init_struct_pid;

extern const struct file_operations pidfd_fops; +extern int pidfd_create(struct pid *pid);

static inline struct pid *get_pid(struct pid *pid) { diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index e2870fe1be5b..989055e0b501 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -929,6 +929,7 @@ asmlinkage long sys_clock_adjtime32(clockid_t which_clock, struct old_timex32 __user *tx); asmlinkage long sys_syncfs(int fd); asmlinkage long sys_setns(int fd, int nstype); +asmlinkage long sys_pidfd_open(pid_t pid, unsigned int flags); asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg, unsigned int vlen, unsigned flags); asmlinkage long sys_process_vm_readv(pid_t pid, diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h index dee7292e1df6..94a257a93d20 100644 --- a/include/uapi/asm-generic/unistd.h +++ b/include/uapi/asm-generic/unistd.h @@ -832,9 +832,11 @@ __SYSCALL(__NR_io_uring_setup, sys_io_uring_setup) __SYSCALL(__NR_io_uring_enter, sys_io_uring_enter) #define __NR_io_uring_register 427 __SYSCALL(__NR_io_uring_register, sys_io_uring_register) +#define __NR_pidfd_open 428 +__SYSCALL(__NR_pidfd_open, sys_pidfd_open)

#undef __NR_syscalls -#define __NR_syscalls 428 +#define __NR_syscalls 429

/*

• 32 bit systems traditionally used different

diff --git a/kernel/fork.c b/kernel/fork.c index 737db1828437..980cc1d2b8d4 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1714,7 +1714,7 @@ const struct file_operations pidfd_fops = {

• Return: On success, a cloexec pidfd is returned.

•     On error, a negative errno number will be returned.
  

*/ -static int pidfd_create(struct pid *pid) +int pidfd_create(struct pid *pid) { int fd;

diff --git a/kernel/pid.c b/kernel/pid.c index 20881598bdfa..237d18d6ecb8 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,53 @@ 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 we support blocking operations on pidfds, we'll want to be able to put them in non-blocking mode. Does it make sense to accept and ignore O_NONBLOCK here now?

•   if (pid <= 0)
  

•           return -EINVAL;
  

WDYT of defining pid == 0 to mean "open myself"?

•   p = find_get_pid(pid);
  

•   if (!p)
  

•           return -ESRCH;
  

•   rcu_read_lock();
  

•   tsk = pid_task(p, PIDTYPE_PID);
  

•   if (!tsk)
  

•           ret = -ESRCH;
  

•   else if (unlikely(!thread_group_leader(tsk)))
  

•           ret = -EINVAL;
  

•   else
  

•           ret = 0;
  

•   rcu_read_unlock();
  

•   fd = ret ?: pidfd_create(p);
  

•   put_pid(p);
  

•   return fd;
  

+}