fix corner cases in exec of ELF

put an invalid page below the stack
have fork() handle invalid pages

8 files changed, 84 insertions, 37 deletions

diff --git a/defs.h b/defs.h
index 4a63154..b691099 100644
--- a/defs.h
+++ b/defs.h
@@ -163,7 +163,8 @@ void            freevm(pde_t*);
 void            inituvm(pde_t*, char*, char*, uint);
 int             loaduvm(pde_t*, char*, struct inode *ip, uint, uint);
 pde_t*          copyuvm(pde_t*,uint);
-void            loadvm(struct proc*);
+void            switchuvm(struct proc*);
+void            switchkvm();
 
 // number of elements in fixed-size array
 #define NELEM(x) (sizeof(x)/sizeof((x)[0]))
diff --git a/exec.c b/exec.c
index 8a92e99..4f11695 100644
--- a/exec.c
+++ b/exec.c
@@ -43,13 +43,16 @@ exec(char *path, char **argv)
       goto bad;
     if (!allocuvm(pgdir, (char *)ph.va, ph.memsz))
       goto bad;
-    sz += PGROUNDUP(ph.memsz);
+    if(ph.va + ph.memsz > sz)
+      sz = ph.va + ph.memsz;
     if (!loaduvm(pgdir, (char *)ph.va, ip, ph.offset, ph.filesz))
       goto bad;
   }
   iunlockput(ip);
 
   // Allocate and initialize stack at sz
+  sz = PGROUNDUP(sz);
+  sz += PGSIZE; // leave an invalid page
   if (!allocuvm(pgdir, (char *)sz, PGSIZE))
     goto bad;
   mem = uva2ka(pgdir, (char *)sz);
@@ -95,7 +98,7 @@ exec(char *path, char **argv)
   proc->tf->eip = elf.entry;  // main
   proc->tf->esp = sp;
 
-  loadvm(proc); 
+  switchuvm(proc); 
 
   freevm(oldpgdir);
 
diff --git a/kalloc.c b/kalloc.c
index 5661105..7695006 100644
--- a/kalloc.c
+++ b/kalloc.c
@@ -1,9 +1,8 @@
 // Physical memory allocator, intended to allocate
-// memory for user processes. Allocates in 4096-byte "pages".
+// memory for user processes. Allocates in 4096-byte pages.
 // Free list is kept sorted and combines adjacent pages into
 // long runs, to make it easier to allocate big segments.
-// One reason the page size is 4k is that the x86 segment size
-// granularity is 4k.
+// This combining is not useful now that xv6 uses paging.
 
 #include "types.h"
 #include "defs.h"
diff --git a/mmu.h b/mmu.h
index 76a6f1b..f4fc732 100644
--- a/mmu.h
+++ b/mmu.h
@@ -129,7 +129,6 @@ struct segdesc {
 #define PTE_ADDR(pte)	((uint) (pte) & ~0xFFF)
 
 typedef uint pte_t;
-extern pde_t    *kpgdir;
 
 // Control Register flags
 #define CR0_PE		0x00000001	// Protection Enable
diff --git a/proc.c b/proc.c
index dd6f27e..f799a4d 100644
--- a/proc.c
+++ b/proc.c
@@ -145,7 +145,7 @@ growproc(int n)
   if (!allocuvm(proc->pgdir, (char *)proc->sz, n))
     return -1;
   proc->sz += n;
-  loadvm(proc);
+  switchuvm(proc);
   return 0;
 }
 
@@ -214,9 +214,10 @@ scheduler(void)
       // to release ptable.lock and then reacquire it
       // before jumping back to us.
       proc = p;
-      loadvm(p);
+      switchuvm(p);
       p->state = RUNNING;
       swtch(&cpu->scheduler, proc->context);
+      switchkvm();
 
       // Process is done running for now.
       // It should have changed its p->state before coming back.
@@ -242,7 +243,6 @@ sched(void)
     panic("sched running");
   if(readeflags()&FL_IF)
     panic("sched interruptible");
