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

[pwmarcz]

Found while investigating #2419.

Description of the changes

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.

How to test this PR?

• Should fix the mknod crash in Stress-ng test suite causing memory fault and heap corruption failure in Graphene #2419
  • Note that the mknod stress test still fails, because our mknod doesn't create sockets, and hit EMFILE from hst on creating too many named pipes). However, it should not crash anymore.
• I added a short snippet to the end of mkfifo that crashes in same manner on master.
• To verify the operations on dentries (e.g. an entry is invalidated and removed later), I added dump_dcache(NULL); to shim_exit.c, and ran a few sanity checks with graphene-direct python -c "<script>":
  • import os; open('x', 'w').close(); os.unlink('x') (should show an invalid dentry)
  • ...; os.path.exists('x') (should show a new, negative dentry, with no mode etc. set)
  • ...; os.readdir(".") (should show no dentry, due to lookup triggering dentry_gc())

---

This change is 

[dimakuv]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 2 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/src/sys/shim_file.c, line 39 at r1 (raw file):

    struct shim_dentry* dir = NULL;
    struct shim_dentry* dent = NULL;
    int ret = 0;

Can we remove = 0 now, so that the compiler may catch errors on unassigned-ret paths?

---

LibOS/shim/test/regression/mkfifo.c, line 112 at r1 (raw file):

        }
    }

Can we add TEST OK here, just for sanity?

[pwmarcz]

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions, 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)

a discussion (no related file):
Update: Unfortunately the FS code doesn't seem to be ready for this fix :( It conflicts with reference counting in strfs/tmpfs.

I could proceed without adding dput, but it will make the memory leak (when deleting a dentry) worse, as we'll keep creating and destroying new dentries.

I will prepare a quick fix for this issue with mknod specifically.

---

---

LibOS/shim/src/sys/shim_file.c, line 39 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we remove = 0 now, so that the compiler may catch errors on unassigned-ret paths?

Done.

---

LibOS/shim/test/regression/mkfifo.c, line 112 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we add TEST OK here, just for sanity?

Done.