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.

12 files changed, 102 insertions, 117 deletions

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
@@ -656,19 +656,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.
  *
  * Swap currently running thread.
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
@@ -175,29 +175,6 @@ SYSCALL_DEFINE2(spawn)(struct tcb *t, sys_arg_t bin, sys_arg_t interp)
 }
 
 /**
- * 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
  * currently running.
@@ -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 <common/test.h>
+
+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