use openat() calls against lower directory and avoid chdir()

[chrisd8088]

I realized while working on additional extensions to the test suite to test directory projection that if we rely on a fixed working directory and a chdir() in our FUSE loop thread, then that's actually going to make life difficult for users of our library. They'll expect to be able to chdir() themselves and unfortunately the working directory is global in a process, so this will bork all our file operations.

So, the second approach I proposed in my first thoughts on #3 is perhaps the best of both worlds -- that is, relative paths and no fixed working directory requirement.

This technique may also allow (with some caveats?) for changes to the absolute path to our lower directory to change, without triggering any problems inside our library.

So the approach here is to hold open a file descriptor on the lower directory, and then make all calls relative to that using the openat() family of calls.

The only hurdle I've bumped into so far is that FIFOs can hang the fgetxattr() and flistxattr() calls, so the code tries to avoid making those calls against pipes. See the gory details in the code and commit comments for bbdbd9ba6d853eab31444e068eb70fd13b1d98d6.

My hope is that if this looks to you like a good approach, then with any revisions you suggest, it could be merged into your high-level branch and then ultimately into master via your PR #3.

Thanks for taking a look, and for all the work on getting this into place!

[kivikakk]

The changes are good, but we now get some E_LOOP errors in normal operations that we weren't before.

Before:

debian ~/mount$ ls -l
total 16
drwxr-xr-x 3 kivikakk kivikakk 4096 Jan 22 16:28 a
drwxr-xr-x 2 kivikakk kivikakk 4096 Jan 10 14:14 b
lrwxrwxrwx 1 kivikakk kivikakk    6 Jan 29 11:19 link -> target
-rw-r--r-- 1 kivikakk kivikakk    6 Jan 29 11:18 target
drwxr-xr-x 2 kivikakk kivikakk 4096 Jan 23 15:30 yeplet'sdothis
debian ~/mount$

After:

debian ~/mount$ ls -l
ls: link: Too many levels of symbolic links
total 16
drwxr-xr-x 3 kivikakk kivikakk 4096 Jan 22 16:28 a
drwxr-xr-x 2 kivikakk kivikakk 4096 Jan 10 14:14 b
lrwxrwxrwx 1 kivikakk kivikakk    6 Jan 29 11:19 link -> target
-rw-r--r-- 1 kivikakk kivikakk    6 Jan 29 11:18 target
drwxr-xr-x 2 kivikakk kivikakk 4096 Jan 23 15:30 yeplet'sdothis
debian ~/mount$

We need to be careful about our use of openat and O_NOFOLLOW.

[kivikakk]

Here's a comparison between getting the extended attributes in our mounted filesystem vs. the lower (ext4):

debian ~/mount$ getfattr -d link target
# file: link
user.hello="hello"

# file: target
user.hello="hello"

debian ~/mount$ getfattr -d -h link target
getfattr: link: Too many levels of symbolic links
# file: target
user.hello="hello"

debian ~/mount$ cd ../lower
debian ~/lower$ getfattr -d link target
# file: link
user.hello="hello"

# file: target
user.hello="hello"

debian ~/lower$ getfattr -d -h link target
# file: target
user.hello="hello"

debian ~/lower$

Looks like it just quietly returns nothing for symlinks when we pass -h. The getfattr source reveals that -h makes it call lgetxattr instead of getxattr, which seems to imply the glib source reference above must be mistaken; it only returns -1 for lgetxattr, but here we see that clearly lgetxattr does still function correctly for non-symlinks.

Reading the syscall source, it looks like VFS happily pushes it right through to the underlying FS. ext4 does appear to have support for it in the source too. Honestly, I'm pretty confused.

[kivikakk]

