From 151edb44eea9bf25ec466133e9dbef87bd6b1372 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 13 Mar 2015 18:02:01 +0100 Subject: Add missing mutex-locking in procesc.c ; discovered design fault somewhere. --- src/kernel/core/frame.c | 2 ++ src/kernel/core/paging.c | 19 +++++----- src/kernel/core/prng.c | 9 ++++- src/kernel/core/sys.c | 6 +++- src/kernel/core/thread.c | 11 +++--- src/kernel/user/process.c | 90 ++++++++++++++++++++++++++++++++++++++++------- 6 files changed, 109 insertions(+), 28 deletions(-) (limited to 'src/kernel') diff --git a/src/kernel/core/frame.c b/src/kernel/core/frame.c index 489d010..255abb6 100644 --- a/src/kernel/core/frame.c +++ b/src/kernel/core/frame.c @@ -53,6 +53,7 @@ uint32_t frame_alloc(size_t n) { nused_frames += n; mutex_unlock(&frame_allocator_mutex); + /*dbg_printf("AF 0x%p\n", i * 32 + j);*/ return i * 32 + j; } } @@ -65,6 +66,7 @@ void frame_free(uint32_t base, size_t n) { mutex_lock(&frame_allocator_mutex); for (size_t i = 0; i < n; i++) { + /*dbg_printf("FF 0x%p\n", base + i);*/ uint32_t idx = INDEX_FROM_BIT(base + i); uint32_t ofs = OFFSET_FROM_BIT(base + i); if (frame_bitset[idx] & (0x1 << ofs)) { diff --git a/src/kernel/core/paging.c b/src/kernel/core/paging.c index fa22879..f7ebf9c 100644 --- a/src/kernel/core/paging.c +++ b/src/kernel/core/paging.c @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -25,8 +24,6 @@ struct page_directory { user_pf_handler_t user_pfh; void* user_pfh_data; - - mutex_t mutex; }; @@ -60,6 +57,11 @@ void page_fault_handler(registers_t *regs) { ASSERT(current_thread->user_ex_handler != 0); current_thread->user_ex_handler(regs); } else { + if (pd->user_pfh == 0) { + dbg_printf("Error: usermode page fault on PD with no user PFH.\n"); + dbg_printf("PD: 0x%p, kernel PD: 0x%p\n", get_current_pagedir(), get_kernel_pagedir()); + PANIC("Un-handlable usermode PF.\n"); + } ASSERT(pd->user_pfh != 0); pd->user_pfh(pd->user_pfh_data, regs, vaddr); } @@ -122,7 +124,6 @@ void paging_setup(void* kernel_data_end) { // setup kernel_pd_d structure kernel_pd_d.phys_addr = (size_t)&kernel_pd - K_HIGHHALF_ADDR; - kernel_pd_d.mutex = MUTEX_UNLOCKED; // setup kernel_pt0 ASSERT(PAGE_OF_ADDR(K_HIGHHALF_ADDR) == 0); // kernel is 4M-aligned @@ -201,15 +202,15 @@ bool pd_map_page(void* vaddr, uint32_t frame_id, bool rw) { bool on_kernel_pd = (size_t)vaddr >= K_HIGHHALF_ADDR || current_thread == 0; - pagedir_t *pdd = (on_kernel_pd ? &kernel_pd_d : current_thread->current_pd_d); + // pagedir_t *pdd = (on_kernel_pd ? &kernel_pd_d : current_thread->current_pd_d); pagetable_t *pd = (on_kernel_pd ? &kernel_pd : current_pd); - mutex_lock(&pdd->mutex); + int st = enter_critical(CL_NOINT); if (!(pd->page[pt] & PTE_PRESENT)) { uint32_t new_pt_frame = frame_alloc(1); if (new_pt_frame == 0) { - mutex_unlock(&pdd->mutex); + exit_critical(st); return false; } @@ -227,7 +228,7 @@ bool pd_map_page(void* vaddr, uint32_t frame_id, bool rw) { | (rw ? PTE_RW : 0); invlpg(vaddr); - mutex_unlock(&pdd->mutex); + exit_critical(st); return true; } @@ -236,7 +237,6 @@ void pd_unmap_page(void* vaddr) { uint32_t page = PAGE_OF_ADDR(vaddr); pagetable_t *pd = ((size_t)vaddr >= K_HIGHHALF_ADDR ? &kernel_pd : current_pd); - // no need to lock the PD's mutex if (!(pd->page[pt] & PTE_PRESENT)) return; if (!(current_pt[pt].page[page] & PTE_PRESENT)) return; @@ -270,7 +270,6 @@ pagedir_t *create_pagedir(user_pf_handler_t pf, void* pfd) { if (!map_ok) goto error; pd->phys_addr = pd_phys * PAGE_SIZE; - pd->mutex = MUTEX_UNLOCKED; pd->user_pfh = pf; pd->user_pfh_data = pfd; diff --git a/src/kernel/core/prng.c b/src/kernel/core/prng.c index dbffba5..77dff7d 100644 --- a/src/kernel/core/prng.c +++ b/src/kernel/core/prng.c @@ -11,6 +11,8 @@ static const uint32_t a = 16807; static const uint32_t m = 0x7FFFFFFF; uint16_t prng_word() { + int st = enter_critical(CL_NOINT); + if (++n >= 100) { n = 0; if (entropy_count) { @@ -18,7 +20,12 @@ uint16_t prng_word() { } } x = (x * a) % m; - return x & 0xFFFF; + + uint16_t ret = x & 0xFFFF; + + exit_critical(st); + + return ret; } void prng_bytes(uint8_t* out, size_t nbytes) { diff --git a/src/kernel/core/sys.c b/src/kernel/core/sys.c index 554c95e..290fdfa 100644 --- a/src/kernel/core/sys.c +++ b/src/kernel/core/sys.c @@ -71,7 +71,7 @@ void load_kernel_symbol_map(char* text, size_t len) { void kernel_stacktrace(uint32_t ebp, uint32_t eip) { int i = 0; - while (ebp >= K_HIGHHALF_ADDR && eip >= K_HIGHHALF_ADDR && i++ < 10) { + while (ebp >= K_HIGHHALF_ADDR && eip >= K_HIGHHALF_ADDR) { char* sym = 0; if (kernel_symbol_map != 0) sym = btree_lower(kernel_symbol_map, (void*)eip); @@ -80,6 +80,10 @@ void kernel_stacktrace(uint32_t ebp, uint32_t eip) { uint32_t *d = (uint32_t*)ebp; ebp = d[0]; eip = d[1]; + if (i++ == 20) { + dbg_printf("| ..."); + break; + } } } diff --git a/src/kernel/core/thread.c b/src/kernel/core/thread.c index 1009ec7..53b3e70 100644 --- a/src/kernel/core/thread.c +++ b/src/kernel/core/thread.c @@ -43,8 +43,6 @@ void set_pit_frequency(uint32_t freq) { int enter_critical(int level) { asm volatile("cli"); - /*dbg_printf(" >%d< ", level);*/ - if (current_thread == 0) return CL_EXCL; int prev_level = current_thread->critical_level; @@ -52,18 +50,20 @@ int enter_critical(int level) { if (current_thread->critical_level < CL_NOINT) asm volatile("sti"); + /*dbg_printf(" >%d< ", current_thread->critical_level);*/ + return prev_level; } void exit_critical(int prev_level) { asm volatile("cli"); - /*dbg_printf(" <%d> ", prev_level);*/ - if (current_thread == 0) return; if (prev_level < current_thread->critical_level) current_thread->critical_level = prev_level; if (current_thread->critical_level < CL_NOINT) asm volatile("sti"); + + /*dbg_printf(" <%d> ", current_thread->critical_level);*/ } // ================== // @@ -315,12 +315,15 @@ bool wait_on_many(void** x, size_t n) { waiters = current_thread->next_waiter; } else { ASSERT(waiters != 0); + bool deleted = false; for (thread_t *w = waiters; w->next_waiter != 0; w = w->next_waiter) { if (w->next_waiter == current_thread) { w->next_waiter = current_thread->next_waiter; + deleted = true; break; } } + ASSERT(deleted); } exit_critical(st); diff --git a/src/kernel/user/process.c b/src/kernel/user/process.c index 00a7e44..2a4d98f 100644 --- a/src/kernel/user/process.c +++ b/src/kernel/user/process.c @@ -143,14 +143,14 @@ bool start_process(process_t *p, void* entry) { bool stack_ok = mmap(p, (void*)USERSTACK_ADDR, USERSTACK_SIZE, MM_READ | MM_WRITE); if (!stack_ok) return false; + p->status = PS_RUNNING; + bool ok = process_new_thread(p, entry, (void*)USERSTACK_ADDR + USERSTACK_SIZE); if (!ok) { munmap(p, (void*)USERSTACK_ADDR); return false; } - p->status = PS_RUNNING; - return true; } @@ -304,9 +304,13 @@ process_t *process_find_child(process_t *p, int pid) { } void process_get_status(process_t *p, proc_status_t *st) { + mutex_lock(&p->lock); + st->pid = p->pid; st->status = p->status; st->exit_code = p->exit_code; + + mutex_unlock(&p->lock); } void process_wait(process_t *p, proc_status_t *st, bool wait) { @@ -433,7 +437,16 @@ bool proc_add_fs(process_t *p, fs_t *fs, const char* name) { char *n = strdup(name); if (n == 0) return false; - bool add_ok = hashtbl_add(p->filesystems, n, fs); + bool add_ok = false; + + mutex_lock(&p->lock); + + if (hashtbl_find(p->filesystems, n) != 0) goto end; + + add_ok = hashtbl_add(p->filesystems, n, fs); + +end: + mutex_unlock(&p->lock); if (!add_ok) { free(n); return false; @@ -443,21 +456,35 @@ bool proc_add_fs(process_t *p, fs_t *fs, const char* name) { } fs_t *proc_find_fs(process_t *p, const char* name) { - return (fs_t*)hashtbl_find(p->filesystems, name); + mutex_lock(&p->lock); + + fs_t *ret = (fs_t*)hashtbl_find(p->filesystems, name); + + mutex_unlock(&p->lock); + return ret; } void proc_rm_fs(process_t *p, const char* name) { fs_t *fs = proc_find_fs(p, name); if (fs) { + mutex_lock(&p->lock); + unref_fs(fs); hashtbl_remove(p->filesystems, name); + + mutex_unlock(&p->lock); } } int proc_add_fd(process_t *p, fs_handle_t *f) { int fd = p->next_fd++; + mutex_lock(&p->lock); + bool add_ok = hashtbl_add(p->files, (void*)fd, f); + + mutex_unlock(&p->lock); + if (!add_ok) return 0; return fd; @@ -466,24 +493,40 @@ int proc_add_fd(process_t *p, fs_handle_t *f) { bool proc_add_fd_as(process_t *p, fs_handle_t *f, int fd) { if (fd <= 0) return false; - if (hashtbl_find(p->files, (void*)fd) != 0) return false; + bool add_ok = false; + + mutex_lock(&p->lock); + + if (hashtbl_find(p->files, (void*)fd) != 0) goto end; if (fd >= p->next_fd) p->next_fd = fd + 1; - bool add_ok = hashtbl_add(p->files, (void*)fd, f); + add_ok = hashtbl_add(p->files, (void*)fd, f); + +end: + mutex_unlock(&p->lock); return add_ok; } fs_handle_t *proc_read_fd(process_t *p, int fd) { - return (fs_handle_t*)hashtbl_find(p->files, (void*)fd); + mutex_lock(&p->lock); + + fs_handle_t *ret = (fs_handle_t*)hashtbl_find(p->files, (void*)fd); + + mutex_unlock(&p->lock); + return ret; } void proc_close_fd(process_t *p, int fd) { - fs_handle_t *x = proc_read_fd(p, fd); + mutex_lock(&p->lock); + + fs_handle_t *x = (fs_handle_t*)hashtbl_find(p->files, (void*)fd); if (x != 0) { unref_file(x); hashtbl_remove(p->files, (void*)fd); } + + mutex_unlock(&p->lock); } // ============================= // @@ -491,12 +534,17 @@ void proc_close_fd(process_t *p, int fd) { // ============================= // user_region_t *find_user_region(process_t *proc, const void* addr) { + mutex_lock(&proc->lock); + user_region_t *r = (user_region_t*)btree_lower(proc->regions_idx, addr); - if (r == 0) return 0; - ASSERT(addr >= r->addr); + if (r != 0) { + ASSERT(addr >= r->addr); - if (addr >= r->addr + r->size) return 0; + if (addr >= r->addr + r->size) r = 0; + } + + mutex_unlock(&proc->lock); return r; } @@ -511,6 +559,8 @@ bool mmap(process_t *proc, void* addr, size_t size, int mode) { user_region_t *r = (user_region_t*)malloc(sizeof(user_region_t)); if (r == 0) return false; + mutex_lock(&proc->lock); + r->proc = proc; r->addr = addr; r->size = size; @@ -531,11 +581,13 @@ bool mmap(process_t *proc, void* addr, size_t size, int mode) { pager_add_map(r->pager, r); + mutex_unlock(&proc->lock); return true; error: if (r && r->pager) delete_pager(r->pager); if (r) free(r); + mutex_unlock(&proc->lock); return false; } @@ -557,6 +609,8 @@ bool mmap_file(process_t *proc, fs_handle_t *h, size_t offset, void* addr, size_ user_region_t *r = (user_region_t*)malloc(sizeof(user_region_t)); if (r == 0) return false; + mutex_lock(&proc->lock); + r->addr = addr; r->size = size; @@ -576,10 +630,12 @@ bool mmap_file(process_t *proc, fs_handle_t *h, size_t offset, void* addr, size_ r->next_in_proc = proc->regions; proc->regions = r; + mutex_unlock(&proc->lock); return true; error: if (r) free(r); + mutex_unlock(&proc->lock); return false; } @@ -591,6 +647,9 @@ bool mchmap(process_t *proc, void* addr, int mode) { if (r->file != 0) { if ((mode & MM_WRITE) && !(r->file->mode & FM_WRITE)) return false; } + + mutex_lock(&proc->lock); + r->mode = mode; // change mode on already mapped pages @@ -610,6 +669,7 @@ bool mchmap(process_t *proc, void* addr, int mode) { } switch_pagedir(save_pd); + mutex_unlock(&proc->lock); return true; } @@ -617,6 +677,8 @@ bool munmap(process_t *proc, void* addr) { user_region_t *r = find_user_region(proc, addr); if (r == 0) return false; + mutex_lock(&proc->lock); + if (proc->regions == r) { proc->regions = r->next_in_proc; } else { @@ -645,6 +707,8 @@ bool munmap(process_t *proc, void* addr) { } switch_pagedir(save_pd); + mutex_unlock(&proc->lock); + pager_rm_map(r->pager, r); if (r->file != 0) unref_file(r->file); @@ -656,7 +720,7 @@ bool munmap(process_t *proc, void* addr) { } void dbg_dump_proc_memmap(process_t *proc) { - //WARNING not thread safe + mutex_lock(&proc->lock); dbg_printf("/ Region map for process %d\n", proc->pid); @@ -671,6 +735,8 @@ void dbg_dump_proc_memmap(process_t *proc) { } dbg_printf("\\ ----\n"); + + mutex_unlock(&proc->lock); } // =============================== // -- cgit v1.2.3