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/common/libalgo/btree.c | 56 ++++++++++++------------ src/common/libalgo/hashtbl.c | 2 +- src/common/libkogata/slab_alloc.c | 3 +- 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 +++++++++++++++++++++++++++++++++------ 9 files changed, 140 insertions(+), 58 deletions(-) diff --git a/src/common/libalgo/btree.c b/src/common/libalgo/btree.c index 9ea4a15..6bc0fdc 100644 --- a/src/common/libalgo/btree.c +++ b/src/common/libalgo/btree.c @@ -56,18 +56,18 @@ size_t btree_count(btree_t *i) { // TREE ROTATIONS // // ============== // -static int height(btree_item_t *i) { +static inline int height(btree_item_t *i) { if (i == 0) return 0; return i->height; } -static void recalc_height(btree_item_t *i) { +void btree_recalc_height(btree_item_t *i) { if (i == 0) return; i->height = MAX(height(i->left), height(i->right)) + 1; } -static btree_item_t* rotate_left(btree_item_t *i) { +btree_item_t* btree_rotate_left(btree_item_t *i) { // I(X(a, b), Y) -> a X b I Y -> X(a, I(b, Y)) btree_item_t *x = i->left; @@ -77,13 +77,13 @@ static btree_item_t* rotate_left(btree_item_t *i) { x->right = i; i->left = b; - recalc_height(i); - recalc_height(x); + btree_recalc_height(i); + btree_recalc_height(x); return x; } -static btree_item_t* rotate_right(btree_item_t *i) { +btree_item_t* btree_rotate_right(btree_item_t *i) { // I(X, Y(a,b)) -> X I a Y b -> Y(I(X, a), b) btree_item_t *y = i->right; @@ -93,20 +93,20 @@ static btree_item_t* rotate_right(btree_item_t *i) { y->left = i; i->right = a; - recalc_height(i); - recalc_height(y); + btree_recalc_height(i); + btree_recalc_height(y); return y; } -static btree_item_t* equilibrate(btree_item_t *i) { +btree_item_t* btree_equilibrate(btree_item_t *i) { if (i == 0) return 0; while (height(i->left) - height(i->right) >= 2) - i = rotate_left(i); + i = btree_rotate_left(i); while (height(i->right) - height(i->left) >= 2) - i = rotate_right(i); + i = btree_rotate_right(i); return i; } @@ -124,9 +124,9 @@ bool btree_add(btree_t *t, void* key, void* val) { } else { root->right = insert(t, root->right, i); } - recalc_height(root); + btree_recalc_height(root); - return equilibrate(root); + return btree_equilibrate(root); } btree_item_t *x = (btree_item_t*)malloc(sizeof(btree_item_t)); @@ -135,7 +135,7 @@ bool btree_add(btree_t *t, void* key, void* val) { x->key = key; x->val = val; x->left = x->right = 0; - recalc_height(x); + btree_recalc_height(x); t->root = insert(t, t->root, x); t->nitems++; @@ -151,8 +151,8 @@ void btree_remove(btree_t *t, const void* key) { return i->right; } else { i->left = extract_smallest(i->left, out_smallest); - recalc_height(i); - return equilibrate(i); + btree_recalc_height(i); + return btree_equilibrate(i); } } @@ -162,10 +162,10 @@ void btree_remove(btree_t *t, const void* key) { int c = t->cf(key, i->key); if (c < 0) { i->left = remove_aux(t, i->left, key); - return equilibrate(i); + return btree_equilibrate(i); } else if (c > 0) { i->right = remove_aux(t, i->right, key); - return equilibrate(i); + return btree_equilibrate(i); } else { // remove item i @@ -178,8 +178,8 @@ void btree_remove(btree_t *t, const void* key) { new_i->left = i->left; new_i->right = new_i_right; - recalc_height(new_i); - new_i = equilibrate(new_i); + btree_recalc_height(new_i); + new_i = btree_equilibrate(new_i); } if (t->releasef) t->releasef(i->key, i->val); @@ -201,8 +201,8 @@ void btree_remove_v(btree_t *t, const void* key, const void* val) { return i->right; } else { i->left = extract_smallest(i->left, out_smallest); - recalc_height(i); - return equilibrate(i); + btree_recalc_height(i); + return btree_equilibrate(i); } } @@ -212,10 +212,10 @@ void btree_remove_v(btree_t *t, const void* key, const void* val) { int c = t->cf(key, i->key); if (c < 0) { i->left = remove_aux(t, i->left, key, val); - return equilibrate(i); + return btree_equilibrate(i); } else if (c > 0) { i->right = remove_aux(t, i->right, key, val); - return equilibrate(i); + return btree_equilibrate(i); } else if (i->val == val) { // remove item i @@ -228,8 +228,8 @@ void btree_remove_v(btree_t *t, const void* key, const void* val) { new_i->left = i->left; new_i->right = new_i_right; - recalc_height(new_i); - new_i = equilibrate(new_i); + btree_recalc_height(new_i); + new_i = btree_equilibrate(new_i); } if (t->releasef) t->releasef(i->key, i->val); @@ -240,8 +240,8 @@ void btree_remove_v(btree_t *t, const void* key, const void* val) { } else { i->left = remove_aux(t, i->left, key, val); i->right = remove_aux(t, i->right, key, val); - recalc_height(i); - return equilibrate(i); + btree_recalc_height(i); + return btree_equilibrate(i); } } diff --git a/src/common/libalgo/hashtbl.c b/src/common/libalgo/hashtbl.c index ddc56e1..13185c2 100644 --- a/src/common/libalgo/hashtbl.c +++ b/src/common/libalgo/hashtbl.c @@ -61,7 +61,7 @@ void delete_hashtbl(hashtbl_t *ht) { free(ht); } -static void hashtbl_check_size(hashtbl_t *ht) { +void hashtbl_check_size(hashtbl_t *ht) { size_t nsize = 0; if (4 * ht->nitems < ht->size) nsize = ht->size / 2; if (4 * ht->nitems > 3 * ht->size) nsize = ht->size * 2; diff --git a/src/common/libkogata/slab_alloc.c b/src/common/libkogata/slab_alloc.c index 6d0b9d6..837a965 100644 --- a/src/common/libkogata/slab_alloc.c +++ b/src/common/libkogata/slab_alloc.c @@ -175,6 +175,7 @@ void* slab_alloc(mem_allocator_t* a, size_t sz) { add_free_descriptor(a, fcd); return 0; } + /*dbg_printf("New cache 0x%p\n", fc->region_addr);*/ fc->n_free_objs = 0; fc->first_free_obj = 0; @@ -195,6 +196,7 @@ void* slab_alloc(mem_allocator_t* a, size_t sz) { ASSERT(fc->first_free_obj != 0); object_t *x = fc->first_free_obj; + /*dbg_printf("Alloc 0x%p\n", x);*/ fc->first_free_obj = x->next; fc->n_free_objs--; @@ -226,7 +228,6 @@ void* slab_alloc(mem_allocator_t* a, size_t sz) { } void slab_free(mem_allocator_t* a, void* addr) { - for (int i = 0; a->types[i].obj_size != 0; i++) { size_t region_size = PAGE_SIZE * a->types[i].pages_per_cache; for (cache_t *r = a->slabs[i].first_cache; r != 0; r = r->next_cache) { 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