[LibOS] Do not reuse dentries after removing a file (fixes mknod crash)

LibOS/shim/include/shim_fs.h

@@ -91,14 +91,9 @@ struct shim_fs_ops {
 
 #define DENTRY_VALID       0x0001 /* this dentry is verified to be valid */
 #define DENTRY_NEGATIVE    0x0002 /* recently deleted or inaccessible */
-#define DENTRY_PERSIST     0x0008 /* added as a persistent dentry */
 #define DENTRY_ISLINK      0x0080 /* this dentry is a link */
 #define DENTRY_ISDIRECTORY 0x0100 /* this dentry is a directory */
-#define DENTRY_LOCKED      0x0200 /* locked by mountpoints at children */
 /* These flags are not used */
-//#define DENTRY_REACHABLE    0x0400  /* permission checked to be reachable */
-//#define DENTRY_UNREACHABLE  0x0800  /* permission checked to be unreachable */
-#define DENTRY_LISTED      0x1000 /* children in directory listed */
 #define DENTRY_SYNTHETIC   0x4000 /* Auto-generated dentry to connect a mount point in the        \
                                    * manifest to the root, when one or more intermediate          \
                                    * directories do not exist on the underlying FS. The semantics \
@@ -121,7 +116,7 @@ struct shim_dentry {
     int state; /* flags for managing state */
 
     /* File name, maximum of NAME_MAX characters. By convention, the root has an empty name. Does
-     * not change. */
+     * not change. The name is unique for all valid dentries in a given siblings list. */
     struct shim_qstr name;
 
     /* Mounted filesystem this dentry belongs to. Does not change. */
@@ -628,7 +623,7 @@ struct shim_dentry* get_new_dentry(struct shim_mount* mount, struct shim_dentry*
                                    const char* name, size_t name_len);
 
 /*!
- * \brief Search for a child of a dentry with a given name
+ * \brief Search for a valid child of a dentry with a given name
  *
  * \param parent the dentry to search under
  * \param name name of searched dentry
@@ -638,6 +633,10 @@ struct shim_dentry* get_new_dentry(struct shim_mount* mount, struct shim_dentry*
  *
  * The caller should hold `g_dcache_lock`.
  *
+ * This function retrieves a valid (DENTRY_VALID bit set) dentry with a given name. Invalid dentries
+ * are ignored: even if an invalid dentry exists, you should create a fresh one (the old one will
+ * likely be deleted soon, see `dentry_gc`).
+ *
  * If found, the reference count on the returned dentry is incremented.
  */
 struct shim_dentry* lookup_dcache(struct shim_dentry* parent, const char* name, size_t name_len);

LibOS/shim/src/fs/shim_dcache.c

@@ -214,7 +214,7 @@ struct shim_dentry* lookup_dcache(struct shim_dentry* parent, const char* name,
     struct shim_dentry* tmp;
     struct shim_dentry* dent;
     LISTP_FOR_EACH_ENTRY_SAFE(dent, tmp, &parent->children, siblings) {
-        if (qstrcmpstr(&dent->name, name, name_len) == 0) {
+        if ((dent->state & DENTRY_VALID) && qstrcmpstr(&dent->name, name, name_len) == 0) {
             get_dentry(dent);
             return dent;
         }
@@ -378,7 +378,6 @@ static void dump_dentry(struct shim_dentry* dent, unsigned int level) {
     buf_printf(&buf, "[%6.6s ", dent->mount ? dent->mount->fs->name : "");
 
     DUMP_FLAG(DENTRY_VALID, "V", ".");
-    DUMP_FLAG(DENTRY_LISTED, "L", ".");
     DUMP_FLAG(DENTRY_SYNTHETIC, "S", ".");
     buf_printf(&buf, "%3d] ", (int)REF_GET(dent->ref_count));
 
@@ -445,9 +444,6 @@ BEGIN_CP_FUNC(dentry) {
         clear_lock(&new_dent->lock);
         REF_SET(new_dent->ref_count, 0);
 
-        /* we don't checkpoint children dentries, so need to list directory again */
-        new_dent->state &= ~DENTRY_LISTED;
-
         if (new_dent->type != S_IFIFO) {
             /* not FIFO, no need to keep data (FIFOs stash internal FDs into data field) */
             new_dent->data = NULL;

LibOS/shim/src/fs/str/fs.c

@@ -44,7 +44,7 @@ int str_open(struct shim_handle* hdl, struct shim_dentry* dent, int flags) {
 int str_dput(struct shim_dentry* dent) {
     struct shim_str_data* data = dent->data;
 
-    if (!data || REF_DEC(data->ref_count) > 1)
+    if (!data || REF_DEC(data->ref_count) > 0)
         return 0;
 
     if (data->str) {

LibOS/shim/src/fs/tmpfs/fs.c

@@ -172,7 +172,7 @@ static int tmpfs_dput(struct shim_dentry* dent) {
     lock(&dent->lock);
     struct shim_tmpfs_data* tmpfs_data = dent->data;
 
-    if (!tmpfs_data || REF_DEC(tmpfs_data->str_data.ref_count) > 1) {
+    if (!tmpfs_data || REF_DEC(tmpfs_data->str_data.ref_count) > 0) {
         unlock(&dent->lock);
         return 0;
     }

LibOS/shim/src/sys/shim_file.c

@@ -36,7 +36,7 @@ long shim_do_unlinkat(int dfd, const char* pathname, int flag) {
 
     struct shim_dentry* dir = NULL;
     struct shim_dentry* dent = NULL;
-    int ret = 0;
+    int ret;
 
     if (*pathname != '/' && (ret = get_dirfd_dentry(dfd, &dir)) < 0)
         return ret;
@@ -65,14 +65,10 @@ long shim_do_unlinkat(int dfd, const char* pathname, int flag) {
         if ((ret = dent->fs->d_ops->unlink(dent->parent, dent)) < 0) {
             goto out;
         }
-    } else {
-        dent->state |= DENTRY_PERSIST;
     }
 
-    if (flag & AT_REMOVEDIR)
-        dent->state &= ~DENTRY_ISDIRECTORY;
-
-    dent->state |= DENTRY_NEGATIVE;
+    dent->state &= ~DENTRY_VALID;
+    ret = 0;
 out:
     if (dir)
         put_dentry(dir);
@@ -127,12 +123,10 @@ long shim_do_rmdir(const char* pathname) {
     if (dent->fs && dent->fs->d_ops && dent->fs->d_ops->unlink) {
         if ((ret = dent->fs->d_ops->unlink(dent->parent, dent)) < 0)
             goto out;
-    } else {
-        dent->state |= DENTRY_PERSIST;
     }
 
-    dent->state &= ~DENTRY_ISDIRECTORY;
-    dent->state |= DENTRY_NEGATIVE;
+    dent->state &= ~DENTRY_VALID;
+    ret = 0;
 out:
     put_dentry(dent);
     return ret;
@@ -170,8 +164,6 @@ long shim_do_fchmodat(int dfd, const char* filename, mode_t mode) {
     if (dent->fs && dent->fs->d_ops && dent->fs->d_ops->chmod) {
         if ((ret = dent->fs->d_ops->chmod(dent, mode)) < 0)
             goto out_dent;
-    } else {
-        dent->state |= DENTRY_PERSIST;
     }
 
     dent->perm = mode;
@@ -202,8 +194,6 @@ long shim_do_fchmod(int fd, mode_t mode) {
     if (dent->fs && dent->fs->d_ops && dent->fs->d_ops->chmod) {
         if ((ret = dent->fs->d_ops->chmod(dent, mode)) < 0)
             goto out;
-    } else {
-        dent->state |= DENTRY_PERSIST;
     }
 
     dent->perm = mode;
@@ -539,12 +529,13 @@ static int do_rename(struct shim_dentry* old_dent, struct shim_dentry* new_dent)
     /* TODO: Add appropriate checks for hardlinks once they get implemented. */
 
     int ret = old_dent->fs->d_ops->rename(old_dent, new_dent);
-    if (!ret) {
-        old_dent->state |= DENTRY_NEGATIVE;
-        new_dent->state &= ~DENTRY_NEGATIVE;
-    }
+    if (ret < 0)
+        return ret;
 
-    return ret;
+    old_dent->state &= ~DENTRY_VALID;
+    new_dent->state &= ~DENTRY_NEGATIVE;
+
+    return 0;
 }
 
 long shim_do_rename(const char* oldpath, const char* newpath) {

LibOS/shim/test/regression/mkfifo.c

@@ -93,6 +93,23 @@ int main(int argc, char** argv) {
             perror("[parent] unlink error");
             return 1;
         }
+
+        /* Check if we can create a normal file with the same name. */
+        fd = open(FIFO_PATH, O_CREAT | O_TRUNC, 0600);
+        if (fd < 0) {
+            perror("[parent] open error");
+            return 1;
+        }
+        if (close(fd) < 0) {
+            perror("[parent] close error");
+            return 1;
+        }
+        if (unlink(FIFO_PATH) < 0) {
+            perror("[parent] unlink error");
+            return 1;
+        }
+
+        printf("[parent] TEST OK\n");
     }
 
     return 0;

LibOS/shim/test/regression/test_libos.py

@@ -865,6 +865,7 @@ def test_092_pipe_ocloexec(self):
     def test_095_mkfifo(self):
         stdout, _ = self.run_binary(['mkfifo'], timeout=60)
         self.assertIn('read on FIFO: Hello from write end of FIFO!', stdout)
+        self.assertIn('[parent] TEST OK', stdout)
 
     def test_100_socket_unix(self):
         stdout, _ = self.run_binary(['unix'])