[LibOS] Add O_PATH support to open syscall

[boryspoplawski]

Description of the changes

I'm not sure if this doesn't break something - couldn't wrap my head around semantics of some fs operations in Gramine. Hopefully it's all fine...

Fixes #591

How to test this PR?

Added a new test.

---

This change is 

[boryspoplawski]

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

---

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

    file_off_t (*seek)(struct shim_handle* hdl, file_off_t offset, int whence);

    /* move, copy: rename or duplicate the file */

These 3 removals are kinda unrelated, but the change is small enough I decided not to create another PR. I could create another commit though.

---

LibOS/shim/src/sys/shim_open.c line 113 at r1 (raw file):

             | O_EXCL | O_LARGEFILE | O_NOATIME | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK | O_PATH
             | O_SYNC | O_TMPFILE | O_TRUNC;

We might want to explicitly fail on unsupported (but valid) flags. Do we? If yes, which ones?

[dimakuv]

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 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 @boryspoplawski)

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

These 3 removals are kinda unrelated, but the change is small enough I decided not to create another PR. I could create another commit though.

So move and copy functions were never used at all? And hdrop was exactly the same as close?

---

LibOS/shim/include/shim_types.h line 248 at r1 (raw file):

#ifndef O_ASYNC
#define O_ASYNC 020000
#endif

Why did you add it here? It was introduced in some newer Linux, so you just want to be sure? But from what I google, this was always in Linux...

---

