[LibOS,Pal,common,tools] Improved protected files performance #1681
Conversation
Improved performance with the following changes: - Mapped encrypted file into untrusted memory to replace reading or writing a single node. - Removed `encrypted` part inside `file_node_t` structure. The encrypted content is directly retrived from the file mapped memroy. Signed-off-by: Yao Ji <[email protected]>
There was a problem hiding this comment.
Reviewed 1 of 13 files at r1, all commit messages.
Reviewable status: 1 of 13 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @yao-ji)
common/src/protected_files/protected_files.c line 247 at r1 (raw file):
// 2. set the IV+GMAC in the parent MHT // [3. set the need_writing flag for all the parents] data = lruc_get_first(pf->cache);
Can we save data of the first lruc_get_first(pf->cache); at the beginning of this function to avoid calling it again? I see a lot of them in this function.
common/src/protected_files/protected_files.c line 272 at r1 (raw file):
} data_node->need_writing = false;
I suppose all data_node->need_writing are set to false. Followingdirty_count will always be 0;
There was a problem hiding this comment.
Reviewable status: 1 of 13 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @llly)
common/src/protected_files/protected_files.c line 272 at r1 (raw file):
Previously, llly (Li Xun) wrote…
I suppose all
data_node->need_writingare set tofalse. Followingdirty_countwill always be 0;
Here we only write nodes withFILE_DATA_NODE_TYPE, while the following dirty_count records nodes with FILE_MHT_NODE_TYPE.
There was a problem hiding this comment.
Reviewed 12 of 13 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @yao-ji)
common/src/protected_files/protected_files.c line 272 at r1 (raw file):
Previously, yao-ji wrote…
Here we only write nodes with
FILE_DATA_NODE_TYPE, while the followingdirty_countrecords nodes withFILE_MHT_NODE_TYPE.
Get it. Then one while loop is enough to handle both types.
libos/src/fs/libos_fs_encrypted.c line 738 at r1 (raw file):
assert(enc->pal_handle); /* Recreate file map: set handle->file.addr to NULL and call PalStreamSetLength */
Better to zero handle->file.addr in handle_deserialize function, then these code and pal_attr.addr are not needed.
pal/include/pal/pal.h line 329 at r1 (raw file):
#define PAL_OPTION_NONBLOCK 0x2 #define PAL_OPTION_PASSTHROUGH 0x4 /*!< Disregard `sgx.{allowed,trusted}_files` */ #define PAL_OPTION_ENCRYPTED_FILE 0x8 /*!< Open encrypted file */
Suggest name PAL_OPTION_MAP_FILE, which means will map the file to untrusted memory when opening.
pal/src/host/linux-sgx/pal_files.c line 417 at r1 (raw file):
return unix_to_pal_error(ret); }
Remove spaces
pal/src/host/linux-sgx/pal_host.h line 53 at r1 (raw file):
char* realpath; size_t total; bool encrypted; /* flag to indicate encrypted files */
Suggest name mapped, which means whether need to map file to untrusted memory.
pal/src/host/linux-sgx/pal_host.h line 54 at r1 (raw file):
size_t total; bool encrypted; /* flag to indicate encrypted files */ void* addr; /* mapped address, used only for encrypted files */
Can we reuse void* umem; below? It is also address mmapped in untrusted memory.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @llly and @yao-ji)
a discussion (no related file):
I strongly insist on keeping the old implementation too. In particular, pf_read_f() and pf_write_f() callbacks must be kept.
This has two important benefits:
- I am currently very unsure about the security implications of this perf optimization. See my other comment.
- Keeping the old implementation will allow to introduce a manifest switch
sys.encrypted_files.experimental__zero_copy = true|false. - So that users can always choose the old implementation, which is known to be secure and battle tested.
- In some future, when we are sure about the implementation and its security, we can remove this switch and use mmapped-based zero-copy optimization always.
- Keeping the old implementation will allow to introduce a manifest switch
- This will allow no changes to
tools/sgx/common/pf_util.c. This tooling is not performance critical, so I would prefer to not touch it, at least for now.- We won't need any modifications to
pf_util.c. This will ease the review of this PR. - In some future, we may want to replace that implementation too. But currently there is no need.
- We won't need any modifications to
a discussion (no related file):
I propose to keep pf_read_f() and pf_write_f() callbacks.
With this in mind, why can't we encapsulate the mmap-based zero-copy optimization inside these two callbacks? This will tremendously simplify this PR. (We will still need to propagate the addr at which the file was mapped, but that's much less intrusive changes than the current state.)
-- commits line 8 at r1:
memory
common/src/protected_files/protected_files.c line 266 at r1 (raw file):
status = g_cb_aes_gcm_encrypt(&gcm_crypto_data->key, &g_empty_iv, NULL, 0, // aad data_node->decrypted.data.data, PF_NODE_SIZE, data_node_addr, &gcm_crypto_data->gmac);
This is scary!
You encrypt the input plaintext data (data_node->decrypted.data.data) directly into untrusted out-of-enclave memory (data_node_addr).
What is the guarantee that the encryption crypto doesn't accidentally keep some temporary data in this untrusted memory? The crypto logic is buried somewhere inside g_cb_aes_gcm_encrypt(), which in the end maps to mbedtls_gcm_crypt_and_tag():
gramine/libos/src/fs/libos_fs_encrypted.c
Lines 105 to 107 in e282ee5
gramine/common/src/crypto/adapters/mbedtls_adapter.c
Lines 147 to 149 in e282ee5
gramine/common/src/crypto/adapters/mbedtls_adapter.c
Lines 163 to 164 in e282ee5
Did you analyze the internal code of mbedtls_gcm_crypt_and_tag()? Or does the mbedTLS developer team provide such a guarantee?
Ditto for all other such places (both encryption and decryption of data).
libos/src/fs/libos_fs_encrypted.c line 738 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Better to zero
handle->file.addrinhandle_deserializefunction, then these code andpal_attr.addrare not needed.
+1
pal/include/pal/pal.h line 329 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Suggest name
PAL_OPTION_MAP_FILE, which means will map the file to untrusted memory when opening.
Btw, this is what is done for sgx.trusted_files already. I feel like we need to consolidate the similar logic of trusted and encrypted (protected) files before going deep into this PR. In other words, I feel like we need a series of preliminary PRs that e.g. introduce this PAL_OPTION_MAP_FILE.
We need to discuss this more.
pal/src/host/linux/pal_files.c line 204 at r1 (raw file):
handle->file.addr = addr; handle->file.total = length; }
This is definitely a wrong place to do the remapping of encrypted files. I feel like this logic should be done inside LibOS, and using some additional existing PAL API, not PalStreamSetLength().
Otherwise we're abusing the PAL API. And just generally -- this is a bad programming practice and a potential WTF place.
We'll need to discuss how to implement that correctly.
pal/src/host/linux-sgx/pal_files.c line 112 at r1 (raw file):
} } }
Our file logic in PAL got tremendously complicated. Sorry, we need to untangle our existing code before I will accept this PR. We just pile hack on top of another hack.
I think we need to:
- First move trusted files logic inside LibOS (or at least in
common/code). - Make the logic of "mmap into untrusted memory" generic for both trusted and encrypted files.
- Make the distinction between three types of files (allowed, trusted and encrypted) very clear in PAL.
pal/src/host/linux-sgx/pal_files.c line 426 at r1 (raw file):
return unix_to_pal_error(ret); } }
ditto (we're abusing the PAL API)
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @llly, and @yao-ji)
a discussion (no related file):
why can't we encapsulate the mmap-based zero-copy optimization inside these two callbacks?
+1, I have the same question - @yao-ji: what is the blocker here?
-- commits line 8 at r1:
-> retrieved
Code quote:
retrived
common/src/protected_files/protected_files.c line 210 at r1 (raw file):
if (pf->addr == NULL) { pf->last_error = PF_STATUS_INVALID_ADDRESS;
-> PF_STATUS_INVALID_FILE_SIZE
Code quote:
PF_STATUS_INVALID_ADDRESScommon/src/protected_files/protected_files.c line 262 at r1 (raw file):
// encrypt the data, this also saves the gmac of the operation in the mht crypto // node void* data_node_addr = (unsigned char*)pf->addr +
pls remove trailing white space
common/src/protected_files/protected_files.c line 266 at r1 (raw file):
You encrypt the input plaintext data (
data_node->decrypted.data.data) directly into untrusted out-of-enclave memory (data_node_addr).
What is the guarantee that the encryption crypto doesn't accidentally keep some temporary data in this untrusted memory?
Yes, I think there is no guarantee there. It at least needs some thorough analysis, but better not to do this in-place.
common/src/protected_files/protected_files.c line 336 at r1 (raw file):
void* mht_node_addr = (unsigned char*)pf->addr + PF_NODE_SIZE * file_mht_node->physical_node_number;
pls align
libos/src/fs/libos_fs_encrypted.c line 738 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1
+1
pal/include/pal/pal.h line 329 at r1 (raw file):
this is what is done for
sgx.trusted_filesalready. I feel like we need to consolidate the similar logic of trusted and encrypted (protected) files before going deep into this PR
+1
pal/src/host/linux/pal_files.c line 204 at r1 (raw file):
I feel like this logic should be done inside LibOS, and using some additional existing PAL API, not
PalStreamSetLength().
+1
pal/src/host/linux-sgx/pal_host.h line 54 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Can we reuse
void* umem;below? It is also address mmapped in untrusted memory.
+1, I think we should discuss whether it's possible to consolidate the similar logic of trusted (where umem is used) and encrypted files.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin, @llly, and @yao-ji)
common/src/protected_files/protected_files.c line 266 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
You encrypt the input plaintext data (
data_node->decrypted.data.data) directly into untrusted out-of-enclave memory (data_node_addr).
What is the guarantee that the encryption crypto doesn't accidentally keep some temporary data in this untrusted memory?Yes, I think there is no guarantee there. It at least needs some thorough analysis, but better not to do this in-place.
Yes, better not to do this in-place. We can simply have a scratch (temporary) memory buffer of size PF_NODE_SIZE, and use it for all intermediate encryption/decryption steps.
This will lead to an additional memory copy, but at least the implementation will be security-safe. You'll also need to re-do the evaluation.
There was a problem hiding this comment.
Reviewable status: 5 of 15 files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @llly)
a discussion (no related file):
Previously, kailun-qin (Kailun Qin) wrote…
why can't we encapsulate the mmap-based zero-copy optimization inside these two callbacks?
+1, I have the same question - @yao-ji: what is the blocker here?
mmap can be moved inside read/write callbacks, but munmap cannot. It is clearer in logic to map/unmap file during file open/close.
After the fix, there is no security concern. I have updated the performance data. Adding a temp buffer makes around 3~6% downgrade, which I think is not significant. So we don't need to keep the old implementation.
common/src/protected_files/protected_files.c line 210 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
->
PF_STATUS_INVALID_FILE_SIZE
This is an address error. Why change to file size error?
common/src/protected_files/protected_files.c line 247 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Can we save
dataof the firstlruc_get_first(pf->cache);at the beginning of this function to avoid calling it again? I see a lot of them in this function.
This function changes internal pointer which is also used by lruc_get_next().
common/src/protected_files/protected_files.c line 262 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
pls remove trailing white space
Done.
common/src/protected_files/protected_files.c line 266 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, better not to do this in-place. We can simply have a scratch (temporary) memory buffer of size
PF_NODE_SIZE, and use it for all intermediate encryption/decryption steps.This will lead to an additional memory copy, but at least the implementation will be security-safe. You'll also need to re-do the evaluation.
Fixed by adding a temp buffer when doing encryption. Performance data is also updated, which indicates a 3~6% downgrade. My question is whether we have to do the same during decryption. The input untrusted memory is a const pointer.
common/src/protected_files/protected_files.c line 272 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Get it. Then one
whileloop is enough to handle both types.
data_node should be updated before mht_node, since it saves key and gmac data into parent mht_node.
common/src/protected_files/protected_files.c line 336 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
pls align
Done.
libos/src/fs/libos_fs_encrypted.c line 738 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
+1
Moved zero pointer logic into handle_deserialize function.
For now, we still need the remap logic. If we can come up with a PAL API that can do mmap/munmap untrusted memory, we can merge the remaining logic into encrypted_file_internal_open(), which makes more sense.
pal/src/host/linux/pal_files.c line 204 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I feel like this logic should be done inside LibOS, and using some additional existing PAL API, not
PalStreamSetLength().+1
Since map untrusted memory acts differently on Linux host and SGX host, I don't think it can be done inside LibOS using existing PAL API.
I think the better way is to create new PAL API to map/unmap untrusted memory. Then we can keep all the existing PAL file operations unchanged.
pal/src/host/linux-sgx/pal_files.c line 417 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Remove spaces
Done.
pal/src/host/linux-sgx/pal_host.h line 53 at r1 (raw file):
Previously, llly (Li Xun) wrote…
Suggest name
mapped, which means whether need to map file to untrusted memory.
Done.
There was a problem hiding this comment.
Reviewed 4 of 10 files at r2.
Reviewable status: 9 of 15 files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @yao-ji)
pal/include/pal/pal.h line 329 at r2 (raw file):
#define PAL_OPTION_NONBLOCK 0x2 #define PAL_OPTION_PASSTHROUGH 0x4 /*!< Disregard `sgx.{allowed,trusted}_files` */ #define PAL_OPTION_MAP_FILE 0x8 /*!< Map encrypted file to untrusted memory */
Better to remove encrypted. Pal can't handle and don't need to know file is encrypted or not.
Code quote:
Map encrypted filepal/src/host/linux/pal_files.c line 78 at r2 (raw file):
hdl->file.seekable = !S_ISFIFO(st.st_mode); /* map file into memroy when open encrypted files */
typo
Code quote:
memroy pal/src/host/linux-sgx/pal_files.c line 102 at r2 (raw file):
} /* map file into untrusted memroy when open encrypted files */
typo
Code quote:
memroy 
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, @llly, and @yao-ji)
a discussion (no related file):
Previously, yao-ji wrote…
mmapcan be moved inside read/write callbacks, butmunmapcannot. It is clearer in logic to map/unmap file during file open/close.After the fix, there is no security concern. I have updated the performance data. Adding a temp buffer makes around 3~6% downgrade, which I think is not significant. So we don't need to keep the old implementation.
I think mmap and munmap can be called just fine in libos open/close wrappers. The address can be kept within the structure pointed to by pf_handle_t. There is thus no need to modify the interface, or anything within common/src/protected_files/ at all.
Keeping the current interface in place also ensures some security by construction — since only the explicitly requested read/writes can touch the untrusted memory. The callbacks should also take care of using the proper helpers (sgx_copy_from_enclave/sgx_copy_to_enclave) to access untrusted memory.
There was a problem hiding this comment.
Reviewed 1 of 10 files at r2.
Reviewable status: 9 of 15 files reviewed, 20 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, @llly, and @yao-ji)
Suggestion:
Improve
-- commits line 2 at r2:
I'd just skip the component tag, it touches practically the whole codebase.
Suggestion:
This commit improves performance with the following changes:
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @llly, @mwkmwkmwk, and @yao-ji)
a discussion (no related file):
After the fix, there is no security concern. I have updated the performance data. Adding a temp buffer makes around 3~6% downgrade, which I think is not significant.
Thanks, at a first glance, looks good to me! Also the 3-6% drop in performance is tolerable.
So we don't need to keep the old implementation.
I disagree with this. I'd like to keep the old read/write callbacks inside protected_files/. At least for the benefit of pf_util.c. But yes, we don't need to new manifest option (sys.encrypted_files.experimental__zero_copy) that I proposed above.
The callbacks should also take care of using the proper helpers (
sgx_copy_from_enclave/sgx_copy_to_enclave) to access untrusted memory.
@mwkmwkmwk I'm confused what you mean by "the callbacks". Do you mean that libos_fs_encrypted.c code must replace current cb_read() and cb_write() implementations to perform memcpy (more precisely, SGX-specific memory copy untrusted<->trusted) instead of PalStreamRead() and PalStreamWrite()?
- If I understand your proposal correctly, then how do you plan to use
sgx_copy_from_enclave()/sgx_copy_to_enclave()inside the LibOS? That's prohibited (such things are encapsulated inside PAL). - Or maybe you mean that LibOS still calls
PalStreamRead()andPalStreamWrite()in itscb_read()andcb_write()callbacks, but these PAL implementations (on SGX PAL) see that there is untrusted memory as the source is already mmapped, so they will use that.
I like the second approach a lot. It also bodes well with Trusted Files.
If we can come up with a PAL API that can do
mmap/munmapuntrusted memory, we can merge the remaining logic intoencrypted_file_internal_open(), which makes more sense.
@yao-ji Why do we need to come up with a separate API? You introduced the PAL_OPTION_MAP_FILE flag, so why can't we do PalStreamMap(..., PAL_OPTION_MAP_FILE)? That seems enough.
I'd like to discuss and agree on the design and implementation steps during the next Gramine meeting. This async discussion is too slow and haphazard.
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, @llly, and @yao-ji)
a discussion (no related file):
Do you mean that
libos_fs_encrypted.ccode must replace currentcb_read()andcb_write()implementations to perform memcpy (more precisely, SGX-specific memory copy untrusted<->trusted) instead ofPalStreamRead()andPalStreamWrite()?
The biggest point here is that currently the PR just copies untrusted memory in and out with memcpy() instead of proper wrappers, which is insecure because of e.g. CVE-2022-21166. And the proposed solution is to design the code differently and go through the read/write operations, which will call PalStreamRead() and PalStreamWrite() respectively, which know how to handle the untrusted memory. And I think this is what you described in the second point.
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @llly, @mkow, and @yao-ji)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Do you mean that
libos_fs_encrypted.ccode must replace currentcb_read()andcb_write()implementations to perform memcpy (more precisely, SGX-specific memory copy untrusted<->trusted) instead ofPalStreamRead()andPalStreamWrite()?The biggest point here is that currently the PR just copies untrusted memory in and out with
memcpy()instead of proper wrappers, which is insecure because of e.g. CVE-2022-21166. And the proposed solution is to design the code differently and go through the read/write operations, which will callPalStreamRead()andPalStreamWrite()respectively, which know how to handle the untrusted memory. And I think this is what you described in the second point.
@mkow Yes, I understand the problem with memcpy(). I agree with calling PalStreamRead() and PalStreamWrite() and doing proper copying inside of them. This will look very much like what Trusted Files do.
Anyway, we should discuss this PR and alternative design next week: #1772
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @llly, @mkow, and @yao-ji)
common/src/protected_files/protected_files.c line 273 at r2 (raw file):
} memcpy(data_node_addr, temp_enc_node, PF_NODE_SIZE);
All these memcpy should be sgx_copy_from_enclave() or sgx_copy_to_enclave().
Here and everywhere where we copy untrusted <-> trusted.
(But as mentioned in a top-level comment by @mwkmwkmwk and @mkow, it's worth trying a different design with read/write callbacks, and not here.)
common/src/protected_files/protected_files.c line 699 at r2 (raw file):
status = g_cb_aes_gcm_decrypt(&gcm_crypto_data->key, &g_empty_iv, NULL, 0, file_data_node_addr, PF_NODE_SIZE, file_data_node->decrypted.data.data, &gcm_crypto_data->gmac);
Why didn't you introduce a bounce buffer (temp_enc_node) here?
common/src/protected_files/protected_files.c line 770 at r2 (raw file):
status = g_cb_aes_gcm_decrypt(&gcm_crypto_data->key, &g_empty_iv, NULL, 0, file_mht_node_addr, PF_NODE_SIZE, &file_mht_node->decrypted.mht, &gcm_crypto_data->gmac);
Why didn't you introduce a bounce buffer (temp_enc_node) here?
common/src/protected_files/protected_files.c line 895 at r2 (raw file):
(unsigned char*)pf->addr + PF_NODE_SIZE, PF_NODE_SIZE, &pf->root_mht.decrypted.mht, &pf->encrypted_part_plain.mht_gmac);
Why didn't you introduce a bounce buffer (temp_enc_node) here?
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @llly, @mkow, and @yao-ji)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I strongly insist on keeping the old implementation too. In particular,
pf_read_f()andpf_write_f()callbacks must be kept.This has two important benefits:
- I am currently very unsure about the security implications of this perf optimization. See my other comment.
- Keeping the old implementation will allow to introduce a manifest switch
sys.encrypted_files.experimental__zero_copy = true|false.- So that users can always choose the old implementation, which is known to be secure and battle tested.
- In some future, when we are sure about the implementation and its security, we can remove this switch and use mmapped-based zero-copy optimization always.
- This will allow no changes to
tools/sgx/common/pf_util.c. This tooling is not performance critical, so I would prefer to not touch it, at least for now.
- We won't need any modifications to
pf_util.c. This will ease the review of this PR.- In some future, we may want to replace that implementation too. But currently there is no need.
UPDATE: I just realized that mmap() may be detrimental for performance in certain cases, like very large RW files. I recall some users working on up to 1TB encrypted files, so having an additional 1TB mmapped in untrusted memory feels like too much.
So I think we still need an explicit manifest option, to switch from read/write to mmap.
See https://stackoverflow.com/questions/45972/mmap-vs-reading-blocks/151819#151819
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 24 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin, @llly, @mkow, and @yao-ji)
a discussion (no related file):
@yao-ji @kailun-qin What is the status of this PR?
I believe this PR should go after the Trusted Files re-implementation (moving Trusted Files from PAL to LibOS). This reimplementation is done in PR #1812, so I would recommend to base the new version of this PR on top of #1812. This will save us from complex rebases afterwards.
Also, do you have the design ironed out now? Please re-check the discussion that we had in #1772.
Description of the changes
Fixes #1599
LibOS
cb_read,cb_writecallback. Addcb_truncatecallback to truncate and remap file when file size changed.rs_encrypted_filefunction.Pal
PAL_OPTION_ENCRYPTED_FILEto indicate opening an encrypted file when usingPalStreamOpen.encryptedand a pointeraddrstoring mapping address to the fields ofPAL_HANDLE.addrinPAL_STREAM_ATTR.file_open, map the file and store address inaddrfield. Unmap the file infile_destory. Remap the file infile_setlength.addrfield infile_attrsetbyhdlandfile_attrquerybyhdl.common
Improve performance of
protected fileswith the following changes:memcpyfrom/to the mapping address.encryptedpart insidefile_node_tstructure. The encrypted content is directly retrived from the file mapped memory.tools
Modify
pf_utilsimilar to the changes in LibOS and Pal.Performance test
In release mode, read is around 60% faster, and write is around 35% faster.
Read
Write
How to test this PR?
Using LibOS and Pal regression test.
gramine-sgx-pf-cryptandgramine-sgx-pf-tampercan be used to testpf_util.Performance test result is from iozone using
fread/fwrite.This change is