From b25156373cd89792b1c457b77699bb7f9beb430d Mon Sep 17 00:00:00 2001 From: Kimplul Date: Mon, 26 Aug 2024 18:51:10 +0300 Subject: improve killing processes/threads, test + Remove sys_kill() as we should be using a two-stage process where a thread is first orphaned by sys_detach(), and then the thread itself calls sys_exit() after it has done all necessary cleanup in init. --- include/kmi/syscalls.h | 3 --- include/kmi/tcb.h | 15 +++++++------ include/kmi/uapi.h | 13 ----------- src/orphanage.c | 8 +++---- src/tcb.c | 59 ++++++++++++++++---------------------------------- src/uapi/dispatch.c | 1 - src/uapi/ipc.c | 3 ++- src/uapi/proc.c | 41 ++++++----------------------------- src/vmem.c | 12 +++++----- tests/common/test.h | 10 ++------- tests/detach/init.c | 52 ++++++++++++++++++++++++++++++++++++++++++++ tests/detach/source.mk | 2 ++ 12 files changed, 102 insertions(+), 117 deletions(-) create mode 100644 tests/detach/init.c create mode 100644 tests/detach/source.mk diff --git a/include/kmi/syscalls.h b/include/kmi/syscalls.h index 28e7505..a4d2c52 100644 --- a/include/kmi/syscalls.h +++ b/include/kmi/syscalls.h @@ -120,9 +120,6 @@ enum sys_code { /** Execute new binary in new process space. */ SYS_SPAWN, - /** Kill thread. */ - SYS_KILL, - /** Switch running process. */ SYS_SWAP, diff --git a/include/kmi/tcb.h b/include/kmi/tcb.h index 6cc63b5..d61845f 100644 --- a/include/kmi/tcb.h +++ b/include/kmi/tcb.h @@ -354,27 +354,28 @@ bool running(struct tcb *t); bool zombie(struct tcb *t); /** - * Add a reference to a process. + * Add a reference to a thread. * Instead of lists of threads that belong to a process, we give the process' * owning thread a reference counter. When a process is killed, a 'dead' bit is * set, and the thread that owns the process is unreferenced. All data * associated with the process can immediately be freed, but the tid is still - * reserved until the reference count reaches zero. + * reserved until the reference count reaches zero. In this sense, a process is + * seen as some memory that some thread happens to own. * * We have to make sure that all ways a process might be entered check that the * process is still alive, and unmapping pages causes other threads to update * their page tables as well. Then if a segfault happens, we can check if it was * due to being in a dead process. This is still largely TODO. * - * @param p Process to reference. + * @param t Thread to reference. */ -void reference_proc(struct tcb *p); +void reference_thread(struct tcb *t); /** - * Unreference a process. + * Unreference a thread. * - * @param p Process to unreference. + * @param t Thread to unreference. */ -void unreference_proc(struct tcb *p); +void unreference_thread(struct tcb *t); #endif /* KMI_TCB_H */ diff --git a/include/kmi/uapi.h b/include/kmi/uapi.h index 6e5f865..b0740a9 100644 --- a/include/kmi/uapi.h +++ b/include/kmi/uapi.h @@ -655,19 +655,6 @@ SYSCALL_DECLARE2(exec, bin, interp); */ SYSCALL_DECLARE2(spawn, bin, interp); -/** - * Kill syscall. Requests that all memory in process \p pid is freed and all - * threads are orphaned. If \p pid is not a process, nothing is done. - * - * @param t Current tcb. - * @param pid Thread ID to kill. 0 if self. - * @param b Unused. - * @param c Unused. - * @param d Unused. - * @param e Unused. - */ -SYSCALL_DECLARE1(kill, pid); - /** * Swap syscall. * diff --git a/src/orphanage.c b/src/orphanage.c index 526affc..797468d 100644 --- a/src/orphanage.c +++ b/src/orphanage.c @@ -29,16 +29,16 @@ void unorphanize(struct tcb *t) /* attach to init process */ struct tcb *init = get_tcb(1); - reference_proc(init); + reference_thread(init); + + free_stack(t); + reset_rpc_stack(t); id_t old_rid = t->rid; t->rid = 1; t->pid = 1; t->eid = 1; - free_stack(t); - reset_rpc_stack(t); - t->proc = init->proc; use_vmem(t->proc.vmem); alloc_stack(t); diff --git a/src/tcb.c b/src/tcb.c index 238d8e3..93ce502 100644 --- a/src/tcb.c +++ b/src/tcb.c @@ -120,7 +120,7 @@ stat_t alloc_stack(struct tcb *t) void free_stack(struct tcb *t) { - free_uvmem(t, t->thread_stack); + free_uvmem(get_proc(t), t->thread_stack); } struct tcb *create_thread(struct tcb *p) @@ -180,7 +180,7 @@ struct tcb *create_thread(struct tcb *p) } setup_rpc_stack(t); - reference_proc(p); + reference_thread(p); t->regs = (vm_t)t; @@ -235,15 +235,9 @@ struct tcb *create_proc(struct tcb *p) static stat_t __destroy_thread_data(struct tcb *t) { assert(t->refcount == 0); + assert(zombie(t)); /* remove ourselves from the thread pool */ - /** @todo this should be at the top of the function, and be wrapped in - * some kind of lock that checks that nobody reads the value while we're - * setting it to zero. get_tcb() should accordingly increment the - * reference count atomically. Also, an unget_tcb() is needed to - * decrement the reference count I guess? if we didn't have the BKL that - * is - */ tcbs[t->tid] = 0; /* forcefully free last struggling bits of memory, assuming we own the @@ -259,14 +253,10 @@ static stat_t __destroy_thread_data(struct tcb *t) stat_t destroy_thread(struct tcb *t) { assert(tcbs); - assert(!is_proc(t)); - - /* mark us as zombies */ - set_bits(t->state, TCB_ZOMBIE); - t->rid = 0; + assert(t->tid != 1); /* remove reference to root process */ - unreference_proc(get_rproc(t)); + unreference_thread(get_rproc(t)); free_stack(t); @@ -282,12 +272,12 @@ stat_t destroy_thread(struct tcb *t) * let the handler check if the thread is still interested in the * interrupt */ - /* someone still relies on us existing, don't actually free thread data - * quite yet */ - if (t->refcount) - return OK; + /* mark us as zombies */ + set_bits(t->state, TCB_ZOMBIE); + t->rid = 0; - return __destroy_thread_data(t); + unreference_thread(t); + return OK; } stat_t destroy_proc(struct tcb *p) @@ -295,15 +285,6 @@ stat_t destroy_proc(struct tcb *p) assert(tcbs); assert(is_proc(p)); - /** @todo currently we don't care who else is in the address space when - * we start freeing stuff, one fairly simple way to deal with this is to - * just not care. A thread that tries to access some bit of freed memory - * will cause a segfault (eventually at least), and we can just check in - * the segfault handler if the thread has become orphaned. - * Currently no segfault handler exists, though. */ - - set_bits(p->state, TCB_ZOMBIE); - /* clear all privately owned memory regions, keep shared ones alive for * now */ clear_uvmem(p); @@ -313,25 +294,23 @@ stat_t destroy_proc(struct tcb *p) return OK; } -void reference_proc(struct tcb *p) +void reference_thread(struct tcb *t) { - if (!p) + if (!t) return; - assert(is_proc(p)); - p->refcount++; + t->refcount++; } -void unreference_proc(struct tcb *p) +void unreference_thread(struct tcb *t) { - if (!p) + if (!t) return; - assert(is_proc(p)); - p->refcount--; - if (zombie(p) && p->refcount == 0) { - dbg("thread %ld is completely destroyed\n", (long)p->tid); - __destroy_thread_data(p); + t->refcount--; + if (t->refcount == 0) { + info("thread %ld is completely destroyed\n", (long)t->tid); + __destroy_thread_data(t); } } diff --git a/src/uapi/dispatch.c b/src/uapi/dispatch.c index 859543a..2eef5bc 100644 --- a/src/uapi/dispatch.c +++ b/src/uapi/dispatch.c @@ -71,7 +71,6 @@ void handle_syscall(sys_arg_t syscall, sys_arg_t a, sys_arg_t b, case SYS_FORK: sys_fork(t, a, b, c, d, e); break; case SYS_EXEC: sys_exec(t, a, b, c, d, e); break; case SYS_SPAWN: sys_spawn(t, a, b, c, d, e); break; - case SYS_KILL: sys_kill(t, a, b, c, d, e); break; case SYS_SWAP: sys_swap(t, a, b, c, d, e); break; case SYS_SET_CONF: sys_set_conf(t, a, b, c, d, e); break; case SYS_GET_CONF: sys_get_conf(t, a, b, c, d, e); break; diff --git a/src/uapi/ipc.c b/src/uapi/ipc.c index 850178c..0fba899 100644 --- a/src/uapi/ipc.c +++ b/src/uapi/ipc.c @@ -71,7 +71,7 @@ static inline void finalize_rpc(struct tcb *t, struct tcb *r, vm_t s) { clone_uvmem(r->proc.vmem, t->rpc.vmem); set_return(t, r->callback); - reference_proc(r); + reference_thread(r); t->pid = r->rid; /* make sure updates are visible when swapping to the new virtual memory */ @@ -446,6 +446,7 @@ SYSCALL_DEFINE4(ipc_resp)(struct tcb *t, sys_arg_t d0, sys_arg_t d1, /* inform requester who answered (pid) in the case of the request being * kicked forward */ enable_irqs(); + unreference_thread(get_cproc(t)); leave_rpc(t, SYS_RET6(OK, t->pid, d0, d1, d2, d3)); } diff --git a/src/uapi/proc.c b/src/uapi/proc.c index c0fe5f0..b90433f 100644 --- a/src/uapi/proc.c +++ b/src/uapi/proc.c @@ -174,29 +174,6 @@ SYSCALL_DEFINE2(spawn)(struct tcb *t, sys_arg_t bin, sys_arg_t interp) return_args1(t, n->pid); } -/** - * Kill syscall handler. - * - * @param t Current tcb. - * @param pid Process to kill. - * \todo Implement. - * - * @return ERR_PERM if not capable to kill, otherwise OK. - */ -SYSCALL_DEFINE1(kill)(struct tcb *t, sys_arg_t pid) -{ - struct tcb *c = get_cproc(t); - if (!(has_cap(c->caps, CAP_PROC))) - return_args1(t, ERR_PERM); - - struct tcb *r = get_tcb(pid); - if (is_proc(r)) - return_args1(t, ERR_INVAL); - - destroy_proc(r); - return_args1(t, OK); -} - /** * Actual worker of swapping between threads. * Assumes that both \p t and \p s exist and that \p s isn't a zombie or @@ -215,7 +192,7 @@ static void swap(struct tcb *t, struct tcb *s) enable_irqs(); if (!is_rpc(s) && orphan(s)) { - orphanize(s); + unorphanize(s); return; } @@ -242,6 +219,12 @@ static void swap(struct tcb *t, struct tcb *s) */ SYSCALL_DEFINE1(exit)(struct tcb *t, sys_arg_t tid) { + /* init thread is not allowed to exit */ + /** @todo what about other possible threads start at init, are they + * allowed to exit? I guess? */ + if (t->tid == 1) + return_args1(t, ERR_INVAL); + if (tid != 0) { struct tcb *s = get_tcb(tid); if (!s) @@ -284,17 +267,7 @@ SYSCALL_DEFINE1(detach)(struct tcb *t, sys_arg_t tid) if (!o || orphan(o)) return_args1(t, ERR_INVAL); - struct tcb *r = get_tcb(o->rid); - if (r) - unreference_proc(r); - orphanize(o); - - /* generally the thread shouldn't do anything with this information, but - * it fits really nicely into the notification framework so just do it - */ - notify(o, NOTIFY_ORPHANED); - return_args1(t, OK); } diff --git a/src/vmem.c b/src/vmem.c index 6fb5474..96d4005 100644 --- a/src/vmem.c +++ b/src/vmem.c @@ -46,11 +46,11 @@ static stat_t __copy_mapped_region(struct tcb *d, struct tcb *s, if (ERR_CODE(v)) return v; + assert(v == start); + if (is_set(m->flags, MR_NONBACKED)) return v; - assert(v == start); - /* note that we use uvmem.vmem instead of proc.vmem, this is just to * make sure that zombies don't eat our brains */ stat_t res = copy_region(d->uvmem.vmem, s->uvmem.vmem, v, v, size); @@ -80,7 +80,7 @@ static stat_t __copy_shared_region(struct tcb *d, struct mem_region *m) vm_t start = m->start * BASE_PAGE_SIZE; vm_t end = m->end * BASE_PAGE_SIZE; - reference_proc(s); + reference_thread(s); size_t size = end - start; vm_t v = alloc_fixed_region(&d->uvmem.region, start, size, &size, @@ -115,7 +115,7 @@ static vm_t __clone_shared_region(struct tcb *d, struct tcb *s, vm_t start = m->start * BASE_PAGE_SIZE; vm_t end = m->end * BASE_PAGE_SIZE; - reference_proc(s); + reference_thread(s); size_t size = end - start; vm_t v = alloc_shared_region(&d->uvmem.region, size, &size, @@ -129,7 +129,7 @@ static vm_t __clone_shared_region(struct tcb *d, struct tcb *s, return v; /* cleanup on error */ - unreference_proc(s); + unreference_thread(s); free_region(&d->uvmem.region, v); unmap_fixed_region(d->uvmem.vmem, v, size); return res; @@ -145,7 +145,7 @@ static void __free_mapping(struct tcb *t, struct mem_region *m) { struct tcb *owner = get_tcb(m->pid); if (owner) - unreference_proc(owner); + unreference_thread(owner); if (is_set(m->flags, MR_NONBACKED)) return; diff --git a/tests/common/test.h b/tests/common/test.h index 18ac388..0b53b52 100644 --- a/tests/common/test.h +++ b/tests/common/test.h @@ -249,12 +249,6 @@ static inline id_t sys_spawn(uintptr_t bin, uintptr_t interp) return r.s; } -static inline enum sys_status sys_kill(id_t pid) -{ - struct sys_ret r = syscall1(SYS_KILL, pid); - return r.s; -} - static inline enum sys_status sys_swap(id_t tid) { struct sys_ret r = syscall1(SYS_SWAP, tid); @@ -323,9 +317,9 @@ static inline enum sys_status sys_detach(id_t tid) return r.s; } -static inline enum sys_status sys_exit() +static inline enum sys_status sys_exit(id_t tid) { - struct sys_ret r = syscall0(SYS_EXIT); + struct sys_ret r = syscall1(SYS_EXIT, tid); return r.s; } diff --git a/tests/detach/init.c b/tests/detach/init.c new file mode 100644 index 0000000..d3f282e --- /dev/null +++ b/tests/detach/init.c @@ -0,0 +1,52 @@ +#include + +START(pid, tid, d0, d1, d2, d3) +{ + UNUSED(pid); + UNUSED(tid); + UNUSED(d1); + UNUSED(d2); + UNUSED(d3); + + if (d0 == SYS_USER_SPAWNED) { + size_t prev_ram = sys_conf_get(CONF_RAM_USAGE, 0); + printf("ram usage before new process: %zx\n", prev_ram); + + printf("forking\n"); + id_t our_tid = 0; + id_t new_id = sys_fork(&our_tid); + check(new_id > 0, "error from fork\n"); + + /* not in general but in this case */ + check(new_id != 0, "in child, should never be in child\n"); + + printf("checking detach of nonsense id\n"); + enum sys_status r = sys_detach(200); + check(r != OK, "nonsense id succeeded?\n"); + + printf("detaching child\n"); + r = sys_detach(new_id); + check(r == OK, "detaching child failed\n"); + + printf("swapping to detached/orphaned thread\n"); + r = sys_swap(new_id); + check(r == OK, "swapping to detached thread failed\n"); + + size_t new_ram = sys_conf_get(CONF_RAM_USAGE, 0); + printf("ram usage after new process: %zx\n", new_ram); + + check(new_ram == prev_ram, "fork/detach leaked memory\n"); + ok(); + } + else if (d0 == SYS_USER_ORPHANED) { + printf("exiting with nonsense swap id\n"); + enum sys_status r = sys_exit(200); + check(r != OK, "nonsense exit succeeded (but returned...?)\n"); + + printf("exiting with sensible swap id\n"); + r = sys_exit(1); + check(0, "exit with sensible id returned\n"); + } + + check(0, "missed SYS_USER_ORPHANED\n"); +} diff --git a/tests/detach/source.mk b/tests/detach/source.mk new file mode 100644 index 0000000..843562f --- /dev/null +++ b/tests/detach/source.mk @@ -0,0 +1,2 @@ +DO != ./scripts/gen-prog -n detach -p init init.c +DO != ./scripts/gen-simple -n detach -p init -- cgit v1.3