From 573c865bc31cef40187db2d274f280fa89724d36 Mon Sep 17 00:00:00 2001 From: Isaku Yamahata Date: Mon, 29 Apr 2019 15:25:40 -0700 Subject: [PATCH] [LibOS] Introduce stack trampoline in shim_ipc_helper() Previously, shim_ipc_helper() implementation was fragile because it changed the stack in the middle of execution via switch_stack(). This commit introduces the stack trampoline to switch stack. --- LibOS/shim/include/shim_internal.h | 14 ---- LibOS/shim/src/ipc/shim_ipc_helper.c | 101 +++++++++++++++------------ 2 files changed, 56 insertions(+), 59 deletions(-) diff --git a/LibOS/shim/include/shim_internal.h b/LibOS/shim/include/shim_internal.h index 3865ec50..467195f1 100644 --- a/LibOS/shim/include/shim_internal.h +++ b/LibOS/shim/include/shim_internal.h @@ -759,20 +759,6 @@ bool test_user_string (const char * addr); int object_wait_with_retry(PAL_HANDLE handle); #ifdef __x86_64__ -#define SWITCH_STACK(stack_top) \ - ({ \ - void * _rsp, * _rbp; \ - void * _stack = (stack_top); \ - __asm__ volatile ("movq %%rsp, %0" : "=r"(_rsp) :: "memory"); \ - __asm__ volatile ("movq %%rbp, %0" : "=r"(_rbp) :: "memory"); \ - _rsp = _stack - (_rbp - _rsp); \ - _rbp = _stack; \ - __asm__ volatile ("movq %0, %%rsp" :: "r"(_rsp) : "memory"); \ - __asm__ volatile ("movq %0, %%rbp" :: "r"(_rbp) : "memory"); \ - __asm__ volatile ("movq %%rbp, %0" : "=r"(_stack) :: "memory"); \ - _stack; \ - }) - #define __SWITCH_STACK(stack_top, func, arg) \ do { \ /* 16 Bytes align of stack */ \ diff --git a/LibOS/shim/src/ipc/shim_ipc_helper.c b/LibOS/shim/src/ipc/shim_ipc_helper.c index 88cb1605..70d306e3 100644 --- a/LibOS/shim/src/ipc/shim_ipc_helper.c +++ b/LibOS/shim/src/ipc/shim_ipc_helper.c @@ -805,40 +805,36 @@ next: #define IPC_HELPER_STACK_SIZE (allocsize * 4) #define IPC_HELPER_LIST_INIT_SIZE 32 -static void shim_ipc_helper (void * arg) +static void shim_ipc_helper_end(struct shim_thread * self) { - /* set ipc helper thread */ - struct shim_thread * self = (struct shim_thread *) arg; - if (!arg) - return; + /* Put our handle map reference */ + if (self->handle_map) + put_handle_map(self->handle_map); - __libc_tcb_t tcb; - allocate_tls(&tcb, false, self); - debug_setbuf(&tcb.shim_tcb, true); - debug("set tcb to %p\n", &tcb); - - lock(&ipc_helper_lock); - bool notme = (self != ipc_helper_thread); - unlock(&ipc_helper_lock); - - if (notme) { - put_thread(self); - DkThreadExit(); - return; + /* Another thread may be calling shim_clean(). Lower the chances of our + * IPC helper thread and another thread competing on shim_clean/shim_terminate + * (which is benign due to protection via ipc_helper_lock) by adding a barrier + * to ensure reading the latest IPC helper state. */ + COMPILER_BARRIER(); + if (ipc_helper_state == HELPER_HANDEDOVER) { + debug("ipc helper thread is the last thread, process exiting\n"); + shim_terminate(0); // Same as shim_clean(), but this is the official termination function } - debug("ipc helper thread started\n"); + lock(&ipc_helper_lock); + ipc_helper_state = HELPER_NOTALIVE; + ipc_helper_thread = NULL; + unlock(&ipc_helper_lock); + put_thread(self); + debug("ipc helper thread terminated\n"); - void * stack = allocate_stack(IPC_HELPER_STACK_SIZE, allocsize, false); + DkThreadExit(); +} - if (!stack) - goto end; - - self->stack_top = stack + IPC_HELPER_STACK_SIZE; - self->stack = stack; - SWITCH_STACK(stack + IPC_HELPER_STACK_SIZE); - self = get_cur_thread(); - stack = self->stack; +static void __shim_ipc_helper (void * dummy) +{ + struct shim_thread * self = get_cur_thread(); + void * stack = self->stack; int port_num = 0, port_size = IPC_HELPER_LIST_INIT_SIZE; struct shim_ipc_port ** local_pobjs = stack, * pobj; @@ -1020,28 +1016,43 @@ update_list: } end: - /* DP: Put our handle map reference */ - if (self->handle_map) - put_handle_map(self->handle_map); + shim_ipc_helper_end(self); +} - /* shim_clean ultimately calls del_all_ipc_ports(), which reacquires the - * helper lock. Err on the side of caution by adding a barrier to ensure - * reading the latest ipc helper state. - */ - COMPILER_BARRIER(); - if (ipc_helper_state == HELPER_HANDEDOVER) { - debug("ipc helper thread is the last thread, process exiting\n"); - shim_terminate(0); // Same as shim_clean(), but this is the official termination function - } +static void shim_ipc_helper (void * arg) +{ + /* set ipc helper thread */ + struct shim_thread * self = (struct shim_thread *) arg; + if (!arg) + return; + + __libc_tcb_t tcb; + allocate_tls(&tcb, false, self); + debug_setbuf(&tcb.shim_tcb, true); + debug("set tcb to %p\n", &tcb); lock(&ipc_helper_lock); - ipc_helper_state = HELPER_NOTALIVE; - ipc_helper_thread = NULL; + bool notme = (self != ipc_helper_thread); unlock(&ipc_helper_lock); - put_thread(self); - debug("ipc helper thread terminated\n"); - DkThreadExit(); + if (notme) { + put_thread(self); + DkThreadExit(); + return; + } + + debug("ipc helper thread started\n"); + + void * stack = allocate_stack(IPC_HELPER_STACK_SIZE, allocsize, false); + + if (!stack) { + shim_ipc_helper_end(self); + return; + } + + self->stack_top = stack + IPC_HELPER_STACK_SIZE; + self->stack = stack; + __SWITCH_STACK(self->stack_top, __shim_ipc_helper, NULL); } /* This function shoudl be called with the ipc_helper_lock held */