[LibOS] Do not reuse dentries after removing a file

This change ensures that dentries for removed files are not reused.
Instead of marking a dentry as negative, we mark it as invalid, and make
sure that a new one is created during next lookup.

This fixes a crash where we create a named pipe, remove it, and then
reuse the same object for a normal file. More generally, it ensures that
filesystems always receive a dentry in a consistent state.

Several unused dentry flags are also removed.

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 children of a given dentry. */
     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

@@ -100,6 +100,12 @@ void get_dentry(struct shim_dentry* dent) {
 }
 
 static void free_dentry(struct shim_dentry* dent) {
+    if (dent->fs && dent->fs->d_ops && dent->fs->d_ops->dput) {
+        int ret = dent->fs->d_ops->dput(dent);
+        if (ret < 0)
+            log_warning("dput() failed on %s: %d", qstrgetstr(&dent->name), ret);
+    }
+
     if (dent->mount) {
         put_mount(dent->mount);
     }
@@ -118,9 +124,6 @@ static void free_dentry(struct shim_dentry* dent) {
         put_mount(dent->attached_mount);
     }
 
-    /* XXX: We are leaking `data` field here. This field seems to have different meaning for
-     * different dentries and how to free it is a mystery to me. */
-
     destroy_lock(&dent->lock);
 
     free_mem_obj_to_mgr(dentry_mgr, dent);
@@ -214,7 +217,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 +381,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 +447,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/sys/shim_file.c

@@ -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,21 @@ 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;
+        }
     }
 
     return 0;