have kill() lock before looking at p->pid

document wait()'s use of np->parent w/o holding lock.

3 files changed, 32 insertions, 36 deletions

diff --git a/kernel/proc.c b/kernel/proc.c
index 5c2d4ce..1c222d3 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -367,19 +367,24 @@ wait(void)
     // Scan through table looking for exited children.
     havekids = 0;
     for(np = proc; np < &proc[NPROC]; np++){
-      if(np->parent != p)
-        continue;
-      acquire(&np->lock);
-      havekids = 1;
-      if(np->state == ZOMBIE){
-        // Found one.
-        pid = np->pid;
-        freeproc(np);
+      // this code uses np->parent without holding np->lock.
+      // acquiring the lock first would cause a deadlock,
+      // since np might be an ancestor, and we already hold p->lock.
+      if(np->parent == p){
+        // np->parent can't change here because only the parent
+        // changes it, and we're the parent.
+        acquire(&np->lock);
+        havekids = 1;
+        if(np->state == ZOMBIE){
+          // Found one.
+          pid = np->pid;
+          freeproc(np);
+          release(&np->lock);
+          release(&p->lock);
+          return pid;
+        }
         release(&np->lock);
-        release(&p->lock);
-        return pid;
       }
-      release(&np->lock);
     }
 
     // No point waiting if we don't have any children.
@@ -397,10 +402,10 @@ wait(void)
 // Per-CPU process scheduler.
 // Each CPU calls scheduler() after setting itself up.
 // Scheduler never returns.  It loops, doing:
-//  - choose a process to run
-//  - swtch to start running that process
+//  - choose a process to run.
+//  - swtch to start running that process.
 //  - eventually that process transfers control
-//      via swtch back to the scheduler.
+//    via swtch back to the scheduler.
 void
 scheduler(void)
 {
@@ -409,7 +414,7 @@ scheduler(void)
   
   c->proc = 0;
   for(;;){
-    // Enable interrupts on this processor.
+    // Give devices a brief chance to interrupt.
     intr_on();
 
     for(p = proc; p < &proc[NPROC]; p++) {
@@ -508,7 +513,7 @@ sleep(void *chan, struct spinlock *lk)
   // change p->state and then call sched.
   // Once we hold p->lock, we can be
   // guaranteed that we won't miss any wakeup
-  // (wakeup runs with p->lock locked),
+  // (wakeup locks p->lock),
   // so it's okay to release lk.
   if(lk != &p->lock){  //DOC: sleeplock0
     acquire(&p->lock);  //DOC: sleeplock1
@@ -559,24 +564,24 @@ wakeup(void *chan)
 
 // Kill the process with the given pid.
 // Process won't exit until it returns
-// to user space (see trap in trap.c).
+// to user space (see usertrap() in trap.c).
 int
 kill(int pid)
 {
   struct proc *p;
 
   for(p = proc; p < &proc[NPROC]; p++){
+    acquire(&p->lock);
     if(p->pid == pid){
-      acquire(&p->lock);
-      if(p->pid != pid)
-        panic("kill");
       p->killed = 1;
-      // Wake process from sleep if necessary.
-      if(p->state == SLEEPING)
+      if(p->state == SLEEPING){
+        // Wake process from sleep().
         p->state = RUNNABLE;
+      }
       release(&p->lock);
       return 0;
     }
+    release(&p->lock);
   }
   return -1;
 }
diff --git a/kernel/syscall.c b/kernel/syscall.c
index a054da2..adbad73 100644
--- a/kernel/syscall.c
+++ b/kernel/syscall.c
@@ -163,8 +163,8 @@ static uint64 (*syscalls[])(void) = {
 [SYS_close]   sys_close,
 };
 
-static void
-dosyscall(void)
+void
+syscall(void)
 {
   int num;
   struct proc *p = myproc();
@@ -180,15 +180,3 @@ dosyscall(void)
     p->tf->a0 = -1;
   }
 }
-
-void
-syscall()
-{
-    if(myproc()->killed)
-      exit();
-    dosyscall();
-    if(myproc()->killed)
-      exit();
-    return;
-}
-
diff --git a/kernel/trap.c b/kernel/trap.c
index 27cfd32..eb7f6cf 100644
--- a/kernel/trap.c
+++ b/kernel/trap.c
@@ -53,6 +53,9 @@ usertrap(void)
   if(r_scause() == 8){
     // system call
 
+    if(p->killed)
+      exit();
+
     // sepc points to the ecall instruction,
     // but we want to return to the next instruction.
     p->tf->epc += 4;