[LibOS] Add truncate to zero support #651
Conversation
oshogbo
force-pushed
the
oshogbo/ftruncate_0
branch
from
1a469a4
to
43c02b7
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @oshogbo)
a discussion (no related file):
Please also fix comments here:
These are all occurences I could find, the rest you already fixed in this PR. But please check as well (I grepped for truncate, truncation, set_size).
a discussion (no related file):
What do we do about #648, since it is a duplicate of your enabled tests in this PR?
I wouldn't mind actually a separate test for PFs in LibOS regression tests, that will operate on the file, then truncate it to zero, then continue operations. From what I understand, the current truncate tests under test/fs/ do not have such a "complex" flow of operations, and I'm afraid this first revision of the PR doesn't really work in such complex cases.
-- commits line 2 at r1:
It's not the LibOS component, and it doesn't mention Protected Files at all.
I suggest:
[common] Protected Files: Add support for `ftruncate(0)`
Generic support for `ftruncate(<smaller size>)` is hard to implement in Protected Files,
but support for the ftruncate(0)` corner case is easy and important: `fopen(..., "w")`
from GLIBC uses `openat(.., O_TRUNC)`. This ends up calling `pf_set_size(0)`.
Thus, this commit enables a common scenario of opening an existing PF file in write
mode. New FS tests are enabled to test it.
common/src/protected_files/protected_files.c line 1270 at r1 (raw file):
path_len = strlen(pf->encrypted_part_plain.path); memset(&pf->file_metadata, 0, sizeof(pf->file_metadata));
This looks brutal, erasing the whole metadata part. Are you sure the file is "workable" after that? Do we have a test that does something like:
... write/read with protected file ...
ftruncate(pf, 0);
... write/read with protected file ...
close(pf);
pf = open(<pf path>, "r");
... read from pf and verify it contains what we've written to it
pf->file_metadata contains such important fields as metadata_key_id, and you just nullified it. So how can the PF continue read/write operations if there is no key ID associated with it anymore? Or maybe the reconstruction of file_metadata is kicked somewhere after calling this function?
(
common/src/protected_files/protected_files.c line 1270 at r1 (raw file):
path_len = strlen(pf->encrypted_part_plain.path); memset(&pf->file_metadata, 0, sizeof(pf->file_metadata));
Why do you use memset instead of erase_memory? I think you should also use the latter in PFs.
common/src/protected_files/protected_files.c line 1274 at r1 (raw file):
memcpy(path, pf->encrypted_part_plain.path, path_len); erase_memory(&pf->encrypted_part_plain, sizeof(pf->encrypted_part_plain)); memcpy(pf->encrypted_part_plain.path, path, path_len);
Could you group the variables and operations on each part of the pf object? Right now it's messy: you start with extracting the path from encrypted_part_plain, then you switch to erasing file_metadata, and then continue with encrypted_part_plain.
It's hard to read.
common/src/protected_files/protected_files.c line 1274 at r1 (raw file):
memcpy(path, pf->encrypted_part_plain.path, path_len); erase_memory(&pf->encrypted_part_plain, sizeof(pf->encrypted_part_plain)); memcpy(pf->encrypted_part_plain.path, path, path_len);
Again, this looks brutal. You only keep the encrypted_part_plain.path value intact. But what about e.g. mht_key?
common/src/protected_files/protected_files.c line 1291 at r1 (raw file):
free(file_node); lruc_remove_last(pf->cache); }
Ok, these operations on need_writing ... and cache look reasonable.
LibOS/shim/test/fs/test_enc.py line 84 at r1 (raw file):
self.assertTrue(filecmp.cmp(self.INPUT_FILES[i], dec_path, shallow=False)) # overrides TC_00_FileSystem to change input dir (from plaintext to encrypted)
Why did you remove this comment? This comment is correct -- the first line here is indeed changed (from INPUT_FILES to ENCRYPTED_FILES). Please restore this comment.
LibOS/shim/test/fs/test_enc.py line 93 at r1 (raw file):
self.assertTrue(os.path.isfile(output_path)) # overrides TC_00_FileSystem to change input dir (from plaintext to encrypted)
This comment is wrong, it doesn't overwrite anything. Just remove this comment.
LibOS/shim/test/fs/test_enc.py line 98 at r1 (raw file):
file_path = os.path.join(self.OUTPUT_DIR, 'test_101') # new file stdout, stderr = self.run_binary(['open_flags', file_path]) self.verify_open_flags(stdout, stderr)
Actually, this whole test (101) is exactly the same as in the base class TC_00_FileSystem, so you don't need to overwrite it. Thus, we can simply remove this whole test from here, and then the base test will be used instead.
I think the only reason why this test was copy-pasted in here was to add that expectedFailure decorator. Now that you don't have it, you don't need to re-write this test at all.
LibOS/shim/test/fs/test_enc.py line 127 at r1 (raw file):
# overrides TC_00_FileSystem to decrypt output def verify_size(self, file, size):
This function is not even used?
LibOS/shim/test/fs/test_enc.py line 135 at r1 (raw file):
stdout, stderr = self.run_binary(['truncate', path_1, path_2, str(size_out)]) stdout, stderr = self.run_binary(['truncate', path_1, path_2, str(size_out)])
You run the same thing twice?
LibOS/shim/test/fs/test_enc.py line 140 at r1 (raw file):
self.assertIn('open(' + path_2 + ') output OK', stdout) self.assertIn('ftruncate(' + path_2 + ') to ' + str(size_out) + ' OK', stdout) self.assertIn('close(' + path_2 + ') output OK', stdout)
You don't call self.verify_size(path_1, path_2) here at all. What's the reason?
LibOS/shim/test/fs/test_enc.py line 142 at r1 (raw file):
self.assertIn('close(' + path_2 + ') output OK', stdout) # TODO: File truncation to arbitrary size.
You may also add a comment like this:
# overrides TC_00_FileSystem to change input dir (from plaintext to encrypted) and
# because file truncation from greater to small size is not yet implemented
But not blocking, as it is pretty obvious.
LibOS/shim/test/fs/test_enc.py line 156 at r1 (raw file):
self.verify_truncate_test(path_1, path_2, 1000) self.verify_truncate_test(path_1, path_2, 0) self.verify_truncate_test(path_1, path_2, 0)
I don't like that your logic for this test is completely different from the logic in test_fs.py. Could you use the same logic? There is an unnecessary divergence here, and ideally (when PFs support proper truncation), the only change will be INPUT_FILES -> ENCRYPTED_FILES.
In particular, the test_fs.py version does this:
def test_140_file_truncate(self):
self.do_truncate_test(0, 1)
self.do_truncate_test(0, 16)
I.e., it gets an input file of size 0 and expands it to size 1, then gets an input file of size 0 and expands it to size 16, and so on. So each do_truncate_test() is stateless and doesn't depend on the previous runs, unlike your current solution.
LibOS/shim/test/fs/test_fs.py line 62 at r1 (raw file):
self.assertIn('fclose(' + path + ') ' + mode + ' 2 OK', stdout) # overrides TC_00_FileSystem to change input dir (from plaintext to encrypted)
Please revert this change; this comment doesn't belong to this file but to the test_enc.py. I guess you confused the two files when doing this change.
LibOS/shim/test/fs/test_fs.py line 297 at r1 (raw file):
self.do_copy_test('copy_sendfile', 60) @expectedFailureIf(HAS_SGX)
Unrelated, but why do the following mmap tests fail on SGX? It would be good to examine why this happens, and a comment with explanations here.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 17 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @oshogbo)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What do we do about #648, since it is a duplicate of your enabled tests in this PR?
I wouldn't mind actually a separate test for PFs in LibOS regression tests, that will operate on the file, then truncate it to zero, then continue operations. From what I understand, the current
truncatetests undertest/fs/do not have such a "complex" flow of operations, and I'm afraid this first revision of the PR doesn't really work in such complex cases.
Actually it has in the open_flags.c but maybe we want to have a separate one, let's think about it afterwords.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
It's not the LibOS component, and it doesn't mention Protected Files at all.
I suggest:
[common] Protected Files: Add support for `ftruncate(0)` Generic support for `ftruncate(<smaller size>)` is hard to implement in Protected Files, but support for the ftruncate(0)` corner case is easy and important: `fopen(..., "w")` from GLIBC uses `openat(.., O_TRUNC)`. This ends up calling `pf_set_size(0)`. Thus, this commit enables a common scenario of opening an existing PF file in write mode. New FS tests are enabled to test it.
Done.
common/src/protected_files/protected_files.c line 1270 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This looks brutal, erasing the whole metadata part. Are you sure the file is "workable" after that? Do we have a test that does something like:
... write/read with protected file ... ftruncate(pf, 0); ... write/read with protected file ... close(pf); pf = open(<pf path>, "r"); ... read from pf and verify it contains what we've written to it)
pf->file_metadatacontains such important fields asmetadata_key_id, and you just nullified it. So how can the PF continue read/write operations if there is no key ID associated with it anymore? Or maybe the reconstruction offile_metadatais kicked somewhere after calling this function?
(
Yes, this is done by "open flags" test, with O_TRUNC.
This is brutal. But my idea was to truncate the original file to 0 and reinitialize the internal status to the same as the ipf_open is leaving it when creating a new file.
common/src/protected_files/protected_files.c line 1270 at r1 (raw file):
erase_memory
I wasn't sure when exactly should we use erase_memory.
I will fix that.
common/src/protected_files/protected_files.c line 1274 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Again, this looks brutal. You only keep the
encrypted_part_plain.pathvalue intact. But what about e.g.mht_key?
Answered in metadata_key_id thread.
common/src/protected_files/protected_files.c line 1274 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you group the variables and operations on each part of the
pfobject? Right now it's messy: you start with extracting thepathfromencrypted_part_plain, then you switch to erasingfile_metadata, and then continue withencrypted_part_plain.It's hard to read.
Sure, make sense.
Thanks.
LibOS/shim/test/fs/test_enc.py line 84 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why did you remove this comment? This comment is correct -- the first line here is indeed changed (from
INPUT_FILEStoENCRYPTED_FILES). Please restore this comment.
Sorry, my bad.
LibOS/shim/test/fs/test_enc.py line 93 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This comment is wrong, it doesn't overwrite anything. Just remove this comment.
Done.
LibOS/shim/test/fs/test_enc.py line 98 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, this whole test (
101) is exactly the same as in the base classTC_00_FileSystem, so you don't need to overwrite it. Thus, we can simply remove this whole test from here, and then the base test will be used instead.I think the only reason why this test was copy-pasted in here was to add that
expectedFailuredecorator. Now that you don't have it, you don't need to re-write this test at all.
Done.
Code quote:
test_101_open_flagsLibOS/shim/test/fs/test_enc.py line 127 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This function is not even used?
Done.
LibOS/shim/test/fs/test_enc.py line 135 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You run the same thing twice?
Done.
LibOS/shim/test/fs/test_enc.py line 140 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You don't call
self.verify_size(path_1, path_2)here at all. What's the reason?
Because the size of file is not the same as size of the content, I wasn't sure what to do with it.
But you have pointed out to me the verify_size which make now sense.
Code quote:
verify_sizeLibOS/shim/test/fs/test_enc.py line 156 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't like that your logic for this test is completely different from the logic in
test_fs.py. Could you use the same logic? There is an unnecessary divergence here, and ideally (when PFs support proper truncation), the only change will beINPUT_FILES->ENCRYPTED_FILES.In particular, the
test_fs.pyversion does this:def test_140_file_truncate(self): self.do_truncate_test(0, 1) self.do_truncate_test(0, 16)I.e., it gets an input file of size 0 and expands it to size 1, then gets an input file of size 0 and expands it to size 16, and so on. So each
do_truncate_test()is stateless and doesn't depend on the previous runs, unlike your current solution.
Yes, I wanted to have an option to have a dependable test because I wanted for example to check that we are still able to read file after the truncation.
LibOS/shim/test/fs/test_fs.py line 62 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please revert this change; this comment doesn't belong to this file but to the
test_enc.py. I guess you confused the two files when doing this change.
Done.
LibOS/shim/test/fs/test_fs.py line 297 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Unrelated, but why do the following mmap tests fail on SGX? It would be good to examine why this happens, and a comment with explanations here.
I guess why should create an issue about it?
I can work on this afterwords.
Code quote:
# overrides TC_00_FileSystem to change input dir (from plaintext to encrypted)
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 17 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @oshogbo)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please also fix comments here:
These are all occurences I could find, the rest you already fixed in this PR. But please check as well (I grepped for
truncate,truncation,set_size).
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @oshogbo)
a discussion (no related file):
When looking into this PR, I found a potential bug:
Here the code assumes that pf->encrypted_part_plain.size >= pf->offset. But this may be wrong, especially now that we have ftruncate(0) -- this function does not change the offset of the file but nullifies the size. So this line of code may have an integer overflow. We should add an explicit check for this (and maybe come up with a C test that exposes this bug).
Or maybe this case can never be hit... IIUC, the Protected File can only be opened for read or only for write. In the for-read case, ftruncate(0) is not allowed. In the for-write case, we never hit this line (because it is inside ipf_read()). Anyway, need to analyze a bit more and at least add an assert here.
@oshogbo Could you look into this? I think an assert would be enough, with a comment explaining why offset can never be greater than size in the current code.
common/src/protected_files/protected_files.c line 1270 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Yes, this is done by "open flags" test, with O_TRUNC.
This is brutal. But my idea was to truncate the original file to 0 and reinitialize the internal status to the same as theipf_openis leaving it when creating a new file.
But ipf_open also calls ipf_init_existing_file for the existing file (the case I care about here!) or ipf_init_new_file for the non-existing file (the case which you modelled your PR on).
UPDATE: I did a quick GDB session, with the following change:
diff --git a/LibOS/shim/test/regression/rw_file.c b/LibOS/shim/test/regression/rw_file.c
@@ -115,6 +115,13 @@ static ssize_t stdio_file_rw(const char* path, char* buf, size_t count, bool do_
return -1;
}
+ /* for testing of Protected/Encrypted files (used in sealed_file.c test) */
+ if (do_write) {
+ fflush(f);
+ ftruncate(fileno(f), 0);
+ rewind(f);
+ stdio_fd_rw(f, buf, count, do_write);
+ }
+
int close_ret = fclose(f);
if (close_ret < 0) {
int ret_errno = errno;Here I wanted to test the case: a protected file is opened, it is written to (we make sure it's not cached in the user-space buffer by doing an explicit fflush), then truncated to zero, then written to again. Everything works.
I went a bit deeper into the current PF code (https://github.com/gramineproject/gramine/blob/fa7b91304f555946956422c706efa4ca0ad847f2/common/src/protected_files/protected_files.c), and I can confirm that all the metadata fields are updated on demand. The function to look at is ipf_get_data_node(), and from then check the logic of called funcs.
LibOS/shim/test/fs/test_enc.py line 156 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Yes, I wanted to have an option to have a dependable test because I wanted for example to check that we are still able to read file after the truncation.
OK
LibOS/shim/test/fs/test_fs.py line 297 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I guess why should create an issue about it?
I can work on this afterwords.
Looks like you had a typo: why -> we.
Sure, let's create an issue.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
When looking into this PR, I found a potential bug:
Here the code assumes that
pf->encrypted_part_plain.size >= pf->offset. But this may be wrong, especially now that we haveftruncate(0)-- this function does not change the offset of the file but nullifies the size. So this line of code may have an integer overflow. We should add an explicit check for this (and maybe come up with a C test that exposes this bug).Or maybe this case can never be hit... IIUC, the Protected File can only be opened for read or only for write. In the for-read case,
ftruncate(0)is not allowed. In the for-write case, we never hit this line (because it is insideipf_read()). Anyway, need to analyze a bit more and at least add anasserthere.@oshogbo Could you look into this? I think an assert would be enough, with a comment explaining why offset can never be greater than size in the current code.
Sure, let me look into this outside this PR.
common/src/protected_files/protected_files.c line 1270 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But
ipf_openalso callsipf_init_existing_filefor the existing file (the case I care about here!) oripf_init_new_filefor the non-existing file (the case which you modelled your PR on).UPDATE: I did a quick GDB session, with the following change:
diff --git a/LibOS/shim/test/regression/rw_file.c b/LibOS/shim/test/regression/rw_file.c @@ -115,6 +115,13 @@ static ssize_t stdio_file_rw(const char* path, char* buf, size_t count, bool do_ return -1; } + /* for testing of Protected/Encrypted files (used in sealed_file.c test) */ + if (do_write) { + fflush(f); + ftruncate(fileno(f), 0); + rewind(f); + stdio_fd_rw(f, buf, count, do_write); + } + int close_ret = fclose(f); if (close_ret < 0) { int ret_errno = errno;Here I wanted to test the case: a protected file is opened, it is written to (we make sure it's not cached in the user-space buffer by doing an explicit fflush), then truncated to zero, then written to again. Everything works.
I went a bit deeper into the current PF code (https://github.com/gramineproject/gramine/blob/fa7b91304f555946956422c706efa4ca0ad847f2/common/src/protected_files/protected_files.c), and I can confirm that all the metadata fields are updated on demand. The function to look at is
ipf_get_data_node(), and from then check the logic of called funcs.
I'm glad we agree that this works fine.
My general idea was that if we just running real truncate(0) on this file. This almost similar to removing and creating new file. So we can treat this file as a newly created, so all metadata should be cleared as well, and we should follow the ifp_open path for a new file.
LibOS/shim/test/fs/test_fs.py line 297 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Looks like you had a typo:
why->we.Sure, let's create an issue.
Yes :)
There was a problem hiding this comment.
Reviewable status: all 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: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @oshogbo)
LibOS/shim/test/fs/test_fs.py line 297 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Yes :)
I don't think this was done (let's create an issue)?
There was a problem hiding this comment.
Reviewable status: all 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: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
LibOS/shim/test/fs/test_fs.py line 297 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't think this was done (
let's create an issue)?
Done: #663
|
Jenkins, test this please |
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, 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
There was a problem hiding this comment.
Reviewed 2 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @oshogbo)
a discussion (no related file):
Nice! This was a painful corner case, since even open(O_TRUNC) failed without it.
FYI: I don't see any problems with this code, but I don't know much about Protected Files internals.
common/src/protected_files/protected_files.c line 1285 at r3 (raw file):
while ((data = lruc_get_last(pf->cache)) != NULL) { file_node_t* file_node = (file_node_t*)data;
Reindent to 4g spaces?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @oshogbo)
common/src/protected_files/protected_files.c line 139 at r3 (raw file):
} static void ipf_init_root_mht(file_node_t *mht) {
Suggestion:
file_node_t* LibOS/shim/test/fs/test_enc.py line 96 at r3 (raw file):
# overrides TC_00_FileSystem to change input dir (from plaintext to encrypted) # doesn't work because encrypted files do not support truncation (and the test opens an # existing, non-empty file with O_TRUNC)
Why have you deleted this tests' invocation? Sounds like it should work now?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)
LibOS/shim/test/fs/test_enc.py line 96 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why have you deleted this tests' invocation? Sounds like it should work now?
Because this test (test_101_open_flags) is exactly the same as the one in the base-class test_fs.py:
gramine/LibOS/test/fs/test_fs.py
Line 82 in 5faa9eb
So by deleting this test, we remove the overwrite, and this test will be taken from the base class. It will be executed.
Also see one of our previous (and resolved) discussions in this PR.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @oshogbo)
LibOS/shim/test/fs/test_enc.py line 96 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Because this test (
test_101_open_flags) is exactly the same as the one in the base-classtest_fs.py:gramine/LibOS/test/fs/test_fs.py
Line 82 in 5faa9eb
So by deleting this test, we remove the overwrite, and this test will be taken from the base class. It will be executed.
Also see one of our previous (and resolved) discussions in this PR.
Oh, I see, didn't notice that these are just overrides for some tests and everything is actually run.
oshogbo
force-pushed
the
oshogbo/ftruncate_0
branch
from
c1053f1
to
5590472
Compare
|
Jenkins, retest this please |
There was a problem hiding this comment.
Reviewable status: 1 of 8 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 @dimakuv, @mkow, and @pwmarcz)
common/src/protected_files/protected_files.c line 139 at r3 (raw file):
} static void ipf_init_root_mht(file_node_t *mht) {
Done.
common/src/protected_files/protected_files.c line 1285 at r3 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Reindent to 4g spaces?
Done.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 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 @mkow)
oshogbo
force-pushed
the
oshogbo/ftruncate_0
branch
from
5590472
to
a996189
Compare
There was a problem hiding this comment.
Reviewed 4 of 7 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status: all 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 @oshogbo)
common/src/protected_files/protected_files.c line 1273 at r5 (raw file):
memcpy(pf->encrypted_part_plain.path, path, path_len); erase_memory(&pf->file_metadata, sizeof(pf->file_metadata));
No need to erase this, it resides on the host disk in plaintext. Can be just a memset.
libos/test/fs/test_enc.py line 95 at r5 (raw file):
# overrides TC_00_FileSystem to change input dir (from plaintext to encrypted) # doesn't work because encrypted files do not support truncation (and the test opens an
How does this work? Why don't we need to override the input dir for test_101_open_flags, but we do for other tests?
There was a problem hiding this comment.
Reviewable status: all 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 @dimakuv and @mkow)
common/src/protected_files/protected_files.c line 1273 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
No need to erase this, it resides on the host disk in plaintext. Can be just a
memset.
Hym, I think @dimakuv proposed using erase_memory on this one.
https://reviewable.io/reviews/gramineproject/gramine/651#-N4fa2oVBSNbr2CqAYML
If I understand correctly @dimakuv, we just want to be sure that the zeroing really happens and none of the flags will be set.
libos/test/fs/test_enc.py line 95 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
How does this work? Why don't we need to override the input dir for
test_101_open_flags, but we do for other tests?
We actually do override the INPUT_DIR/OUTPUT_DIR in setUpClass.
So it should override the one from the original test, right?
Or you meant something else?
There was a problem hiding this comment.
Reviewable status: all 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 @dimakuv and @oshogbo)
common/src/protected_files/protected_files.c line 1273 at r5 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Hym, I think @dimakuv proposed using
erase_memoryon this one.
https://reviewable.io/reviews/gramineproject/gramine/651#-N4fa2oVBSNbr2CqAYMLIf I understand correctly @dimakuv, we just want to be sure that the zeroing really happens and none of the flags will be set.
Yeah, but this buffer is not secret. It's literally the first chunk from the disk cached :)
libos/test/fs/test_enc.py line 95 at r5 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
We actually do override the
INPUT_DIR/OUTPUT_DIRinsetUpClass.
So it should override the one from the original test, right?
Or you meant something else?
Ok, seems we do, so it looks like it works.
But this looks weird - why all the tests have that comment, if it's not actually where the dir is overwritten? I originally pasted this comment because I saw that you deleted the overriding method with that comment on it (overrides TC_00_FileSystem to change input dir (from plaintext to encrypted)), and it seemed like it should be missing after that removal? I.e. the comment says that the change happens in the overridden method, but now you say that it actually happens in setUpClass?
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 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 @mkow)
common/src/protected_files/protected_files.c line 1273 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Yeah, but this buffer is not secret. It's literally the first chunk from the disk cached :)
@mkow is right, fair enough, this buffer is not secret. Sorry for confusing. We can revert back to classic memset().
libos/test/fs/test_enc.py line 95 at r5 (raw file):
Wait, people, let's backtrack.
We actually do override the
INPUT_DIR/OUTPUT_DIRin setUpClass.
This is not true. We only override OUTPUT_DIR in setUpClass. We do not override INPUT_DIR.
Why don't we need to override the input dir for
test_101_open_flags, but we do for other tests?
Because if you look at this test (https://github.com/gramineproject/gramine/blob/master/libos/test/fs/test_fs.py#L82-L85), you'll notice that there is no INPUT_DIR involved at all. Only the OUTPUT_DIR is needed. If you look at the test file (https://github.com/gramineproject/gramine/blob/master/libos/test/fs/open_flags.c), you'll see that this test creates a new output file. So no input file is needed at all.
So if we combine the two facts -- OUTPUT_DIR is set up in setUpClass in this test_enc.py class, and test_101_open_flags does not use the input dir/files at all -- we see that there is no need to override this method (def test_101_open_flags()) from the base class at all.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 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 @dimakuv and @oshogbo)
libos/test/fs/test_enc.py line 95 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wait, people, let's backtrack.
We actually do override the
INPUT_DIR/OUTPUT_DIRin setUpClass.This is not true. We only override
OUTPUT_DIRin setUpClass. We do not overrideINPUT_DIR.Why don't we need to override the input dir for
test_101_open_flags, but we do for other tests?Because if you look at this test (https://github.com/gramineproject/gramine/blob/master/libos/test/fs/test_fs.py#L82-L85), you'll notice that there is no
INPUT_DIRinvolved at all. Only theOUTPUT_DIRis needed. If you look at the test file (https://github.com/gramineproject/gramine/blob/master/libos/test/fs/open_flags.c), you'll see that this test creates a new output file. So no input file is needed at all.So if we combine the two facts --
OUTPUT_DIRis set up in setUpClass in thistest_enc.pyclass, andtest_101_open_flagsdoes not use the input dir/files at all -- we see that there is no need to override this method (def test_101_open_flags()) from the base class at all.
I see, thanks for the explanation! So, the comment on the overridden test_101_open_flags was wrong before?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 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 @mkow and @oshogbo)
libos/test/fs/test_enc.py line 95 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I see, thanks for the explanation! So, the comment on the overridden
test_101_open_flagswas wrong before?
Yes, the comment was wrong before.
oshogbo
force-pushed
the
oshogbo/ftruncate_0
branch
from
a996189
to
234f531
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
Generic support for `ftruncate(<smaller size>)` is hard to implement in Protected Files, but support for the ftruncate(0)` corner case is easy and important: `fopen(..., "w")` from GLIBC uses `openat(.., O_TRUNC)`. This ends up calling `pf_set_size(0)`. Thus, this commit enables a common scenario of opening an existing PF file in write mode. New FS tests are enabled to test it. Signed-off-by: Mariusz Zaborski <[email protected]>
oshogbo
force-pushed
the
oshogbo/ftruncate_0
branch
from
234f531
to
4634804
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
There was a problem hiding this comment.
Reviewed 4 of 6 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
This adds a truncation to zero. In this case, we are issuing normal truncation and refreshing the internal buffer.
We also drop all pending data.
This should fix the issue: #641
How to test this PR?
New tests are added.
This change is