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

[Daniel Colascione <dancol@google.com>]

On Tue, Oct 30, 2018 at 10:33 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Wed, Oct 31, 2018 at 09:23:39AM +1100, Aleksa Sarai wrote:
>> On 2018-10-30, Joel Fernandes <joel@joelfernandes.org> 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 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.

#include <fcntl.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>

int
main()
{
    int child_pid = fork();
    if (child_pid < 0)
        abort();

    char buf[64];
    int child_procfs_fd;

    if (child_pid == 0) {
        for (;;)
            pause();
        abort();
    }

    printf("child PID is %d\n", child_pid);
    sprintf(buf, "/proc/%d", child_pid);
    child_procfs_fd = open(buf, O_DIRECTORY | O_RDONLY);
    if (child_procfs_fd < 0)
        abort();
    printf("FD# of open /proc/%d is %d\n",
           child_pid,
           child_procfs_fd);
    printf("killing child with SIGKILL\n");
    kill(child_pid, SIGKILL);
    if (wait(NULL) != child_pid)
        abort();
    printf("child is now dead. PROCFS FD STILL OPEN\n");
    for (;;) {
        int new_child_pid = fork();
        if (new_child_pid < 0)
            abort();
        if (new_child_pid == 0)
            _exit(0);
        // printf("new child PID: %d\n", new_child_pid);
        if (wait(NULL) != new_child_pid)
            abort();
        if (new_child_pid == child_pid) {
            printf("FOUND PID COLLISION %d\n", child_pid);
            printf("old child had pid %d. new, "
                   "different child has pid %d. "
                   "procfs directory for old child still open!\n",
                   child_pid, child_pid);
            break;
        }
    }

    return 0;
}