I guess the tl;dr is, if we avoid using openat and O_NOFOLLOW (since that'll always fail on symlinks), we can at least let these calls pass on down to the lower filesystem, which will respond however best it can.

[kivikakk]

Here's my serving suggestion! Curious to hear your thoughts.

diff --git a/lib/projfs.c b/lib/projfs.c
index 58f251e..ef6a369 100644
--- a/lib/projfs.c
+++ b/lib/projfs.c
@@ -439,8 +439,6 @@ static int projfs_op_chmod(char const *path, mode_t mode,
        if (fi)
                res = fchmod(fi->fh, mode);
        else
-               // TODO: as AT_SYMLINK_NOFOLLOW is not implemented,
-               //       should we open(..., O_NOFOLLOW) and then fchmod()?
                res = fchmodat(lowerdir_fd(), lowerpath(path), mode, 0);
        return res == -1 ? -errno : 0;
 }
@@ -466,7 +464,7 @@ static int projfs_op_truncate(char const *path, off_t off,
                res = ftruncate(fi->fh, off);
        else {
                int fd = openat(lowerdir_fd(), lowerpath(path),
-                               O_NOFOLLOW | O_WRONLY);
+                               O_WRONLY);
                if (fd == -1) {
                        res = -1;
                        goto out;
@@ -531,7 +529,7 @@ static int projfs_op_setxattr(char const *path, char const *name,
        if (err > 0)
                goto out;
        // TODO: any way to avoid the small race here after check_fifo()?
-       int fd = openat(lowerdir_fd(), path, O_NOFOLLOW | O_WRONLY);
+       int fd = openat(lowerdir_fd(), path, O_WRONLY);
        if (fd == -1)
                goto out;
        res = fsetxattr(fd, name, value, size, flags);
@@ -555,7 +553,7 @@ static int projfs_op_getxattr(char const *path, char const *name,
        if (err > 0)
                goto out;
        // TODO: any way to avoid the small race here after check_fifo()?
-       int fd = openat(lowerdir_fd(), path, O_NOFOLLOW | O_RDONLY);
+       int fd = openat(lowerdir_fd(), path, O_RDONLY);
        if (fd == -1)
                goto out;
        res = fgetxattr(fd, name, value, size);
@@ -578,7 +576,7 @@ static int projfs_op_listxattr(char const *path, char *list, size_t size)
        if (err > 0)
                goto out;
        // TODO: any way to avoid the small race here after check_fifo()?
-       int fd = openat(lowerdir_fd(), path, O_NOFOLLOW | O_RDONLY);
+       int fd = openat(lowerdir_fd(), path, O_RDONLY);
        if (fd == -1)
                goto out;
        res = flistxattr(fd, list, size);
@@ -601,7 +599,7 @@ static int projfs_op_removexattr(char const *path, char const *name)
        if (err > 0)
                goto out;
        // TODO: any way to avoid the small race here after check_fifo()?
-       int fd = openat(lowerdir_fd(), path, O_NOFOLLOW | O_WRONLY);
+       int fd = openat(lowerdir_fd(), path, O_WRONLY);
        if (fd == -1)
                goto out;
        res = fremovexattr(fd, name);

[kivikakk]

The only hurdle I've bumped into so far is that FIFOs can hang the fgetxattr() and flistxattr() calls, so the code tries to avoid making those calls against pipes. See the gory details in the code and commit comments for bbdbd9b.

What if we use O_NONBLOCK?

When opening a FIFO with O_RDONLY or O_WRONLY set:

• If O_NONBLOCK or O_NDELAY is set, an open() for reading only returns without delay. An open() for writing only returns an error if no process currently has the file open for reading.

• If O_NONBLOCK and O_NDELAY are clear, an open() for reading only blocks until a thread opens the file for writing. An open() for writing only blocks the calling thread until a thread opens the file for reading.

Sounds like we can drop the manual racey check and just let errors bubble through.

[kivikakk]

I've made my suggestions commits in #11.

[chrisd8088]

I love the O_NONBLOCK fix, that's sooo much better! Thank you, @kivikakk! 💯

And re various NOFOLLOW checks, I'm just taking a second look now at the chown, opendir, and open use cases. But in the meantime, I'm going to merge this into the high-level-single-fd branch. Thank you!

[kivikakk]

➡️ #3