[Pal/Linux-SGX] Fix refcounting on open/close of Protected Files #2372
Conversation
Previously, `file_open()` and `file_close()` on a Protected File had weird semnatics: if the same PF was opened twice, there was only one PF context created, but there were several underlying host FDs. Even weirder, the PF context tied itself to the host FD (the very first opened one) using the pointer to this FD: `&pal_handle->file.fd`. This resulted in bugs and cumbersome workarounds: the associated PAL handle could be closed and freed by our common PAL code, and the PF context would contain a dangling pointer. And for `file_attrquery()`, there was a workaround to destroy the PAL context if it seemed to be created only for this attrquery flow. This commit fixes these bugs: the PF context stores the underlying host FD directly, and doesn't open new FDs on subsequent `file_open()`. So, there is always only one host FD for one PF context. As a side effect, the logic of `file_open()` and `file_close()` is refactored. A new LibOS regression test `protected_file` is added for GDB debugging because the dedicated `fs/` tests are harder to debug. Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
There was a problem hiding this comment.
Reviewable status: 0 of 10 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
Pal/src/host/Linux-SGX/db_files.c, line 613 at r1 (raw file):
assert(PF_SUCCESS(pfs)); if (fd_from_attrquery == *(int*)pf_handle) { /* this is a PF opened just for us, close it */
FYI: If you want to understand what was checked here, think of it this way:
- The first time Graphene opened this PF, it put
pf->context.pf_handle = &first_host_fd - The second time Graphene opened the same PF, the ``pf->context.pf_handle` is not changed
- So when we arrive here, we basically check "Was it the first time Graphene opened this PF, just to do attrquery?"
This was super-brittle and led to dangling pointers, as found in #2360.
The new code simply opens the PF again and closes it afterwards. This works because PF has a refcount.
Pal/src/host/Linux-SGX/protected-files/protected_files.h, line 244 at r1 (raw file):
* \return PF status */ pf_status_t pf_set_mode(pf_context_t* pf, pf_file_mode_t mode);
FYI: I had to add these helpers to prevent the case of "two handles try to open the same PF in writable mode" -- this is prohibited by PFS.
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
|
Jenkins, retest Jenkins-20.04 please (too old, Jenkins doesn't have it in the logs anymore) |
|
This fix is hacky. We need to refactor Protected Files implementation completely. |
Description of the changes
Previously,
file_open()andfile_close()on a Protected File had weird semnatics: if the same PF was opened twice, there was only one PF context created, but there were several underlying host FDs. Even weirder, the PF context tied itself to the host FD (the very first opened one) using the pointer to this FD:&pal_handle->file.fd.This resulted in bugs and cumbersome workarounds: the associated PAL handle could be closed and freed by our common PAL code, and the PF context would contain a dangling pointer. And for
file_attrquery(), there was a workaround to destroy the PAL context if it seemed to be created only for this attrquery flow (this was based on this pointer-to-pal-handle-fd).This PR fixes these bugs: the PF context stores the underlying host FD directly, and doesn't open new FDs on subsequent
file_open(). So, there is always only one host FD for one PF context. As a side effect, the logic offile_open()andfile_close()is refactored.A new LibOS regression test
protected_fileis added for GDB debugging because the dedicatedfs/tests are harder to debug.Fixes #2360.
How to test this PR?
A new test is added. Also,
fs/pf-testshould succeed.This change is