Re: [RFC PATCH] Implement /proc/pid/kill

[Daniel Colascione]

On Wed, Oct 31, 2018 at 12:57 AM, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> On Tue, Oct 30, 2018 at 11:10:47PM +0000, Daniel Colascione wrote:
>> On Tue, Oct 30, 2018 at 10:33 PM, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
>> > On Wed, Oct 31, 2018 at 09:23:39AM +1100, Aleksa Sarai wrote:
>> >> On 2018-10-30, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
>> >> > On Wed, Oct 31, 2018 at 07:45:01AM +1100, Aleksa Sarai wrote:
>> >> > [...]
>> >> > > > > (Unfortunately
>> >> > > > > there are lots of things that make it a bit difficult to use /proc/$pid
>> >> > > > > exclusively for introspection of a process -- especially in the context
>> >> > > > > of containers.)
>> >> > > >
>> >> > > > Tons of things already break without a working /proc. What do you have in mind?
>> >> > >
>> >> > > Heh, if only that was the only blocker. :P
>> >> > >
>> >> > > The basic problem is that currently container runtimes either depend on
>> >> > > some non-transient on-disk state (which becomes invalid on machine
>> >> > > reboots or dead processes and so on), or on long-running processes that
>> >> > > keep file descriptors required for administration of a container alive
>> >> > > (think O_PATH to /dev/pts/ptmx to avoid malicious container filesystem
>> >> > > attacks). Usually both.
>> >> > >
>> >> > > What would be really useful would be having some way of "hiding away" a
>> >> > > mount namespace (of the pid1 of the container) that has all of the
>> >> > > information and bind-mounts-to-file-descriptors that are necessary for
>> >> > > administration. If the container's pid1 dies all of the transient state
>> >> > > has disappeared automatically -- because the stashed mount namespace has
>> >> > > died. In addition, if this was done the way I'm thinking with (and this
>> >> > > is the contentious bit) hierarchical mount namespaces you could make it
>> >> > > so that the pid1 could not manipulate its current mount namespace to
>> >> > > confuse the administrative process. You would also then create an
>> >> > > intermediate user namespace to help with several race conditions (that
>> >> > > have caused security bugs like CVE-2016-9962) we've seen when joining
>> >> > > containers.
>> >> > >
>> >> > > Unfortunately this all depends on hierarchical mount namespaces (and
>> >> > > note that this would just be that NS_GET_PARENT gives you the mount
>> >> > > namespace that it was created in -- I'm not suggesting we redesign peers
>> >> > > or anything like that). This makes it basically a non-starter.
>> >> > >
>> >> > > But if, on top of this ground-work, we then referenced containers
>> >> > > entirely via an fd to /proc/$pid then you could also avoid PID reuse
>> >> > > races (as well as being able to find out implicitly whether a container
>> >> > > has died thanks to the error semantics of /proc/$pid). And that's the
>> >> > > way I would suggest doing it (if we had these other things in place).
>> >> >
>> >> > I didn't fully follow exactly what you mean. If you can explain for the
>> >> > layman who doesn't know much experience with containers..
>> >> >
>> >> > Are you saying that keeping open a /proc/$pid directory handle is not
>> >> > sufficient to prevent PID reuse while the proc entries under /proc/$pid are
>> >> > being looked into? If its not sufficient, then isn't that a bug? If it is
>> >> > sufficient, then can we not just keep the handle open while we do whatever we
>> >> > want under /proc/$pid ?
>> >>
>> >> Sorry, I went on a bit of a tangent about various internals of container
>> >> runtimes. My main point is that I would love to use /proc/$pid because
>> >> it makes reuse handling very trivial and is always correct, but that
>> >> there are things which stop us from being able to use it for everything
>> >> (which is what my incoherent rambling was on about).
>> >
>> > Ok thanks. So I am guessing if the following sequence works, then Dan's patch is not
>> > needed.
>> >
>> > 1. open /proc/<pid> directory
>> > 2. inspect /proc/<pid> or do whatever with <pid>
>> > 3. Issue the kill on <pid>
>> > 4. Close the /proc/<pid> directory opened in step 1.
>> >
>> > So unless I missed something, the above sequence will not cause any PID reuse
>> > races.
>>
>> Keeping a /proc/$PID directory file descriptor open does not prevent
>> $PID being used to name some other process. If it could, you could
>> pretty quickly fill a whole system's process table. See the program
>> below, which demonstrates the PID collision.
>
> I know. We both were not sure about that earlier, that's why I requested you
> to write the program when we were privately chatting. Now I'm sure because
> Aleska answered that and the you program you wrote showed that too.

I don't think that this behavior was ever in doubt from my side.

> I wonder if this cannot be plumbed by just making the /proc/$PID directory
> opens hold a reference to task_struct (and a reference to whatever else is
> supposed to prevent the PID from getting reused), instead of introducing a
> brand new API.

That *is* a brand-new API ---- just spelled the same as an old API.
Besides, the PID-preserving handle approach has a problem with
rlimits. In particular, a user that is otherwise limited by
RLIMIT_NPROC could squat on far more entries in the process table than
he could otherwise. (And the whole point of RLIMIT_NPROC is to limit
process table squatting.) You can't just make procfs directory FDs
count against RLIMIT_NPROC, because that'd break existing user code
that assumes that procfs FDs *don't* count against the user process
limit.

>> I think Aleksa's larger point is that it's useful to treat processes
>> as other file-descriptor-named, poll-able, wait-able resources.
>> Consistency is important. A process is just another system resource,
>> and like any other system resource, you should be open to hold a file
>> descriptor to it and do things to that process via that file
>> descriptor. The precise form of this process-handle FD is up for
>> debate. The existing /proc/$PID directory FD is a good candidate for a
>> process handle FD, since it does almost all of what's needed. But
>> regardless of what form a process handle FD takes, we need it. I don't
>> see a case for continuing to treat processes in a non-unixy,
>> non-file-descriptor-based manner.
>
> So wait, how is that supposed to address what you're now saying above
> "quickly fill a whole process table"? You either want this, or you don't :)

I don't understand what you're getting at.