-  lcr3(PADDR(kpgdir));   // Switch to the kernel page table
   intena = cpu->intena;
   swtch(&proc->context, cpu->scheduler);
   cpu->intena = intena;
@@ -414,8 +414,8 @@ wait(void)
         // Found one.
         pid = p->pid;
         kfree(p->kstack, KSTACKSIZE);
-	p->kstack = 0;
-	freevm(p->pgdir);
+        p->kstack = 0;
+        freevm(p->pgdir);
         p->state = UNUSED;
         p->pid = 0;
         p->parent = 0;
diff --git a/proc.h b/proc.h
index 5e5d031..7d97dfa 100644
--- a/proc.h
+++ b/proc.h
@@ -16,7 +16,7 @@
 // Contexts are stored at the bottom of the stack they
 // describe; the stack pointer is the address of the context.
 // The layout of the context matches the layout of the stack in swtch.S
-// at "Switch stacks" comment. Switch itself doesn't save eip explicitly,
+// at the "Switch stacks" comment. Switch doesn't save eip explicitly,
 // but it is on the stack and allocproc() manipulates it.
 struct context {
   uint edi;
@@ -31,7 +31,7 @@ enum procstate { UNUSED, EMBRYO, SLEEPING, RUNNABLE, RUNNING, ZOMBIE };
 // Per-process state
 struct proc {
   uint sz;                     // Size of process memory (bytes)
-  pde_t* pgdir;                // linear address of proc's pgdir
+  pde_t* pgdir;                // Linear address of proc's pgdir
   char *kstack;                // Bottom of kernel stack for this process
   enum procstate state;        // Process state
   volatile int pid;            // Process ID
@@ -48,6 +48,7 @@ struct proc {
 // Process memory is laid out contiguously, low addresses first:
 //   text
 //   original data and bss
+//   invalid page
 //   fixed-size stack
 //   expandable heap
 
diff --git a/usertests.c b/usertests.c
index 2bd21ba..247cc95 100644
--- a/usertests.c
+++ b/usertests.c
@@ -1261,6 +1261,29 @@ sbrktest(void)
   printf(stdout, "sbrk test OK\n");
 }
 
+void
+stacktest(void)
+{
+  printf(stdout, "stack test\n");
+  char dummy = 1;
+  char *p = &dummy;
+  int ppid = getpid();
+  int pid = fork();
+  if(pid < 0){
+    printf(stdout, "fork failed\n");
+    exit();
+  }
+  if(pid == 0){
+    // should cause a trap:
+    p[-4096] = 'z';
+    kill(ppid);
+    printf(stdout, "stack test failed: page before stack was writeable\n");
+    exit();
+  }
+  wait();
+  printf(stdout, "stack test OK\n");
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -1272,6 +1295,7 @@ main(int argc, char *argv[])
   }
   close(open("usertests.ran", O_CREATE));
 
+  stacktest();
   sbrktest();
 
   opentest();
diff --git a/vm.c b/vm.c
index 9ea5e92..6914dd3 100644
--- a/vm.c
+++ b/vm.c
@@ -8,13 +8,20 @@
 
 // The mappings from logical to linear are one to one (i.e.,
 // segmentation doesn't do anything).
-// The mapping from linear to physical are one to one for the kernel.
-// The mappings for the kernel include all of physical memory (until
-// PHYSTOP), including the I/O hole, and the top of physical address
-// space, where additional devices are located.
-// The kernel itself is linked to be at 1MB, and its physical memory
-// is also at 1MB.
-// Physical memory for user programs is allocated from physical memory
+// There is one page table per process, plus one that's used
+// when a CPU is not running any process (kpgdir).
+// A user process uses the same page table as the kernel; the
+// page protection bits prevent it from using anything other
+// than its memory.
+// 
+// setupkvm() and exec() set up every page table like this:
+//   0..640K          : user memory (text, data, stack, heap)
+//   640K..1M         : mapped direct (for IO space)
+//   1M..kernend      : mapped direct (for the kernel's text and data)
+//   kernend..PHYSTOP : mapped direct (kernel heap and user pages)
+//   0xfe000000..0    : mapped direct (devices such as ioapic)
+//
+// The kernel allocates memory for its heap and for user memory
 // between kernend and the end of physical memory (PHYSTOP).
 // The virtual address space of each user program includes the kernel
 // (which is inaccessible in user mode).  The user program addresses
@@ -31,7 +38,7 @@ static uint kerndata;
 static uint kerndsz;
 static uint kernend;
 static uint freesz;
-pde_t *kpgdir;         // One kernel page table for scheduler procs
+static pde_t *kpgdir;  // for use in scheduler()
 
 // return the address of the PTE in page table pgdir
 // that corresponds to linear address va.  if create!=0,
@@ -114,9 +121,9 @@ ksegment(void)
   proc = 0;
 }
 
-// Setup address space and current process task state.
+// Switch h/w page table and TSS registers to point to process p.
 void
-loadvm(struct proc *p)
+switchuvm(struct proc *p)
 {
   pushcli();
 
@@ -128,14 +135,21 @@ loadvm(struct proc *p)
   ltr(SEG_TSS << 3);
 
   if (p->pgdir == 0)
-    panic("loadvm: no pgdir\n");
+    panic("switchuvm: no pgdir\n");
 
   lcr3(PADDR(p->pgdir));  // switch to new address space
   popcli();
 }
 
-// Setup kernel part of a page table. Linear adresses map one-to-one
-// on physical addresses.
+// Switch h/w page table register to the kernel-only page table, for when
+// no process is running.
+void
+switchkvm()
+{
+  lcr3(PADDR(kpgdir));   // Switch to the kernel page table
+}
+
+// Set up kernel part of a page table.
 pde_t*
 setupkvm(void)
 {
@@ -163,6 +177,10 @@ setupkvm(void)
   return pgdir;
 }
 
+// return the physical address that a given user address
+// maps to. the result is also a kernel logical address,
+// since the kernel maps the physical memory allocated to user
+// processes directly.
 char*
 uva2ka(pde_t *pgdir, char *uva)
 {    
@@ -266,6 +284,8 @@ inituvm(pde_t *pgdir, char *addr, char *init, uint sz)
   }
 }
 
