PM: resolve fork/kill race condition

When a process forks, VFS is informed on behalf of the child. This is
correct, because otherwise signals to the new child could get lost.
However, that means that the parent is not blocked from being killed
by a signal while the child is blocked on this VFS call. As a result,
by the time that the VFS reply comes in, the parent may already be
dead, and the child may thus have been assigned a new parent: INIT.

Previously, PM would blindly reply to the parent when the VFS reply
for the fork came in. Thus, it could end up sending a reply to INIT,
even though INIT did not issue the fork(2) call. This could end up
satisfying a different call from INIT (typically waitpid(2)) and then
cause an error when that other call was complete.

It would be possible to set VFS_CALL on both forking parent and child.
This patch instead adds a flag (NEW_PARENT) to note that a process's
parent has changed during a VFS call.

Change-Id: Iad930b2e441db54fe6f7d2fd011f0f6a26e2923d

diff --git a/servers/pm/forkexit.c b/servers/pm/forkexit.c
index 68ef6df3c62721831b273badb0a5aaa491ae13e3..638f29e96cdce151ae9bce7c33fb663614ed1492 100644 (file)
--- a/servers/pm/forkexit.c
+++ b/servers/pm/forkexit.c
@@ -368,6 +368,13 @@ int dump_core;                     /* flag indicating whether to dump core */
                /* 'rmp' now points to a child to be disinherited. */
                rmp->mp_parent = INIT_PROC_NR;
 
+               /* If the process is making a VFS call, remember that we set
+                * a new parent. This prevents FORK from replying to the wrong
+                * parent upon completion.
+                */
+               if (rmp->mp_flags & VFS_CALL)
+                       rmp->mp_flags |= NEW_PARENT;
+
                /* Notify new parent. */
                if (rmp->mp_flags & ZOMBIE)
                        check_parent(rmp, TRUE /*try_cleanup*/);

diff --git a/servers/pm/main.c b/servers/pm/main.c
index 96d94787b2040f609c6efe4967c7db68f6cd9ce5..002f4926ab7b879042578d19451a90b4df2004ce 100644 (file)
--- a/servers/pm/main.c
+++ b/servers/pm/main.c
@@ -383,7 +383,7 @@ static void handle_vfs_reply()
 {
   struct mproc *rmp;
   endpoint_t proc_e;
-  int r, proc_n;
+  int r, proc_n, new_parent;
 
   /* PM_REBOOT is the only request not associated with a process.
    * Handle its reply first.
@@ -411,7 +411,8 @@ static void handle_vfs_reply()
   if (!(rmp->mp_flags & VFS_CALL))
        panic("handle_vfs_reply: reply without request: %d", call_nr);
 
-  rmp->mp_flags &= ~VFS_CALL;
+  new_parent = rmp->mp_flags & NEW_PARENT;
+  rmp->mp_flags &= ~(VFS_CALL | NEW_PARENT);
 
   if (rmp->mp_flags & UNPAUSED)
        panic("handle_vfs_reply: UNPAUSED set on entry: %d", call_nr);
@@ -453,28 +454,29 @@ static void handle_vfs_reply()
 
   case PM_FORK_REPLY:
        /* Schedule the newly created process ... */
-       r = (OK);
+       r = OK;
        if (rmp->mp_scheduler != KERNEL && rmp->mp_scheduler != NONE) {
                r = sched_start_user(rmp->mp_scheduler, rmp);
        }
 
        /* If scheduling the process failed, we want to tear down the process
         * and fail the fork */
-       if (r != (OK)) {
+       if (r != OK) {
                /* Tear down the newly created process */
                rmp->mp_scheduler = NONE; /* don't try to stop scheduling */
                exit_proc(rmp, -1, FALSE /*dump_core*/);
 
-               /* Wake up the parent with a failed fork */
-               setreply(rmp->mp_parent, -1);
-
+               /* Wake up the parent with a failed fork (unless dead) */
+               if (!new_parent)
+                       setreply(rmp->mp_parent, -1);
        }
        else {
                /* Wake up the child */
                setreply(proc_n, OK);
 
-               /* Wake up the parent */
-               setreply(rmp->mp_parent, rmp->mp_pid);
+               /* Wake up the parent, unless the parent is already dead */
+               if (!new_parent)
+                       setreply(rmp->mp_parent, rmp->mp_pid);
        }
 
        break;

diff --git a/servers/pm/mproc.h b/servers/pm/mproc.h
index 26f5dc1c71b3fa93a4095298449fb8df0b25bcf9..ab2a2bd3f01576c022f46a06a370cbad41b65c76 100644 (file)
--- a/servers/pm/mproc.h
+++ b/servers/pm/mproc.h
@@ -84,6 +84,7 @@ EXTERN struct mproc {
 #define SIGSUSPENDED   0x00100 /* set by SIGSUSPEND system call */
 #define REPLY          0x00200 /* set if a reply message is pending */
 #define VFS_CALL               0x00400 /* set if waiting for VFS (normal calls) */
+#define NEW_PARENT     0x00800 /* process's parent changed during VFS call */
 #define UNPAUSED       0x01000 /* VFS has replied to unpause request */
 #define PRIV_PROC      0x02000 /* system process, special privileges */
 #define PARTIAL_EXEC   0x04000 /* process got a new map but no content */

diff --git a/test/test79.c b/test/test79.c
index 3c7504e1e1537489aac4d78e7f21788b3d84d716..b5948c67355356442f365d0d613aeb9224c77262 100644 (file)
--- a/test/test79.c
+++ b/test/test79.c
@@ -36,6 +36,7 @@ enum {
        JOB_BLOCK_PM,
        JOB_BLOCK_VFS,
        JOB_CALL_PM_VFS,
+       JOB_FORK,
        NR_JOBS
 };
 
@@ -295,6 +296,27 @@ worker_proc(struct link *parent)
                uid = getuid();
                for (;;)
                        setuid(uid);
+               break;
+       case JOB_FORK:
+               /*
+                * The child exits immediately; the parent kills the child
+                * immediately.  The outcome mostly depends on scheduling.
+                * Varying process priorities may yield different tests.
+                */
+               for (;;) {
+                       pid_t pid = fork();
+                       switch (pid) {
+                       case 0:
+                               exit(0);
+                       case -1:
+                               e(1);
+                               break;
+                       default:
+                               kill(pid, SIGKILL);
+                               if (wait(NULL) != pid) e(0);
+                       }
+               }
+               break;
        default:
                e(0);
                exit(1);