* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-30 17:24 UTC (permalink / raw)
To: Christian Brauner
Cc: Daniel Colascione, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagara>
In-Reply-To: <20190330171215.3yrfxwodstmgzmxy@brauner.io>
On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
>
>
> To clarify, what the Android guys really wanted to be part of the api is
> a way to get race-free access to metadata associated with a given pidfd.
> And the idea was that *if and only if procfs is mounted* you could do:
>
> int pidfd = pidfd_open(1234, 0);
>
> int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
And my claim is that this is three system calls - one of them very
hacky - to just do
int pidfd = open("/proc/%d", O_PATH);
and you're done. It acts as the pidfd _and_ the way to get the
associated status files etc.
So there is absolutely zero advantage to going through pidfd_open().
No. No. No.
So the *only* reason for "pidfd_open()" is if you don't have /proc in
the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
Yeah, yeah, if you want to avoid going through the pathname
translation, that's one thing, but if that's your aim, then you again
should also just admit that PIDFD_TO_PROCFD is disgusting and wrong,
and you're basically saying "ok, I'm not going to do /proc at all".
So I'm ok with the whole "simpler, faster, no-proc pidfd", but then it
really has to be *SIMPLER* and *NO PROCFS*.
PIDFD_TO_PROCFD violates *everything*.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 17:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Daniel Colascione, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy <nagara>
In-Reply-To: <CAHk-=whF6gnRVbJaArZna4e=tejfrzmNQtRbWjnuSSKpBn+jQg@mail.gmail.com>
On Sat, Mar 30, 2019 at 10:24:19AM -0700, Linus Torvalds wrote:
> On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
> >
> >
> > To clarify, what the Android guys really wanted to be part of the api is
> > a way to get race-free access to metadata associated with a given pidfd.
> > And the idea was that *if and only if procfs is mounted* you could do:
> >
> > int pidfd = pidfd_open(1234, 0);
> >
> > int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> > int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
>
> And my claim is that this is three system calls - one of them very
> hacky - to just do
>
> int pidfd = open("/proc/%d", O_PATH);
>
> and you're done. It acts as the pidfd _and_ the way to get the
> associated status files etc.
>
> So there is absolutely zero advantage to going through pidfd_open().
>
> No. No. No.
>
> So the *only* reason for "pidfd_open()" is if you don't have /proc in
> the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
>
> Yeah, yeah, if you want to avoid going through the pathname
> translation, that's one thing, but if that's your aim, then you again
> should also just admit that PIDFD_TO_PROCFD is disgusting and wrong,
> and you're basically saying "ok, I'm not going to do /proc at all".
>
> So I'm ok with the whole "simpler, faster, no-proc pidfd", but then it
Understood.
> really has to be *SIMPLER* and *NO PROCFS*.
>
> PIDFD_TO_PROCFD violates *everything*.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-30 17:50 UTC (permalink / raw)
To: Linus Torvalds
Cc: Christian Brauner, Daniel Colascione, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy <nagar>
In-Reply-To: <CAHk-=whF6gnRVbJaArZna4e=tejfrzmNQtRbWjnuSSKpBn+jQg@mail.gmail.com>
On Sat, Mar 30, 2019 at 5:24 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
> >
> >
> > To clarify, what the Android guys really wanted to be part of the api is
> > a way to get race-free access to metadata associated with a given pidfd.
> > And the idea was that *if and only if procfs is mounted* you could do:
> >
> > int pidfd = pidfd_open(1234, 0);
> >
> > int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> > int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
>
> And my claim is that this is three system calls - one of them very
> hacky - to just do
>
> int pidfd = open("/proc/%d", O_PATH);
>
> and you're done. It acts as the pidfd _and_ the way to get the
> associated status files etc.
>
> So there is absolutely zero advantage to going through pidfd_open().
>
> No. No. No.
>
> So the *only* reason for "pidfd_open()" is if you don't have /proc in
> the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
>
> Yeah, yeah, if you want to avoid going through the pathname
> translation, that's one thing, but if that's your aim, then you again
> should also just admit that PIDFD_TO_PROCFD is disgusting and wrong,
> and you're basically saying "ok, I'm not going to do /proc at all".
>
> So I'm ok with the whole "simpler, faster, no-proc pidfd", but then it
> really has to be *SIMPLER* and *NO PROCFS*.
>
(Resending because accidently it wasn't a reply-all)
If you go with pidfd_open, that should also mean you remove the
ability to be able to use /proc/<PID> dir fds in pidfd_send_signal.
Otherwise the semantics are hairy: I can only pidfd_open a task
reachable from my active namespace, but somehow also be able to open a
pidfd if I happen to see someone's /proc in my mount namespace and
have the access to open it?
> PIDFD_TO_PROCFD violates *everything*.
>
> Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 17:52 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Linus Torvalds, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam
In-Reply-To: <CAGLj2rHk5pPYg-uo4HjYQQkJJjM0ePohtLJr2TrucuOaU_skxw@mail.gmail.com>
On Sat, Mar 30, 2019 at 05:50:20PM +0000, Jonathan Kowalski wrote:
> On Sat, Mar 30, 2019 at 5:24 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
> > >
> > >
> > > To clarify, what the Android guys really wanted to be part of the api is
> > > a way to get race-free access to metadata associated with a given pidfd.
> > > And the idea was that *if and only if procfs is mounted* you could do:
> > >
> > > int pidfd = pidfd_open(1234, 0);
> > >
> > > int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> > > int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
> >
> > And my claim is that this is three system calls - one of them very
> > hacky - to just do
> >
> > int pidfd = open("/proc/%d", O_PATH);
> >
> > and you're done. It acts as the pidfd _and_ the way to get the
> > associated status files etc.
> >
> > So there is absolutely zero advantage to going through pidfd_open().
> >
> > No. No. No.
> >
> > So the *only* reason for "pidfd_open()" is if you don't have /proc in
> > the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
> >
> > Yeah, yeah, if you want to avoid going through the pathname
> > translation, that's one thing, but if that's your aim, then you again
> > should also just admit that PIDFD_TO_PROCFD is disgusting and wrong,
> > and you're basically saying "ok, I'm not going to do /proc at all".
> >
> > So I'm ok with the whole "simpler, faster, no-proc pidfd", but then it
> > really has to be *SIMPLER* and *NO PROCFS*.
> >
>
> (Resending because accidently it wasn't a reply-all)
>
> If you go with pidfd_open, that should also mean you remove the
> ability to be able to use /proc/<PID> dir fds in pidfd_send_signal.
>
> Otherwise the semantics are hairy: I can only pidfd_open a task
> reachable from my active namespace, but somehow also be able to open a
You can easily setns() to another pid namespace and get a pidfd there.
That's how most namespace interactions work right now. We already had
that discussion.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-30 17:59 UTC (permalink / raw)
To: Christian Brauner
Cc: Linus Torvalds, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam
In-Reply-To: <20190330175241.4itdnx3tl5upzjxd@brauner.io>
On Sat, Mar 30, 2019 at 5:52 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Sat, Mar 30, 2019 at 05:50:20PM +0000, Jonathan Kowalski wrote:
> > On Sat, Mar 30, 2019 at 5:24 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
> > > >
> > > >
> > > > To clarify, what the Android guys really wanted to be part of the api is
> > > > a way to get race-free access to metadata associated with a given pidfd.
> > > > And the idea was that *if and only if procfs is mounted* you could do:
> > > >
> > > > int pidfd = pidfd_open(1234, 0);
> > > >
> > > > int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> > > > int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
> > >
> > > And my claim is that this is three system calls - one of them very
> > > hacky - to just do
> > >
> > > int pidfd = open("/proc/%d", O_PATH);
> > >
> > > and you're done. It acts as the pidfd _and_ the way to get the
> > > associated status files etc.
> > >
> > > So there is absolutely zero advantage to going through pidfd_open().
> > >
> > > No. No. No.
> > >
> > > So the *only* reason for "pidfd_open()" is if you don't have /proc in
> > > the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
> > >
> > > Yeah, yeah, if you want to avoid going through the pathname
> > > translation, that's one thing, but if that's your aim, then you again
> > > should also just admit that PIDFD_TO_PROCFD is disgusting and wrong,
> > > and you're basically saying "ok, I'm not going to do /proc at all".
> > >
> > > So I'm ok with the whole "simpler, faster, no-proc pidfd", but then it
> > > really has to be *SIMPLER* and *NO PROCFS*.
> > >
> >
> > (Resending because accidently it wasn't a reply-all)
> >
> > If you go with pidfd_open, that should also mean you remove the
> > ability to be able to use /proc/<PID> dir fds in pidfd_send_signal.
> >
> > Otherwise the semantics are hairy: I can only pidfd_open a task
> > reachable from my active namespace, but somehow also be able to open a
>
> You can easily setns() to another pid namespace and get a pidfd there.
> That's how most namespace interactions work right now. We already had
> that discussion.
Only if it is a child namespace, or you have the relevant capabilities to setns.
Currently, if I just put a task in PID namespace, it can see /proc of
an ancestor PID namespace, and opendir /proc/<PID>, this is accepted
by pidfd_send_signal.
If you ever allow signalling across PID namespaces (because file
descriptors should be able to do that, they are not namespaced, see
files, sockets, etc), it will become a problem. Getting pidfds from
outside my active namespace should require userspace cooperation.
So, opening a pidfd should be limited to what *I* can see in my
namespace, like every other namespace. That is what a namespace is,
and PIDs have their own namespace, they're not exposed in the
filesystem namespace.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jann Horn @ 2019-03-30 18:00 UTC (permalink / raw)
To: Linus Torvalds
Cc: Christian Brauner, Daniel Colascione, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov, Nagarathnam
In-Reply-To: <CAHk-=whF6gnRVbJaArZna4e=tejfrzmNQtRbWjnuSSKpBn+jQg@mail.gmail.com>
On Sat, Mar 30, 2019 at 6:24 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
> > To clarify, what the Android guys really wanted to be part of the api is
> > a way to get race-free access to metadata associated with a given pidfd.
> > And the idea was that *if and only if procfs is mounted* you could do:
> >
> > int pidfd = pidfd_open(1234, 0);
> >
> > int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> > int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
>
> And my claim is that this is three system calls - one of them very
> hacky - to just do
>
> int pidfd = open("/proc/%d", O_PATH);
>
> and you're done. It acts as the pidfd _and_ the way to get the
> associated status files etc.
>
> So there is absolutely zero advantage to going through pidfd_open().
>
> No. No. No.
>
> So the *only* reason for "pidfd_open()" is if you don't have /proc in
> the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
So if, in the future, there is some sort of "create a new task and
return an fd to it" syscall, do you think it should always return
pidfds, or do you think it should return fds to /proc if procfs is
available? And if it should return fds to /proc, does that mean that
this "create a task" API should take an extra argument with a file
descriptor to the procfs instance you want to use?
(This can't always be implemented easily in userspace on top of normal
clone(), because if you create a task without a termination signal -
like a thread -, its PID can be recycled under you.)
An API like this would have less complexity stuffed into a single
syscall if it always returns pidfds, and if you then actually want an
fd to procfs, you can do the conversion that requires specifying a
procfs instance separately.
Of course, if you think that we shouldn't add an API for
pidfd-to-procfs conversion before we have an API for
clone()-with-an-fd-retval, that's understandable.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-30 18:02 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Linus Torvalds, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam
In-Reply-To: <CAGLj2rEtNNs0BXHuGkjpT4seHuh=Lj79iVYP-n117+Dv+ThcJA@mail.gmail.com>
On Sat, Mar 30, 2019 at 05:59:34PM +0000, Jonathan Kowalski wrote:
> On Sat, Mar 30, 2019 at 5:52 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Sat, Mar 30, 2019 at 05:50:20PM +0000, Jonathan Kowalski wrote:
> > > On Sat, Mar 30, 2019 at 5:24 PM Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > >
> > > > On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
> > > > >
> > > > >
> > > > > To clarify, what the Android guys really wanted to be part of the api is
> > > > > a way to get race-free access to metadata associated with a given pidfd.
> > > > > And the idea was that *if and only if procfs is mounted* you could do:
> > > > >
> > > > > int pidfd = pidfd_open(1234, 0);
> > > > >
> > > > > int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> > > > > int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
> > > >
> > > > And my claim is that this is three system calls - one of them very
> > > > hacky - to just do
> > > >
> > > > int pidfd = open("/proc/%d", O_PATH);
> > > >
> > > > and you're done. It acts as the pidfd _and_ the way to get the
> > > > associated status files etc.
> > > >
> > > > So there is absolutely zero advantage to going through pidfd_open().
> > > >
> > > > No. No. No.
> > > >
> > > > So the *only* reason for "pidfd_open()" is if you don't have /proc in
> > > > the first place. In which case the whole PIDFD_TO_PROCFD is bogus.
> > > >
> > > > Yeah, yeah, if you want to avoid going through the pathname
> > > > translation, that's one thing, but if that's your aim, then you again
> > > > should also just admit that PIDFD_TO_PROCFD is disgusting and wrong,
> > > > and you're basically saying "ok, I'm not going to do /proc at all".
> > > >
> > > > So I'm ok with the whole "simpler, faster, no-proc pidfd", but then it
> > > > really has to be *SIMPLER* and *NO PROCFS*.
> > > >
> > >
> > > (Resending because accidently it wasn't a reply-all)
> > >
> > > If you go with pidfd_open, that should also mean you remove the
> > > ability to be able to use /proc/<PID> dir fds in pidfd_send_signal.
> > >
> > > Otherwise the semantics are hairy: I can only pidfd_open a task
> > > reachable from my active namespace, but somehow also be able to open a
> >
> > You can easily setns() to another pid namespace and get a pidfd there.
> > That's how most namespace interactions work right now. We already had
> > that discussion.
>
> Only if it is a child namespace, or you have the relevant capabilities to setns.
>
> Currently, if I just put a task in PID namespace, it can see /proc of
> an ancestor PID namespace, and opendir /proc/<PID>, this is accepted
> by pidfd_send_signal.
>
> If you ever allow signalling across PID namespaces (because file
That's not on the plate for now and pidfd_send_signal() is checking for
that to not allow it.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Joel Fernandes @ 2019-03-31 1:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Daniel Colascione, Christian Brauner, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov <oleg@
In-Reply-To: <CAHk-=wi1kQDVN22qv7M=bWU+CngZZXcPAMKCsDZMeuY3k2tkyg@mail.gmail.com>
On Sat, Mar 30, 2019 at 09:18:12AM -0700, Linus Torvalds wrote:
> On Sat, Mar 30, 2019 at 9:16 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > And no, we are *NOT* making pidfd_open() into some "let's expose /proc
> > even when it's not mounted" horror. Seriously. That's disgusting,
> > wrong, insecure and stupid.
>
> And btw, this is not debatable. In fact, this whole discussion is
> making me feel like I should just revert pidfd, not because the code I
> merged is wrong, but because people are clearly intending to do
> completely inappropriate things with this.
>
> Get your act together. Stop hacking up garbage.
>
> Linus
I am supportive of Linus's view of keeping /proc separate from pidfd. I
didn't really care about "pidfd" solving any racing issues with /proc/<pid>/*
access. My main interest in pidfd is because we can implement "waiting"
semantics in the future using something like a pidfd_wait call, which solves
one of the issues of Android's low-memory where it needs to be notified as
quickly as possible about a non-child process's death. Android Low memory
killer right now just keeps checking for /proc/<pid> 's existence which is
slow and more CPU intense for this.
As I said I don't really care about "pidfd" solving any racing issues with
/proc/<pid>/* accesses - because I still find it hard to imagine that the pid
number can be reused easily from the time you know which <pid> to deal with,
to the time when you want to read, say, the /proc/<pid>/status file. I am yet
to see any real data to show that such overflow happens - you literally need
32k process deaths and forks in such a short time frame, and on 64-bit, that
number is really high that its not even an issue. And if this is really an
issue, then you can just open a handle to /proc/<pid> at process creation
time and keep it around. If the <pid> is reused, you can still use openat(2)
on that handle without any races.
What I think will be most immediately valuable for Android in my opinion is
the pidfd_open() and pidfd_send_signal() syscalls, along with the future
pidfd_wait() that we're working on to solve the issue with Android's Low
memory killer I mentioned above.
Now, in relation to Daniel's concern with procfs, I feel we need to be clear
what is the meaning of a "pidfd" and it should consistently work across all
APIs no matter how pidfd is obtained. There shouldn't be like "if you obtain
pidfd using this method and it only works with these APIs", that's just plain
wrong IMO.
For example: with pidfd_open() or whatever that ends up being called is
available, the pidfd obtained is not tied to /proc. However, if one obtained
a pidfd using open("/proc/<pid>/", ..), then that pidfd *is* tied to /proc.
pidfd_send_signal will happily work on this "pidfd". Both these methods of
obtainings pidfd(s) make openat(2) work on them inconsistently. In one case
openat(2) fails, and in the other case it succeeds. I feel there should be no
ambiguity between how "pidfd" works with openat(2). Either make openat(2)
never work on any "pidfd", or always make it work on it.
I would say, don't call a /proc/<pid> file descriptor as a "pidfd". Next, to
remedy all this, I feel pidfd_send_signal *should not* work on fds that are
obtained by opening of /proc/<pid> if we can agree those are not pidfds.
Only the ones obtained using the proposed pidfd_open (or pidfd_clone) should
be allowed to be used with pidfd_send_signal.
So I believe the call to tgid_pidfd_to_pid() from pidfd_to_pid in Christian's
patch 3/5 needs to go away.
+static struct pid *pidfd_to_pid(const struct file *file)
+{
+ if (file->f_op == &pidfd_fops)
+ return file->private_data;
+
+ return tgid_pidfd_to_pid(file); <-------- Should just return error
+}
+
I agree with Christian that lets not put pidfd_send_signal at risk. Let us
make the notion of a pidfd and the APIs that work on it *consistent* and make
it do just what we really need for our usescases. Which for Android, is,
pidfd_open/clone, pidfd_send_signal and pidfd_wait. I hope I added some
clarity around the usecases.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Neil Horman @ 2019-03-31 2:11 UTC (permalink / raw)
To: Paul Moore
Cc: Richard Guy Briggs, Ondrej Mosnacek, linux-api, containers, LKML,
David Howells, Linux-Audit Mailing List, netfilter-devel,
Eric W . Biederman, Simo Sorce, netdev, linux-fsdevel, Eric Paris,
Serge E. Hallyn
In-Reply-To: <CAHC9VhQiFw32s9QNK4WQDM6rbWHqOhm1AVa2JvY=19V4S+=YhA@mail.gmail.com>
On Thu, Mar 28, 2019 at 06:00:21PM -0400, Paul Moore wrote:
> On Thu, Mar 28, 2019 at 5:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-03-28 11:46, Paul Moore wrote:
> > > On Wed, Mar 27, 2019 at 9:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > >
> > > > On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > > > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > Audit events could happen in a network namespace outside of a task
> > > > > > context due to packets received from the net that trigger an auditing
> > > > > > rule prior to being associated with a running task. The network
> > > > > > namespace could be in use by multiple containers by association to the
> > > > > > tasks in that network namespace. We still want a way to attribute
> > > > > > these events to any potential containers. Keep a list per network
> > > > > > namespace to track these audit container identifiiers.
> > > > > >
> > > > > > Add/increment the audit container identifier on:
> > > > > > - initial setting of the audit container identifier via /proc
> > > > > > - clone/fork call that inherits an audit container identifier
> > > > > > - unshare call that inherits an audit container identifier
> > > > > > - setns call that inherits an audit container identifier
> > > > > > Delete/decrement the audit container identifier on:
> > > > > > - an inherited audit container identifier dropped when child set
> > > > > > - process exit
> > > > > > - unshare call that drops a net namespace
> > > > > > - setns call that drops a net namespace
> > > > > >
> > > > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > ---
> > > > > > include/linux/audit.h | 19 ++++++++++++
> > > > > > kernel/audit.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > > kernel/nsproxy.c | 4 +++
> > > > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > > > index fa19fa408931..70255c2dfb9f 100644
> > > > > > --- a/include/linux/audit.h
> > > > > > +++ b/include/linux/audit.h
> > > > > > @@ -27,6 +27,7 @@
> > > > > > #include <linux/ptrace.h>
> > > > > > #include <linux/namei.h> /* LOOKUP_* */
> > > > > > #include <uapi/linux/audit.h>
> > > > > > +#include <linux/refcount.h>
> > > > > >
> > > > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > > > > >
> > > > > > extern struct audit_task_info init_struct_audit;
> > > > > >
> > > > > > +struct audit_contid {
> > > > > > + struct list_head list;
> > > > > > + u64 id;
> > > > > > + refcount_t refcount;
> > > > >
> > > > > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > > > > if we could just make it a regular unsigned int (we don't need the
> > > > > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > > > > checking, so it's probably better to leave it as is...
> > > >
> > > > Since the update is done using rcu-safe methods, do we even need the
> > > > spin_lock? Neil? Paul?
> > >
> > > As discussed, the refcount field is protected against simultaneous
> > > writes by the spinlock that protects additions/removals from the list
> > > as a whole so I don't believe the refcount_t atomicity is critical in
> > > this regard.
> > >
> > > Where it gets tricky, and I can't say I'm 100% confident on my answer
> > > here, is if refcount was a regular int and we wanted to access it
> > > outside of a spinlock (to be clear, it doesn't look like this patch
> > > currently does this). With RCU, if refcount was a regular int
> > > (unsigned or otherwise), I believe it would be possible for different
> > > threads of execution to potentially see different values of refcount
> > > (assuming one thread was adding/removing from the list). Using a
> > > refcount_t would protect against this, alternatively, taking the
> > > spinlock should also protect against this.
> >
> > Ok, from the above it isn't clear to me if you are happy with the
> > current code or would prefer any changes, or from below that you still
> > need to work it through to make a pronouncement. It sounds to me you
> > would be ok with *either* spinlock *or* refcount_t, but don't see the
> > need for both.
>
> To be fair you didn't ask if I was "happy" with the approach above,
> you asked if we needed the spinlock/refcount_t. I believe I answered
> that question as comprehensively as I could, but perhaps you wanted a
> hard yes or no? In that case, since refcount_t is obviously safer, I
> would stick with that for now just to limit the number of possible
> failures. If someone smarter than you or I comes along and
> definitively says you are 100% safe to use an int, then go ahead and
> use an int.
>
> Beyond that, I'm still in the process of reviewing your patches, but I
> haven't finished yet, so no "pronouncement" or whatever you want to
> call it.
>
We definately need the spinlock, as its not meant to protect the refcount
alterations, its meant to protect parallel modifications to the list pointers.
Without the spinlock, the list can become corrupted (protecting the refcount is
just a byproduct).
Because of that byproduct, the atomicity of the refcount isn't required, and so
we could modify it to be an int or some other non-atomic ordinal type, but as I
noted, I don't think thats a good idea. The refcount type helps denote clearly
what the variable is used for, making it more readable, and given the relative
non-performance critical path that the reference count is read/written in, and
the relatively more expensive locking we are already doing there, I think there
is more value in using the refcount to make the code legible than making
marginally more performant by altering it to be an int type.
Neil
> --
> paul moore
> www.paul-moore.com
>
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jann Horn @ 2019-03-31 2:34 UTC (permalink / raw)
To: Joel Fernandes
Cc: Linus Torvalds, Daniel Colascione, Christian Brauner,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Ol
In-Reply-To: <20190331010716.GA189578@google.com>
On Sun, Mar 31, 2019 at 3:07 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> As I said I don't really care about "pidfd" solving any racing issues with
> /proc/<pid>/* accesses - because I still find it hard to imagine that the pid
> number can be reused easily from the time you know which <pid> to deal with,
> to the time when you want to read, say, the /proc/<pid>/status file.
There have been several Android security bugs related to PID reuse.
> I am yet
> to see any real data to show that such overflow happens - you literally need
> 32k process deaths and forks in such a short time frame
This seems very inaccurate to me.
The time frame in which the PID has to wrap around is not the time
between process death and use of the PID. It is the time between *the
creation* of the old process and the use of the PID. Consider the
following sequence of events:
- process A starts with PID 1000
- some time passes in which some process repeatedly forks, with PIDs
wrapping around to 999
- process B starts an attempt to access process A (using PID 1000)
- process A dies
- process C spawns with PID 1000
- process B accidentally accesses process C
Also, it's probably worth clarifying that here, "processes" means "threads".
If there are a lot of active processes, that reduces the number of
times you have to clone() to get the PID to wrap around.
> and on 64-bit, that
> number is really high
Which number is really high on 64-bit? Checking on a walleye phone,
pid_max is still only 32768:
walleye:/ # cat /proc/sys/kernel/pid_max
32768
walleye:/ #
> that its not even an issue. And if this is really an
> issue, then you can just open a handle to /proc/<pid> at process creation
> time and keep it around. If the <pid> is reused, you can still use openat(2)
> on that handle without any races.
But not if you want to implement something like killall in a
race-free way, for example.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Joel Fernandes @ 2019-03-31 4:08 UTC (permalink / raw)
To: Jann Horn
Cc: Linus Torvalds, Daniel Colascione, Christian Brauner,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Ol
In-Reply-To: <CAG48ez387kGbD_dLqEbZ1U9rKFbfP0EF9cMVjQT6nwc9=Y9CNw@mail.gmail.com>
On Sun, Mar 31, 2019 at 04:34:57AM +0200, Jann Horn wrote:
> On Sun, Mar 31, 2019 at 3:07 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > As I said I don't really care about "pidfd" solving any racing issues with
> > /proc/<pid>/* accesses - because I still find it hard to imagine that the pid
> > number can be reused easily from the time you know which <pid> to deal with,
> > to the time when you want to read, say, the /proc/<pid>/status file.
>
> There have been several Android security bugs related to PID reuse.
Yes PID reuse will be a problem till we have pidfd_clone and
pidfd_send_signal (and any other pidfd related syscalls). I've never denied
PID reuse is *currently* a problem and the set of pidfd syscalls being
proposed are designed to avoid those. So I'm not fully sure what you mean.
Anyway, I would love to see those security bugs you mentioned if you could
point me to them.
> > I am yet
> > to see any real data to show that such overflow happens - you literally need
> > 32k process deaths and forks in such a short time frame
>
> This seems very inaccurate to me.
>
> The time frame in which the PID has to wrap around is not the time
> between process death and use of the PID. It is the time between *the
> creation* of the old process and the use of the PID. Consider the
> following sequence of events:
>
> - process A starts with PID 1000
> - some time passes in which some process repeatedly forks, with PIDs
> wrapping around to 999
> - process B starts an attempt to access process A (using PID 1000)
> - process A dies
> - process C spawns with PID 1000
> - process B accidentally accesses process C
>
> Also, it's probably worth clarifying that here, "processes" means "threads".
>
> If there are a lot of active processes, that reduces the number of
> times you have to clone() to get the PID to wrap around.
Ok, that's fair and I take your point. But I wonder what access you're
talking about, is it killing the process? If yes, pidfd_clone +
pidfd_send_signal will solve that in the race free way without relying on the
PID number. Is it accessing /proc/<pid>/? then see below.
> > and on 64-bit, that
> > number is really high
>
> Which number is really high on 64-bit? Checking on a walleye phone,
> pid_max is still only 32768:
>
> walleye:/ # cat /proc/sys/kernel/pid_max
> 32768
> walleye:/ #
Ok. I was talking about the theoretical limit of pid_max on a 64-bit
platform. But since we are talking about NOT relying on the PID number in the
first place, we can move on from this point.
> > that its not even an issue. And if this is really an
> > issue, then you can just open a handle to /proc/<pid> at process creation
> > time and keep it around. If the <pid> is reused, you can still use openat(2)
> > on that handle without any races.
>
> But not if you want to implement something like killall in a
> race-free way, for example.
I am not at all talking about killing processes in your last quote of my
email above, I'm talking about access to /proc/<pid>/ files.
As I said, at the time of process creation, you can obtain an fd by opening
/proc/<pid>/ and keep it open. Then you can do an openat(2) on that fd
without worrying at <pid> reuse, no? And then access all the files that way.
As for killall in Android. I don't think that "killing processes by name" is
relied on for the runtime operation of Android. That would be a very bad
idea. Low memory killer does not kill processes by name. It kills processes
by the PID number using kill(2) which we'd like to replace with
pidfd_send_signal.
Again if you want to convince Linus about having a "pidfd to procfd"
conversion mechanism, then by all means go for it. I just don't think it is
urgently necessary (and others may disagree with me on this), but I wouldn't
care if such a mechanism existed either. Whatever we do, I just want the
notion of "pidfd" to be consistent as I mentioned in my previous email.
thank you!
- Joel
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jann Horn @ 2019-03-31 4:46 UTC (permalink / raw)
To: Joel Fernandes
Cc: Linus Torvalds, Daniel Colascione, Christian Brauner,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Ol
In-Reply-To: <20190331040810.GB189578@google.com>
On Sun, Mar 31, 2019 at 6:08 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> On Sun, Mar 31, 2019 at 04:34:57AM +0200, Jann Horn wrote:
> > On Sun, Mar 31, 2019 at 3:07 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > > As I said I don't really care about "pidfd" solving any racing issues with
> > > /proc/<pid>/* accesses - because I still find it hard to imagine that the pid
> > > number can be reused easily from the time you know which <pid> to deal with,
> > > to the time when you want to read, say, the /proc/<pid>/status file.
> >
> > There have been several Android security bugs related to PID reuse.
>
> Yes PID reuse will be a problem till we have pidfd_clone and
> pidfd_send_signal (and any other pidfd related syscalls). I've never denied
> PID reuse is *currently* a problem and the set of pidfd syscalls being
> proposed are designed to avoid those. So I'm not fully sure what you mean.
> Anyway, I would love to see those security bugs you mentioned if you could
> point me to them.
https://bugs.chromium.org/p/project-zero/issues/detail?id=851
"Android: racy getpidcon usage permits binder service replacement"
https://bugs.chromium.org/p/project-zero/issues/detail?id=853
"Android: debuggerd mitigation bypass and infoleak"
https://bugs.chromium.org/p/project-zero/issues/detail?id=1404
"Android: Hardware Service Manager Arbitrary Service Replacement due
to getpidcon"
https://bugs.chromium.org/p/project-zero/issues/detail?id=1406
"Android: Permission bypass in KeyStore service due to getpidcon"
https://bugs.chromium.org/p/project-zero/issues/detail?id=1741
"Android: getpidcon() usage in hardware binder servicemanager permits
ACL bypass"
> > > I am yet
> > > to see any real data to show that such overflow happens - you literally need
> > > 32k process deaths and forks in such a short time frame
> >
> > This seems very inaccurate to me.
> >
> > The time frame in which the PID has to wrap around is not the time
> > between process death and use of the PID. It is the time between *the
> > creation* of the old process and the use of the PID. Consider the
> > following sequence of events:
> >
> > - process A starts with PID 1000
> > - some time passes in which some process repeatedly forks, with PIDs
> > wrapping around to 999
> > - process B starts an attempt to access process A (using PID 1000)
> > - process A dies
> > - process C spawns with PID 1000
> > - process B accidentally accesses process C
> >
> > Also, it's probably worth clarifying that here, "processes" means "threads".
> >
> > If there are a lot of active processes, that reduces the number of
> > times you have to clone() to get the PID to wrap around.
>
> Ok, that's fair and I take your point. But I wonder what access you're
> talking about, is it killing the process? If yes, pidfd_clone +
> pidfd_send_signal will solve that in the race free way without relying on the
> PID number.
Sure, given a pidfd_clone() syscall, as long as the parent of the
process is giving you a pidfd for it and you don't have to deal with
grandchildren created by fork() calls outside your control, that
works.
> Is it accessing /proc/<pid>/? then see below.
> > > and on 64-bit, that
> > > number is really high
> >
> > Which number is really high on 64-bit? Checking on a walleye phone,
> > pid_max is still only 32768:
> >
> > walleye:/ # cat /proc/sys/kernel/pid_max
> > 32768
> > walleye:/ #
>
> Ok. I was talking about the theoretical limit of pid_max on a 64-bit
> platform. But since we are talking about NOT relying on the PID number in the
> first place, we can move on from this point.
(pid_t is a signed 32-bit value, that's UAPI.)
> > > that its not even an issue. And if this is really an
> > > issue, then you can just open a handle to /proc/<pid> at process creation
> > > time and keep it around. If the <pid> is reused, you can still use openat(2)
> > > on that handle without any races.
> >
> > But not if you want to implement something like killall in a
> > race-free way, for example.
>
> I am not at all talking about killing processes in your last quote of my
> email above, I'm talking about access to /proc/<pid>/ files.
>
> As I said, at the time of process creation, you can obtain an fd by opening
> /proc/<pid>/ and keep it open. Then you can do an openat(2) on that fd
> without worrying at <pid> reuse, no? And then access all the files that way.
Yeah, if you're the parent of a process with a termination signal, that works.
> As for killall in Android. I don't think that "killing processes by name" is
> relied on for the runtime operation of Android. That would be a very bad
> idea. Low memory killer does not kill processes by name. It kills processes
> by the PID number using kill(2) which we'd like to replace with
> pidfd_send_signal.
Yeah, I somehow lost context when replying to your mail here; sorry
about that. I didn't mean to imply that this is a usecase on Android.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-31 14:52 UTC (permalink / raw)
To: Jann Horn
Cc: Joel Fernandes, Daniel Colascione, Christian Brauner,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg
In-Reply-To: <CAG48ez2gb94SqS30Ai4+VBHhnzBp5Po9_u00nMrvUW6Wqq6hPA@mail.gmail.com>
On Sat, Mar 30, 2019 at 9:47 PM Jann Horn <jannh@google.com> wrote:
>
> Sure, given a pidfd_clone() syscall, as long as the parent of the
> process is giving you a pidfd for it and you don't have to deal with
> grandchildren created by fork() calls outside your control, that
> works.
Don't do pidfd_clone() and pidfd_wait().
Both of those existing system calls already get a "flags" argument.
Just make a WPIDFD (for waitid) and CLONE_PIDFD (for clone) bit, and
make the existing system calls just take/return a pidfd.
Side note: we could (should?) also make the default maxpid just be
larger. It needs to fit in an 'int', but MAXINT instead of 65535 would
likely alreadt make a lot of these attacks harder.
There was some really old legacy reason why we actually limited it to
65535 originally. It was old and crufty even back when..
Linus
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-31 15:05 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jann Horn, Joel Fernandes, Daniel Colascione, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov
In-Reply-To: <CAHk-=wiZ40LVjnXSi9iHLE_-ZBsWFGCgdmNiYZUXn1-V5YBg2g@mail.gmail.com>
On Sun, Mar 31, 2019 at 07:52:28AM -0700, Linus Torvalds wrote:
> On Sat, Mar 30, 2019 at 9:47 PM Jann Horn <jannh@google.com> wrote:
> >
> > Sure, given a pidfd_clone() syscall, as long as the parent of the
> > process is giving you a pidfd for it and you don't have to deal with
> > grandchildren created by fork() calls outside your control, that
> > works.
>
> Don't do pidfd_clone() and pidfd_wait().
>
> Both of those existing system calls already get a "flags" argument.
> Just make a WPIDFD (for waitid) and CLONE_PIDFD (for clone) bit, and
> make the existing system calls just take/return a pidfd.
Yes, that's one of the options I was considering but was afraid of
pitching it because of the very massive opposition I got
regarding"multiplexers". I'm perfectly happy with doing it this way.
>
> Side note: we could (should?) also make the default maxpid just be
> larger. It needs to fit in an 'int', but MAXINT instead of 65535 would
> likely alreadt make a lot of these attacks harder.
Yes, agreed.
>
> There was some really old legacy reason why we actually limited it to
> 65535 originally. It was old and crufty even back when..
So Jann and I have been thinking about going forward with the following
idea:
With the pidfd_open() patchset I have pidfds are simple anone inode file
descriptors stashing a reference to struct pid of a process. I have
mentioned this is in prior mails. This cleanly decouples pidfds from
procfs completely.
The reason why we want to use pidfds with no connection to a specific procfs
instance, even in environments that have procfs, is that we would like to add
the API to clone with CLONE_PIDFD that you just mentioned that creates a
new process or thread and returns a pidfd to it. In the context of such
a syscall, it would be awkward to have the kernel open a file in some
procfs instance, since then userspace would have to specify which procfs
instance the fd should come from.
There is an argument to be made that for consistency's sake we should - although
I don't feel strongly about it - disallow the usage of pidfd_send_signal() with
fds referring to /proc/<pid> then. Unless you want this to always work. If you
want this to work then we would simply submit pidfd_open() for the 5.2
window. If you agree that it makes sense to only have pidfd_open() file
descriptors working with pidfd_send_signal() we would send a revert for
pidfd_send_signal() now and resubmit it together with pidfd_open()
during the 5.2. merge window. This decouples pidfds completely from
procfs not just when it is not compiled in or mounted.
I very much care about this being done right. If this means temporarily
kicking pidfd_send_signal() out until 5.2 I'm happy to do so.
Btw, the /proc/<pid> race issue that is mentioned constantly is simply
avoidable by placing the pid that the pidfd has stashed relative to the
callers' procfs mount's pid namespace in the pidfd's fdinfo. So there's
not even a need to really go through /proc/<pid> in the first place. A
caller wanting to get metadata access and avoid a race with pid
recycling can then simply do:
int pidfd = pidfd_open(pid, 0);
int pid = parse_fdinfo("/proc/self/fdinfo/<pidfd>");
int procpidfd = open("/proc/<pid>", ...);
/* Test if process still exists by sending signal 0 through our pidfd. */
int ret = pidfd_send_signal(pid, 0, NULL, PIDFD_SIGNAL_THREAD);
if (ret < 0 && errno == ESRCH) {
/* pid has been recycled and procpidfd refers to another process */
}
Christian
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Daniel Colascione @ 2019-03-31 15:21 UTC (permalink / raw)
To: Christian Brauner
Cc: Linus Torvalds, Jann Horn, Joel Fernandes, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg
In-Reply-To: <20190331150507.zpyugdvtmr6rgpda@brauner.io>
On Sun, Mar 31, 2019 at 8:05 AM Christian Brauner <christian@brauner.io> wrote:
>
> On Sun, Mar 31, 2019 at 07:52:28AM -0700, Linus Torvalds wrote:
> > On Sat, Mar 30, 2019 at 9:47 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > Sure, given a pidfd_clone() syscall, as long as the parent of the
> > > process is giving you a pidfd for it and you don't have to deal with
> > > grandchildren created by fork() calls outside your control, that
> > > works.
> >
> > Don't do pidfd_clone() and pidfd_wait().
> >
> > Both of those existing system calls already get a "flags" argument.
> > Just make a WPIDFD (for waitid) and CLONE_PIDFD (for clone) bit, and
> > make the existing system calls just take/return a pidfd.
>
> Yes, that's one of the options I was considering but was afraid of
> pitching it because of the very massive opposition I got
> regarding"multiplexers". I'm perfectly happy with doing it this way.
This approach is fine by me, FWIW. I like it more than a general-purpose pidctl.
> Btw, the /proc/<pid> race issue that is mentioned constantly is simply
> avoidable by placing the pid that the pidfd has stashed relative to the
> callers' procfs mount's pid namespace in the pidfd's fdinfo. So there's
> not even a need to really go through /proc/<pid> in the first place. A
> caller wanting to get metadata access and avoid a race with pid
> recycling can then simply do:
>
> int pidfd = pidfd_open(pid, 0);
> int pid = parse_fdinfo("/proc/self/fdinfo/<pidfd>");
> int procpidfd = open("/proc/<pid>", ...);
IMHO, it's worth documenting this procedure in the pidfd man page.
> /* Test if process still exists by sending signal 0 through our pidfd. */
Are you planning on officially documenting this incantation in the
pidfd man page?
> int ret = pidfd_send_signal(pid, 0, NULL, PIDFD_SIGNAL_THREAD);
> if (ret < 0 && errno == ESRCH) {
> /* pid has been recycled and procpidfd refers to another process */
> }
I was going to suggest that WNOHANG also works for this purpose, but
that idea raises another question: normally, you can wait*(2) on a
process only once. Are you imagining waitid on a pidfd succeeding more
than one? ISTM that the pidfd would need to internally store not just
a struct pid, but the exit status as well or some way to get to it.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-31 15:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jann Horn, Joel Fernandes, Daniel Colascione, Christian Brauner,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov <oleg@
In-Reply-To: <CAHk-=wiZ40LVjnXSi9iHLE_-ZBsWFGCgdmNiYZUXn1-V5YBg2g@mail.gmail.com>
On Sun, Mar 31, 2019 at 3:59 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Mar 30, 2019 at 9:47 PM Jann Horn <jannh@google.com> wrote:
> >
> > Sure, given a pidfd_clone() syscall, as long as the parent of the
> > process is giving you a pidfd for it and you don't have to deal with
> > grandchildren created by fork() calls outside your control, that
> > works.
>
> Don't do pidfd_clone() and pidfd_wait().
>
> Both of those existing system calls already get a "flags" argument.
> Just make a WPIDFD (for waitid) and CLONE_PIDFD (for clone) bit, and
> make the existing system calls just take/return a pidfd.
clone is out of flags, so there will have to be a new system call.
I am not sure about the waitid bit. Are you suggesting it takes a
pidfd and waits using it? I was thinking if we could make the pidfd
itself pollable and readable for exit status. At pidfd_open time, you
pass the flag and only if you're a parent you get a readable instance,
if not, a pollable one for everyone (eg. for an indirect child as a
reaper), and it fails for threads.
Then, the pidfd clone2 returns can also be polled and read from.
The main pain point is, currently when I ptrace from a thread a
process, I need to use waitpid (waitid throws away ptrace critical
information), and since ptrace works on a thread by thread basis, only
the attached thread can do the waitpid. This means I cannot do
anything else from the attached thread concurrently. waitfd was
supposed to solve this (back in 2009) but it never made it in, and
clone4 from Josh Triplett did something similar (returned exit status
over the clonefd).
FreeBSD's process descriptors are also pollable (which is where all
this work was originally inspired from) and it would help with
adoption if semantics were similar. Besides that, it would help
libraries to be able to host their own set of children without
affecting the entire process's waiting logic oe mucking with the
SIGCHLD handler (you wouldn't need signals).
>
> Side note: we could (should?) also make the default maxpid just be
> larger. It needs to fit in an 'int', but MAXINT instead of 65535 would
> likely alreadt make a lot of these attacks harder.
>
> There was some really old legacy reason why we actually limited it to
> 65535 originally. It was old and crufty even back when..
>
> Linus
>
> Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Andy Lutomirski @ 2019-03-31 20:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Christian Brauner, Daniel Colascione, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov <oleg@
In-Reply-To: <CAHk-=whF6gnRVbJaArZna4e=tejfrzmNQtRbWjnuSSKpBn+jQg@mail.gmail.com>
> On Mar 30, 2019, at 11:24 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
>>
>>
>> To clarify, what the Android guys really wanted to be part of the api is
>> a way to get race-free access to metadata associated with a given pidfd.
>> And the idea was that *if and only if procfs is mounted* you could do:
>>
>> int pidfd = pidfd_open(1234, 0);
>>
>> int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
>> int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
>
> And my claim is that this is three system calls - one of them very
> hacky - to just do
>
> int pidfd = open("/proc/%d", O_PATH);
Hi Linus-
I want to re-check this because I think Christian’s example was bad. I proposed these ioctls, but that wasn’t the intended use. The real point is:
int pidfd = new_improved_clone(...);
To be useful, this type of API *must* work without proc mounted.
And, later:
openat(fd to pidfd’s proc directory, “status”, ...);
And we want a non-utterly-crappy way to do this. The ioctl is certainly ugly, but it *works*.
Another approach is:
pid_t pid = pidfd_get_pid(pidfd);
sprintf(buf, “/proc/%d”, pid);
int procfd = open(buf, O_PATH);
if (pidfd_get_pid(pidfd) != pid) {
we lose;
}
But this is clunky.
Do you think the clunky version is okay, or do you have a suggestion for making it better?
—Andy
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-31 21:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christian Brauner, Daniel Colascione, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov <oleg@
In-Reply-To: <132107F4-F56B-4D6E-9E00-A6F7C092E6BD@amacapital.net>
On Sun, Mar 31, 2019 at 1:38 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> openat(fd to pidfd’s proc directory, “status”, ...);
>
> And we want a non-utterly-crappy way to do this. The ioctl is certainly ugly, but it *works*.
It's beyond clunky. It's a disgrace.
If people really want equivalency between open("/proc/%d") and some
new pidfd_open(), then just *make* the two equivalent.
No effing crappy ioctl idiocy to create one from the other. Just make
the damn things be the exact same thing (and then if we extend clone()
to return one, make that return the same exact thing too).
Btw, we have several clone bits left:
- if we don't have CLONE_PARENT set, the low 8 bits are still available
- ignoring that, wehave bit #12 free: It used to be CLONE_IDLETASK
long long ago, but it was always kernel-only so it's never been
exposed as a user space bit.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-31 21:10 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov
In-Reply-To: <CAHk-=wiaLtH41Mc5qAjOeCWavPqV0DhS581KYa0QBt8uraTK1Q@mail.gmail.com>
On Sun, Mar 31, 2019 at 02:03:25PM -0700, Linus Torvalds wrote:
> On Sun, Mar 31, 2019 at 1:38 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > openat(fd to pidfd’s proc directory, “status”, ...);
> >
> > And we want a non-utterly-crappy way to do this. The ioctl is certainly ugly, but it *works*.
>
> It's beyond clunky. It's a disgrace.
>
> If people really want equivalency between open("/proc/%d") and some
> new pidfd_open(), then just *make* the two equivalent.
I don't think that we want or can make them equivalent since that would
mean we depend on procfs. If userspace really wants to turn a pidfd into
an fd for /proc/<pid> then they can be burdened to do so by parsing out
the pid relative to their procfs pid namespace from the pidfds fdinfo:
int pidfd = pidfd_open(pid, 0);
int pid = parse_fdinfo("/proc/self/fdinfo/<pidfd>");
int procpidfd = open("/proc/<pid>", ...);
/* Test if process still exists by sending signal 0 through our pidfd. */
int ret = pidfd_send_signal(pid, 0, NULL, PIDFD_SIGNAL_THREAD);
if (ret < 0 && errno == ESRCH) {
/* pid has been recycled and procpidfd refers to another process */
}
it's race free and no ioctl() is needed.
>
> No effing crappy ioctl idiocy to create one from the other. Just make
> the damn things be the exact same thing (and then if we extend clone()
> to return one, make that return the same exact thing too).
>
> Btw, we have several clone bits left:
>
> - if we don't have CLONE_PARENT set, the low 8 bits are still available
>
> - ignoring that, wehave bit #12 free: It used to be CLONE_IDLETASK
> long long ago, but it was always kernel-only so it's never been
> exposed as a user space bit.
That's good to know.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-31 21:17 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov
In-Reply-To: <20190331211041.vht7dnqg4e4bilr2@brauner.io>
On Sun, Mar 31, 2019 at 2:10 PM Christian Brauner <christian@brauner.io> wrote:
>
> I don't think that we want or can make them equivalent since that would
> mean we depend on procfs.
Sure we can.
If /proc is enabled, then you always do that dance YOU ALREADY WROTE
THE CODE FOR to do the stupid ioctl.
And if /procfs isn't enabled, then you don't do that.
Ta-daa. Done. No stupid ioctl, and now /proc and pidfd_open() return
the same damn thing.
And guess what? If /proc isn't enabled, then obviously pidfd_open()
gives you the /proc-less thing, but at least there is no crazy "two
different file descriptors for the same thing" situation, because then
the /proc one doesn't exist.
Notice? No incompatibility. No crazy stupid new "convert one to the
other", because "the other model" NEVER EXISTS. There is only one
pidfd - it might be proc-less if CONFIG_PROC isn't there, but let's
face it, nobody even cares, because nobody ever disabled /proc anyway.
And no need for some new "convert" interface (ioctl or other).
Problem solved.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-31 21:19 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg
In-Reply-To: <132107F4-F56B-4D6E-9E00-A6F7C092E6BD@amacapital.net>
On Sun, Mar 31, 2019 at 02:09:03PM -0600, Andy Lutomirski wrote:
>
>
> > On Mar 30, 2019, at 11:24 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >> On Sat, Mar 30, 2019 at 10:12 AM Christian Brauner <christian@brauner.io> wrote:
> >>
> >>
> >> To clarify, what the Android guys really wanted to be part of the api is
> >> a way to get race-free access to metadata associated with a given pidfd.
> >> And the idea was that *if and only if procfs is mounted* you could do:
> >>
> >> int pidfd = pidfd_open(1234, 0);
> >>
> >> int procfd = open("/proc", O_RDONLY | O_CLOEXEC);
> >> int procpidfd = ioctl(pidfd, PIDFD_TO_PROCFD, procfd);
> >
> > And my claim is that this is three system calls - one of them very
> > hacky - to just do
> >
> > int pidfd = open("/proc/%d", O_PATH);
>
> Hi Linus-
>
> I want to re-check this because I think Christian’s example was bad. I proposed these ioctls, but that wasn’t the intended use. The real point is:
Getting metadata access was pushed as essential originally which is why
this ioctl() came up in the first place. The concerns about
CLONE_PIDFD were not relevant when this came up [1]:
<quote>
> And how do you propose, given one of these handle objects, getting a
> process's current priority, or its current oom score, or its list of
> memory maps? As I mentioned in my original email, and which nobody has
> addressed, if you don't use a dirfd as your process handle or you
> don't provide an easy way to get one of these proc directory FDs, you
> need to duplicate a lot of metadata access interfaces.
An API that takes a process handle object and an fd pointing at /proc
(the root of the proc fs) and gives you back a proc dirfd would do the
trick. You could do this with no new kernel features at all if you're
willing to read the pid, call openat(2), and handle the races in user
code.
<quote>
[1]: https://lore.kernel.org/lkml/CALCETrUFrFKC2YTLH7ViM_7XPYk3LNmNiaz6s8wtWo1pmJQXzg@mail.gmail.com/
>
> int pidfd = new_improved_clone(...);
>
> To be useful, this type of API *must* work without proc mounted.
>
> And, later:
>
> openat(fd to pidfd’s proc directory, “status”, ...);
>
> And we want a non-utterly-crappy way to do this. The ioctl is certainly ugly, but it *works*.
>
> Another approach is:
>
> pid_t pid = pidfd_get_pid(pidfd);
> sprintf(buf, “/proc/%d”, pid);
> int procfd = open(buf, O_PATH);
> if (pidfd_get_pid(pidfd) != pid) {
> we lose;
> }
>
> But this is clunky.
>
> Do you think the clunky version is okay, or do you have a suggestion for making it better?
>
> —Andy
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-31 22:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov
In-Reply-To: <CAHk-=wi3AE1-iRQ_7LOeSMNAMrGxRdC=gTjD30duVw4XRchcNQ@mail.gmail.com>
On Sun, Mar 31, 2019 at 02:17:48PM -0700, Linus Torvalds wrote:
> On Sun, Mar 31, 2019 at 2:10 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > I don't think that we want or can make them equivalent since that would
> > mean we depend on procfs.
>
> Sure we can.
>
> If /proc is enabled, then you always do that dance YOU ALREADY WROTE
> THE CODE FOR to do the stupid ioctl.
>
> And if /procfs isn't enabled, then you don't do that.
>
> Ta-daa. Done. No stupid ioctl, and now /proc and pidfd_open() return
> the same damn thing.
>
> And guess what? If /proc isn't enabled, then obviously pidfd_open()
> gives you the /proc-less thing, but at least there is no crazy "two
> different file descriptors for the same thing" situation, because then
> the /proc one doesn't exist.
>
> Notice? No incompatibility. No crazy stupid new "convert one to the
> other", because "the other model" NEVER EXISTS. There is only one
> pidfd - it might be proc-less if CONFIG_PROC isn't there, but let's
> face it, nobody even cares, because nobody ever disabled /proc anyway.
Thanks for the input. The problem Jann and I saw with this is that it
would be awkward to have the kernel open a file in some procfs instance,
since then userspace would have to specify which procfs instance the fd
should come from. Yes, it could probably always be the callers procfs
instance. But I'm concerned how this will interact with procfs mount
options such as hidepid={1,2} and pid namespaces.
One concern is what should happen when there's no procfs mount in the
callers mount namespace. Do we check for that in pidfd_open() and at
CLONE_PIDFD time and then refuse to give the caller a pidfd in that case
or do they still get a dirfd to /proc/<pid>? If we refuse it would mean
that having procfs mounted is necessary to get a pidfd if we don't
refuse it would mean that we're making process metadata available
without procfs being mounted which an administrator migh not want by not
having mounted procfs.
Another question is what happens when procfs is mounted with
hidepid={1,2}. If the caller accidently learns a pid they could
pidfd_open() it and circumvent hidepid={1,2}. So we would need to verify
the procfs mount options as well. I think these problems would be gone
if pidfds were opaque handles.
Christian
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-31 22:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Christian Brauner, Andy Lutomirski, Daniel Colascione, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov
In-Reply-To: <CAHk-=wi3AE1-iRQ_7LOeSMNAMrGxRdC=gTjD30duVw4XRchcNQ@mail.gmail.com>
On Sun, Mar 31, 2019 at 10:18 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Mar 31, 2019 at 2:10 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > I don't think that we want or can make them equivalent since that would
> > mean we depend on procfs.
>
> Sure we can.
>
> If /proc is enabled, then you always do that dance YOU ALREADY WROTE
> THE CODE FOR to do the stupid ioctl.
>
> And if /procfs isn't enabled, then you don't do that.
>
> Ta-daa. Done. No stupid ioctl, and now /proc and pidfd_open() return
> the same damn thing.
So this is already inherently broken with the /proc filesystem mounted
with hidepid=2. What type of object will it return when CONFIG_PROC_FS
is enabled, but I cannot see anyone's /proc entry except the
processe's running as my own user.
>
> And guess what? If /proc isn't enabled, then obviously pidfd_open()
> gives you the /proc-less thing, but at least there is no crazy "two
> different file descriptors for the same thing" situation, because then
> the /proc one doesn't exist.
There will be when you have no procfs mount for the PID namespace.
What do you return in that case? You run into the problem of two types
of descriptors on the same system.
If you choose to error out, the whole API is useless.
Yes, you can now argue that if I use hidepid=2 and cannot see other
process's /proc entry other than my own user, I wouldn't be able to
kill them anyway, so erroring out is fine, but let's not forget
CAP_KILL.
>
> Notice? No incompatibility. No crazy stupid new "convert one to the
> other", because "the other model" NEVER EXISTS. There is only one
> pidfd - it might be proc-less if CONFIG_PROC isn't there, but let's
> face it, nobody even cares, because nobody ever disabled /proc anyway.
I agree that the ioctl really sucks, but using /proc sucks even further.
Processes have their own namespace, and that is not the filesystem, it
is the PID namespace. As such, pidfd_open should be my open() for an
addressable task I can see in my PID namespace, unrelated to /proc.
Then, processes could pass a pidfd for something in their namespace
*elsewhere*, crossing namespace boundaries, as file descriptors remain
unaffected by that, and subject to kernel permissions, you could
implement some neat communication primitive just through such process
descriptors (which are stable).
It's like the filesystem in some sense, the processes form some
hierarchy, so I should be only be able to open something I can address
in this namespace. It is otherwise a gross layering violation if the
/proc filesystem works like some sort of backdoor to open some
different kind of descriptors pidfd calls accept (and it ends up
working for those I cannot even see).
>
> And no need for some new "convert" interface (ioctl or other).
>
> Problem solved.
>
> Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-03-31 22:16 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov
In-Reply-To: <20190331220259.qntxynluk765hpnt@brauner.io>
On Sun, Mar 31, 2019 at 3:03 PM Christian Brauner <christian@brauner.io> wrote:
>
> Thanks for the input. The problem Jann and I saw with this is that it
> would be awkward to have the kernel open a file in some procfs instance,
> since then userspace would have to specify which procfs instance the fd
> should come from.
I would actually suggest we just make the rules be that the
pidfd_open() always return the internal /proc entry regardless of any
mount-point (or any "hidepid") but also suggest that exactly *because*
it gives you visibility into the target pid, you'd basically require
the strictest kind of control of the process you're trying to get the
pidfd of.
Ie likely something along the lines of
ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)
kind of requirements.
But honestly, just how much do you need pidfd_open()? If this is
purely because somebody goes "oh, ASCII is expensive", then just stop
doing it entirely. It's not. It's fine. Going throuigh a filesystem is
a *good* thing, exactly because it allows MIS to control it.
So it's entirely possible that the right answer is: "just open
/proc/<pid>/", and accept the fact that everybody has it anyway, and
people who don't have it don't get the new functionality (with the
possible exception of clone(CLONE_PIDFD), which only gives you access
to a child you created yourself.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-31 22:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Daniel Colascione, Jann Horn, Andrew Lutomirski,
David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov
In-Reply-To: <CAHk-=wh0jgRkZiNdFD96Zpjx+_G+NVSHieAt+QgWCQBJ2A-5Aw@mail.gmail.com>
On Sun, Mar 31, 2019 at 03:16:47PM -0700, Linus Torvalds wrote:
> On Sun, Mar 31, 2019 at 3:03 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > Thanks for the input. The problem Jann and I saw with this is that it
> > would be awkward to have the kernel open a file in some procfs instance,
> > since then userspace would have to specify which procfs instance the fd
> > should come from.
>
> I would actually suggest we just make the rules be that the
> pidfd_open() always return the internal /proc entry regardless of any
> mount-point (or any "hidepid") but also suggest that exactly *because*
> it gives you visibility into the target pid, you'd basically require
> the strictest kind of control of the process you're trying to get the
> pidfd of.
>
> Ie likely something along the lines of
>
> ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)
I can live with that but I would like to hear what Jann thinks too if
that's ok.
Christian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox