Browse Source

[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.
Isaku Yamahata 5 years ago
parent
commit
573c865bc3
2 changed files with 55 additions and 58 deletions
  1. 0 14
      LibOS/shim/include/shim_internal.h
  2. 55 44
      LibOS/shim/src/ipc/shim_ipc_helper.c

+ 0 - 14
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 */                           \

+ 55 - 44
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);
+    /* 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
+    }
 
     lock(&ipc_helper_lock);
-    bool notme = (self != ipc_helper_thread);
+    ipc_helper_state = HELPER_NOTALIVE;
+    ipc_helper_thread = NULL;
     unlock(&ipc_helper_lock);
+    put_thread(self);
+    debug("ipc helper thread terminated\n");
 
-    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)
-        goto end;
+    DkThreadExit();
+}
 
-    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 */