Fix locking issues with CPU refactor

There was a deadlock when cpu_run called cpu_step_to_interrupt with mem read-locked, and it would write-lock mem to clean up jetsam.
newbiecodingman · Jun 9, 2020 · 6400172 · 6400172
1 parent 269eb6e
commit 6400172
Show file tree

Hide file tree

Showing 10 changed files with 54 additions and 75 deletions.
diff --git a/emu/cpu.h b/emu/cpu.h
@@ -8,8 +8,7 @@
 
 struct cpu_state;
 struct tlb;
-void cpu_run(struct cpu_state *cpu);
-int cpu_step_to_interrupt(struct cpu_state *cpu, struct tlb *tlb);
+int cpu_run_to_interrupt(struct cpu_state *cpu, struct tlb *tlb);
 
 union mm_reg {
     qword_t qw;

diff --git a/emu/tlb.c b/emu/tlb.c
@@ -4,6 +4,7 @@
 void tlb_init(struct tlb *tlb, struct mem *mem) {
     tlb->mem = mem;
     tlb->dirty_page = TLB_PAGE_EMPTY;
+    tlb->mem_changes = mem->changes;
     tlb_flush(tlb);
 }
 

diff --git a/emu/tlb.h b/emu/tlb.h
@@ -15,6 +15,7 @@ struct tlb_entry {
 struct tlb {
     struct mem *mem;
     page_t dirty_page;
+    unsigned mem_changes;
     struct tlb_entry entries[TLB_SIZE];
 };
 

diff --git a/jit/jit.c b/jit/jit.c
@@ -164,13 +164,12 @@ static inline size_t jit_cache_hash(addr_t ip) {
     return (ip ^ (ip >> 12)) % JIT_CACHE_SIZE;
 }
 
-int cpu_step_to_interrupt(struct cpu_state *cpu, struct tlb *tlb) {
+static int cpu_step_to_interrupt(struct cpu_state *cpu, struct tlb *tlb) {
     struct jit *jit = cpu->mem->jit;
     struct jit_block *cache[JIT_CACHE_SIZE] = {};
     struct jit_frame frame = {.cpu = *cpu, .ret_cache = {}};
 
     int interrupt = INT_NONE;
-
     while (interrupt == INT_NONE) {
         addr_t ip = frame.cpu.eip;
         size_t cache_index = jit_cache_hash(ip);
@@ -219,24 +218,10 @@ int cpu_step_to_interrupt(struct cpu_state *cpu, struct tlb *tlb) {
             interrupt = INT_TIMER;
     }
 
-    jit = cpu->mem->jit;
-    lock(&jit->lock);
-    if (!list_empty(&jit->jetsam)) {
-        // write-lock the mem to wait until other jit threads get to
-        // this point, so they will all clear out their block pointers
-        // TODO: use RCU for better performance
-        unlock(&jit->lock);
-        write_wrlock(&cpu->mem->lock);
-        lock(&jit->lock);
-        jit_free_jetsam(jit);
-        write_wrunlock(&cpu->mem->lock);
-    }
-    unlock(&jit->lock);
-
     return interrupt;
 }
 
-static int cpu_step(struct cpu_state *cpu, struct tlb *tlb) {
+static int cpu_single_step(struct cpu_state *cpu, struct tlb *tlb) {
     struct gen_state state;
     gen_start(cpu->eip, &state);
     gen_step32(&state, tlb);
@@ -245,34 +230,35 @@ static int cpu_step(struct cpu_state *cpu, struct tlb *tlb) {
 
     struct jit_block *block = state.block;
     struct jit_frame frame = {.cpu = *cpu};
-    read_wrlock(&cpu->mem->lock);
     int interrupt = jit_enter(block, &frame, tlb);
-    read_wrunlock(&cpu->mem->lock);
     *cpu = frame.cpu;
     jit_block_free(NULL, block);
-    if (interrupt == INT_NONE && cpu->tf)
+    if (interrupt == INT_NONE)
         interrupt = INT_DEBUG;
     return interrupt;
 }
 
-void cpu_run(struct cpu_state *cpu) {
-    struct tlb tlb;
-    tlb_init(&tlb, cpu->mem);
-
+int cpu_run_to_interrupt(struct cpu_state *cpu, struct tlb *tlb) {
     read_wrlock(&cpu->mem->lock);
-    unsigned changes = cpu->mem->changes;
-
-    while (true) {
-        int interrupt = (cpu->tf ? cpu_step : cpu_step_to_interrupt)(cpu, &tlb);
-        if (interrupt != INT_NONE) {
-            read_wrunlock(&cpu->mem->lock);
-            handle_interrupt(cpu->trapno = interrupt);
-            read_wrlock(&cpu->mem->lock);
+    if (cpu->mem->changes != tlb->mem_changes)
+        tlb_init(tlb, cpu->mem);
+    int interrupt = (cpu->tf ? cpu_single_step : cpu_step_to_interrupt)(cpu, tlb);
+    cpu->trapno = interrupt;
+    read_wrunlock(&cpu->mem->lock);
 
-            tlb.mem = cpu->mem;
-            if (cpu->mem->changes != changes)
-                tlb_flush(&tlb);
-            changes = cpu->mem->changes;
-        }
+    struct jit *jit = cpu->mem->jit;
+    lock(&jit->lock);
+    if (!list_empty(&jit->jetsam)) {
+        // write-lock the mem to wait until other jit threads get to
+        // this point, so they will all clear out their block pointers
+        // TODO: use RCU for better performance
+        unlock(&jit->lock);
+        write_wrlock(&cpu->mem->lock);
+        lock(&jit->lock);
+        jit_free_jetsam(jit);
+        write_wrunlock(&cpu->mem->lock);
     }
+    unlock(&jit->lock);
+
+    return interrupt;
 }
diff --git a/kernel/calls.c b/kernel/calls.c
@@ -236,7 +236,6 @@ syscall_t syscall_table[] = {
 #define NUM_SYSCALLS (sizeof(syscall_table) / sizeof(syscall_table[0]))
 
 void handle_interrupt(int interrupt) {
-    TRACE_(instr, "\n");
     struct cpu_state *cpu = &current->cpu;
     if (interrupt == INT_SYSCALL) {
         unsigned syscall_num = cpu->eax;

diff --git a/kernel/task.c b/kernel/task.c
@@ -5,6 +5,7 @@
 #include "kernel/calls.h"
 #include "kernel/task.h"
 #include "emu/memory.h"
+#include "emu/tlb.h"
 
 __thread struct task *current;
 
@@ -91,16 +92,21 @@ void task_destroy(struct task *task) {
     free(task);
 }
 
-void (*task_run_hook)(void) = NULL;
+void task_run_current() {
+    struct cpu_state *cpu = &current->cpu;
+    struct tlb tlb;
+    tlb_init(&tlb, current->mem);
+    while (true) {
+        int interrupt = cpu_run_to_interrupt(cpu, &tlb);
+        handle_interrupt(interrupt);
+    }
+}
 
-static void *task_run(void *task) {
+static void *task_thread(void *task) {
     current = task;
     set_thread_name(current->comm);
-    if (task_run_hook)
-        task_run_hook();
-    else
-        cpu_run(&current->cpu);
-    die("task_run returned"); // above function call should never return
+    task_run_current();
+    die("task_thread returned"); // above function call should never return
 }
 
 static pthread_attr_t task_thread_attr;
@@ -110,7 +116,7 @@ __attribute__((constructor)) static void create_attr() {
 }
 
 void task_start(struct task *task) {
-    if (pthread_create(&task->thread, &task_thread_attr, task_run, task) < 0)
+    if (pthread_create(&task->thread, &task_thread_attr, task_thread, task) < 0)
         die("could not create thread");
 }
 

diff --git a/kernel/task.h b/kernel/task.h
@@ -162,10 +162,9 @@ struct task *pid_get_task_zombie(dword_t id); // don't return null if the task e
 
 #define MAX_PID (1 << 15) // oughta be enough
 
-// When a thread is created to run a new process, this function is used.
-extern void (*task_run_hook)(void);
 // TODO document
 void task_start(struct task *task);
+void task_run_current();
 
 extern void (*exit_hook)(struct task *task, int code);
 

diff --git a/main.c b/main.c
@@ -15,5 +15,5 @@ int main(int argc, char *const argv[]) {
     }
     do_mount(&procfs, "proc", "/proc", "", 0);
     do_mount(&devptsfs, "devpts", "/dev/pts", "", 0);
-    cpu_run(&current->cpu);
+    task_run_current();
 }
diff --git a/tools/ptraceomatic.c b/tools/ptraceomatic.c
@@ -258,21 +258,15 @@ static void pt_copy_to_real(int pid, addr_t start, size_t size) {
 static void step_tracing(struct cpu_state *cpu, struct tlb *tlb, int pid, int sender, int receiver) {
     // step fake cpu
     cpu->tf = 1;
-    unsigned changes = cpu->mem->changes;
-    int interrupt = cpu_step_to_interrupt(cpu, tlb);
-    if (interrupt != INT_NONE) {
-        cpu->trapno = interrupt;
-        // hack to clean up before the exit syscall
-        if (interrupt == INT_SYSCALL && cpu->eax == 1) {
-            if (kill(pid, SIGKILL) < 0) {
-                perror("kill tracee during exit");
-                exit(1);
-            }
+    int interrupt = cpu_run_to_interrupt(cpu, tlb);
+    // hack to clean up before the exit syscall
+    if (interrupt == INT_SYSCALL && cpu->eax == 1) {
+        if (kill(pid, SIGKILL) < 0) {
+            perror("kill tracee during exit");
+            exit(1);
         }
-        handle_interrupt(interrupt);
     }
-    if (cpu->mem->changes != changes)
-        tlb_flush(tlb);
+    handle_interrupt(interrupt);
 
     // step real cpu
     // intercept cpuid, rdtsc, and int $0x80, though
@@ -501,7 +495,7 @@ int main(int argc, char *const argv[]) {
             printk("failure: resetting cpu\n");
             *cpu = old_cpu;
             __asm__("int3");
-            cpu_step_to_interrupt(cpu, &tlb);
+            cpu_run_to_interrupt(cpu, &tlb);
         }
         undefined_flags = undefined_flags_mask(cpu, &tlb);
         old_cpu = *cpu;

diff --git a/tools/unicornomatic.c b/tools/unicornomatic.c
@@ -205,14 +205,8 @@ static void _mem_sync(struct tlb *tlb, uc_engine *uc, addr_t addr, dword_t size)
 void step_tracing(struct cpu_state *cpu, struct tlb *tlb, uc_engine *uc) {
     // step ish
     addr_t old_brk = current->mm->brk; // this is important
-    unsigned changes = cpu->mem->changes;
-    int interrupt = cpu_step32(cpu, tlb);
-    if (interrupt != INT_NONE) {
-        cpu->trapno = interrupt;
-        handle_interrupt(interrupt);
-    }
-    if (cpu->mem->changes != changes)
-        tlb_flush(tlb);
+    int interrupt = cpu_run_to_interrupt(cpu, tlb);
+    handle_interrupt(interrupt);
 
     // step unicorn
     uc_interrupt = -1;
@@ -344,7 +338,7 @@ void step_tracing(struct cpu_state *cpu, struct tlb *tlb, uc_engine *uc) {
                 break;
 
             case 91: // munmap
-                if (cpu->eax >= 0)
+                if ((int) cpu->eax >= 0)
                     uc_unmap(uc, cpu->ebx, cpu->ecx);
                 break;
 
@@ -503,7 +497,7 @@ int main(int argc, char *const argv[]) {
             printk("resetting cpu\n");
             *cpu = old_cpu;
             debugger;
-            cpu_step32(cpu, &tlb);
+            cpu_run_to_interrupt(cpu, &tlb);
         }
         undefined_flags = undefined_flags_mask(cpu, &tlb);
         old_cpu = *cpu;