Fixes MAP_PRIVATE bug in mmap()
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 31 Jan 2012 04:00:30 +0000 (20:00 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 31 Jan 2012 04:02:39 +0000 (20:02 -0800)
Flags were being set, instead of checking.  Further, we needed to make
private copies (or CoW) on any private mapping, independently of its
PROT status, due to some other issues we have with libc and loadelf.

Note that we have a couple other bugs still, related to mmap of files
across fork or mmap of regions of files exceeding the filesize.

kern/arch/i686/ros/mmu.h
kern/src/elf.c
kern/src/mm.c
tests/test_mmap_ipc.c [new file with mode: 0644]

index e6fb28d..64a1d1d 100644 (file)
@@ -69,7 +69,7 @@ typedef unsigned long pde_t;
 #define JPGOFF(la)     (((uintptr_t) (la)) & 0x003FFFFF)
 
 // construct PTE from PPN and flags
-#define PTE(ppn, flags) ((ppn) << PTXSHIFT | (flags))
+#define PTE(ppn, flags) ((ppn) << PTXSHIFT | PGOFF(flags))
 
 // construct linear address from indexes and offset
 #define PGADDR(d, t, o)        ((void*SNT) ((d) << PDXSHIFT | (t) << PTXSHIFT | (o)))
index e5eb5e4..d5a3646 100644 (file)
@@ -57,7 +57,7 @@ static int load_one_elf(struct proc *p, struct file *f, uintptr_t pgoffset,
        if (!phdrs || f->f_op->read(f, phdrs, e_phnum * phsz, &f_off) == -1)
                goto fail;
 
-       int flags = MAP_FIXED | MAP_PRIVATE;
+       int flags = MAP_FIXED | MAP_PRIVATE;    /* TODO: why private? */
 
        for (int i = 0; i < e_phnum; i++) {
                proghdr32_t* ph32 = (proghdr32_t*)phdrs + i;
index e0a2461..133b5fc 100644 (file)
@@ -615,9 +615,13 @@ int __handle_page_fault(struct proc *p, uintptr_t va, int prot)
                retval = pm_load_page(vmr->vm_file->f_mapping, f_idx, &a_page);
                if (retval)
                        return retval;
-               /* If we want a private map that is writable, we'll preemptively give
-                * you a new page.  In the future, we want to CoW this. */
-               if ((vmr->vm_flags |= MAP_PRIVATE) && (vmr->vm_prot |= PROT_WRITE)) {
+               /* If we want a private map, we'll preemptively give you a new page.  We
+                * used to just care if it was private and writable, but were running
+                * into issues with libc changing its mapping (map private, then
+                * mprotect to writable...)  In the future, we want to CoW this anyway,
+                * so it's not a big deal. */
+               if ((vmr->vm_flags & MAP_PRIVATE))
+               {
                        struct page *cache_page = a_page;
                        if (upage_alloc(p, &a_page, FALSE)) {
                                page_decref(cache_page);        /* was the original a_page */
@@ -625,6 +629,10 @@ int __handle_page_fault(struct proc *p, uintptr_t va, int prot)
                        }
                        memcpy(page2kva(a_page), page2kva(cache_page), PGSIZE);
                        page_decref(cache_page);                /* was the original a_page */
+                       /* Debugging */
+                       if (!(vmr->vm_prot & PROT_WRITE))
+                               printd("[kernel] private, but unwritable file mapping of %s "
+                                      "at va %08p\n", file_name(vmr->vm_file), va);
                }
                /* if this is an executable page, we might have to flush the instruction
                 * cache if our HW requires it. */
diff --git a/tests/test_mmap_ipc.c b/tests/test_mmap_ipc.c
new file mode 100644 (file)
index 0000000..aede6c6
--- /dev/null
@@ -0,0 +1,46 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/types.h>
+
+#include <sys/mman.h>
+#include <fcntl.h>
+#include <parlib.h>
+#include <timing.h>
+       
+int main(void)
+{
+       int pFile, *first;
+       pid_t pid;
+       pFile = open ("hello.txt", O_RDWR | O_CREAT, (mode_t)0600);
+       /* this mmap will give you a Bus Error on linux if you try to map more
+        * pages than the file contains (i think)... */
+       first = (int*)mmap(0, 8192, PROT_READ | PROT_WRITE, MAP_SHARED, pFile, 0);
+       first[0] = 3;
+       printf("the first number after initialization is %d at %08p\n", first[0],
+              first);
+       munmap(first, 8192);
+       if ((pid = fork()) < 0) {
+               perror("fork error");
+               exit(1);
+       }
+       if (pid == 0) {
+               first = (int*)mmap(0, 8192, PROT_READ | PROT_WRITE, MAP_SHARED, pFile,
+                                  0);
+               /* delay here, to avoid the race a bit */
+               udelay(1000000);
+               printf("After fork in the parent, the first number is %d\n", first[0]);
+               first[0] = 99;
+               printf("Pid 0 sees value %d at mmapped address %08p\n", first[0],
+                      first);
+       }
+       else
+       {
+               first = (int*)mmap(0, 8192, PROT_READ | PROT_WRITE, MAP_SHARED, pFile,
+                                  0);
+               printf("After fork in the child, the first number is %d\n", first[0]);
+               first[0] = 11;
+               printf("Child pid %d sees value %d at mmapped address %08p\n", pid,
+                      first[0], first);
+       }
+}