summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Morris <[email protected]>2014-08-04 06:13:49 -0400
committerRobert Morris <[email protected]>2014-08-04 06:13:49 -0400
commit020c8e2384877ffc13579f633ac3c723f80baf8c (patch)
treebcc593b814cfc416c6ba08d386bd6d269c7fa157
parent86188d9d49fe62a2f4d8b0677d33608b3b949336 (diff)
downloadxv6-labs-020c8e2384877ffc13579f633ac3c723f80baf8c.tar.gz
xv6-labs-020c8e2384877ffc13579f633ac3c723f80baf8c.tar.bz2
xv6-labs-020c8e2384877ffc13579f633ac3c723f80baf8c.zip
use acquire/release to force order for pid=np->pid;np->state=RUNNING
for bug reported by [email protected] and [email protected]
-rw-r--r--TRICKS18
-rw-r--r--proc.c10
-rw-r--r--proc.h2
3 files changed, 19 insertions, 11 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;".
-
-This works because proc.h marks the pid as volatile.
+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.
+
+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