From 020c8e2384877ffc13579f633ac3c723f80baf8c Mon Sep 17 00:00:00 2001
From: Robert Morris <rtm@csail.mit.edu>
Date: Mon, 4 Aug 2014 06:13:49 -0400
Subject: [PATCH] use acquire/release to force order for
pid=np->pid;np->state=RUNNING for bug reported by symingz@gmail.com and
cs1100254@cse.iitd.ernet.in
---
TRICKS | 16 ++++++++++------
proc.c | 10 +++++++---
proc.h | 2 +-
3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/TRICKS b/TRICKS
index 0338279..8d1439f 100644
--- a/TRICKS
+++ b/TRICKS
@@ -116,21 +116,25 @@ processors will need it.
---
The code in fork needs to read np->pid before
-setting np->state to RUNNABLE.
+setting np->state to RUNNABLE. The following
+is not a correct way to do this:
int
fork(void)
{
...
- pid = np->pid;
np->state = RUNNABLE;
- return pid;
+ return np->pid; // oops
}
After setting np->state to RUNNABLE, some other CPU
might run the process, it might exit, and then it might
get reused for a different process (with a new pid), all
-before the return statement. So it's not safe to just do
-"return np->pid;".
+before the return statement. So it's not safe to just
+"return np->pid". Even saving a copy of np->pid before
+setting np->state isn't safe, since the compiler is
+allowed to re-order statements.
-This works because proc.h marks the pid as volatile.
+The real code saves a copy of np->pid, then acquires a lock
+around the write to np->state. The acquire() prevents the
+compiler from re-ordering.
diff --git a/proc.c b/proc.c
index bcdbfea..e19539c 100644
--- a/proc.c
+++ b/proc.c
@@ -153,10 +153,16 @@ fork(void)
if(proc->ofile[i])
np->ofile[i] = filedup(proc->ofile[i]);
np->cwd = idup(proc->cwd);
+
+ safestrcpy(np->name, proc->name, sizeof(proc->name));
pid = np->pid;
+
+ // lock to force the compiler to emit the np->state write last.
+ acquire(&ptable.lock);
np->state = RUNNABLE;
- safestrcpy(np->name, proc->name, sizeof(proc->name));
+ release(&ptable.lock);
+
return pid;
}
@@ -455,5 +461,3 @@ procdump(void)
cprintf("\n");
}
}
-
-
diff --git a/proc.h b/proc.h
index 6561ad3..3b9c3ac 100644
--- a/proc.h
+++ b/proc.h
@@ -57,7 +57,7 @@ struct proc {
pde_t* pgdir; // Page table
char *kstack; // Bottom of kernel stack for this process
enum procstate state; // Process state
- volatile int pid; // Process ID
+ int pid; // Process ID
struct proc *parent; // Parent process
struct trapframe *tf; // Trap frame for current syscall
struct context *context; // swtch() here to run process