Ensure dentry type and permissions are always stored

[pwmarcz]

This change is in preparation for bigger refactoring: simplifying directory handles, getdents and getdents64. It also unblocks changing the mount semantics (indirectly: there are special files, namely Unix sockets and named pipes, that get super-special treatment in Graphene and having the type field will allow me to make that explicit).

In principle, this refactoring could be taken further:

• remove mode() callback
• remove DENTRY_DIRECTORY, DENTRY_ISLINK flags, as they're redundant now
• remove various type fields throughout the code

However, when I tried to do any of these, the change snowballed, so I want to make further changes gradually. The important part is that the type field is available as part of dentry, because it unblocks many other simplifications in the code.

How to test this PR?

The existing tests are good enough, but for manual testing/debugging I was also adding dump_dcache(NULL); to shim_exit.c, and checked if any file type/permissions are missing.

---

This change is 

[dimakuv]

Reviewed 12 of 12 files at r1.
Reviewable status: all 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) (waiting on @pwmarcz)

---

LibOS/shim/src/fs/shim_dcache.c, line 375 at r1 (raw file):

static void dump_dentry_mode(struct print_buf* buf, mode_t type, mode_t perm) {
    buf_printf(buf, "%06o ", type | perm);
    

Please remove whitespace

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 109 at r1 (raw file):

}

static int proc_ipc_thread_link_open(struct shim_handle* hdl, const char* name, int flags) {

Why was this open callback removed? It was never used?

---

LibOS/shim/src/fs/proc/thread.c, line 139 at r1 (raw file):

}

static int proc_thread_link_open(struct shim_handle* hdl, const char* name, int flags) {

ditto (why removed?)

[pwmarcz]

Reviewable status: all 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) (waiting on @dimakuv)

---

LibOS/shim/src/fs/shim_dcache.c, line 375 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove whitespace

Done.

---

LibOS/shim/src/fs/proc/ipc-thread.c, line 109 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why was this open callback removed? It was never used?

This is a callback for symlinks (/proc/xxx/{cwd,exe,root}). You are not able to open() them directly. This open implementation attempted to open the target file, but as long as they're properly marked as symlinks (with S_IFLNK), the filesystem will use the follow_link operation.

(And actually ipc-thread.c seems pretty broken right now... I created issue #2385 for this).

---

LibOS/shim/src/fs/proc/thread.c, line 139 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto (why removed?)

Same as above - this is for symlinks (/proc/xxx/{cwd,exe,root}), you cannot open them.

You can check using Busybox that they're indeed treated as symlinks:

$ graphene-direct busybox sh -c 'ls -l /proc/1/'
total 0
-r--------    0 root     root             0 Jan  1  1970 cmdline
lr--------    0 root     root             0 Jan  1  1970 cwd -> /
lr--------    0 root     root             0 Jan  1  1970 exe -> /busybox
dr-x------    3 root     root          4096 Jan  1  1970 fd
-r--------    0 root     root             0 Jan  1  1970 maps
lr--------    0 root     root             0 Jan  1  1970 root -> /
dr-x------    3 root     root          4096 Jan  1  1970 task

[mkow]

Reviewed 11 of 12 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 6 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 and @pwmarcz)

---

LibOS/shim/include/shim_fs.h, line 180 at r1 (raw file):

the caller will update dentry

You mean rather "the caller has to update"? I.e. this is a requirement for the callers, right?

---

LibOS/shim/src/fs/chroot/fs.c, line 237 at r1 (raw file):

            break;
        default:
            log_warning("unknown PAL handle type");

not an error?

---

LibOS/shim/src/fs/chroot/fs.c, line 340 at r1 (raw file):

        stat->st_ctime = (time_t)data->ctime;
        stat->st_nlink = data->nlink;
   }

this is misindented

[dimakuv]

Reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

[pwmarcz]

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

LibOS/shim/include/shim_fs.h, line 180 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

the caller will update dentry

You mean rather "the caller has to update"? I.e. this is a requirement for the callers, right?

I assumed this describes the interface from the perspective of whoever will be implementing chmod, not calling it. See for instance Linux with phrases like "called when...", "should return...", "the caller will attempt to mount..."

I changed it to "has to update" for consistency, but I'll return here when I have an actual contract for these functions, and write detailed comments. Right now I would be afraid of lying in them :)

---

LibOS/shim/src/fs/chroot/fs.c, line 237 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

not an error?

The original code simply omitted default, but yeah, let's at least return an error. Changed to return -EINVAL.

---

LibOS/shim/src/fs/chroot/fs.c, line 340 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

this is misindented

Fixed.

[mkow]

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion, 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 @pwmarcz)

---

LibOS/shim/src/fs/chroot/fs.c, line 237 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

The original code simply omitted default, but yeah, let's at least return an error. Changed to return -EINVAL.

What I meant is even stronger: Shouldn't Graphene emit error message and exit here? Sounds like an implementation bug if the handle is weird?

[pwmarcz]

Reviewable status: all files reviewed, 1 unresolved discussion, 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 @mkow)

---

LibOS/shim/src/fs/chroot/fs.c, line 237 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What I meant is even stronger: Shouldn't Graphene emit error message and exit here? Sounds like an implementation bug if the handle is weird?

I'm not 100% sure that doesn't ever happen, but OK, let's fail fast. The tests are not complaining so far.

[mkow]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

[dimakuv]

Reviewed 1 of 2 files at r2, 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[mkow]

Reviewed 1 of 2 files at r3.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

[dimakuv]

Reviewed 1 of 12 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved