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

[Daniel Colascione]

On Wed, Oct 31, 2018 at 12:42 AM, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> On Wed, Oct 31, 2018 at 09:49:08AM +1100, Aleksa Sarai wrote:
>> On 2018-10-30, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> 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.
>>
>> (Sorry, I misunderstood your original question.)
>>
>> The problem is that holding /proc/$pid doesn't stop the PID from dying
>> and being reused. The benefit of holding open /proc/$pid is that you
>> will get an error if you try to use it *after* the PID has died -- which
>> means that you don't need to worry about explicitly checking for PID
>> reuse if you are only operating with the file descriptor and not the
>> PID.
>>
>> So that sequence won't always work. There is a race where the pid might
>> die and be recycled by the time you call kill(2) -- after you've done
>> step 2. By tying step 2 and 3 together -- in this patch -- you remove
>> the race (since in order to resolve the "kill" procfs file VFS must
>> resolve the PID first -- atomically).
>
> Makes sense, thanks.
>
>> Though this race window is likely very tiny, and I wonder how much PID
>> churn you really need to hit it.
>
> Yeah that's what I asked initially how much of a problem it really is.

It's fundamentally impossible to use the process stuff today in a
race-free manner today. That the race occurs rarely isn't a good
reason to fix it. The fixes people are proposing are all lightweight,
so I don't understand this desire to stick with the status quo.
There's a longstanding API bug here. We can fix it, so we should.