Removes user_mem_check, fixes syscall bug
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 8 Apr 2014 05:31:26 +0000 (22:31 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 8 Apr 2014 05:31:26 +0000 (22:31 -0700)
This was a good one.  When not MAP_POPULATE-ing anonymous memory,
syscalls where part of the struct was on a page that hadn't been faulted
in yet would fail a user_mem_check.  Silently.  But good thing we set
that global racy variable!  Since run_local_syscall() would just return,
we'd pop back to userspace and the syscall would never complete, because
it never started.  6.5 giraffe-hours btw (* 2).

kern/include/umem.h
kern/src/syscall.c
kern/src/testing.c
kern/src/umem.c

index fa17d26..13269d2 100644 (file)
@@ -15,13 +15,6 @@ static inline bool is_user_rwaddr(void *addr, size_t len);
 /* Same deal, but read-only */
 static inline bool is_user_raddr(void *addr, size_t len);
 
-/* Can they use the area in the manner of perm? */
-void *user_mem_check(struct proc *p, const void *DANGEROUS va, size_t len,
-                     size_t align, int perm);
-/* Kills them if they can't use the area in the manner of perm */
-void *user_mem_assert(struct proc *p, const void *DANGEROUS va, size_t len, 
-                      size_t align, int perm);
-
 /* Copy from proc p into the kernel's dest from src */
 int memcpy_from_user(struct proc *p, void *dest, const void *DANGEROUS va,
                      size_t len);
index e626a9b..0fd1e05 100644 (file)
@@ -1895,12 +1895,14 @@ void run_local_syscall(struct syscall *sysc)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
 
-       /* TODO: (UMEM) assert / pin the memory for the sysc */
        assert(irq_is_enabled());       /* in case we proc destroy */
-       /* Abort on mem check failure, for now */
-       if (!user_mem_check(pcpui->cur_proc, sysc, sizeof(struct syscall),
-                           sizeof(uintptr_t), PTE_USER_RW))
+       /* In lieu of pinning, we just check the sysc and will PF on the user addr
+        * later (if the addr was unmapped).  Which is the plan for all UMEM. */
+       if (!is_user_rwaddr(sysc, sizeof(struct syscall))) {
+               printk("[kernel] bad user addr %p (+%p) in %s (user bug)\n", sysc,
+                      sizeof(struct syscall), __FUNCTION__);
                return;
+       }
        pcpui->cur_kthread->sysc = sysc;        /* let the core know which sysc it is */
        sysc->retval = syscall(pcpui->cur_proc, sysc->num, sysc->arg0, sysc->arg1,
                               sysc->arg2, sysc->arg3, sysc->arg4, sysc->arg5);
@@ -1911,7 +1913,6 @@ void run_local_syscall(struct syscall *sysc)
        if ((current_errstr()[0] != 0) && (!sysc->err))
                sysc->err = EUNSPECIFIED;
        finish_sysc(sysc, pcpui->cur_proc);
-       /* Can unpin (UMEM) at this point */
        pcpui->cur_kthread->sysc = 0;   /* no longer working on sysc */
 }
 
