Free enclave state when last ethread exits. #759
Conversation
vtikoo
requested review from
wintersteiger,
davidchisnall,
paulcallen,
prp and
SeanTAllen
| @@ -216,6 +218,14 @@ static void _sgxlkl_enclave_show_attribute(const void* sgxlkl_enclave_base) | |||
| } | |||
| #endif | |||
|
|
|||
| static inline void dec_ethread_counter() { | |||
| if (a_fetch_add(ðreads_minus_1, -1) == 0) { | |||
There was a problem hiding this comment.
In new code, please use C11 atomic instead of the musl functions. Since we want sequential consistency, just declare the variable with the _Atomic qualifier as _Atomic(int).
| @@ -12,6 +13,7 @@ | |||
| #include "shared/env.h" | |||
| #include "shared/timer_dev.h" | |||
|
|
|||
| int ethreads_minus_1 = -1; | |||
There was a problem hiding this comment.
Please add a comment explaining why this need the minus one bit.
| @@ -379,7 +391,10 @@ int sgxlkl_enclave_init(const sgxlkl_shared_memory_t* shared_memory) | |||
| SGXLKL_VERBOSE("calling _dlstart_c()\n"); | |||
| _dlstart_c((size_t)sgxlkl_enclave_base); | |||
|
|
|||
There was a problem hiding this comment.
It feels as if this should be earlier. Since this is the first ethread, why not skip it entirely and initialise the global assuming the initial ethread exists (if it doesn’t, nothing happens).
| @@ -234,9 +244,11 @@ void sgxlkl_ethread_init(void) | |||
| } | |||
|
|
|||
| /* Initialization completed, now run the scheduler */ | |||
| a_inc(ðreads_minus_1); | |||
There was a problem hiding this comment.
I think there’s a race here. What happens if we call this just after another ethread has exited and deleted the enclave state? This won’t happen in normal operation, but an attacker can trigger it.
I think we want to add a second atomic counter on the number of exited threads and increment it before decrementing this counter on thread exit. At this point, we want to check if the exited threads counter is >0 and, if so, exit.
There was a problem hiding this comment.
That's right, we definitely need to run this later. I think there is in deed a data race where @davidchisnall suspects one and his suggested fix should work.
On the general subject of cleanup: since we don't clean up the thread state, we might as well not clean up the config and drop sgxlkl_free_enclave_state entirely.
I agree. There is no real benefit in trying to do this: the enclave will be destroyed on exit and all state will be cleaned up. The main thing I think we need is a guard on entry to the ecalls to make sure the host can't try to launch new ethreads when we are shutting down. We probably also need a guard here to make sure that we only ever get one initial thread: sgx-lkl/src/enclave/enclave_oe.c Line 339 in c4885aa |
|
Closing this PR, as there is consensus on not freeing enclave state. I 'll create a new one with the |
Summary
sgxlkl_enclave_state stateis being cleaned up duringlkl_termination_thread. This is too early for the lthread scheduler which relies onstate->config.modefor context switches. According to @prp this is a cause of other failures as well.This PR moves
sgxlkl_free_enclave_stateto run later, when the last ethread exits.