[LibOS] Split shim_mount and shim_fs

[pwmarcz]

(I think this finally unblocks the "mount semantics" refactoring! I'm checking that right now, but it's going to be a separate PR.)

This change replaces most usages of shim_mount with shim_fs. Graphene uses the shim_mount structure to represent a mounted filesystem, but in most cases we're only interested in the function table.

The main reason for this change is special treatment of sockets and FIFOs: before, we replaced the fs field (actually shim_mount) for their dentries, so that they are served by a different filesystem. However, that loses information about which part of the tree the dentry actually is in.

The new solution is to store both shim_mount and shim_fs for a dentry. This follows Linux, which stores both super_block and inode_iperations for an inode, and replaces the latter in case of sockets and FIFOs. This will allow me to refactor the way mounted filesystems are represented.

As a bonus, this change removes some hacks like unwarranted usage of *_builtin_fs and storing filesystem type separately for checkpointing purposes.

---

This change is 

[dimakuv]

Reviewed 25 of 25 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 (1 more required, approved so far: ITL) (waiting on @pwmarcz)

---

LibOS/shim/include/shim_fs.h, line 593 at r1 (raw file):

 *
 * If the `mount` parameter is provided, the resulting dentry's `mount` field will be initialized to
 * it, and the `fs` field will initialized to `mount->fs` (but you can override `fs` later for

will be initialized

---

LibOS/shim/include/shim_fs.h, line 594 at r1 (raw file):

 * If the `mount` parameter is provided, the resulting dentry's `mount` field will be initialized to
 * it, and the `fs` field will initialized to `mount->fs` (but you can override `fs` later for
 * special files). Otherwise, both `mount` and `fs` parameters will be left as NULL.

What's the point of having dentry.mount and dentry.fs as NULL? Is it used in Graphene? Can we add an assert that mount != NULL?

---

LibOS/shim/src/bookkeep/shim_handle.c, line 102 at r1 (raw file):

        put_mount(mount);
    } else {
        exec->fs = &chroot_builtin_fs;

Actually, what is the point of this hack? If we end up here, then the executable is not really mounted, so it is like a "pseudo" file. How is this useful?

---

LibOS/shim/src/bookkeep/shim_handle.c, line 655 at r1 (raw file):

        lock(&hdl->lock);
        struct shim_fs* fs = hdl->fs;

Can we remove this fs alias and use hdl->fs in the below two lines of code?

---

LibOS/shim/src/fs/shim_fs.c, line 615 at r1 (raw file):

 * Note that checkpointing the `shim_fs` structure copies it, instead of using a pointer to
 * corresponding global object on the remote side. This does not waste too much memory (because each
 * global object is only copied once), but it means that `shim_fs` objects cannot be compared by

Btw, did you check that each global object is only copied once is actually true? I know we have this GET_FROM_CP_MAP(obj) that avoids duplication of already-checkpointed objects (based on pointer value), but I never checked if this macro actually works as expected.

---

LibOS/shim/src/fs/pipe/fs.c, line 264 at r1 (raw file):

struct shim_fs fifo_builtin_fs = {
    .name   = "fifo",

Why do we have hard-coded fifo instead of macro like URI_TYPE_FIFO?

Or maybe the question should be the other way around: why do we have URI_TYPE_PIPE instead of hard-coded pipe?

[pwmarcz]

Rebased to fix typo in a commit message: inode_iperations -> inode_operations.

Reviewable status: 20 of 25 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)

---

LibOS/shim/include/shim_fs.h, line 593 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

will be initialized

Fixed.

---

LibOS/shim/include/shim_fs.h, line 594 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What's the point of having dentry.mount and dentry.fs as NULL? Is it used in Graphene? Can we add an assert that mount != NULL?

There's the g_dentry_root, which is initialized as "mount-less" dentry. But it does not use get_new_dentry().

---

LibOS/shim/src/bookkeep/shim_handle.c, line 102 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, what is the point of this hack? If we end up here, then the executable is not really mounted, so it is like a "pseudo" file. How is this useful?

I removed it and I got an answer immediately by running the tests:

See the host_root_fs test. It mounts / as root, but specifies the executable as relative URI same as other tests (file:host_root_fs). So we're not able to determine where host_root_fs is in the mounted filesystem, and execute this line, initializing a dentry-less handle.

Apparently this code for finding a mount works mostly for the common case of mounting file:. as root, and specifying executable in relative URI.

I think a proper fix would be to deal with absolute URIs, both for executable and for mounted filesystems. The manifest may still specify to mount file:., but internally we translate it to absolute path the manifest itself (probably supplied by PAL). Does that make sense?

(I'd like to leave a TODO / explanation here after we agree on something).

---

LibOS/shim/src/bookkeep/shim_handle.c, line 655 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we remove this fs alias and use hdl->fs in the below two lines of code?

Done.

---

LibOS/shim/src/fs/shim_fs.c, line 615 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Btw, did you check that each global object is only copied once is actually true? I know we have this GET_FROM_CP_MAP(obj) that avoids duplication of already-checkpointed objects (based on pointer value), but I never checked if this macro actually works as expected.

Yes, I added debug prints to both branches in this function and they confirm that:

[P26031:T1:getdents] fs chroot
[P26031:T1:getdents] fs existing chroot
[P26031:T1:getdents] fs proc
[P26031:T1:getdents] fs existing proc
[P26031:T1:getdents] fs dev
[P26031:T1:getdents] fs existing dev
[P26031:T1:getdents] fs existing chroot
[P26031:T1:getdents] fs existing chroot
[P26031:T1:getdents] fs sys
[P26031:T1:getdents] fs existing sys
[P26031:T1:getdents] fs existing chroot
[P26031:T1:getdents] fs existing chroot
[P26031:T1:getdents] fs existing chroot
[P26031:T1:getdents] fs existing chroot

---

LibOS/shim/src/fs/pipe/fs.c, line 264 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why do we have hard-coded fifo instead of macro like URI_TYPE_FIFO?

Or maybe the question should be the other way around: why do we have URI_TYPE_PIPE instead of hard-coded pipe?

Yeah, looks inconsistent. I'd rather hardcode the name, it seems like a misuse of URI_TYPE_*.

(Also I found one for eventfd and changed it).

[dimakuv]

Reviewed 5 of 5 files at r2.
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: ITL) (waiting on @pwmarcz)

---

LibOS/shim/src/bookkeep/shim_handle.c, line 102 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I removed it and I got an answer immediately by running the tests:

See the host_root_fs test. It mounts / as root, but specifies the executable as relative URI same as other tests (file:host_root_fs). So we're not able to determine where host_root_fs is in the mounted filesystem, and execute this line, initializing a dentry-less handle.

Apparently this code for finding a mount works mostly for the common case of mounting file:. as root, and specifying executable in relative URI.

I think a proper fix would be to deal with absolute URIs, both for executable and for mounted filesystems. The manifest may still specify to mount file:., but internally we translate it to absolute path the manifest itself (probably supplied by PAL). Does that make sense?

(I'd like to leave a TODO / explanation here after we agree on something).

I didn't understand your proposal for a fix. Could you maybe describe on the example of host_root_fs? Here is the current host_root_fs.manifest.template for ease of discussion (I retained only the relevant lines):

libos.entrypoint = "file:host_root_fs"

fs.root.type = "chroot"
fs.root.path = "/"
fs.root.uri = "file:/"

sgx.trusted_files.host_root_fs = "file:host_root_fs"

[pwmarcz]

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: ITL) (waiting on @dimakuv)

---

LibOS/shim/src/bookkeep/shim_handle.c, line 102 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I didn't understand your proposal for a fix. Could you maybe describe on the example of host_root_fs? Here is the current host_root_fs.manifest.template for ease of discussion (I retained only the relevant lines):

libos.entrypoint = "file:host_root_fs"

fs.root.type = "chroot"
fs.root.path = "/"
fs.root.uri = "file:/"

sgx.trusted_files.host_root_fs = "file:host_root_fs"

OK. Following that example, let's say we run this from /home/user/graphene/LibOS/shim/test/regression, and all relative paths should be resolved relative to this directory.

PAL would determine that path during bootstrapping, and pass it to LibOS as something like g_pal_control->host_cwd_uri = "file:/home/user/graphene/LibOS/shim/test/regression".

Then:

const char* exec_uri = g_pal_control->executable;
// exec_uri = "file:host_root_fs"

exec_uri = to_abspath(g_pal_control->host_cwd_uri, exec_uri);
// exec_uri = "file:/home/user/graphene/LibOS/shim/test/regression"

struct shim_mount* mount;
const char* exec_rel_path;
find_mount(exec_uri, &mount, &exec_rel_path);
// mount = <chroot mount for />
// mount->uri = "file:/"
// exec_rel_path = "home/user/graphene/LibOS/shim/test/regression"

struct shim_dentry* dent;
path_lookupat(mount->mount_point, exec_rel_path, ..., &dent);

The same code would work in the "regular" case, i.e. when fs.root.uri = "file:.", because we would also convert mount->uri` to be an absolute. It would also work for other combinations of mount URI / mount path / executable URI that are probably broken right now.

[pwmarcz]

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: ITL) (waiting on @dimakuv)

---

LibOS/shim/src/bookkeep/shim_handle.c, line 102 at r1 (raw file):

The same code would work in the "regular" case, i.e. when fs.root.uri = "file:.", because we would also convert mount->uri to be an absolute path.

Sorry about the typo, can't fix on Reviewable.

[dimakuv]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

---

LibOS/shim/src/bookkeep/shim_handle.c, line 102 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

The same code would work in the "regular" case, i.e. when fs.root.uri = "file:.", because we would also convert mount->uri to be an absolute path.

Sorry about the typo, can't fix on Reviewable.

Understood. Yes, this proposal looks good to me.

[pwmarcz]

Reviewable status: 24 of 25 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: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

LibOS/shim/src/bookkeep/shim_handle.c, line 102 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Understood. Yes, this proposal looks good to me.

I added a comment to this code to explain the situation.

[dimakuv]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

[mkow]

Reviewed 20 of 25 files at r1, 4 of 5 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

LibOS/shim/src/fs/shim_dcache.c, line 473 at r3 (raw file):

        new_dent->state &= ~DENTRY_LISTED;

        if (new_dent->type != S_IFIFO) {

How does this type field correspond to the underlying FS implementation on our side? Seems like the information in these fields overlap and may be hard to always keep them in sync?

[pwmarcz]

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

LibOS/shim/src/fs/shim_dcache.c, line 473 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

How does this type field correspond to the underlying FS implementation on our side? Seems like the information in these fields overlap and may be hard to always keep them in sync?

Currently, yes, unfortunately individual filesystems (mostly chroot) also keep some type information, and do this messy synchronization.

My goal (in a few PRs...) is to remove these extra sources of information, and always write to type. This already happened for file permissions.

[mkow]

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[dimakuv]

Reviewed 12 of 12 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Reviewed 12 of 12 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved