From: Christian Brauner <christian@brauner.io> To: Jonathan Kowalski <bl0pbl33p@gmail.com> Cc: Jann Horn <jannh@google.com>, Konstantin Khlebnikov <khlebnikov@yandex-team.ru>, Andy Lutomirski <luto@kernel.org>, David Howells <dhowells@redhat.com>, "Serge E. Hallyn" <serge@hallyn.com>, "Eric W. Biederman" <ebiederm@xmission.com>, Linux API <linux-api@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>, Kees Cook <keescook@chromium.org>, Alexey Dobriyan <adobriyan@gmail.com>, Thomas Gleixner <tglx@linutronix.de>, Michael Kerrisk-manpages <mtk.manpages@gmail.com>, "Dmitry V. Levin" <ldv@altlinux.org>, Andrew Morton <akpm@linux-foundation.org>, Oleg Nesterov <oleg@redhat.com>, Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>, Aleksa Sarai <cyphar@cyphar.com>, Al Viro <viro@zeniv.linux.org.uk>, Joel Fernandes <joel@joelfernandes.org>, Daniel Colascione <dancol@google.com> Subject: Re: [PATCH 3/4] signal: support pidctl() with pidfd_send_signal() Date: Mon, 25 Mar 2019 21:05:54 +0100 [thread overview] Message-ID: <20190325200552.fs4exsj3fsls7adu@brauner.io> (raw) In-Reply-To: <CAGLj2rGTK2xLhDhBjh1wSpAiuxbLBt-AwREB5kA8LeW4JFzgsg@mail.gmail.com> On Mon, Mar 25, 2019 at 06:28:53PM +0000, Jonathan Kowalski wrote: > On Mon, Mar 25, 2019 at 4:21 PM Christian Brauner <christian@brauner.io> wrote: > > > > Let pidfd_send_signal() use pidfds retrieved via pidctl(). With this patch > > pidfd_send_signal() becomes independent of procfs. This fullfils the > > request made when we merged the pidfd_send_signal() patchset. The > > pidfd_send_signal() syscall is now always available allowing for it to be > > used by users without procfs mounted or even users without procfs support > > compiled into the kernel. > > > > Signed-off-by: Christian Brauner <christian@brauner.io> > > Reviewed-by: David Howells <dhowells@redhat.com> > > Acked-by: Serge Hallyn <serge@hallyn.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: "Eric W. Biederman" <ebiederm@xmission.com> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Alexey Dobriyan <adobriyan@gmail.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Jann Horn <jannh@google.com > > Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> > > Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > > Cc: Jonathan Kowalski <bl0pbl33p@gmail.com> > > Cc: "Dmitry V. Levin" <ldv@altlinux.org> > > Cc: Andy Lutomirsky <luto@kernel.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Oleg Nesterov <oleg@redhat.com> > > Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com> > > Cc: Aleksa Sarai <cyphar@cyphar.com> > > Cc: Al Viro <viro@zeniv.linux.org.uk> > > --- > > kernel/signal.c | 20 +++++++++----------- > > kernel/sys_ni.c | 3 --- > > 2 files changed, 9 insertions(+), 14 deletions(-) > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > index b7953934aa99..d77183be1677 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -3513,7 +3513,6 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig) > > return kill_something_info(sig, &info, pid); > > } > > > > -#ifdef CONFIG_PROC_FS > > /* > > * Verify that the signaler and signalee either are in the same pid namespace > > * or that the signaler's pid namespace is an ancestor of the signalee's pid > > @@ -3521,16 +3520,13 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig) > > */ > > static bool access_pidfd_pidns(struct pid *pid) > > If it is agreed upon to remove the ability of using /proc/<PID> as a > pidfd pidfd_send_signal accepts, is lifting this check acceptable? > > The system call as is does not allow for a process to acquire a pidfd > without being in an ancestor namespace or the same namespace. Thus, > there are good reasons to allow for this to work and be able to work > around the limitations imposed by pid namespaces if userspace > explicitly decides to do so, by passing around the pidfd to some other > process. The original problem with this was - I think - that we hadn't settled what certain values in struct siginfo_t should be set to when signaling e.g. into ancestor pid namespaces. And if we did, we need to hook into a widely used function in the kernel (I don't have the appropriate thread handy atm) and we decided to punt on this until userspace really needs this feature. > > Also, you would need to translate this only once, when filling in the > structure. The other process is bound to its pid ns as long as it is > alive, therefore it would be simple without having to do anything > fancy at read side (unlike unix sockets). > > > { > > + int ret; > > struct pid_namespace *active = task_active_pid_ns(current); > > struct pid_namespace *p = ns_of_pid(pid); > > > > - for (;;) { > > - if (!p) > > - return false; > > - if (p == active) > > - break; > > - p = p->parent; > > - } > > + ret = pidnscmp(active, p); > > + if (ret < 0) > > + return false; > > > > return true; > > } > > @@ -3581,12 +3577,15 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > if (flags) > > return -EINVAL; > > > > - f = fdget_raw(pidfd); > > + f = fdget(pidfd); > > if (!f.file) > > return -EBADF; > > > > /* Is this a pidfd? */ > > - pid = tgid_pidfd_to_pid(f.file); > > + if (f.file->f_op == &pidfd_fops) > > + pid = f.file->private_data; > > + else > > + pid = tgid_pidfd_to_pid(f.file); > > if (IS_ERR(pid)) { > > ret = PTR_ERR(pid); > > goto err; > > @@ -3625,7 +3624,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > fdput(f); > > return ret; > > } > > -#endif /* CONFIG_PROC_FS */ > > > > static int > > do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info) > > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c > > index d21f4befaea4..4d9ae5ea6caf 100644 > > --- a/kernel/sys_ni.c > > +++ b/kernel/sys_ni.c > > @@ -167,9 +167,6 @@ COND_SYSCALL(syslog); > > > > /* kernel/sched/core.c */ > > > > -/* kernel/signal.c */ > > -COND_SYSCALL(pidfd_send_signal); > > - > > /* kernel/sys.c */ > > COND_SYSCALL(setregid); > > COND_SYSCALL(setgid); > > -- > > 2.21.0 > >
WARNING: multiple messages have this Message-ID (diff)
From: Christian Brauner <christian@brauner.io> To: Jonathan Kowalski <bl0pbl33p@gmail.com> Cc: Jann Horn <jannh@google.com>, Konstantin Khlebnikov <khlebnikov@yandex-team.ru>, Andy Lutomirski <luto@kernel.org>, David Howells <dhowells@redhat.com>, "Serge E. Hallyn" <serge@hallyn.com>, "Eric W. Biederman" <ebiederm@xmission.com>, Linux API <linux-api@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>, Kees Cook <keescook@chromium.org>, Alexey Dobriyan <adobriyan@gmail.com>, Thomas Gleixner <tglx@linutronix.de>, Michael Kerrisk-manpages <mtk.manpages@gmail.com>, "Dmitry V. Levin" <ldv@altlinux.org>, Andrew Morton <akpm@linux-foundation.org>, Oleg Nesterov <oleg@redhat.com>, Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>, Aleksa Sarai <cyphar@cyphar.com>, Al Viro <viro@zeniv.linux.org.uk> Subject: Re: [PATCH 3/4] signal: support pidctl() with pidfd_send_signal() Date: Mon, 25 Mar 2019 21:05:54 +0100 [thread overview] Message-ID: <20190325200552.fs4exsj3fsls7adu@brauner.io> (raw) In-Reply-To: <CAGLj2rGTK2xLhDhBjh1wSpAiuxbLBt-AwREB5kA8LeW4JFzgsg@mail.gmail.com> On Mon, Mar 25, 2019 at 06:28:53PM +0000, Jonathan Kowalski wrote: > On Mon, Mar 25, 2019 at 4:21 PM Christian Brauner <christian@brauner.io> wrote: > > > > Let pidfd_send_signal() use pidfds retrieved via pidctl(). With this patch > > pidfd_send_signal() becomes independent of procfs. This fullfils the > > request made when we merged the pidfd_send_signal() patchset. The > > pidfd_send_signal() syscall is now always available allowing for it to be > > used by users without procfs mounted or even users without procfs support > > compiled into the kernel. > > > > Signed-off-by: Christian Brauner <christian@brauner.io> > > Reviewed-by: David Howells <dhowells@redhat.com> > > Acked-by: Serge Hallyn <serge@hallyn.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: "Eric W. Biederman" <ebiederm@xmission.com> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Alexey Dobriyan <adobriyan@gmail.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Jann Horn <jannh@google.com > > Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> > > Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > > Cc: Jonathan Kowalski <bl0pbl33p@gmail.com> > > Cc: "Dmitry V. Levin" <ldv@altlinux.org> > > Cc: Andy Lutomirsky <luto@kernel.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Oleg Nesterov <oleg@redhat.com> > > Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com> > > Cc: Aleksa Sarai <cyphar@cyphar.com> > > Cc: Al Viro <viro@zeniv.linux.org.uk> > > --- > > kernel/signal.c | 20 +++++++++----------- > > kernel/sys_ni.c | 3 --- > > 2 files changed, 9 insertions(+), 14 deletions(-) > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > index b7953934aa99..d77183be1677 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -3513,7 +3513,6 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig) > > return kill_something_info(sig, &info, pid); > > } > > > > -#ifdef CONFIG_PROC_FS > > /* > > * Verify that the signaler and signalee either are in the same pid namespace > > * or that the signaler's pid namespace is an ancestor of the signalee's pid > > @@ -3521,16 +3520,13 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig) > > */ > > static bool access_pidfd_pidns(struct pid *pid) > > If it is agreed upon to remove the ability of using /proc/<PID> as a > pidfd pidfd_send_signal accepts, is lifting this check acceptable? > > The system call as is does not allow for a process to acquire a pidfd > without being in an ancestor namespace or the same namespace. Thus, > there are good reasons to allow for this to work and be able to work > around the limitations imposed by pid namespaces if userspace > explicitly decides to do so, by passing around the pidfd to some other > process. The original problem with this was - I think - that we hadn't settled what certain values in struct siginfo_t should be set to when signaling e.g. into ancestor pid namespaces. And if we did, we need to hook into a widely used function in the kernel (I don't have the appropriate thread handy atm) and we decided to punt on this until userspace really needs this feature. > > Also, you would need to translate this only once, when filling in the > structure. The other process is bound to its pid ns as long as it is > alive, therefore it would be simple without having to do anything > fancy at read side (unlike unix sockets). > > > { > > + int ret; > > struct pid_namespace *active = task_active_pid_ns(current); > > struct pid_namespace *p = ns_of_pid(pid); > > > > - for (;;) { > > - if (!p) > > - return false; > > - if (p == active) > > - break; > > - p = p->parent; > > - } > > + ret = pidnscmp(active, p); > > + if (ret < 0) > > + return false; > > > > return true; > > } > > @@ -3581,12 +3577,15 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > if (flags) > > return -EINVAL; > > > > - f = fdget_raw(pidfd); > > + f = fdget(pidfd); > > if (!f.file) > > return -EBADF; > > > > /* Is this a pidfd? */ > > - pid = tgid_pidfd_to_pid(f.file); > > + if (f.file->f_op == &pidfd_fops) > > + pid = f.file->private_data; > > + else > > + pid = tgid_pidfd_to_pid(f.file); > > if (IS_ERR(pid)) { > > ret = PTR_ERR(pid); > > goto err; > > @@ -3625,7 +3624,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > fdput(f); > > return ret; > > } > > -#endif /* CONFIG_PROC_FS */ > > > > static int > > do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info) > > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c > > index d21f4befaea4..4d9ae5ea6caf 100644 > > --- a/kernel/sys_ni.c > > +++ b/kernel/sys_ni.c > > @@ -167,9 +167,6 @@ COND_SYSCALL(syslog); > > > > /* kernel/sched/core.c */ > > > > -/* kernel/signal.c */ > > -COND_SYSCALL(pidfd_send_signal); > > - > > /* kernel/sys.c */ > > COND_SYSCALL(setregid); > > COND_SYSCALL(setgid); > > -- > > 2.21.0 > >
next prev parent reply other threads:[~2019-03-25 20:06 UTC|newest] Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-25 16:20 [PATCH 0/4] pid: add pidctl() Christian Brauner 2019-03-25 16:20 ` [PATCH 1/4] Make anon_inodes unconditional Christian Brauner 2019-03-25 16:20 ` [PATCH 2/4] pid: add pidctl() Christian Brauner 2019-03-25 17:20 ` Mika Penttilä 2019-03-25 19:59 ` Christian Brauner 2019-03-25 19:59 ` Christian Brauner 2019-03-25 18:18 ` Jann Horn 2019-03-25 18:18 ` Jann Horn 2019-03-25 19:58 ` Christian Brauner 2019-03-25 19:58 ` Christian Brauner 2019-03-26 16:07 ` Joel Fernandes 2019-03-26 16:07 ` Joel Fernandes 2019-03-26 16:15 ` Christian Brauner 2019-03-26 16:15 ` Christian Brauner 2019-03-25 16:20 ` [PATCH 3/4] signal: support pidctl() with pidfd_send_signal() Christian Brauner 2019-03-25 18:28 ` Jonathan Kowalski 2019-03-25 18:28 ` Jonathan Kowalski 2019-03-25 20:05 ` Christian Brauner [this message] 2019-03-25 20:05 ` Christian Brauner 2019-03-25 18:39 ` Jann Horn 2019-03-25 18:39 ` Jann Horn 2019-03-25 19:41 ` Christian Brauner 2019-03-25 19:41 ` Christian Brauner 2019-03-25 16:20 ` [PATCH 4/4] tests: add pidctl() tests Christian Brauner 2019-03-25 16:48 ` [PATCH 0/4] pid: add pidctl() Daniel Colascione 2019-03-25 16:48 ` Daniel Colascione 2019-03-25 17:05 ` Konstantin Khlebnikov 2019-03-25 17:07 ` Daniel Colascione 2019-03-25 17:07 ` Daniel Colascione 2019-03-25 17:36 ` Joel Fernandes 2019-03-25 17:36 ` Joel Fernandes 2019-03-25 17:53 ` Daniel Colascione 2019-03-25 17:53 ` Daniel Colascione 2019-03-25 18:19 ` Jonathan Kowalski 2019-03-25 18:19 ` Jonathan Kowalski 2019-03-25 18:57 ` Daniel Colascione 2019-03-25 18:57 ` Daniel Colascione 2019-03-25 19:42 ` Jonathan Kowalski 2019-03-25 19:42 ` Jonathan Kowalski 2019-03-25 20:14 ` Daniel Colascione 2019-03-25 20:14 ` Daniel Colascione 2019-03-25 20:34 ` Jann Horn 2019-03-25 20:34 ` Jann Horn 2019-03-25 20:40 ` Jonathan Kowalski 2019-03-25 20:40 ` Jonathan Kowalski 2019-03-25 21:14 ` Jonathan Kowalski 2019-03-25 21:14 ` Jonathan Kowalski 2019-03-25 21:15 ` Jann Horn 2019-03-25 21:15 ` Jann Horn 2019-03-25 20:40 ` Christian Brauner 2019-03-25 20:40 ` Christian Brauner 2019-03-25 20:15 ` Christian Brauner 2019-03-25 20:15 ` Christian Brauner 2019-03-25 21:11 ` Joel Fernandes 2019-03-25 21:11 ` Joel Fernandes 2019-03-25 21:17 ` Daniel Colascione 2019-03-25 21:17 ` Daniel Colascione 2019-03-25 21:19 ` Jann Horn 2019-03-25 21:19 ` Jann Horn 2019-03-25 21:43 ` Joel Fernandes 2019-03-25 21:43 ` Joel Fernandes 2019-03-25 21:54 ` Jonathan Kowalski 2019-03-25 21:54 ` Jonathan Kowalski 2019-03-25 22:07 ` Daniel Colascione 2019-03-25 22:07 ` Daniel Colascione 2019-03-25 22:37 ` Jonathan Kowalski 2019-03-25 22:37 ` Jonathan Kowalski 2019-03-25 23:14 ` Daniel Colascione 2019-03-25 23:14 ` Daniel Colascione 2019-03-26 3:03 ` Joel Fernandes 2019-03-26 3:03 ` Joel Fernandes 2019-03-25 16:56 ` David Howells 2019-03-25 16:56 ` David Howells 2019-03-25 16:58 ` Daniel Colascione 2019-03-25 16:58 ` Daniel Colascione 2019-03-25 23:39 ` Andy Lutomirski 2019-03-25 23:39 ` Andy Lutomirski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190325200552.fs4exsj3fsls7adu@brauner.io \ --to=christian@brauner.io \ --cc=adobriyan@gmail.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=bl0pbl33p@gmail.com \ --cc=cyphar@cyphar.com \ --cc=dancol@google.com \ --cc=dhowells@redhat.com \ --cc=ebiederm@xmission.com \ --cc=jannh@google.com \ --cc=joel@joelfernandes.org \ --cc=keescook@chromium.org \ --cc=khlebnikov@yandex-team.ru \ --cc=ldv@altlinux.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=mtk.manpages@gmail.com \ --cc=nagarathnam.muthusamy@oracle.com \ --cc=oleg@redhat.com \ --cc=serge@hallyn.com \ --cc=tglx@linutronix.de \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.