Glue code for using snmalloc in EDP #601
Conversation
jethrogb
requested review from
nshyrei,
aditijannu,
raoulstrackx,
NirjharRoyiitk and
BobbyAtFortanix
jethrogb
force-pushed
the
jb/snmalloc-edp
branch
2 times, most recently
from
69da725
to
ff36c0d
Compare
.github/workflows/build.yml
Outdated
| @@ -48,7 +50,7 @@ jobs: | |||
| rustup update | |||
|
|
|||
| - name: Cargo test --all --exclude sgxs-loaders | |||
| run: cargo test --verbose --locked --all --exclude sgxs-loaders --exclude async-usercalls && [ "$(echo $(nm -D target/debug/sgx-detect|grep __vdso_sgx_enter_enclave))" = "w __vdso_sgx_enter_enclave" ] | |||
| run: true || cargo test --verbose --locked --all --exclude sgxs-loaders --exclude async-usercalls --exclude snmalloc-edp && [ "$(echo $(nm -D target/debug/sgx-detect|grep __vdso_sgx_enter_enclave))" = "w __vdso_sgx_enter_enclave" ] | |||
There was a problem hiding this comment.
There appears to be an unrelated CI issue. Once fixed, please remove true || here and in two more places.
error: struct `Unverified` is never constructed
--> intel-sgx/ias/src/api.rs:196:19
|
196 | pub(crate) struct Unverified {}
| ^^^^^^^^^^```
snmalloc-edp/tests/global_alloc.rs
Outdated
| unsafe fn with_thread_allocator<F: FnOnce() -> R, R>(f: F) -> R { | ||
| unsafe { | ||
| let layout = alloc::Layout::from_size_align(sn_alloc_size, sn_alloc_align).unwrap(); | ||
| // TODO: bootstrap the thread-local allocator allocation a different way |
There was a problem hiding this comment.
@jethrogb @raoulstrackx Is this todo still relevant? Because with @raoulstrackx we have figured that the way to use sn_thread_init(allocator);
and sn_thread_cleanup(allocator); is to put them around https://github.com/rust-lang/rust/blob/master/library/std/src/sys/pal/sgx/thread.rs#L43.
The function is nice to have for testing purposes, but I don't fully understand it's intended use case in rust-stdlib.
There was a problem hiding this comment.
put them around https://github.com/rust-lang/rust/blob/master/library/std/src/sys/pal/sgx/thread.rs#L43.
That is not right. That code is only run for additional threads besides the main thread. It should be somewhere in entry.
Is this todo still relevant?
Yes you still need to allocate memory for the thread allocator.
|
|
||
| namespace snmalloc { | ||
| void register_clean_up() { | ||
| // TODO: not sure what this is supposed to do |
There was a problem hiding this comment.
I haven't found what register_clean_up is supposed to do exactly. The search in snmalloc repository gives me this https://github.com/search?q=repo%3Amicrosoft%2Fsnmalloc%20register_clean_up&type=code, where the first match looks like this
inline void register_clean_up()
{
error("Critical Error: This should never be called.");
}
which matches the current change in the PR.
cc @raoulstrackx
|
|
||
| static inline uint64_t get_entropy64() { | ||
| long long unsigned int result = 0; | ||
| while (_rdrand64_step(&result) != 1) |
There was a problem hiding this comment.
This should probably be a bounded loop that aborts after some number of tries
| #define SNMALLOC_SGX | ||
| #define SNMALLOC_USE_SMALL_CHUNKS | ||
| #define SNMALLOC_MEMORY_PROVIDER PALEdpSgx | ||
| #define OPEN_ENCLAVE |
There was a problem hiding this comment.
Is this really needed?
There was a problem hiding this comment.
Perhaps not, the impact in snmalloc/ds_core/mitigations.h should be investigated.
There was a problem hiding this comment.
Looks like these mitigations are applied only to checked builds i.e. when SNMALLOC_CHECK_CLIENT is set. So it is disabled by default.
| #define SNMALLOC_MEMORY_PROVIDER PALEdpSgx | ||
| #define OPEN_ENCLAVE | ||
| // needed for openenclave header: | ||
| #define OE_OK 0 |
There was a problem hiding this comment.
Is this really needed?
There was a problem hiding this comment.
This is needed if OPEN_ENCLAVE is defined.
| run: | | ||
| git submodule update --init --recursive | ||
| detect_cxx_include_path() { | ||
| for path in $(clang++-12 -print-search-dirs|sed -n 's/^libraries:\s*=//p'|tr : ' '); do |
There was a problem hiding this comment.
Should use CXX_x86_64_fortanix_unknown_sgx
Putting this as a crate here, but this could also just live in the Rust source tree, if they accept the submodule.