Re: [PATCH v2 0/5] pid: add pidfd_open()

[Daniel Colascione]

On Mon, Apr 1, 2019 at 9:07 AM Jonathan Kowalski <bl0pbl33p@xxxxxxxxx> wrote:
>
> On Mon, Apr 1, 2019 at 4:55 PM Daniel Colascione <dancol@xxxxxxxxxx> wrote:
> >
> > On Mon, Apr 1, 2019 at 8:36 AM Linus Torvalds
> > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Apr 1, 2019 at 4:41 AM Aleksa Sarai <cyphar@xxxxxxxxxx> wrote:
> > > >
> > > > Eric pitched a procfs2 which would *just* be the PIDs some time ago (in
> > > > an attempt to make it possible one day to mount /proc inside a container
> > > > without adding a bunch of masked paths), though it was just an idea and
> > > > I don't know if he ever had a patch for it.
> >
> > Couldn't this mode just be a relatively simple procfs mount option
> > instead of a whole new filesystem? It'd be a bit like hidepid, right?
> > The internal bind mount option and the no-dotdot-traversal options
> > also look good to me.
> >
> > > I wonder if we really want a fill procfs2, or maybe we could just make
> > > the pidfd readable (yes, it's a directory file descriptor, but we
> > > could allow reading).
> >
> > What would read(2) read?
> >
> > > What are the *actual* use cases for opening /proc files through it? If
> > > it's really just for a small subset that android wants to do this
> > > (getting basic process state like "running" etc), rather than anything
> > > else, then we could skip the whole /proc linking entirely and go the
> > > other way instead (ie open_pidfd() would get that limited IO model,
> > > and we could make the /proc directory node get the same limited IO
> > > model).
> >
> > We do a lot of process state inspection and manipulation, including
> > reading and writing the oom killer adjustment score, reading smaps,
> > and the occasional cgroup manipulation. More generally, I'd also like
> > to be able to write a race-free pkill(1). Doing this work via pidfd
> > would be convenient. More generally, we can't enumerate the specific
> > use cases, because what we want to do with processes isn't bounded in
> > advance, and we regularly find new things in /proc/pid that we want to
> > read and write. I'd rather not prematurely limit the applicability of
> > the pidfd interface, especially when there's a simple option (the
> > procfs directory file descriptor approach) that doesn't require
> > in-advance enumeration of supported process inspection and
> > manipulation actions or a separate per-option pidfd equivalent. I very
> > much want a general-purpose API that reuses the metadata interfaces
> > the kernel already exposes. It's not clear to me how this rich
> > interface could be matched by read(2) on a pidfd.
>
> With the POLLHUP model on a simple pidfd, you'd know when the process
> you were referring to is dead (and one can map POLLPRI to dead and
> POLLHUP to zombie, etc).

I agree that there needs to be *some* kind of pollable file descriptor
that fires on process death. Should it be the pifd itself? I've been
thinking that a pollable directory would be too weird, but if that's
not actually a problem, I'm fine with it.

Mapping different state transitions to different poll bits is an
interesting idea, but I'm also worried about making poll the "source
of truth" with respect to process state as reported by a pidfd.
Consider a socket: for a socket, read(2)/recv(2) is the "source of
truth" and poll is just a hint that says "now is a good time to
attempt a read". Some other systems even allow for spurious wakeups
from poll. It's also worth keeping in mind that some pre-existing
event loops let you provide "is readable" and "is writable" callbacks,
but don't support the more exotic poll notification bits. That's why
I've tried to keep my proposals limited to poll signaling readability.

But I'm not really that picky. I just want something that works.

> This is just an extension of the child process model, since you'd know
> when it's dead, there's no race involved with opening the wrong
> /proc/<PID> entry. The entire thing is already non-racy for direct
> children, the same model can be extended to non-direct ones.
>
> This simplifies a lot of things, now I am essentially just passing a
> file descriptor pinning the struct pid associated with the original
> task, and not process state around to others (I may even want the
> other process to not read that stuff out even if it was allowed to, as
> it wouldn't have been able to otherwise, due to being a in a different
> mount namespace). KISS.
>
> The upshot is this same descriptor can be returned from clone, which
> would allow you to directly register it in your event loop (like
> signalfd, timerfd, file fd, sockets, etc) and POLLIN generated for you
> to read back its exit status (it is arguable if non-parents should be
> returned a readable instance from pidfd_open, but parents sure
> should). You can disable SIGCHLD for the child, and read back exit
> status much later. The entire point of waiting and reaping was that
> it'd be lost, but now you have a descriptor where it is kept for you
> to consume.

There's a subtlety: suppose I'm a library and I want to create a
private subprocess. I use the new clone facility, whatever it is, and
get a pidfd back. I need to be able to read the child's exit status
even if the child exits before clone returns in the parent. Doesn't
this requirement imply that the pidfd, kernel-side, contain something
a bit more than a struct pid?