bug fix: reparent() sometimes deadlocked

bug fix: exit() sometimes released a different parent lock than it acquired
usertests

1 files changed, 32 insertions, 15 deletions

diff --git a/kernel/proc.c b/kernel/proc.c
index 0d91a59..4f6caee 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -286,11 +286,11 @@ fork(void)
 }
 
 // Pass p's abandoned children to init.
-// Caller must hold p->lock and parent->lock.
+// Caller must hold p->lock.
 void
-reparent(struct proc *p, struct proc *parent) {
+reparent(struct proc *p)
+{
   struct proc *pp;
-  int child_of_init = (p->parent == initproc);
 
   for(pp = proc; pp < &proc[NPROC]; pp++){
     // this code uses pp->parent without holding pp->lock.
@@ -302,13 +302,10 @@ reparent(struct proc *p, struct proc *parent) {
       // because only the parent changes it, and we're the parent.
       acquire(&pp->lock);
       pp->parent = initproc;
-      if(pp->state == ZOMBIE) {
-        if(!child_of_init)
-          acquire(&initproc->lock);
-        wakeup1(initproc);
-        if(!child_of_init)
-          release(&initproc->lock);
-      }
+      // we should wake up init here, but that would require
+      // initproc->lock, which would be a deadlock, since we hold
+      // the lock on one of init's children (pp). this is why
+      // exit() always wakes init (before acquiring any locks).
       release(&pp->lock);
     }
   }
@@ -339,20 +336,38 @@ exit(int status)
   end_op();
   p->cwd = 0;
 
-  acquire(&p->parent->lock);
-    
+  // we might re-parent a child to init. we can't be precise
+  // about waking up init, since we can't acquire its lock once
+  // we've acquired any other proc lock. so wake up init whether
+  // that's necessary or not. init may miss this wakeup,
+  // that that seems harmless.
+  acquire(&initproc->lock);
+  wakeup1(initproc);
+  release(&initproc->lock);
+
+  // grab a copy of p->parent, to ensure that we unlock the
+  // same parent we locked. in case our parent gives us away
+  // to init while we're waiting for the parent lock.
+  acquire(&p->lock);
+  struct proc *original_parent = p->parent;
+  release(&p->lock);
+  
+  // we need the parent's lock in order to wake it up from wait().
+  // the parent-then-child rule says we have to lock it first.
+  acquire(&original_parent->lock);
+
   acquire(&p->lock);
 
   // Give any children to init.
-  reparent(p, p->parent);
+  reparent(p);
 
   // Parent might be sleeping in wait().
-  wakeup1(p->parent);
+  wakeup1(original_parent);
 
   p->xstate = status;
   p->state = ZOMBIE;
 
-  release(&p->parent->lock);
+  release(&original_parent->lock);
 
   // Jump into the scheduler, never to return.
   sched();
@@ -564,6 +579,8 @@ wakeup(void *chan)
 static void
 wakeup1(struct proc *p)
 {
+  if(!holding(&p->lock))
+    panic("wakeup1");
   if(p->chan == p && p->state == SLEEPING) {
     p->state = RUNNABLE;
   }