@@ -1922,8 +1923,10 @@ void prep_syscalls(struct proc *p, struct syscall *sysc, unsigned int nr_syscs)
 {
        int retval;
        /* Careful with pcpui here, we could have migrated */
-       if (!nr_syscs)
+       if (!nr_syscs) {
+               printk("[kernel] No nr_sysc, probably a bug, user!\n");
                return;
+       }
        /* For all after the first call, send ourselves a KMSG (TODO). */
        if (nr_syscs != 1)
                warn("Only one supported (Debutante calls: %d)\n", nr_syscs);
index 147f554..ee5a797 100644 (file)
@@ -980,11 +980,10 @@ void test_ucq(void)
 
                printk("Running the alarm handler!\n");
                printk("NR msg per page: %d\n", NR_MSG_PER_PAGE);
-               /* might not be mmaped yet, if not, abort */
-               if (!user_mem_check(p, ucq, PGSIZE, 1, PTE_USER_RW)) {
-                       printk("Not mmaped yet\n");
-                       goto abort;
-               }
+               /* might not be mmaped yet, if not, abort.  We used to user_mem_check,
+                * but now we just touch it and PF. */
+               char touch = *(char*)ucq;
+               asm volatile ("" : : "r"(touch));
                /* load their address space */
                old_proc = switch_to(p);
                /* So it's mmaped, see if it is ready (note that this is dangerous) */
index e8d83c5..c7ae3d1 100644 (file)
 #include <smp.h>
 
 /**
- * @brief Global variable used to store erroneous virtual addresses as the
- *        result of a failed user_mem_check().
- *
- * zra: What if two checks fail at the same time? Maybe this should be per-cpu?
- *
- */
-static void *DANGEROUS RACY user_mem_check_addr;
-
-/**
- * @brief Check that an environment is allowed to access the range of memory
- * [va, va+len) with permissions 'perm | PTE_P'.
- *
- * Normally 'perm' will contain PTE_U at least, but this is not required.  The
- * function get_va_perms only checks for PTE_U, PTE_W, and PTE_P.  It won't
- * check for things like PTE_PS, PTE_A, etc.
- * 'va' and 'len' need not be page-aligned;
- *
- * A user program can access a virtual address if:
- *     -# the address is below ULIM
- *     -# the page table gives it permission.  
- *
- * If there is an error, 'user_mem_check_addr' is set to the first
- * erroneous virtual address.
- *
- * @param p    the process associated with the user program trying to access
- *             the virtual address range
- * @param va   the first virtual address in the range
- * @param len  the length of the virtual address range
- * @param perm the permissions the user is trying to access the virtual address 
- *             range with
- *
- * @return VA a pointer of type COUNT(len) to the address range
- * @return NULL trying to access this range of virtual addresses is not allowed
- */
-void *user_mem_check(struct proc *p, const void *DANGEROUS va, size_t len,
-                     size_t align, int perm)
-{
-       align--;
-       if(((align+1) & align) || ((uintptr_t)va & align) || (len & align))
-               return NULL;
-
-       // TODO - will need to sort this out wrt page faulting / PTE_P
-       // also could be issues with sleeping and waking up to find pages
-       // are unmapped, though i think the lab ignores this since the 
-       // kernel is uninterruptible
-       void *DANGEROUS start, *DANGEROUS end;
-       size_t num_pages, i;
-       int page_perms = 0;
-
-       perm |= PTE_P;
-       start = (void*)ROUNDDOWN((uintptr_t)va, PGSIZE);
-       end = (void*)ROUNDUP((uintptr_t)va + len, PGSIZE);
-       if (start >= end) {
-               warn("Blimey!  Wrap around in VM range calculation!");  
-               return NULL;
-       }
-       num_pages = LA2PPN(end - start);
-       for (i = 0; i < num_pages; i++, start += PGSIZE) {
-               page_perms = get_va_perms(p->env_pgdir, start);
-               // ensures the bits we want on are turned on.  if not, error out
-               if ((page_perms & perm) != perm) {
-                       if (i == 0)
-                               user_mem_check_addr = (void*DANGEROUS)va;
-                       else
-                               user_mem_check_addr = start;
-                       return NULL;
-               }
-       }
-       // this should never be needed, since the perms should catch it
-       if ((uintptr_t)end > ULIM) {
-               warn ("I suck - Bug in user permission mappings!");
-               return NULL;
-       }
-       return (void *COUNT(len))TC(va);
-}
-
-/**
- * @brief Checks that process 'p' is allowed to access the range
- * of memory [va, va+len) with permissions 'perm | PTE_U'. Destroy 
- * process 'p' if the assertion fails.
- *
- * This function is identical to user_mem_assert() except that it has a side
- * affect of destroying the process 'p' if the memory check fails.
- *
- * @param p    the process associated with the user program trying to access
- *             the virtual address range
- * @param va   the first virtual address in the range
- * @param len  the length of the virtual address range
- * @param perm the permissions the user is trying to access the virtual address 
- *             range with
- *
- * @return VA a pointer of type COUNT(len) to the address range
- * @return NULL trying to access this range of virtual addresses is not allowed
- *              process 'p' is destroyed
- *
- * GIANT WARNING: this could fuck up your refcnting for p if p was an
- * edible/refcounted reference.  (fix is to return, or just not use this) */
-void *user_mem_assert(struct proc *p, const void *DANGEROUS va, size_t len,
-                      size_t align, int perm)
-{
-       void* res = user_mem_check(p, va, len, align, perm | PTE_USER_RO);
-       if (!res) {
-               cprintf("[%08x] user_mem_check assertion failure for "
-                       "va %08x\n", p->pid, user_mem_check_addr);
-               /* won't be freed til the caller decrefs */
-               proc_destroy(p);
-        return NULL;
-       }
-    return res;
-}
-
-/**
  * @brief Copies data from a user buffer to a kernel buffer.
  * 
  * @param p    the process associated with the user program