graphene icon indicating copy to clipboard operation
graphene copied to clipboard

[LibOS] Filesystem code needs to be rewritten

Open mkow opened this issue 5 years ago • 4 comments

Description of the problem

Our old filesystem implementation requires complete rewrite.

  • The code is ugly and unmaintainable, especially everything around dentry structures.
  • There are tons of bugs there, each time we touch this subsystem in one place something unrelated in another place breaks.
  • Refcounting is totally broken, most likely we still have memory leaks and use-after-free bugs around there.
  • Symlinks doesn't work correctly (@boryspoplawski should remember more details, see e.g. #1387)
  • Mountpoints have inconsistent and not too reasonable semantics. See e.g. #1605.
  • We may need to add inode abstraction to emulate some of the Linux features correctly.

It's hard to implement any new feature for the filesystem without dealing with this issue first.

Related issues

  • #36 ("The files in the '/proc' directory are empty")
  • #82 ("Cannot allow the files in current directory to be accessed by Graphene")
  • #382 ("chroot_rename corrupts handles open to old file")
  • #437 ("Basic file locking function support in LibOS")
  • #469 ("Function "dir_read" in PAL layer doesn't handle the case where offset > 0")
  • #496 ("Drop one from the two dent's mode variables used in shim_dentry")
  • #516 ("[LibOS] Missing support for readlink syscall")
  • #570 ("[PAL/SGX] file_attrquery hangs if called on a named pipe")
  • #744 ("[LibOS] FS-specific checkpoint callback must not leak checkpoint data")
  • #786 ("statfs returns hard-coded values instead of real data")
  • #787 ("chown/fchownat are no-ops")
  • #836 ("[LibOS] Rework the getdents design to remove "struct shim_dir_handle dir_info"")
  • #903 ("[LibOS] rename syscall is not working")
  • #944 ("FS-related operations are racy and need proper locking")
  • #948 ("[LibOS] Multiple issues with /proc")
  • #951 ("[LibOS] sendfile does not work correctly")
  • #952 ("[LibOS] Listing a directory does not show mountpoints")
  • #1166 ("[Pal] file_open() creates files even if they aren't whitelisted")
  • #1188 ("[Linux-SGX] File writes with write() are not synced to a mmap'd buffer")
  • #1248 ("[LibOS] Unlink of non-readable file is broken")
  • #1337 ("UTF-8 characters in trusted/allowed file names")
  • #1387 ("[LibOS] Targets of links are always assumed to be under chroot")
  • #1605 ("undefined root directory and mount semantics")
  • #1802 ("[LibOS] Missing support for flock syscall")
  • #1841 ("[LibOS] dentry cache for filesystem can cause conflict between stat() result and host file status")
  • #2052 ("assert failed fs/shim_fs.c:533 dent == dent2 in DEBUG mode only")
  • #2069 ("lstat syscall fails to provide the right st.mode, uid gid etc. when the path is a symbolic link")
  • #2104 ("[LibOS] file/dir attributes of chroot are incorrect after created and removed multiply times")
  • #2142 ("[LibOS] Unlink of empty path as dir causes failure")
  • #2143 ("[LibOS] No input validation is done for flag parameter in fchownat syscall")
  • #2144 ("[LibOS] Graphene doesn't support/hold file permissions")
  • #2214 ("Mountpoint order is not preserved in current manifest syntax")

mkow avatar Sep 09 '20 15:09 mkow

I'd like to note that there is no shared and consistent view of the FS across Graphene processes. For example, a parent Graphene process and a child Graphene process will not see new/deleted files from the other process until they actually go and ask the host FS (recall that Graphene e.g. caches directory contents, so if parent listed the directory, then the child created a file in this directory, and parent lists the directory again, the parent won't see the new file).

However, implementing such a distributed view of the file system sounds quite complicated. I think we can get away with this by just purging all cached FS metadata in Graphene on each fork/exec.

dimakuv avatar Sep 09 '20 15:09 dimakuv

One common source of confusion is our chroot semantics of Graphene: people have a hard time wrapping their heads around the concept of chroot jails and just want everything from the host FS to be visible/accessible inside Graphene.

I believe we should implement a "God-like" option in Graphene to bind a single mount point to host's / (something that GSC does already). This way everything is mirrored 1:1. We'll need a couple of exceptions to this rule:

  1. graphene/Runtime/ should take precedence to all other lib search directories.
  2. Stuff like /etc should probably also be possible to redirect.

Basically, we need a way to specify the root dir /, and then "augment" with Graphene-specific directories which take precedence.

dimakuv avatar Sep 09 '20 16:09 dimakuv

I believe we should implement a "God-like" option in Graphene to bind a single mount point to host's /

No, I don't think we should allow this, sounds like a big footgun :) We should allow mounting all paths inside Graphene (including /), but IMO we shouldn't have an option to mount host's / to Graphene.

Ad mounting you described: IMO we should have Linux-like semantics of mounts, that you can stack them and we'll just execute the mount commands from the manifest one by one. This should allow for all possible configurations we may ever need.

mkow avatar Sep 09 '20 18:09 mkow

@pwmarcz It would be good to go through the "master list of issues" again and verify which ones were already fixed/became irrelevant. After this we can close this issue.

I don't want to transfer this issue to the new Gramine repo since it's a meta-issue which is almost completely fixed at this point.

dimakuv avatar Sep 23 '21 11:09 dimakuv