[LibOS,common] Add file recovery support for encrypted files #2082
Conversation
|
Jenkins, retest Jenkins-Direct-24.04-Debug please (fdatasync01 from LTP timed out, known and unrelated to the PR) |
|
|
||
| /* Whether to enable file recovery (used by `chroot_encrypted` filesystem), false if not | ||
| * applicable */ | ||
| bool enable_recovery; |
There was a problem hiding this comment.
what is the behavior if recovery is enabled, disabled and re-enabled?
do you remove the old shadow files when mounting?
There was a problem hiding this comment.
In the first "recovery enabled" run, if the app terminates abruptly, a shadow file will be generated. If recovery is "disabled" in the next run, the shadow file will remain and will not be accessed. When recovery is "re-enabled" in a subsequent run, the recovery file will not be removed upon mounting. However, it will be overwritten during flush() and removed upon closing.
There was a problem hiding this comment.
I think you can't assume that you can replay the shadow file once the flag is turned off, if new data is written to the same offset in the file when the flag is off and then you re-enable it - the file won't be consistent.
There was a problem hiding this comment.
Hmm... Good point. Do we consider this a legitimate usage? If so, alternatively, we could use enable_recovery to control whether a backup is needed on flush, but still perform the file recovery process as long as the update_flag is set (i.e., we restore to the last known good state even during a "disabled" run if a previous run was abruptly terminated).
There was a problem hiding this comment.
I think a better approach is either not allowing non-recoverable mounts after mounting once with recovery, or removing the shadow files in the above scenario since the user has (hopefully knowingly) disabled recovery.
There was a problem hiding this comment.
Thanks for the input! I go w/ the first approach -- not allowing non-recoverable mounts if a recovery is needed.
| * | ||
| * `uri` must not correspond to an existing file. | ||
| * | ||
| * The newly created `libos_encrypted_file` object will have `use_count` set to 1. | ||
| */ | ||
| int encrypted_file_create(const char* uri, mode_t perm, struct libos_encrypted_files_key* key, | ||
| struct libos_encrypted_file** out_enc); | ||
| bool enable_recovery, struct libos_encrypted_file** out_enc); |
There was a problem hiding this comment.
you may want to have some const fs_configuration struct that's passed around, assuming more flags are going to be introduced in the future
There was a problem hiding this comment.
Yeah, I'm okay with having a fs_configuration struct, but currently, we only have this enable_recovery option.
There was a problem hiding this comment.
I left it as is for now.
| } | ||
|
|
||
| for (size_t i = 0; i < nodes_count; i++) { | ||
| ret = read_all(recovery_file_fd, recovery_node, recovery_node_size); |
There was a problem hiding this comment.
Is there an upper bound for the amount of data written in a shadow file?
do you attempt to allocate memory for and read from the full shadow file here?
There was a problem hiding this comment.
Is there an upper bound for the amount of data written in a shadow file?
The upper bound for the data that can be written to a shadow file should be the same as on a typical Linux system. This is determined by e.g., the fs type, available disk space, and the maximum file size supported by the fs.
do you attempt to allocate memory for and read from the full shadow file here?
Sorry, I don't understand this concern. Pls note that this piece of code is on the untrusted side of Gramine.
There was a problem hiding this comment.
If I understand correctly, there's a malloc call which might attempt to allocate TBs if the size is not limited
There was a problem hiding this comment.
Ah, I understand your concern now. I was referring to the theoretical upper bound earlier. Actually, during each flush, only the data that is about to change in the encrypted files cache (which has a default size of 192KB as specified here) will be saved and rewritten to the recovery file.
There was a problem hiding this comment.
Reviewed 26 of 27 files at r1, all commit messages.
Reviewable status: 26 of 27 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @ynonflumintel)
libos/src/fs/libos_fs_encrypted.c line 277 at r1 (raw file):
if (recovery_needed) { log_warning("file recovery needed but failed");
suggest changing the message to 'file recovery attempted but failed' for clarity.
libos/src/fs/libos_fs_encrypted.c line 328 at r1 (raw file):
if (enc->recovery_file_pal_handle) (void)PalStreamDelete(enc->recovery_file_pal_handle, PAL_DELETE_ALL);
wondering if PalStreamDelete(enc->recovery_file_pal_handle, ..) should be invoked as well when pf_close() fails above?
common/src/protected_files/protected_files.c line 452 at r1 (raw file):
assert(pf->host_recovery_file_handle); uint64_t offset = 0;
nitpicking: maybe moving the 'offset' declaration a few lines back, together with 'node'
common/src/protected_files/protected_files.c line 522 at r1 (raw file):
pf->file_status = PF_STATUS_FLUSH_ERROR; DEBUG_PF("failed to write changes to the recovery file"); return false;
maybe use "goto recoverable_error;" instead for consistency?
common/src/protected_files/protected_files.c line 528 at r1 (raw file):
pf->file_status = PF_STATUS_FLUSH_ERROR; DEBUG_PF("failed to set the update flag"); return false;
same as above
There was a problem hiding this comment.
Reviewed 8 of 27 files at r1, all commit messages.
Reviewable status: 26 of 27 files reviewed, 20 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @ynonflumintel)
a discussion (no related file):
We probably should benchmark this before merging (see #2013 (comment)).
a discussion (no related file):
Do I understand correctly (at least looking at #2013 (comment)) that the recovery file stacks all changes that happened while the file was open? This may be a lot for long-running enclaves, even if the file on the disk is itself small, but often modified?
common/src/protected_files/protected_files.h line 222 at r1 (raw file):
* \param create Overwrite file contents if true. * \param key Wrap key. * \param recovery_file_handle (optional)Underlying recovery file handle.
Suggestion:
(optional) Underlying recovery file handle.common/src/protected_files/protected_files.h line 313 at r1 (raw file):
* \param pf PF context. * \param[out] recovery_needed (optional)Whether recovery is needed for \p pf. * \param[out] pos_size (optional)Size of the \p pf node position.
I don't understand this term, "size of position" - is this the size in bytes of a number indicating the position in the file? But that doesn't make much sense.
Code quote:
Size of the \p pf node position.common/src/protected_files/protected_files.h line 314 at r1 (raw file):
* \param[out] recovery_needed (optional)Whether recovery is needed for \p pf. * \param[out] pos_size (optional)Size of the \p pf node position. * \param[out] node_size (optional)Size of the \p pf node data.
ditto (space)
common/src/protected_files/protected_files.h line 319 at r1 (raw file):
*/ pf_status_t pf_get_recovery_info(pf_context_t* pf, bool* recovery_needed, size_t* pos_size, size_t* node_size);
please add out_ prefix to the out arguments
common/src/protected_files/protected_files_format.h line 59 at r1 (raw file):
pf_nonce_t metadata_key_nonce; pf_mac_t metadata_mac; /* GCM mac */ uint8_t update_flag; /* for file recovery */
https://gramine.readthedocs.io/en/latest/devel/encfiles.html will need an update
common/src/protected_files/protected_files_format.h line 59 at r1 (raw file):
pf_nonce_t metadata_key_nonce; pf_mac_t metadata_mac; /* GCM mac */ uint8_t update_flag; /* for file recovery */
or something similar, update_flag sounds very unclear what it means
Suggestion:
has_pending_writepal/include/pal/pal.h line 1050 at r1 (raw file):
* * \param handle Handle to the file. * \param handle Handle to the recovery file.
Parameters don't match the signature.
pal/include/pal/pal.h line 1051 at r1 (raw file):
* \param handle Handle to the file. * \param handle Handle to the recovery file. * \param pos_size Size of the pf node position.
ditto, what's a "size of a position"?
Code quote:
Size of the pf node position.pal/include/pal/pal.h line 1056 at r1 (raw file):
* \returns 0 on success, negative error code on failure. */ int PalEncryptedFileRecovery(PAL_HANDLE file_handle, PAL_HANDLE recovery_file_handle,
This sounds like a bad function name, there's no verb in it and I was confused what it actually does before reading the more detailed documentation.
Code quote:
int PalEncryptedFileRecovery(Documentation/manifest-syntax.rst line 1158 at r1 (raw file):
The ``enable_recovery`` mount parameter determines whether file recovery is enabled for the mount. If omitted, it defaults to ``false``. This feature allows
Please be concise - instead of this just add "(default: false)" after the first mention of this option.
Code quote:
If omitted, it defaults to ``false``. 
kailun-qin
force-pushed
the
kailun-qin/add-encrypted-file-recovery
branch
2 times, most recently
from
00a90f3 to
4f28995
Compare
There was a problem hiding this comment.
Reviewable status: 7 of 34 files reviewed, 20 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @efu39, @mkow, and @ynonflumintel)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Do I understand correctly (at least looking at #2013 (comment)) that the recovery file stacks all changes that happened while the file was open? This may be a lot for long-running enclaves, even if the file on the disk is itself small, but often modified?
No, the recovery file is limited to the current write transaction. During each file flush, only the cached blocks about to change are saved to the recovery file. The recovery file is then truncated and rewritten with the latest pending changes.
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
We probably should benchmark this before merging (see #2013 (comment)).
Sure, I'll run some micro and macro benchmarks later and share the results here.
common/src/protected_files/protected_files.h line 313 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I don't understand this term, "size of position" - is this the size in bytes of a number indicating the position in the file? But that doesn't make much sense.
Yes, it is exactly the size in bytes of a number indicating the position in the file. I made it initially for flexibility, but I have now removed it for clarity. Also added some comments in recover_encrypted_file().
common/src/protected_files/protected_files.h line 314 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto (space)
Done.
common/src/protected_files/protected_files.h line 319 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
please add
out_prefix to the out arguments
Done.
common/src/protected_files/protected_files.c line 452 at r1 (raw file):
Previously, efu39 (Erica Fu) wrote…
nitpicking: maybe moving the 'offset' declaration a few lines back, together with 'node'
Done.
common/src/protected_files/protected_files.c line 522 at r1 (raw file):
Previously, efu39 (Erica Fu) wrote…
maybe use "goto recoverable_error;" instead for consistency?
Done.
common/src/protected_files/protected_files.c line 528 at r1 (raw file):
Previously, efu39 (Erica Fu) wrote…
same as above
Done.
common/src/protected_files/protected_files_format.h line 59 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
https://gramine.readthedocs.io/en/latest/devel/encfiles.html will need an update
Done.
common/src/protected_files/protected_files_format.h line 59 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
or something similar,
update_flagsounds very unclear what it means
Done.
Documentation/manifest-syntax.rst line 1158 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Please be concise - instead of this just add "(default: false)" after the first mention of this option.
Done.
libos/src/fs/libos_fs_encrypted.c line 277 at r1 (raw file):
Previously, efu39 (Erica Fu) wrote…
suggest changing the message to 'file recovery attempted but failed' for clarity.
Done.
libos/src/fs/libos_fs_encrypted.c line 328 at r1 (raw file):
Previously, efu39 (Erica Fu) wrote…
wondering if PalStreamDelete(enc->recovery_file_pal_handle, ..) should be invoked as well when pf_close() fails above?
I intentionally made it this way; added a comment there.
pal/include/pal/pal.h line 1050 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Parameters don't match the signature.
Done.
pal/include/pal/pal.h line 1051 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto, what's a "size of a position"?
Not relevant any more.
pal/include/pal/pal.h line 1056 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This sounds like a bad function name, there's no verb in it and I was confused what it actually does before reading the more detailed documentation.
Done.
common/src/protected_files/protected_files.h line 222 at r1 (raw file):
* \param create Overwrite file contents if true. * \param key Wrap key. * \param recovery_file_handle (optional)Underlying recovery file handle.
Done.
|
|
||
| /* Whether to enable file recovery (used by `chroot_encrypted` filesystem), false if not | ||
| * applicable */ | ||
| bool enable_recovery; |
There was a problem hiding this comment.
Thanks for the input! I go w/ the first approach -- not allowing non-recoverable mounts if a recovery is needed.
| * | ||
| * `uri` must not correspond to an existing file. | ||
| * | ||
| * The newly created `libos_encrypted_file` object will have `use_count` set to 1. | ||
| */ | ||
| int encrypted_file_create(const char* uri, mode_t perm, struct libos_encrypted_files_key* key, | ||
| struct libos_encrypted_file** out_enc); | ||
| bool enable_recovery, struct libos_encrypted_file** out_enc); |
There was a problem hiding this comment.
I left it as is for now.
kailun-qin
force-pushed
the
kailun-qin/add-encrypted-file-recovery
branch
from
4f28995 to
0eb5ec3
Compare
Previously, a fatal error during writes to encrypted files could cause file corruption due to incorrect GMACs and/or encryption keys. To address this, we introduce a file recovery mechanism using a "shadow" recovery file that stores data about to change and a `has_pending_write` flag in the metadata node indicating the start of a write transaction. During file flush, all cached blocks that are about to change are saved to the recovery file in the format of physical node numbers (offsets) plus encrypted block data. Before saving the main file contents, the `has_pending_write` flag is set in the file's metadata node and cleared only when the transaction is complete. If an encrypted file is opened and the `has_pending_write` flag is set, a recovery process starts to revert partial changes using the recovery file, returning to the last known good state. The "shadow" recovery file is cleaned up on file close. This commit adds a new mount parameter `enable_recovery = [true|false]` for encrypted files mounts to optionally enable this feature. We extend the file flush logic of protected files (pf) to include the recovery file dump and the setting/unsetting of the update flag. We make changes to the public pf APIs: the `pf_open()` API is extended to make the pf aware of the underlying recovery file managed by the LibOS, and recovery information (e.g., whether the pf needs recovery) is exposed back to the LibOS via a new `pf_get_recovery_info()` API. To facilitate the LibOS to initiate a file recovery process on file open, a new PAL API `PalRecoverEncryptedFile()` is introduced. Signed-off-by: Kailun Qin <[email protected]>
kailun-qin
force-pushed
the
kailun-qin/add-encrypted-file-recovery
branch
from
0eb5ec3 to
9d29158
Compare
There was a problem hiding this comment.
Reviewed 14 of 27 files at r2, all commit messages.
Reviewable status: 21 of 34 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @efu39, @kailun-qin, and @ynonflumintel)
a discussion (no related file):
Previously, kailun-qin (Kailun Qin) wrote…
No, the recovery file is limited to the current write transaction. During each file flush, only the cached blocks about to change are saved to the recovery file. The recovery file is then truncated and rewritten with the latest pending changes.
Ah. Could you update the comment I liked to, then? I think it's missing that step.
pal/include/pal/pal.h line 1050 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Done.
Not done?
a discussion (no related file):
Could you also update the PR description with update_flag -> has_pending_write?
Suggestion:
`has_pending_write` flag
There was a problem hiding this comment.
Reviewed 1 of 27 files at r2.
Reviewable status: 22 of 34 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @efu39, @kailun-qin, and @ynonflumintel)
pal/src/host/linux-common/file_utils.c line 221 at r2 (raw file):
} ret = write_all(file_fd, recovery_node + sizeof(uint64_t), node_size);
Why not using struct recovery_node_t here? You kind of hardcode its layout here anyways and changing the layout of struct recovery_node_t will silently break this code.
common/src/protected_files/protected_files.c line 1416 at r2 (raw file):
if (out_node_size) *out_node_size = sizeof(((recovery_node_t*)0)->bytes);
Why is this parameter dynamic? Don't we hardcode the node size anyways, in the PF implementation?
kailun-qin
changed the title
!TODO: use below commit msg [LibOS,common] Add file recovery support for encrypted files Previously, a fatal error during writes to encrypted files could cause file corruption due to incorrect GMACs and/or encryption keys. To address this, we introduce a file recovery mechanism using a "shadow" recovery file that stores data about to change and a `has_pending_write` flag in the metadata node indicating the start of a write transaction. During file flush, all cached blocks that are about to change are saved to the recovery file in the format of physical node numbers (offsets) plus encrypted block data. Before saving the main file contents, the `has_pending_write` flag is set in the file's metadata node and cleared only when the transaction is complete. If an encrypted file is opened and the `has_pending_write` flag is set, a recovery process starts to revert partial changes using the recovery file, returning to the last known good state. The "shadow" recovery file is cleaned up on file close. This commit adds a new mount parameter `enable_recovery = [true|false]` for encrypted files mounts to optionally enable this feature. We extend the file flush logic of protected files (pf) to include the recovery file dump and the setting/unsetting of the `has_pending_write` flag. We also extend `pf_open()` to make the pf aware of the underlying recovery file managed by LibOS, and to include an optional recovery check and initiate recovery if needed. Signed-off-by: Kailun Qin <[email protected]>
There was a problem hiding this comment.
Reviewable status: 16 of 34 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @efu39, @kailun-qin, @mkow, and @ynonflumintel)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Ah. Could you update the comment I liked to, then? I think it's missing that step.
Done.
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you also update the PR description with
update_flag->has_pending_write?
Done.
common/src/protected_files/protected_files.c line 1416 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why is this parameter dynamic? Don't we hardcode the node size anyways, in the PF implementation?
Not relevant any more.
pal/include/pal/pal.h line 1050 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Not done?
Not relevant any more.
pal/src/host/linux-common/file_utils.c line 221 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why not using
struct recovery_node_there? You kind of hardcode its layout here anyways and changing the layout ofstruct recovery_node_twill silently break this code.
Yeah, but struct recovery_node_t is defined in the generic pf lib, which is currently decoupled from PAL. I rethought a bit the design and implemented the logic entirely within the common pf. See if you like this better (or not).
-- commits line 23 at r2:
Done.
kailun-qin
force-pushed
the
kailun-qin/add-encrypted-file-recovery
branch
from
fbd5c2a to
9b203b7
Compare
There was a problem hiding this comment.
Reviewed 5 of 27 files at r1, 18 of 18 files at r3, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @efu39, @kailun-qin, and @ynonflumintel)
pal/src/host/linux-common/file_utils.c line 221 at r2 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Yeah, but
struct recovery_node_tis defined in the generic pf lib, which is currently decoupled from PAL. I rethought a bit the design and implemented the logic entirely within the common pf. See if you like this better (or not).
Looks better now, IMO.
libos/test/fs/manifest.template line 14 at r3 (raw file):
{ type = "encrypted", path = "/tmp/enc_input", uri = "file:tmp/enc_input" }, { type = "encrypted", path = "/tmp/enc_output", uri = "file:tmp/enc_output", enable_recovery = true },
Does this test actually tests this feature? If I didn't miss anything, this file is used only in a test which only writes to this file once, so not much will be tested?
Could we have a specialized test which also tests the recovery system?
libos/src/fs/libos_fs_encrypted.c line 182 at r3 (raw file):
if (enc->enable_recovery) { const char* recovery_file_suffix = "_gramine_recovery";
I'd rather append such a suffix, otherwise it will look like concatenated to the original extension.
Suggestion:
".gramine_recovery"libos/src/fs/libos_fs_encrypted.c line 193 at r3 (raw file):
memcpy(recovery_file_uri, enc->uri, uri_len); memcpy(recovery_file_uri + uri_len, recovery_file_suffix, suffix_len); recovery_file_uri[uri_len + suffix_len] = '\0';
These ten lines can be all replaced by just:
char* recovery_file_uri = alloc_concat(enc->uri, -1, ".gramine_recovery", -1);
if (!recovery_file_uri) {
ret = -ENOMEM;
goto out;
}libos/src/fs/libos_fs_encrypted.c line 311 at r3 (raw file):
PalObjectDestroy(enc->recovery_file_pal_handle); enc->recovery_file_pal_handle = NULL; return;
unrelated to the PR, but we can remove this useless return while we're here
common/src/protected_files/protected_files.h line 223 at r3 (raw file):
* \param key Wrap key. * \param recovery_file_handle (optional) Underlying recovery file handle. * \param recovery_file_size Recovery file size.
Is this also optional? If so, what value should it have to mark it as "not set"? Also, why is this an argument? We already pass a handle to the file, why it is the caller who's responsible for retrieving the file size, not this function? Doesn't seem very natural.
update: is this because there's no cb_getsize in pf callbacks?
common/src/protected_files/protected_files.h line 232 at r3 (raw file):
pf_file_mode_t mode, bool create, const pf_key_t* key, pf_handle_t recovery_file_handle, uint64_t recovery_file_size, bool try_cover, pf_context_t** context);
It's a bit worrying that this compiled without any warnings. Any idea why wasn't this detected? I thought there was a warning for that? (mismatch between a declaration and the definition)
Suggestion:
bool try_recovercommon/src/protected_files/protected_files.c line 449 at r3 (raw file):
} static bool ipf_write_recovery_file(pf_context_t* pf) {
This way it's IMO more direct about what it does. When I started reading it I was wondering how does it know which nodes to dump if it only gets pf as an argument.
Suggestion:
ipf_dump_dirty_cache_to_recovery_filecommon/src/protected_files/protected_files.c line 496 at r3 (raw file):
static bool ipf_clear_pending_write(pf_context_t* pf) { assert(pf->metadata_node.plaintext_part.has_pending_write == 0);
It's a bit weird now, that this function may assert-fail if it's called right after ipf_check_recovery_needed() (because it loads the disk node to memory). Maybe ipf_check_recovery_needed() should read the flag from disk, but clear again from the in-memory copy?
common/src/protected_files/protected_files.c line 980 at r3 (raw file):
} size_t recovery_nodes_count = recovery_file_size / sizeof(recovery_node_t);
This is disk size, not memory size.
Suggestion:
uint64_tcommon/src/protected_files/protected_files.c line 980 at r3 (raw file):
} size_t recovery_nodes_count = recovery_file_size / sizeof(recovery_node_t);
Hmm, if we just read until EOF then we wouldn't need to pass the size through the whole hierarchy?
common/src/protected_files/protected_files.c line 982 at r3 (raw file):
size_t recovery_nodes_count = recovery_file_size / sizeof(recovery_node_t); for (size_t i = 0; i < recovery_nodes_count; i++) {
Suggestion:
uint64_tcommon/src/protected_files/protected_files.c line 992 at r3 (raw file):
} size_t offset = recovery_node.physical_node_number;
Just to keep that in mind. I don't think it can lead to any problems, but I prefer to keep untrusted values clearly marked.
Suggestion:
uint64_t untrusted_offsetcommon/src/protected_files/protected_files.c line 1057 at r3 (raw file):
if (!create) { if (!ipf_init_existing_file(pf, path)) goto out;
missing last_error setting?
common/src/protected_files/protected_files.c line 1063 at r3 (raw file):
if (!ipf_recover(pf, recovery_file_size)) goto out;
Missing last_error setting? Also, why no message here, but a message in the if below?
Signed-off-by: Kailun Qin <[email protected]>
There was a problem hiding this comment.
Reviewable status: 31 of 34 files reviewed, 23 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @efu39, @kailun-qin, @mkow, and @ynonflumintel)
common/src/protected_files/protected_files.h line 223 at r3 (raw file):
Is this also optional? If so, what value should it have to mark it as "not set"?
Yes, added a comment.
is this because there's no
cb_getsizein pf callbacks?
Yes, we did the same for handle and underlying_size.
common/src/protected_files/protected_files.h line 232 at r3 (raw file):
Good catch, thanks.
Any idea why wasn't this detected?
Well, in the C standards, the identifiers of parameters in a function declaration are optional while the types are what really matter.
I thought there was a warning for that? (mismatch between a declaration and the definition)
Yeah, but it's not built into the compiler. Instead, it's available in some static analysis tools, e.g., clang-tidy https://clang.llvm.org/extra/clang-tidy/checks/readability/inconsistent-declaration-parameter-name.html.
common/src/protected_files/protected_files.c line 449 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This way it's IMO more direct about what it does. When I started reading it I was wondering how does it know which nodes to dump if it only gets
pfas an argument.
Done.
common/src/protected_files/protected_files.c line 496 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
It's a bit weird now, that this function may assert-fail if it's called right after
ipf_check_recovery_needed()(because it loads the disk node to memory). Maybeipf_check_recovery_needed()should read the flag from disk, but clear again from the in-memory copy?
The check is only used when initializing an existing file. I've now inlined the check, moved it earlier, and ensured the flag is cleared in the in-mem copy.
common/src/protected_files/protected_files.c line 980 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Hmm, if we just read until EOF then we wouldn't need to pass the size through the whole hierarchy?
Well, yes. But reading and applying recovery nodes one by one risks main file corruption if the recovery file size is incorrect. Alternatively, we can dynamically allocate memory for all recovery nodes, read until EOF and validate them as a whole, and then apply them, but this sounds a bit overkill.
common/src/protected_files/protected_files.c line 980 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This is disk size, not memory size.
Done.
common/src/protected_files/protected_files.c line 992 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Just to keep that in mind. I don't think it can lead to any problems, but I prefer to keep untrusted values clearly marked.
Done.
common/src/protected_files/protected_files.c line 1057 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
missing
last_errorsetting?
The last error is already set in ipf_init_existing_file().
common/src/protected_files/protected_files.c line 1063 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Missing
last_errorsetting? Also, why no message here, but a message in the if below?
Not relevant any more.
libos/src/fs/libos_fs_encrypted.c line 182 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd rather append such a suffix, otherwise it will look like concatenated to the original extension.
Done.
libos/src/fs/libos_fs_encrypted.c line 193 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
These ten lines can be all replaced by just:
char* recovery_file_uri = alloc_concat(enc->uri, -1, ".gramine_recovery", -1); if (!recovery_file_uri) { ret = -ENOMEM; goto out; }
Done.
libos/src/fs/libos_fs_encrypted.c line 311 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
unrelated to the PR, but we can remove this useless return while we're here
Done.
libos/test/fs/manifest.template line 14 at r3 (raw file):
Does this test actually tests this feature? If I didn't miss anything, this file is used only in a test which only writes to this file once, so not much will be tested?
Only the extended flush logic, not the recovery flow.
Could we have a specialized test which also tests the recovery system?
Yeah, but it's challenging to test this automatically. Controlling when the test should crash abruptly to generate a valid dump with the pending write flag set is difficult. One way I can think of for testing the recovery flow (not the entire flush-then-recover process) is to use some pre-generated corrupted main files with dumps.
common/src/protected_files/protected_files.c line 982 at r3 (raw file):
size_t recovery_nodes_count = recovery_file_size / sizeof(recovery_node_t); for (size_t i = 0; i < recovery_nodes_count; i++) {
Done.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r4, all commit messages.
Reviewable status: 33 of 34 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @efu39, @kailun-qin, and @ynonflumintel)
common/src/protected_files/protected_files.h line 232 at r3 (raw file):
Well, in the C standards, the identifiers of parameters in a function declaration are optional while the types are what really matter.
Yeah, I now that, but I thought the compilers were already warning about that, not only linters :(
a discussion (no related file):
btw. I think you can mark this PR as ready (it's still a draft)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @efu39, @kailun-qin, and @ynonflumintel)
libos/test/fs/manifest.template line 14 at r3 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Does this test actually tests this feature? If I didn't miss anything, this file is used only in a test which only writes to this file once, so not much will be tested?
Only the extended flush logic, not the recovery flow.
Could we have a specialized test which also tests the recovery system?
Yeah, but it's challenging to test this automatically. Controlling when the test should crash abruptly to generate a valid dump with the pending write flag set is difficult. One way I can think of for testing the recovery flow (not the entire flush-then-recover process) is to use some pre-generated corrupted main files with dumps.
Hmm, what are the exact scenarios when we can end up with a corrupted PF? Is it only if we crash in the middle of ipf_internal_flush()? If so, I guess it's not possible to crash at any point in the app manually and cause this corruption?
Description of the changes
Previously, a fatal error during writes to encrypted files could cause file corruption due to incorrect GMACs and/or encryption keys.
To address this, we introduce a file recovery mechanism using a "shadow" recovery file that stores data about to change and a
has_pending_writeflag in the metadata node indicating the start of a write transaction. During file flush, all cached blocks that are about to change are saved to the recovery file in the format of physical node numbers (offsets) plus encrypted block data. Before saving the main file contents, thehas_pending_writeflag is set in the file's metadata node and cleared only when the transaction is complete. If an encrypted file is opened and thehas_pending_writeflag is set, a recovery process starts to revert partial changes using the recovery file, returning to the last known good state. The "shadow" recovery file is cleaned up on file close.This commit adds a new mount parameter
enable_recovery = [true|false]for encrypted files mounts to optionally enable this feature. We extend the file flush logic of protected files (pf) to include the recovery file dump and the setting/unsetting of thehas_pending_writeflag. We also extendpf_open()to make the pf aware of the underlying recovery file managed by LibOS, and to include an optional recovery check and initiate recovery if needed.How to test this PR?
CI + manual testing.
This change is