x86: clean up __handle_page_fault()
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 8 Dec 2015 19:48:04 +0000 (14:48 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 10 Dec 2015 16:26:40 +0000 (11:26 -0500)
Other than being a mess, there were a couple issues.  One change is that
IRQs are disabled for the entire trap handling.  We're in a kernel trap
handler - turning on IRQs could lead to a mess.  The other change is that
we call handle_page_fault_nofile().  If a user asks the *kernel* to operate
on an mmaped file addr, the kernel will not do this, and we'll end up
killing the process.  If there's a compelling case for handling this, then
we can add it back in later.  For now, I'm opting for safety.

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

index 952eb3e..90e2f42 100644 (file)
@@ -233,19 +233,35 @@ static void handle_fperr(struct hw_trapframe *hw_tf)
        proc_destroy(current);
 }
 
        proc_destroy(current);
 }
 
-static bool __handle_page_fault(struct hw_trapframe *hw_tf, unsigned long *aux)
+static bool __handler_user_page_fault(struct hw_trapframe *hw_tf,
+                                      uintptr_t fault_va, int prot)
 {
 {
-       uintptr_t fault_va = rcr2();
-       int prot = hw_tf->tf_err & PF_ERROR_WRITE ? PROT_WRITE : PROT_READ;
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        int err;
 
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        int err;
 
+       assert(pcpui->owning_proc == pcpui->cur_proc);
+       enable_irq();
+       err = handle_page_fault(pcpui->owning_proc, fault_va, prot);
+       disable_irq();
+       if (err) {
+               if (err == -EAGAIN)
+                       hw_tf->tf_err |= PF_VMR_BACKED;
+               return FALSE;
+       }
+       return TRUE;
+}
+
+static bool __handler_kernel_page_fault(struct hw_trapframe *hw_tf,
+                                        uintptr_t fault_va, int prot)
+{
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       int err;
+
+       /* In general, if there's no cur_proc, a KPF is a bug. */
        if (!pcpui->cur_proc) {
        if (!pcpui->cur_proc) {
+               /* This only runs from test_uaccess(), where it is expected to fail. */
                if (try_handle_exception_fixup(hw_tf))
                        return TRUE;
                if (try_handle_exception_fixup(hw_tf))
                        return TRUE;
-
-               /* still catch KPFs */
-               assert((hw_tf->tf_cs & 3) == 0);
                print_trapframe(hw_tf);
                backtrace_kframe(hw_tf);
                panic("Proc-less Page Fault in the Kernel at %p!", fault_va);
                print_trapframe(hw_tf);
                backtrace_kframe(hw_tf);
                panic("Proc-less Page Fault in the Kernel at %p!", fault_va);
@@ -254,40 +270,26 @@ static bool __handle_page_fault(struct hw_trapframe *hw_tf, unsigned long *aux)
         * holding locks in the kernel and could deadlock when we HPF.  For now, I'm
         * just disabling the lock checker, since it'll flip out when it sees there
         * is a kernel trap.  Will need to think about this a bit, esp when we
         * holding locks in the kernel and could deadlock when we HPF.  For now, I'm
         * just disabling the lock checker, since it'll flip out when it sees there
         * is a kernel trap.  Will need to think about this a bit, esp when we
-        * properly handle bad addrs and whatnot.
+        * properly handle bad addrs and whatnot. */
+       pcpui->__lock_checking_enabled--;
+       /* It is a bug for the kernel to access user memory while holding locks that
+        * are used by handle_page_fault.  At a minimum, this includes p->vmr_lock
+        * and memory allocation locks.
         *
         *
-        * Also consider turning on IRQs globally while we call HPF. */
-       if (in_kernel(hw_tf))
-               pcpui->__lock_checking_enabled--;
-       /* safe to reenable after rcr2 and after disabling lock checking */
-       enable_irq();
-       err = handle_page_fault(pcpui->cur_proc, fault_va, prot);
-       disable_irq();
-       if (in_kernel(hw_tf))
-               pcpui->__lock_checking_enabled++;
+        * In an effort to reduce the number of locks (both now and in the future),
+        * the kernel will not attempt to handle faults on file-back VMRs.  We
+        * probably can turn that on in the future, but I'd rather keep things safe
+        * for now.
+        *
+        * Note that we do not enable IRQs here, unlike in the user case.  Again,
+        * this is to limit the locks we could be grabbing. */
+       err = handle_page_fault_nofile(pcpui->cur_proc, fault_va, prot);
+       pcpui->__lock_checking_enabled++;
        if (err) {
                if (try_handle_exception_fixup(hw_tf))
                        return TRUE;
        if (err) {
                if (try_handle_exception_fixup(hw_tf))
                        return TRUE;
-
-               if (in_kernel(hw_tf)) {
-                       print_trapframe(hw_tf);
-                       backtrace_kframe(hw_tf);
-                       panic("Proc-ful Page Fault in the Kernel at %p!", fault_va);
-                       /* if we want to do something like kill a process or other code, be
-                        * aware we are in a sort of irq-like context, meaning the main
-                        * kernel code we 'interrupted' could be holding locks - even
-                        * irqsave locks. */
-               }
-
-               if (err == -EAGAIN)
-                       hw_tf->tf_err |= PF_VMR_BACKED;
-               *aux = fault_va;
-               return FALSE;
-               /* useful debugging */
-               printk("[%08x] user %s fault va %p ip %p on core %d with err %d\n",
-                      current->pid, prot & PROT_READ ? "READ" : "WRITE", fault_va,
-                      hw_tf->tf_rip, core_id(), err);
                print_trapframe(hw_tf);
                print_trapframe(hw_tf);
+               backtrace_kframe(hw_tf);
                /* Turn this on to help debug bad function pointers */
                printd("rsp %p\n\t 0(rsp): %p\n\t 8(rsp): %p\n\t 16(rsp): %p\n"
                       "\t24(rsp): %p\n", hw_tf->tf_rsp,
                /* Turn this on to help debug bad function pointers */
                printd("rsp %p\n\t 0(rsp): %p\n\t 8(rsp): %p\n\t 16(rsp): %p\n"
                       "\t24(rsp): %p\n", hw_tf->tf_rsp,
@@ -295,10 +297,27 @@ static bool __handle_page_fault(struct hw_trapframe *hw_tf, unsigned long *aux)
                       *(uintptr_t*)(hw_tf->tf_rsp +  8),
                       *(uintptr_t*)(hw_tf->tf_rsp + 16),
                       *(uintptr_t*)(hw_tf->tf_rsp + 24));
                       *(uintptr_t*)(hw_tf->tf_rsp +  8),
                       *(uintptr_t*)(hw_tf->tf_rsp + 16),
                       *(uintptr_t*)(hw_tf->tf_rsp + 24));
+               panic("Proc-ful Page Fault in the Kernel at %p!", fault_va);
+               /* if we want to do something like kill a process or other code, be
+                * aware we are in a sort of irq-like context, meaning the main
+                * kernel code we 'interrupted' could be holding locks - even
+                * irqsave locks. */
        }
        return TRUE;
 }
 
        }
        return TRUE;
 }
 
+static bool __handle_page_fault(struct hw_trapframe *hw_tf, unsigned long *aux)
+{
+       uintptr_t fault_va = rcr2();
+       int prot = hw_tf->tf_err & PF_ERROR_WRITE ? PROT_WRITE : PROT_READ;
+
+       *aux = fault_va;
+       if (in_kernel(hw_tf))
+               return __handler_kernel_page_fault(hw_tf, fault_va, prot);
+       else
+               return __handler_user_page_fault(hw_tf, fault_va, prot);
+}
+
 /* Certain traps want IRQs enabled, such as the syscall.  Others can't handle
  * it, like the page fault handler.  Turn them on on a case-by-case basis. */
 static void trap_dispatch(struct hw_trapframe *hw_tf)
 /* Certain traps want IRQs enabled, such as the syscall.  Others can't handle
  * it, like the page fault handler.  Turn them on on a case-by-case basis. */
 static void trap_dispatch(struct hw_trapframe *hw_tf)