From e6212ccbe0dc5acb03ec806f481122f7ad270b78 Mon Sep 17 00:00:00 2001 From: Isaku Yamahata Date: Thu, 22 Nov 2018 00:18:51 -0800 Subject: [PATCH] [LibOS/ipc] async helper: keep refcount to helper thread on exit Using put_thread(self) to free resources by itself is problematic because the exiting thread is still using them. Actually, put_thread(self) logic in ipc/async helper may cause SEGV depending on who is the last to put_thread(). If the thread is the last one to free thread area, lock/unlock after freeing thread area causes SEGV due to debug message. The solution is to keep reference count of those two helper threads by a (usually master) thread so that the (master) thread will be the last thread to release the reference count. This way self destruction can be avoided. To completely fix this issue, helper threads need to be waited to exit. The issue is tracked by https://github.com/oscarlab/graphene/issues/440. Signed-off-by: Isaku Yamahata --- LibOS/shim/include/shim_ipc.h | 2 +- LibOS/shim/include/shim_utils.h | 2 +- LibOS/shim/src/ipc/shim_ipc_helper.c | 17 ++++++++++++++++- LibOS/shim/src/shim_async.c | 19 ++++++++++++++++--- LibOS/shim/src/sys/shim_exit.c | 18 ++++++++++++++++-- 5 files changed, 50 insertions(+), 8 deletions(-) diff --git a/LibOS/shim/include/shim_ipc.h b/LibOS/shim/include/shim_ipc.h index ec8be978..76585235 100644 --- a/LibOS/shim/include/shim_ipc.h +++ b/LibOS/shim/include/shim_ipc.h @@ -604,7 +604,7 @@ void ipc_parent_exit (struct shim_ipc_port * port, IDTYPE vmid, void ipc_child_exit (struct shim_ipc_port * port, IDTYPE vmid, unsigned int exitcode); -int exit_with_ipc_helper (bool handover); +int exit_with_ipc_helper (bool handover, struct shim_thread ** ret); #define IPC_FORCE_RECONNECT ((void *) -1) diff --git a/LibOS/shim/include/shim_utils.h b/LibOS/shim/include/shim_utils.h index 4de7dd1d..d2d2d83e 100644 --- a/LibOS/shim/include/shim_utils.h +++ b/LibOS/shim/include/shim_utils.h @@ -243,7 +243,7 @@ int64_t install_async_event (PAL_HANDLE object, unsigned long time, void (*callback) (IDTYPE caller, void * arg), void * arg); int create_async_helper (void); -int terminate_async_helper (void); +struct shim_thread * terminate_async_helper (void); extern struct config_store * root_config; diff --git a/LibOS/shim/src/ipc/shim_ipc_helper.c b/LibOS/shim/src/ipc/shim_ipc_helper.c index 7bf42782..8ef10657 100644 --- a/LibOS/shim/src/ipc/shim_ipc_helper.c +++ b/LibOS/shim/src/ipc/shim_ipc_helper.c @@ -1079,8 +1079,19 @@ static int create_ipc_helper (void) return 0; } -int exit_with_ipc_helper (bool handover) +/* + * on success, the reference to the helper thread is returned with + * reference count incremented. + * The caller is responsible to wait for the IPC helper thread to exit + * and release the final reference to free related resources. + * It's problematic for the thread itself to release its resources which it's + * using. For example stack. + * So defer releasing it after its exit and make the releasing the caller + * responsibility. + */ +int exit_with_ipc_helper (bool handover, struct shim_thread ** ret) { + *ret = NULL; if (IN_HELPER() || ipc_helper_state != HELPER_ALIVE) return 0; @@ -1104,6 +1115,10 @@ int exit_with_ipc_helper (bool handover) } ipc_helper_state = new_state; + if (ipc_helper_thread != NULL) { + get_thread(ipc_helper_thread); + *ret = ipc_helper_thread; + } unlock(ipc_helper_lock); set_event(&ipc_helper_event, 1); diff --git a/LibOS/shim/src/shim_async.c b/LibOS/shim/src/shim_async.c index 48b326bd..188487a2 100644 --- a/LibOS/shim/src/shim_async.c +++ b/LibOS/shim/src/shim_async.c @@ -318,14 +318,27 @@ int create_async_helper (void) return 0; } -int terminate_async_helper (void) +/* + * On success, the reference to the thread of async helper is returned with + * reference count incremented. + * It's caller the responsibility to wait for its exit and release the + * final reference to free related resources. + * It's problematic for the thread itself to release its resources which it's + * using. For example stack. + * So defer releasing it after its exit and make the releasing the caller + * responsibility. + */ +struct shim_thread * terminate_async_helper (void) { if (async_helper_state != HELPER_ALIVE) - return 0; + return NULL; lock(async_helper_lock); + struct shim_thread * ret = async_helper_thread; + if (ret) + get_thread(ret); async_helper_state = HELPER_NOTALIVE; unlock(async_helper_lock); set_event(&async_helper_event, 1); - return 0; + return ret; } diff --git a/LibOS/shim/src/sys/shim_exit.c b/LibOS/shim/src/sys/shim_exit.c index 564accfb..b8de29af 100644 --- a/LibOS/shim/src/sys/shim_exit.c +++ b/LibOS/shim/src/sys/shim_exit.c @@ -142,9 +142,23 @@ int try_process_exit (int error_code, int term_signal) if (check_last_thread(cur_thread)) return 0; - terminate_async_helper(); + struct shim_thread * async_thread = terminate_async_helper(); + if (async_thread) + /* TODO: wait for the thread to exit in host. + * This is tracked by the following issue. + * https://github.com/oscarlab/graphene/issues/440 + */ + put_thread(async_thread); /* free resources of the thread */ - if (!exit_with_ipc_helper(true)) + struct shim_thread * ipc_thread; + int ret = exit_with_ipc_helper(true, &ipc_thread); + if (ipc_thread) + /* TODO: wait for the thread to exit in host. + * This is tracked by the following issue. + * https://github.com/oscarlab/graphene/issues/440 + */ + put_thread(ipc_thread); /* free resources of the thread */ + if (!ret) shim_clean(); else DkThreadExit();