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

[Aleksa Sarai <cyphar@cyphar.com>]

[-- Attachment #1: Type: text/plain, Size: 4748 bytes --]

On 2018-10-30, Joel Fernandes <joel@joelfernandes.org> 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).

Though this race window is likely very tiny, and I wonder how much PID
churn you really need to hit it.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]