x86: Ensure boot_pgdir's user entries are unmapped
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 12 Jul 2016 18:30:39 +0000 (14:30 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 19 Jul 2016 15:43:10 +0000 (11:43 -0400)
For sanity reasons, we should never have anything in boot_pgdir below ULIM.
Technically, we could make it work, but not without some thought.  The
issues is that PML4 entries are pointers, pointing to common PML3s.  Any
entry in boot_pgdir is shared with every process, from the PML3 on down.
The kernel expects to manage and synchronize global access to the kernel
mappings (above ULIM).  But memory below ULIM is managed per-process.

Backstory: I was trying to debug a null function pointer by mapping
something at page 0.  The kernel panicked after decreffing a page too many
times.

What happened was that inserting one page at virtual addr 0 created a PML3,
PML2, and PML1 that was shared between every process - not just the page
mapped at 0.  This PTE reach was 512 GB, including the program binary
(which was in the page cache).

The first process was fine.  However, when we forked, the pages for e.g.
busybox's text segment already had PTEs in the new process's address space.
Technically, they were the same PTEs (and PML3, 2, and 1) as the parent
process, since they were shared data structures.

Anyway, map_page_at_addr() saw that the PTE had a mapping, so it decreffed
the page before inserting a new page.  It just so happened that the new
page was the same as the old one, since it was a fork (duplicate_vmrs,
etc).  That page had a single refcnt, since it was managed by the page
cache, causing it to be freed.

Now the page is freed, but it is in the page tables still, since
map_page_at_addr() wrote the PTE with the value of the page.  Basically, it
was a "replace the PTE with its own value", but it triggered a decref.

Next up was the VMR for busybox's MAP_PRIVATE vmr.  Since it's a private
mapping, it gets its own page.  upage_alloc() gives us the most recently
freed page, which was the one that was still in the address space, right at
the top of the text segment.

Eventually the process page faults, probably because of the madness of its
address space.  When it does, the kernel puts every page in the VMRs.  At
least one page (the one we discussed) was in there twice.  The second
decref triggers the kref assert, since we decreffed something that was
already 0.

Good times.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/arch/x86/pmap64.c
kern/src/env.c

index 1c2ac2d..09469a9 100644 (file)
@@ -623,11 +623,28 @@ uintptr_t gva2gpa(struct proc *p, uintptr_t cr3, uintptr_t gva)
  * data segment). */
 int arch_pgdir_setup(pgdir_t boot_copy, pgdir_t *new_pd)
 {
-       kpte_t *kpt = get_cont_pages(1, MEM_WAIT);
+       kpte_t *kpt;
+       epte_t *ept;
+
+       kpt = get_cont_pages(1, MEM_WAIT);
        memcpy(kpt, boot_copy.kpte, PGSIZE);
-       epte_t *ept = kpte_to_epte(kpt);
+       ept = kpte_to_epte(kpt);
        memset(ept, 0, PGSIZE);
 
+       /* This bit of paranoia slows process creation a little, but makes sure that
+        * there is nothing below ULIM in boot_pgdir.  Any PML4 entries copied from
+        * boot_pgdir (e.g. the kernel's memory) will be *shared* among all
+        * processes, including *everything* under the PML4 entries reach (e.g.
+        * PML4_PTE_REACH = 512 GB) and any activity would need to be synchronized.
+        *
+        * We could do this once at boot time, but that would miss out on potential
+        * changes to the boot_pgdir at runtime.
+        *
+        * We could also just memset that region to 0.  For now, I want to catch
+        * whatever mappings exist, since they are probably bugs. */
+       for (int i = 0; i < PML4(ULIM - 1); i++)
+               assert(kpt[i] == 0);
+
        /* VPT and UVPT map the proc's page table, with different permissions. */
        kpt[PML4(VPT)]  = build_kpte(PADDR(kpt), PTE_KERN_RW);
        kpt[PML4(UVPT)] = build_kpte(PADDR(kpt), PTE_USER_RO);
index c43f01e..2abc57c 100644 (file)
@@ -39,7 +39,6 @@ int env_setup_vm(env_t *e)
 
        if ((ret = arch_pgdir_setup(boot_pgdir, &e->env_pgdir)))
                return ret;
-       /* TODO: verify there is nothing below ULIM */
        e->env_cr3 = arch_pgdir_get_cr3(e->env_pgdir);
 
        /* These need to be contiguous, so the kernel can alias them.  Note the