[LibOS] Split shim_mount and shim_fs

LibOS/shim/include/shim_fs.h

@@ -122,8 +122,15 @@ DEFINE_LISTP(shim_dentry);
 struct shim_dentry {
     int state; /* flags for managing state */
 
-    struct shim_mount* fs;     /* this dentry's mounted fs */
-    struct shim_qstr name;     /* caching the file's name. */
+    /* File name, maximum of NAME_MAX characters. By convention, the root has an empty name. */
+    struct shim_qstr name;
+
+    /* Mounted filesystem this dentry belongs to */
+    struct shim_mount* mount;
+
+    /* Filesystem to use for operations on this file: this is usually `mount->fs`, but can be
+     * different in case of special files (such as named pipes or sockets). */
+    struct shim_fs* fs;
 
     struct shim_dentry* parent;
     size_t nchildren;
@@ -216,23 +223,27 @@ struct shim_d_ops {
 #define NAME_MAX 255   /* filename length, NOT including null terminator */
 #define PATH_MAX 4096  /* path size, including null terminator */
 
+struct shim_fs {
+    /* Null-terminated, used in manifest and for uniquely identifying a filesystem. */
+    char name[8];
+    struct shim_fs_ops* fs_ops;
+    struct shim_d_ops* d_ops;
+};
+
 DEFINE_LIST(shim_mount);
 struct shim_mount {
-    char type[8];  // Null-terminated.
+    struct shim_fs* fs;
 
     /*
-     * TODO: this field currently functions as both the mountpoint AND the filesystem root. Instead,
-     * `mount_point` should be the dentry from the parent filesystem, and `root` should be the
-     * filesystem root.
+     * TODO: this field currently functions as both the mountpoint AND the mount root. Instead,
+     * `mount_point` should be the dentry from the parent filesystem, and `root` should be the mount
+     * root.
      */
     struct shim_dentry* mount_point;
 
     struct shim_qstr path;
     struct shim_qstr uri;
 
-    struct shim_fs_ops* fs_ops;
-    struct shim_d_ops* d_ops;
-
     /* TODO: this is unused right now (see `mount_point`) */
     struct shim_dentry* root;
 
@@ -277,19 +288,12 @@ int init_mount(void);
 int mount_fs(const char* mount_type, const char* mount_uri, const char* mount_point,
              struct shim_dentry* parent, struct shim_dentry** dentp, bool make_ancestor);
 int unmount_fs(const char* mount_point);
-int search_builtin_fs(const char* type, struct shim_mount** fs);
 
 void get_mount(struct shim_mount* mount);
 void put_mount(struct shim_mount* mount);
 
 struct shim_mount* find_mount_from_uri(const char* uri);
 
-static inline void set_handle_fs(struct shim_handle* hdl, struct shim_mount* fs) {
-    get_mount(fs);
-    hdl->fs = fs;
-    memcpy(hdl->fs_type, fs->type, sizeof(hdl->fs_type));
-}
-
 int walk_mounts(int (*walk)(struct shim_mount* mount, void* arg), void* arg);
 
 /* functions for dcache supports */
@@ -569,24 +573,24 @@ ino_t dentry_ino(struct shim_dentry* dent);
  * \brief Allocate and initialize a new dentry
  *
  * \param parent the parent node, or NULL if this is supposed to be the dentry root
- * \param fs the filesystem the dentry is under, or NULL
+ * \param mount the mount the dentry is under
  * \param name name of the new dentry
  * \param name_len length of the name
  *
  * \return the new dentry, or NULL in case of allocation failure
  *
  * The caller should hold `g_dcache_lock`.
  *
- * The function will initialize the following fields: `fs` (if provided), `name`, and
- * parent/children links.
+ * The function will initialize the following fields: `mount` and `fs` (if `mount` provided),
+ * `name`, and parent/children links.
  *
  * The reference count of the returned dentry will be 2 if `parent` was provided, 1 otherwise.
  *
- * The `fs` parameter should typically be `parent->fs`, but is passed explicitly to support
- * initializing the root dentry of a newly mounted filesystem. If `fs` is NULL, the resulting
- * dentry's filesystem will be left as NULL.
+ * The `mount` parameter should typically be `parent->mount`, but is passed explicitly to support
+ * initializing the root dentry of a newly mounted filesystem. The `fs` field will be initialized to
+ * `mount->fs`, but you can later change it to support special files.
  */
-struct shim_dentry* get_new_dentry(struct shim_mount* fs, struct shim_dentry* parent,
+struct shim_dentry* get_new_dentry(struct shim_mount* mount, struct shim_dentry* parent,
                                    const char* name, size_t name_len);
 
 /*!
@@ -641,12 +645,18 @@ extern struct shim_d_ops str_d_ops;
 extern struct shim_fs_ops tmp_fs_ops;
 extern struct shim_d_ops tmp_d_ops;
 
-extern struct shim_mount chroot_builtin_fs;
-extern struct shim_mount pipe_builtin_fs;
-extern struct shim_mount fifo_builtin_fs;
-extern struct shim_mount socket_builtin_fs;
-extern struct shim_mount epoll_builtin_fs;
-extern struct shim_mount eventfd_builtin_fs;
+extern struct shim_fs chroot_builtin_fs;
+extern struct shim_fs proc_builtin_fs;
+extern struct shim_fs dev_builtin_fs;
+extern struct shim_fs sys_builtin_fs;
+extern struct shim_fs tmp_builtin_fs;
+extern struct shim_fs pipe_builtin_fs;
+extern struct shim_fs fifo_builtin_fs;
+extern struct shim_fs socket_builtin_fs;
+extern struct shim_fs epoll_builtin_fs;
+extern struct shim_fs eventfd_builtin_fs;
+
+struct shim_fs* find_fs(const char* name);
 
 /* pseudo file systems (separate treatment since they don't have associated dentries) */
 #define DIR_RX_MODE  0555

LibOS/shim/include/shim_handle.h

@@ -291,7 +291,7 @@ struct shim_epoll_handle {
     LISTP_TYPE(shim_epoll_item) fds;
 };
 
-struct shim_mount;
+struct shim_fs;
 struct shim_qstr;
 struct shim_dentry;
 
@@ -303,8 +303,7 @@ struct shim_handle {
 
     REFTYPE ref_count;
 
-    char fs_type[8];
-    struct shim_mount* fs;
+    struct shim_fs* fs;
     struct shim_dentry* dentry;
 
     /* If this handle is registered for any epoll handle, this list contains

LibOS/shim/src/bookkeep/shim_handle.c

@@ -45,7 +45,7 @@ static int init_tty_handle(struct shim_handle* hdl, bool write) {
     if (ret < 0)
         return ret;
 
-    set_handle_fs(hdl, dent->fs);
+    hdl->fs = dent->fs;
     hdl->dentry = dent;
     return 0;
 }
@@ -73,15 +73,27 @@ static inline int init_exec_handle(void) {
     exec->flags    = O_RDONLY;
     exec->acc_mode = MAY_READ;
 
-    struct shim_mount* fs = find_mount_from_uri(g_pal_control->executable);
-    if (fs) {
-        const char* p = g_pal_control->executable + fs->uri.len;
+    /*
+     * Find a mounted filesystem for an executable, and initialize the handle (dentry and fs).
+     *
+     * TODO: The below code is broken, and should be rewritten:
+     *
+     * - Instead of manually constructing the handle, use the proper function for opening a file
+     *   (`open_namei`).
+     *
+     * - The treatment of relative paths is wrong, and really only makes sense for the default case
+     *   when the root mount is "file:." and executable is "file:<executable>". We should probably
+     *   convert both of these to absolute paths first.
+     */
+    struct shim_mount* mount = find_mount_from_uri(g_pal_control->executable);
+    if (mount) {
+        const char* p = g_pal_control->executable + mount->uri.len;
         /* Lookup for `g_pal_control->executable` needs to be done under a given mount point which
          * requires a relative path name. OTOH the one in manifest file can be absolute path. */
         while (*p == '/') {
             p++;
         }
-        int ret = path_lookupat(fs->mount_point, p, LOOKUP_FOLLOW, &exec->dentry);
+        int ret = path_lookupat(mount->mount_point, p, LOOKUP_FOLLOW, &exec->dentry);
         if (ret < 0) {
             log_error("init_exec_handle: cannot find executable in filesystem: %d\n", ret);
             return ret;
@@ -91,15 +103,20 @@ static inline int init_exec_handle(void) {
             log_error("init_exec_handle: executable is not a regular file\n");
             return -EINVAL;
         }
-        if (exec->dentry->fs != fs) {
+        if (exec->dentry->mount != mount) {
             log_error("init_exec_handle: cannot find executable in filesystem, "
                       "it seems to be shadowed by a mount\n");
             return -EINVAL;
         }
-        set_handle_fs(exec, fs);
-        put_mount(fs);
+        exec->fs = mount->fs;
+        put_mount(mount);
     } else {
-        set_handle_fs(exec, &chroot_builtin_fs);
+        /*
+         * TODO: This is for cases where the above lookup fails, e.g. `host_root_fs` regression test
+         * (which mounts "file:/" as root). We construct a dentry-less handle in such cases, but
+         * once the lookup is fixed, we should return an error instead.
+         */
+        exec->fs = &chroot_builtin_fs;
     }
 
     lock(&g_process.fs_lock);
@@ -396,8 +413,8 @@ static inline __attribute__((unused)) const char* __handle_name(struct shim_hand
         return qstrgetstr(&hdl->uri);
     if (hdl->dentry && !qstrempty(&hdl->dentry->name))
         return qstrgetstr(&hdl->dentry->name);
-    if (hdl->fs_type[0])
-        return hdl->fs_type;
+    if (hdl->fs)
+        return hdl->fs->name;
     return "(unknown)";
 }
 
@@ -455,9 +472,6 @@ void put_handle(struct shim_handle* hdl) {
         if (hdl->dentry)
             put_dentry(hdl->dentry);
 
-        if (hdl->fs)
-            put_mount(hdl->fs);
-
         destroy_handle(hdl);
     }
 }
@@ -655,23 +669,18 @@ BEGIN_CP_FUNC(handle) {
         new_hdl = (struct shim_handle*)(base + off);
 
         lock(&hdl->lock);
-        struct shim_mount* fs = hdl->fs;
-        *new_hdl              = *hdl;
+        *new_hdl = *hdl;
 
-        if (fs && fs->fs_ops && fs->fs_ops->checkout)
-            fs->fs_ops->checkout(new_hdl);
+        if (hdl->fs && hdl->fs->fs_ops && hdl->fs->fs_ops->checkout)
+            hdl->fs->fs_ops->checkout(new_hdl);
 
         new_hdl->dentry = NULL;
         REF_SET(new_hdl->ref_count, 0);
         clear_lock(&new_hdl->lock);
 
-        DO_CP_IN_MEMBER(qstr, new_hdl, uri);
+        DO_CP(fs, hdl->fs, &new_hdl->fs);
 
-        if (fs && fs != &fifo_builtin_fs && hdl->dentry) {
-            DO_CP_MEMBER(mount, hdl, new_hdl, fs);
-        } else {
-            new_hdl->fs = NULL;
-        }
+        DO_CP_IN_MEMBER(qstr, new_hdl, uri);
 
         if (hdl->dentry) {
             if (hdl->dentry->state & DENTRY_ISDIRECTORY) {
@@ -734,17 +743,6 @@ BEGIN_RS_FUNC(handle) {
         return -ENOMEM;
     }
 
-    if (!hdl->fs) {
-        assert(hdl->fs_type);
-        search_builtin_fs(hdl->fs_type, &hdl->fs);
-        if (!hdl->fs) {
-            destroy_lock(&hdl->lock);
-            return -EINVAL;
-        }
-    } else {
-        get_mount(hdl->fs);
-    }
-
     if (hdl->dentry) {
         get_dentry(hdl->dentry);
     }

LibOS/shim/src/bookkeep/shim_vma.c

@@ -1294,7 +1294,7 @@ BEGIN_RS_FUNC(vma) {
 
     if (!(vma->flags & VMA_UNMAPPED)) {
         if (vma->file) {
-            struct shim_mount* fs = vma->file->fs;
+            struct shim_fs* fs = vma->file->fs;
             get_handle(vma->file);
 
             if (need_mapped < vma->addr + vma->length) {

LibOS/shim/src/fs/chroot/fs.c

@@ -31,8 +31,7 @@ struct mount_data {
     char root_uri[];
 };
 
-#define HANDLE_MOUNT_DATA(h) ((struct mount_data*)(h)->fs->data)
-#define DENTRY_MOUNT_DATA(d) ((struct mount_data*)(d)->fs->data)
+#define DENTRY_MOUNT_DATA(d) ((struct mount_data*)(d)->mount->data)
 
 static int chroot_mount(const char* uri, void** mount_data) {
     enum shim_file_type type;
@@ -830,9 +829,6 @@ static int chroot_readdir(struct shim_dentry* dent, readdir_callback_t callback,
 }
 
 static int chroot_checkout(struct shim_handle* hdl) {
-    if (hdl->fs == &chroot_builtin_fs)
-        hdl->fs = NULL;
-
     if (hdl->type == TYPE_FILE) {
         struct shim_file_data* data = FILE_HANDLE_DATA(hdl);
         if (data)
@@ -1027,14 +1023,8 @@ struct shim_d_ops chroot_d_ops = {
     .chmod   = &chroot_chmod,
 };
 
-struct mount_data chroot_data = {
-    .root_uri_len = 5,
-    .root_uri     = URI_PREFIX_FILE,
-};
-
-struct shim_mount chroot_builtin_fs = {
-    .type   = "chroot",
+struct shim_fs chroot_builtin_fs = {
+    .name   = "chroot",
     .fs_ops = &chroot_fs_ops,
     .d_ops  = &chroot_d_ops,
-    .data   = &chroot_data,
 };

LibOS/shim/src/fs/dev/fs.c

@@ -211,3 +211,9 @@ struct shim_d_ops dev_d_ops = {
     .stat        = &dev_stat,
     .follow_link = &dev_follow_link,
 };
+
+struct shim_fs dev_builtin_fs = {
+    .name   = "dev",
+    .fs_ops = &dev_fs_ops,
+    .d_ops  = &dev_d_ops,
+};

LibOS/shim/src/fs/eventfd/fs.c

@@ -87,7 +87,7 @@ struct shim_fs_ops eventfd_fs_ops = {
     .poll  = &eventfd_poll,
 };
 
-struct shim_mount eventfd_builtin_fs = {
-    .type   = URI_TYPE_EVENTFD,
+struct shim_fs eventfd_builtin_fs = {
+    .name   = "eventfd",
     .fs_ops = &eventfd_fs_ops,
 };

LibOS/shim/src/fs/pipe/fs.c

@@ -93,11 +93,6 @@ static int pipe_hstat(struct shim_handle* hdl, struct stat* stat) {
     return 0;
 }
 
-static int pipe_checkout(struct shim_handle* hdl) {
-    hdl->fs = NULL;
-    return 0;
-}
-
 static off_t pipe_poll(struct shim_handle* hdl, int poll_type) {
     off_t ret = 0;
 
@@ -183,7 +178,7 @@ static int fifo_open(struct shim_handle* hdl, struct shim_dentry* dent, int flag
          * one end (read or write) in our emulation, so we treat such FIFOs as read-only. This
          * covers most apps seen in the wild (in particular, LTP apps). */
         log_warning("FIFO (named pipe) '%s' cannot be opened in read-write mode in Graphene. "
-                    "Treating it as read-only.\n", qstrgetstr(&dent->fs->path));
+                    "Treating it as read-only.\n", qstrgetstr(&dent->mount->path));
         flags = O_RDONLY;
     }
 
@@ -245,7 +240,6 @@ static struct shim_fs_ops pipe_fs_ops = {
     .read     = &pipe_read,
     .write    = &pipe_write,
     .hstat    = &pipe_hstat,
-    .checkout = &pipe_checkout,
     .poll     = &pipe_poll,
     .setflags = &pipe_setflags,
 };
@@ -261,13 +255,13 @@ static struct shim_d_ops fifo_d_ops = {
     .open = &fifo_open,
 };
 
-struct shim_mount pipe_builtin_fs = {
-    .type   = URI_TYPE_PIPE,
+struct shim_fs pipe_builtin_fs = {
+    .name   = "pipe",
     .fs_ops = &pipe_fs_ops,
 };
 
-struct shim_mount fifo_builtin_fs = {
-    .type   = "fifo",
+struct shim_fs fifo_builtin_fs = {
+    .name   = "fifo",
     .fs_ops = &fifo_fs_ops,
     .d_ops  = &fifo_d_ops,
 };