LibOS/shim/src/fs/shim_namei.c line 457 at r1 (raw file):

        assoc_handle_with_dentry(hdl, dent, flags);
        if (dent->inode->type == S_IFDIR) {

What if it's a symlink (S_IFLNK), and the flags do not contain O_NOFOLLOW? Then we get the hdl which is a symlink. Is this what Linux does, is this how it should work?

---

LibOS/shim/src/fs/shim_namei.c line 463 at r1 (raw file):

         * with -ELOOP when trying to open a symlink.
         *
         * (Linux allows opening a symlink with O_PATH, but Gramine does not support it yet).

Actually, what's the point of this comment? I mean, why is it exactly here? Why did you remove it? Do we support it now (the combination of symlinks + O_PATH)?

I see that the relevant man page snippet seems to be this:

       ELOOP  pathname was a symbolic link, and flags specified
              O_NOFOLLOW but not O_PATH.

But I can't map this text to your current implementation.

UPDATE: Ok, you handle O_PATH above so we can't ever get to this place if flags contained O_PATH. Resolving.

---

LibOS/shim/src/fs/shim_namei.c line 548 at r1 (raw file):

        if (ret == 0) {
            assert(dent);
            get_dentry(dent);

Was that a bug previously? I don't see that the old code got a reference to dent.

UPDATE: Ah, that's because you later do put_dentry(dent). Got it.

---

LibOS/shim/src/fs/shim_namei.c line 747 at r1 (raw file):

    }

    if (!hdl->is_dir && hdl->type != TYPE_PATH) {

The logic "is this handle a directory" got complicated now... Now we have two "directory" handles:

• old hdl->type == TYPE_CHROOT / TYPE_TMPFS / etc. + hdl->is_dir, and
• new hdl->type == TYPE_PATH

Is this maintainable in the long run? Why do you only add this check in this one place, and not any other places? Is it because only this function is used for functionality enabled by O_PATH?

UPDATE: I see why you added it here. Because the man page directly talks about this case:

    openat()
       ...

       The dirfd argument is used in conjunction with the pathname
       argument as follows:

       *  If the pathname given in pathname is relative, then it is
          interpreted relative to the directory referred to by the file
          descriptor dirfd (rather than relative to the current working
          directory of the calling process, as is done by open() for a
          relative pathname).  In this case, dirfd must be a directory
          that was opened for reading (O_RDONLY) or using the O_PATH
          flag.

---

LibOS/shim/src/fs/pipe/fs.c line 134 at r1 (raw file):

static int pipe_hstat(struct shim_handle* hdl, struct stat* stat) {
    /* XXX: DOES THIS WORK LOL

Can we revert this? Having a whole diff on a file just for the sake of a more visible comment is not nice.

---

LibOS/shim/src/sys/shim_open.c line 113 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

We might want to explicitly fail on unsupported (but valid) flags. Do we? If yes, which ones?

This is orthogonal to this PR. Ideally we'd analyze all the available flags and their consequences. I think some flags we may silently ignore (they have no side effects for our Gramine purposes), and on some we should loudly fail. But I don't think we need to decide now. So maybe mark as a TODO or add a GitHub issue.

---

LibOS/shim/test/ltp/ltp.cfg line 268 at r1 (raw file):

skip = yes

# execveat not implemented

Are these changes something completely unrelated, but you stashed them here? I don't mind, just want to understand the rationale.

I guess you were looking through this file in an effort to find O_PATH tests, and accidentally saw these wrong comments?

Also, it looks like you didn't find any O_PATH test in LTP? Weird.

---

LibOS/shim/test/regression/common.h line 9 at r1 (raw file):

#define COMMON_H_

#include "err.h"

Why not <>?

---

LibOS/shim/test/regression/epoll_test.c line 15 at r1 (raw file):

    errx(1, "%d: " msg, __LINE__, ##args)

#define ARRAY_LEN(arr) (sizeof(arr) / sizeof(arr[0]))

It would make sense to move these two macros in common.h as well.

---

LibOS/shim/test/regression/open_opath.c line 50 at r1 (raw file):

    CHECK(unlinkat(mnt_dir_fd, "../tmp/B", 0));

Can we also add an execve test as described in the man page:

                  char buf[PATH_MAX];
                  fd = open("some_prog", O_PATH);
                  snprintf(buf, PATH_MAX, "/proc/self/fd/%d", fd);
                  execl(buf, "some_prog", (char *) NULL);

Though I'm not sure that Gramine already implements /proc/self/fd/ functionality? If not, then ignore this.

But I guess this would be important to test because this is one of the most common usages of O_PATH.

[boryspoplawski]

Reviewable status: all files reviewed, 8 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/include/shim_fs.h line 129 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

So move and copy functions were never used at all? And hdrop was exactly the same as close?

Correct!

---

LibOS/shim/include/shim_types.h line 248 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why did you add it here? It was introduced in some newer Linux, so you just want to be sure? But from what I google, this was always in Linux...

No, but it's defined in a header which we cannot include (bits/fcntl.h, which can be only included by fcntl.h, but we cannot use the latter).
Fun fact (checked just now): it's not defined by the kernel at all - which means it's not used as well.
Which actually means that we should not have it as well. Updated.

---

LibOS/shim/src/fs/shim_namei.c line 457 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What if it's a symlink (S_IFLNK), and the flags do not contain O_NOFOLLOW? Then we get the hdl which is a symlink. Is this what Linux does, is this how it should work?

O_PATH is used to get a fd to a "item" in the filesystem, whatever that is. So yes.

---

LibOS/shim/src/fs/shim_namei.c line 548 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Was that a bug previously? I don't see that the old code got a reference to dent.

UPDATE: Ah, that's because you later do put_dentry(dent). Got it.

Yes, I just didn't want to duplicate the cleanup code and I needed anther exit path for opath handling, so just unified it.

---

LibOS/shim/src/fs/shim_namei.c line 747 at r1 (raw file):
It's all a bit complicated in Gramine right now. We have a shim_handle which has both dentry and inode, but the latter is not to be used by the common code (at least that's what comment in the header says). I'll try to remove dentry and see how it goes (imo it should not be there).

Is this maintainable in the long run?

No idea. I tried removing is_dir completely, but it's impossible currently.

But now I think this change is wrong - we set is_dir for path handles as well. I've removed it, it all still works, with small quire we return different error code in one case (openat(fd_to_file_not_dir, "", O_RDONLY)), but that should not be a problem.

---

LibOS/shim/src/fs/pipe/fs.c line 134 at r1 (raw file):
Sorry, couldn't stop myself from doing this :)

more visible comment

this was hardly the reason

---

LibOS/shim/src/sys/shim_open.c line 113 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is orthogonal to this PR. Ideally we'd analyze all the available flags and their consequences. I think some flags we may silently ignore (they have no side effects for our Gramine purposes), and on some we should loudly fail. But I don't think we need to decide now. So maybe mark as a TODO or add a GitHub issue.

Ok, added a TODO

---

LibOS/shim/test/ltp/ltp.cfg line 268 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Are these changes something completely unrelated, but you stashed them here? I don't mind, just want to understand the rationale.

I guess you were looking through this file in an effort to find O_PATH tests, and accidentally saw these wrong comments?

Also, it looks like you didn't find any O_PATH test in LTP? Weird.

All of these (with changed comments) tests use O_PATH flag, but fail for unrelated reasons.

---

LibOS/shim/test/regression/common.h line 9 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why not <>?

Erm, no idea

---

LibOS/shim/test/regression/epoll_test.c line 15 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

It would make sense to move these two macros in common.h as well.

ERR doesn't seem that useful, but ARRAY_LEN is, so moved it.

---

LibOS/shim/test/regression/open_opath.c line 50 at r1 (raw file):

this is one of the most common usages of O_PATH.

Is it? This doesn't sound useful at all and I've never seen such pattern. Why do you think it is?

But sure, I can add this.

[dimakuv]

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

---

LibOS/shim/src/fs/shim_namei.c line 747 at r1 (raw file):

I'll try to remove dentry and see how it goes (imo it should not be there).

I didn't get this part. You mean you want to remove shim_handle::dentry field? But why?

My question is unrelated to this PR, so I unblocked.

---

LibOS/shim/test/regression/open_opath.c line 50 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

this is one of the most common usages of O_PATH.

Is it? This doesn't sound useful at all and I've never seen such pattern. Why do you think it is?

But sure, I can add this.

The man page gives this example, and my googliing also gave this example (probably just a copy-paste from the man page for illustrative purposes). So I thought it's one of the primary uses. Whatever.

---

LibOS/shim/src/fs/pipe/fs.c line 134 at r1 (raw file):

this was hardly the reason

I know :)

[boryspoplawski]

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

---

LibOS/shim/src/fs/shim_namei.c line 747 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'll try to remove dentry and see how it goes (imo it should not be there).

I didn't get this part. You mean you want to remove shim_handle::dentry field? But why?

My question is unrelated to this PR, so I unblocked.

dentry is (as the name says) directory entry - an entry in file system layout. You open a handle to the actual "item" (which is generally called inode), not specific place in filesystem. Handle does not need to know about specific fs location of the file/inode (actually it can change).

[dimakuv]

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

---

LibOS/shim/src/fs/shim_namei.c line 747 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

dentry is (as the name says) directory entry - an entry in file system layout. You open a handle to the actual "item" (which is generally called inode), not specific place in filesystem. Handle does not need to know about specific fs location of the file/inode (actually it can change).

Yes, you're right. We even have a comment about this:

gramine/LibOS/shim/include/shim_handle.h

Line 154 in 97d3f69

* filesystem (see `shim_inode` in `shim_fs.h`). Eventually, should replace `dentry` fields.

Unfortunately, this was not finished by Pawel.

[dimakuv]

Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved