From: David van Moolenbroek <david@minix3.org>
Date: Tue, 29 Oct 2013 23:39:55 +0000 (+0100)
Subject: PM: resolve fork/kill race condition
X-Git-Tag: v3.3.0~524
X-Git-Url: http://zhaoyanbai.com/repos/?a=commitdiff_plain;h=f310aefcbd1f25ed36822f80b791077e82128132;p=minix.git

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 68ef6df3c..638f29e96 100644
--- 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 96d94787b..002f4926a 100644
--- 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 26f5dc1c7..ab2a2bd3f 100644
--- 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 3c7504e1e..b5948c673 100644
--- 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);