+// given a parent process's page table, create a copy
+// of it for a child.
 pde_t*
 copyuvm(pde_t *pgdir, uint sz)
 {
@@ -278,17 +298,20 @@ copyuvm(pde_t *pgdir, uint sz)
   for (i = 0; i < sz; i += PGSIZE) {
     if (!(pte = walkpgdir(pgdir, (void *)i, 0)))
       panic("copyuvm: pte should exist\n");
-    pa = PTE_ADDR(*pte);
-    if (!(mem = kalloc(PGSIZE)))
-      return 0;
-    memmove(mem, (char *)pa, PGSIZE);
-    if (!mappages(d, (void *)i, PGSIZE, PADDR(mem), PTE_W|PTE_U))
-      return 0;
+    if(*pte & PTE_P){
+      pa = PTE_ADDR(*pte);
+      if (!(mem = kalloc(PGSIZE)))
+        return 0;
+      memmove(mem, (char *)pa, PGSIZE);
+      if (!mappages(d, (void *)i, PGSIZE, PADDR(mem), PTE_W|PTE_U))
+        return 0;
+    }
   }
   return d;
 }
 
-// Gather about physical memory layout.  Called once during boot.
+// Gather information about physical memory layout.
+// Called once during boot.
 void
 pminit(void)
 {
@@ -307,9 +330,6 @@ pminit(void)
   kerndsz = ph[1].memsz;
   freesz = PHYSTOP - kernend;
 
-  cprintf("kerntext@0x%x(sz=0x%x), kerndata@0x%x(sz=0x%x), kernend 0x%x freesz = 0x%x\n", 
-	  kerntext, kerntsz, kerndata, kerndsz, kernend, freesz);
-
   kinit((char *)kernend, freesz);
 }