[LibOS] Check existence of file during chroot_stat()

[dimakuv]

Description of the changes

This PR temporarily opens the file during stat() callback to check if the file still exists on the host. Without this open and if the same file was opened before, Gramine would serve the chroot_stat() callback from the cached dent->inode and wouldn't notice that the file was removed on the host. Note that such check is relevant only to chroot files, because all other file types are either pseudo in-memory files (dev, etc, sys, tmpfs) or fully controlled by Gramine files (encrypted).

Fixes #1422.

How to test this PR?

I don't know how to add such a test to our CI, that will start the Gramine process and then remove the host file under it.

Here is the manual test:

1. Change large_file.c LibOS regression test:

diff --git a/libos/test/regression/large_file.c b/libos/test/regression/large_file.c
@@ -91,6 +91,13 @@ int main(void) {
     if (close(fd) < 0)
         err(1, "close");

+    struct stat st;
+    if (stat(TEST_FILE, &st) < 0)
+        err(1, "stat");
+    sleep(5);
+    if (stat(TEST_FILE, &st) < 0)
+        err(1, "stat");
+
     printf("TEST OK\n");
     return 0;
 }

2. Check native:
   a. <gramine install dir>/lib/x86_64-linux-gnu/gramine/tests/libos/regression/large_file in one terminal
   b. rm -rf libos/test/regression/tmp/large_file in another terminal
   c. native app must complain in first terminal: large_file: stat: No such file or directory

3. Check Gramine:
   a. gramine-direct large_file in one terminal
   b. rm -rf libos/test/regression/tmp/large_file in another terminal
   c. Gramine app must complain in first terminal: large_file: stat: No such file or directory
   d. (without this PR, Gramine app incorrectly succeeds: TEST OK)

---

This change is 

[kailun-qin]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

a discussion (no related file):
I know this addresses the reported issue and helps us preserve the expected Linux behavior. But it's kind of like a ad-hoc fix since the cache can still be out of sync in some other cases (e.g., not remove the file but change the size of it)?

---

[dimakuv]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)

a discussion (no related file):

But it's kind of like a ad-hoc fix since the cache can still be out of sync in some other cases (e.g., not remove the file but change the size of it)?

This is a fair comment. Several solutions then:

1. Do not allow such logic at all. Explain to people that Gramine doesn't expect the host to modify Gramine-used files, and that it's not possible to use Gramine as a passive "listener" of what happens on the host.
2. Keep the partial logic, as in this PR, and solve new use cases as/if people come and complain. I.e., grow the supported use cases organically and on-demand.
3. Make the logic more comprehensive -- e.g. update the size on stat() also, if it was modified on the host. And possibly other fields, see here.

I would prefer solution 3.

But I also don't mind solution 2, since currently Gramine has both things wrong (file existence and file size), and with this PR Gramine will have only one thing wrong (file size). So this PR still sounds like an improvement over the current state, even if an ad-hoc one.

I don't like solution 1. I see no reason to forbid such use cases.

---

[kailun-qin]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

But it's kind of like a ad-hoc fix since the cache can still be out of sync in some other cases (e.g., not remove the file but change the size of it)?

This is a fair comment. Several solutions then:

1. Do not allow such logic at all. Explain to people that Gramine doesn't expect the host to modify Gramine-used files, and that it's not possible to use Gramine as a passive "listener" of what happens on the host.
2. Keep the partial logic, as in this PR, and solve new use cases as/if people come and complain. I.e., grow the supported use cases organically and on-demand.
3. Make the logic more comprehensive -- e.g. update the size on stat() also, if it was modified on the host. And possibly other fields, see here.

I would prefer solution 3.

But I also don't mind solution 2, since currently Gramine has both things wrong (file existence and file size), and with this PR Gramine will have only one thing wrong (file size). So this PR still sounds like an improvement over the current state, even if an ad-hoc one.

I don't like solution 1. I see no reason to forbid such use cases.

OK, make sense. I also vote for solution 3, as it's more correct and seems not complex.

---

[mkow]

Reviewable status: all files reviewed, 1 unresolved discussion, 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)

a discussion (no related file):
I agree that we don't really have the semantics of chroot mounts well defined if the host changes the files when the enclave is still running. We should decide on that before merging this PR.

I would prefer solution 3.

I also vote for solution 3, as it's more correct and seems not complex.

Hmm, I'd rather say that it is actually complex? We'd need to remove all caches for chroot FS and always passthrough everything. But this may behave weirdly with things like realpath(), where we need to traverse a path. And no cache probably means a rather huge perf penalty for SGX?

---

[mwkmwkmwk]

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 @kailun-qin)

---

libos/src/fs/chroot/fs.c line 369 at r1 (raw file):

     * in-memory files (dev, etc, sys, tmpfs) or fully controlled by Gramine files (encrypted). */
    PAL_HANDLE palhdl;
    ret = chroot_temp_open(dent, dent->inode->type, &palhdl);

I'd suggest calling PalStreamAttributesQuery instead; it avoids the need for a second PAL call to close the file afterwards, and as a bonus can be used to actually update the attribute cache.

[dimakuv]

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, @kailun-qin, and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

I agree that we don't really have the semantics of chroot mounts well defined if the host changes the files when the enclave is still running. We should decide on that before merging this PR.

I would prefer solution 3.

I also vote for solution 3, as it's more correct and seems not complex.

Hmm, I'd rather say that it is actually complex? We'd need to remove all caches for chroot FS and always passthrough everything. But this may behave weirdly with things like realpath(), where we need to traverse a path. And no cache probably means a rather huge perf penalty for SGX?

Ok, it's true that it will be a design decision that we should clearly state and agree upon. Created a discussion item for the next meeting: #1660

---

---

libos/src/fs/chroot/fs.c line 369 at r1 (raw file):

Previously, mwkmwkmwk (Marcelina Kościelnicka) wrote…

I'd suggest calling PalStreamAttributesQuery instead; it avoids the need for a second PAL call to close the file afterwards, and as a bonus can be used to actually update the attribute cache.

Currently putting this comment on hold, as we need to discuss the general direction for these "host modifies file while Gramine is running"