proc_destroy() refcnting issues dealt with
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 1 Mar 2011 22:11:46 +0000 (14:11 -0800)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:59 +0000 (17:35 -0700)
proc_destroy() needs to have an edible reference, which it did not have
from __do_mmap().  Incidentally, we just don't call it from there
anymore, but there were a few other unlikely errors that would be
problems.

For now, all code using a struct proc * needs to know where it got the
refcount from.  Code in the syscall path (and mm.c) usually is
'borrowing' the ref from current, and is not able to be passed without
incref to certain functions.  Also note that you can't incref too much,
since functions like proc_destroy() assume they are taking your
"working" reference (i.e., the one you passed to the function) that
should be refcnt'd exactly once.

As this is the most likely way to fuck up ref counting with processes,
if you see something odd with refcnts (kref assertions, env_cr3 != rcr3,
etc), look in places that involve these "might-not-return" functions.

kern/arch/i686/cpuinfo.c
kern/arch/i686/trap.c
kern/arch/sparc/trap.c
kern/src/mm.c
kern/src/syscall.c
kern/src/umem.c

index e3b791f..ff05d48 100644 (file)
@@ -164,7 +164,7 @@ void backtrace(void)
        eip = *(ebp + 1) - 1;
        // jump back a frame (out of backtrace)
        ebp = (uint32_t*)(*ebp);
-       cprintf("Stack Backtrace:\n");
+       printk("Stack Backtrace on Core %d:\n", core_id());
        // on each iteration, ebp holds the stack frame and eip an addr in that func
        while (ebp != 0) {
                debuginfo_eip(eip, &debuginfo);
index b61011d..2b6aaca 100644 (file)
@@ -251,6 +251,7 @@ trap_dispatch(trapframe_t *tf)
                                warn("Unexpected trap from userspace");
                                proc_incref(current, 1);
                                proc_destroy(current);
+                               assert(0);
                                return;
                        }
        }
@@ -376,6 +377,7 @@ void page_fault_handler(struct trapframe *tf)
                print_trapframe(tf);
                proc_incref(current, 1);
                proc_destroy(current);
+               assert(0);
        }
 }
 
index cc743ff..3b48bdd 100644 (file)
@@ -441,8 +441,11 @@ handle_pop_tf(trapframe_t* state)
        set_current_tf(pcpui, &state);
 
        trapframe_t tf, *tf_p = &tf;
-       if(memcpy_from_user(current,&tf,(void*)state->gpr[8],sizeof(tf)))
+       if (memcpy_from_user(current,&tf,(void*)state->gpr[8],sizeof(tf))) {
+               proc_incref(current, 1);
                proc_destroy(current);
+               assert(0);
+       }
 
        proc_secure_trapframe(&tf);
        set_current_tf(pcpui, &tf_p);
@@ -453,8 +456,11 @@ void
 handle_set_tf(trapframe_t* state)
 {
        advance_pc(state);
-       if(memcpy_to_user(current,(void*)state->gpr[8],state,sizeof(*state)))
+       if (memcpy_to_user(current,(void*)state->gpr[8],state,sizeof(*state))) {
+               proc_incref(current, 1);
                proc_destroy(current);
+               assert(0);
+       }
 }
 
 void
index 1fe9d35..9f8e9ef 100644 (file)
@@ -8,7 +8,10 @@
  * In general, error checking / bounds checks are done in the main function
  * (e.g. mmap()), and the work is done in a do_ function (e.g. do_mmap()).
  * Versions of those functions that are called when the memory lock (proc_lock
- * for now) is already held begin with __ (e.g. __do_munmap()).  */
+ * for now) is already held begin with __ (e.g. __do_munmap()).
+ *
+ * Note that if we were called from kern/src/syscall.c, we probably don't have
+ * an edible reference to p. */
 
 #include <frontend.h>
 #include <ros/common.h>
@@ -332,6 +335,7 @@ void *__do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
 {
        len = ROUNDUP(len, PGSIZE);
        int num_pages = len / PGSIZE;
+       int retval;
 
        struct vm_region *vmr, *vmr_temp;
 
@@ -364,16 +368,25 @@ void *__do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
        }
        addr = vmr->vm_base;            /* so we know which pages to populate later */
        vmr = merge_me(vmr);            /* attempts to merge with neighbors */
-       /* Fault in pages now if MAP_POPULATE - die on failure.  We want to populate
-        * the region requested, but we need to be careful and only populate the
-        * requested length and not any merged regions, which is why we set addr
-        * above and use it here. */
+       /* Fault in pages now if MAP_POPULATE.  We want to populate the region
+        * requested, but we need to be careful and only populate the requested
+        * length and not any merged regions, which is why we set addr above and use
+        * it here.
+        *
+        * If HPF errors out, we'll warn for now, since it is likely a bug in
+        * userspace, though since POPULATE is an opportunistic thing, we don't need
+        * to actually kill the process.  If we do kill them, make sure to
+        * incref/decref around proc_destroy, since we likely don't have an edible
+        * reference to p. */
        if (flags & MAP_POPULATE)
-               for (int i = 0; i < num_pages; i++)
-                       if (__handle_page_fault(p, addr + i*PGSIZE, vmr->vm_prot)) {
-                               spin_unlock(&p->proc_lock);
-                               proc_destroy(p);
+               for (int i = 0; i < num_pages; i++) {
+                       retval = __handle_page_fault(p, addr + i * PGSIZE, vmr->vm_prot);
+                       if (retval) {
+                               warn("do_mmap() failing (%d) on addr %08p with prot %p\n",
+                                    retval, addr + i * PGSIZE,  vmr->vm_prot);
+                               break;
                        }
+               }
        return (void*SAFE)TC(addr);
 }
 
index cfb9812..95443e8 100644 (file)
@@ -537,7 +537,11 @@ static int sys_exec(struct proc *p, char *path, size_t path_l,
        env_user_mem_free(p, 0, UMAPTOP);
        if (load_elf(p, program)) {
                kref_put(&program->f_kref);
+               /* Need an edible reference for proc_destroy in case it doesn't return.
+                * sys_exec was given current's ref (counted once just for current) */
+               proc_incref(p, 1);
                proc_destroy(p);
+               proc_decref(p);
                /* We don't want to do anything else - we just need to not accidentally
                 * return to the user (hence the all_out) */
                goto all_out;
index 0f2fd82..e186235 100644 (file)
@@ -111,7 +111,9 @@ void *user_mem_check(struct proc *p, const void *DANGEROUS va, size_t len,
  * @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,
                        int perm)
 {
@@ -124,7 +126,10 @@ void *user_mem_assert(struct proc *p, const void *DANGEROUS va, size_t len,
        if (!res) {
                cprintf("[%08x] user_mem_check assertion failure for "
                        "va %08x\n", p->pid, user_mem_check_addr);
+               /* assuming this is used with an inedible reference */
+               proc_incref(p, 1);
                proc_destroy(p);        // may not return
+               assert(0);
         return NULL;
        }
     return res;