Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added test for rapid enclave creation/termination. #359

Merged
merged 5 commits into from
Jul 17, 2018

Conversation

gupta-ak
Copy link
Contributor

First checkbox of #23.

@CodeMonkeyLeet CodeMonkeyLeet added the testing Issue has to do with testing or quality management label Jun 29, 2018
@CodeMonkeyLeet CodeMonkeyLeet added this to Backlog in Public preview via automation Jun 29, 2018
@CodeMonkeyLeet CodeMonkeyLeet moved this from Backlog to In review in Public preview Jun 29, 2018
@gupta-ak gupta-ak force-pushed the akagup/tests.repeat-enclave branch from c5c23a3 to 5e21535 Compare July 10, 2018 21:54
Copy link
Contributor

@CodeMonkeyLeet CodeMonkeyLeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor cleanups only. It would be helpful to understand the EPC limits and if this test provides an implicit bound of EPC leakage (if any)

@@ -0,0 +1 @@
run enc/create_rapid_enc.signed.so
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove, we shouldn't need to check in the gdb.commands files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I will remove it. I saw a lot of tests having these, so I wasn't sure if they should be checked in.

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#include <openenclave/host.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run format-code … I suspect it will reorder the headers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that format-code actually moves this to the top.

oe_enclave_t* enclaves[MAX_SIMULTANEOUS_ENCLAVES];
oe_result_t result = OE_OK;

int numEnclaves = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for initializing this outside the for loop? It's not actually used outside the loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, numEnclaves is used in the subsequent for loop that starts at line 80 and needs to be declared outside the first for loop. And thus initialized here.

  • if (call_enclave)
  • {
  •    for (int i = 0; i < numEnclaves; i++)
    

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the reason it needs to track numEnclaves is not for the loop at line 80 (it only gets executed on success, where numEnclaves will equal MAX_SIMULTANEOUS_ENCLAVES so it could be replaced with a constant), but so that it invokes the correct number of oe_terminate_enclave calls on cleanup in the failure case. I missed that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

goto Cleanup;
}

OE_TEST(args == 2 * i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably useful to comment that this is an arbitrary test validation that the enclave call behaves this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

#include <thread>
#include <vector>

#define MAX_ENCLAVES 200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we don't have a robust way to validate that all EPC is freed on enclave termination today, do you have a sense for how many simultaneous enclaves fit within existing ~92MB EPC limits (as configured by sign.conf)? Understanding that gives us an implicit bound for how much EPC we could be leaking after 200 sequential runs.

Copy link
Contributor Author

@gupta-ak gupta-ak Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EPC limit is hard to hit. SGX supports paging the EPC via the EWB instruction to evict EPC pages to main memory and ELDB/ELDU instructions for loading those pages back into EPC. The 32 max simultaneous enclaves already goes beyond the 92 MB limit, since each enclave is 4MB, giving 128MB total of EPC memory. I kept the numbers lower in this PR, so the test doesn't take too long, but I've tried much higher values (~1K simutaneous enclaves = 4GB of EPC) and I never failed to create them.

For the paging, the EWB instruction requires a slot in a Version Array EPC page (made via EPA). Each VA page can hold 512 slots, so I guess for an upper limit, it would be if the entire EPC page were Version Array Pages, which gives 92 MB / 4K per page * 512 slots * 4K page per slot = 46 GB limit. Or we might hit a limit in the linux sgx driver before that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linux sgx driver can't page out VA pages, so looks like people are hitting a limit at >40 GB for an 128 MB EPC (See intel/linux-sgx#87). I guess that will give us roughly 30 GB for a 92MB EPC? So, the limit is probably MIN(30 GB, # GB of RAM on the machine).

@anitagov anitagov merged commit 479ccac into master Jul 17, 2018
Public preview automation moved this from In review to Done Jul 17, 2018
@anitagov anitagov deleted the akagup/tests.repeat-enclave branch July 17, 2018 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Issue has to do with testing or quality management
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants