passthrough_ll: remove symlink fallbacks (#514)

* passthrough_ll/hp: remove symlink fallbacks

Path lookup in the kernel has special rules for looking up magic symlinks
under /proc.  If a filesystem operation is instructed to follow symlinks
(e.g. via AT_SYMLINK_FOLLOW or lack of AT_SYMLINK_NOFOLLOW), and the final
component is such a proc symlink, then the target of the magic symlink is
used for the operation, even if the target itself is a symlink.  I.e. path
lookup is always terminated after following a final magic symlink.

I was erronously assuming that in the above case the target symlink would
also be followed, and so workarounds were added for a couple of operations
to handle the symlink case.  Since the symlink can be handled simply by
following the proc symlink, these workardouds are not needed.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Co-authored-by: Miklos Szeredi <mszeredi@redhat.com>

diff --git a/example/passthrough_hp.cc b/example/passthrough_hp.cc
index ddede52830d844161f5af71bbd101b69569d9d9b..3e112660e3198a2df055a37af9e181007e7c96a2 100644 (file)
--- a/example/passthrough_hp.cc
+++ b/example/passthrough_hp.cc
@@ -121,7 +121,6 @@ typedef std::unordered_map<SrcId, Inode> InodeMap;
 
 struct Inode {
     int fd {-1};
-    bool is_symlink {false};
     dev_t src_dev {0};
     ino_t src_ino {0};
     uint64_t nlookup {0};
@@ -217,28 +216,6 @@ static void sfs_getattr(fuse_req_t req, fuse_ino_t ino, fuse_file_info *fi) {
 }
 
 
-#ifdef HAVE_UTIMENSAT
-static int utimensat_empty_nofollow(Inode& inode,
-                                    const struct timespec *tv) {
-    if (inode.is_symlink) {
-        /* Does not work on current kernels, but may in the future:
-           https://marc.info/?l=linux-kernel&m=154158217810354&w=2 */
-        auto res = utimensat(inode.fd, "", tv, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
-        if (res == -1 && errno == EINVAL) {
-            /* Sorry, no race free way to set times on symlink. */
-            errno = EPERM;
-        }
-        return res;
-    }
-
-    char procname[64];
-    sprintf(procname, "/proc/self/fd/%i", inode.fd);
-
-    return utimensat(AT_FDCWD, procname, tv, 0);
-}
-#endif
-
-
 static void do_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
                        int valid, struct fuse_file_info* fi) {
     Inode& inode = get_inode(ino);
@@ -297,7 +274,9 @@ static void do_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
             res = futimens(fi->fh, tv);
         else {
 #ifdef HAVE_UTIMENSAT
-            res = utimensat_empty_nofollow(inode, tv);
+            char procname[64];
+            sprintf(procname, "/proc/self/fd/%i", ifd);
+            res = utimensat(AT_FDCWD, procname, tv, 0);
 #else
             res = -1;
             errno = EOPNOTSUPP;
@@ -378,7 +357,6 @@ static int do_lookup(fuse_ino_t parent, const char *name,
         lock_guard<mutex> g {inode.m};
         inode.src_ino = e->attr.st_ino;
         inode.src_dev = e->attr.st_dev;
-        inode.is_symlink = S_ISLNK(e->attr.st_mode);
         inode.nlookup = 1;
         inode.fd = newfd;
         fs_lock.unlock();
@@ -460,22 +438,6 @@ static void sfs_symlink(fuse_req_t req, const char *link, fuse_ino_t parent,
 }
 
 
-static int linkat_empty_nofollow(Inode& inode, int dfd, const char *name) {
-    if (inode.is_symlink) {
-        auto res = linkat(inode.fd, "", dfd, name, AT_EMPTY_PATH);
-        if (res == -1 && (errno == ENOENT || errno == EINVAL)) {
-            /* Sorry, no race free way to hard-link a symlink. */
-            errno = EOPNOTSUPP;
-        }
-        return res;
-    }
-
-    char procname[64];
-    sprintf(procname, "/proc/self/fd/%i", inode.fd);
-    return linkat(AT_FDCWD, procname, dfd, name, AT_SYMLINK_FOLLOW);
-}
-
-
 static void sfs_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
                      const char *name) {
     Inode& inode = get_inode(ino);
@@ -485,7 +447,9 @@ static void sfs_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
     e.attr_timeout = fs.timeout;
     e.entry_timeout = fs.timeout;
 
-    auto res = linkat_empty_nofollow(inode, inode_p.fd, name);
+    char procname[64];
+    sprintf(procname, "/proc/self/fd/%i", inode.fd);
+    auto res = linkat(AT_FDCWD, procname, inode_p.fd, name, AT_SYMLINK_FOLLOW);
     if (res == -1) {
         fuse_reply_err(req, errno);
         return;
@@ -965,12 +929,6 @@ static void sfs_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
     ssize_t ret;
     int saverr;
 
-    if (inode.is_symlink) {
-        /* Sorry, no race free way to getxattr on symlink. */
-        saverr = ENOTSUP;
-        goto out;
-    }
-
     char procname[64];
     sprintf(procname, "/proc/self/fd/%i", inode.fd);
 
@@ -1014,12 +972,6 @@ static void sfs_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) {
     ssize_t ret;
     int saverr;
 
-    if (inode.is_symlink) {
-        /* Sorry, no race free way to listxattr on symlink. */
-        saverr = ENOTSUP;
-        goto out;
-    }
-
     char procname[64];
     sprintf(procname, "/proc/self/fd/%i", inode.fd);
 
@@ -1062,19 +1014,12 @@ static void sfs_setxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
     ssize_t ret;
     int saverr;
 
-    if (inode.is_symlink) {
-        /* Sorry, no race free way to setxattr on symlink. */
-        saverr = ENOTSUP;
-        goto out;
-    }
-
     char procname[64];
     sprintf(procname, "/proc/self/fd/%i", inode.fd);
 
     ret = setxattr(procname, name, value, size, flags);
     saverr = ret == -1 ? errno : 0;
 
-out:
     fuse_reply_err(req, saverr);
 }
 
@@ -1085,17 +1030,10 @@ static void sfs_removexattr(fuse_req_t req, fuse_ino_t ino, const char *name) {
     ssize_t ret;
     int saverr;
 
-    if (inode.is_symlink) {
-        /* Sorry, no race free way to setxattr on symlink. */
-        saverr = ENOTSUP;
-        goto out;
-    }
-
     sprintf(procname, "/proc/self/fd/%i", inode.fd);
     ret = removexattr(procname, name);
     saverr = ret == -1 ? errno : 0;
 
-out:
     fuse_reply_err(req, saverr);
 }
 #endif
@@ -1221,7 +1159,6 @@ int main(int argc, char *argv[]) {
     // Initialize filesystem root
     fs.root.fd = -1;
     fs.root.nlookup = 9999;
-    fs.root.is_symlink = false;
     fs.timeout = options.count("nocache") ? 0 : 86400.0;
 
     struct stat stat;

diff --git a/example/passthrough_ll.c b/example/passthrough_ll.c
index 5667223e8f332d6f0b82ee28db0381b9e0d05421..06bb50f1c339f7e955c897d3e0385797a972ae6d 100644 (file)
--- a/example/passthrough_ll.c
+++ b/example/passthrough_ll.c
@@ -74,7 +74,6 @@ struct lo_inode {
        struct lo_inode *next; /* protected by lo->mutex */
        struct lo_inode *prev; /* protected by lo->mutex */
        int fd;
-       bool is_symlink;
        ino_t ino;
        dev_t dev;
        uint64_t refcount; /* protected by lo->mutex */
@@ -188,26 +187,6 @@ static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
        fuse_reply_attr(req, &buf, lo->timeout);
 }
 
-static int utimensat_empty_nofollow(struct lo_inode *inode,
-                                   const struct timespec *tv)
-{
-       int res;
-       char procname[64];
-
-       if (inode->is_symlink) {
-               res = utimensat(inode->fd, "", tv,
-                               AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
-               if (res == -1 && errno == EINVAL) {
-                       /* Sorry, no race free way to set times on symlink. */
-                       errno = EPERM;
-               }
-               return res;
-       }
-       sprintf(procname, "/proc/self/fd/%i", inode->fd);
-
-       return utimensat(AT_FDCWD, procname, tv, 0);
-}
-
 static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
                       int valid, struct fuse_file_info *fi)
 {
@@ -268,8 +247,10 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
 
                if (fi)
                        res = futimens(fi->fh, tv);
-               else
-                       res = utimensat_empty_nofollow(inode, tv);
+               else {
+                       sprintf(procname, "/proc/self/fd/%i", ifd);
+                       res = utimensat(AT_FDCWD, procname, tv, 0);
+               }
                if (res == -1)
                        goto out_err;
        }
@@ -332,7 +313,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
                if (!inode)
                        goto out_err;
 
-               inode->is_symlink = S_ISLNK(e->attr.st_mode);
                inode->refcount = 1;
                inode->fd = newfd;
                inode->ino = e->attr.st_ino;
@@ -426,26 +406,6 @@ static void lo_symlink(fuse_req_t req, const char *link,
        lo_mknod_symlink(req, parent, name, S_IFLNK, 0, link);
 }
 
-static int linkat_empty_nofollow(struct lo_inode *inode, int dfd,
-                                const char *name)
-{
-       int res;
-       char procname[64];
-
-       if (inode->is_symlink) {
-               res = linkat(inode->fd, "", dfd, name, AT_EMPTY_PATH);
-               if (res == -1 && (errno == ENOENT || errno == EINVAL)) {
-                       /* Sorry, no race free way to hard-link a symlink. */
-                       errno = EPERM;
-               }
-               return res;
-       }
-
-       sprintf(procname, "/proc/self/fd/%i", inode->fd);
-
-       return linkat(AT_FDCWD, procname, dfd, name, AT_SYMLINK_FOLLOW);
-}
-
 static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
                    const char *name)
 {
@@ -453,13 +413,16 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent,
        struct lo_data *lo = lo_data(req);
        struct lo_inode *inode = lo_inode(req, ino);
        struct fuse_entry_param e;
+       char procname[64];
        int saverr;
 
        memset(&e, 0, sizeof(struct fuse_entry_param));
        e.attr_timeout = lo->timeout;
        e.entry_timeout = lo->timeout;
 
-       res = linkat_empty_nofollow(inode, lo_fd(req, parent), name);
+       sprintf(procname, "/proc/self/fd/%i", inode->fd);
+       res = linkat(AT_FDCWD, procname, lo_fd(req, parent), name,
+                    AT_SYMLINK_FOLLOW);
        if (res == -1)
                goto out_err;
 
@@ -976,12 +939,6 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
                        ino, name, size);
        }
 
-       if (inode->is_symlink) {
-               /* Sorry, no race free way to getxattr on symlink. */
-               saverr = EPERM;
-               goto out;
-       }
-
        sprintf(procname, "/proc/self/fd/%i", inode->fd);
 
        if (size) {
@@ -1032,12 +989,6 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
                        ino, size);
        }
 
-       if (inode->is_symlink) {
-               /* Sorry, no race free way to listxattr on symlink. */
-               saverr = EPERM;
-               goto out;
-       }
-
        sprintf(procname, "/proc/self/fd/%i", inode->fd);
 
        if (size) {
@@ -1088,12 +1039,6 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
                        ino, name, value, size);
        }
 
-       if (inode->is_symlink) {
-               /* Sorry, no race free way to setxattr on symlink. */
-               saverr = EPERM;
-               goto out;
-       }
-
        sprintf(procname, "/proc/self/fd/%i", inode->fd);
 
        ret = setxattr(procname, name, value, size, flags);
@@ -1119,12 +1064,6 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *name)
                        ino, name);
        }
 
-       if (inode->is_symlink) {
-               /* Sorry, no race free way to setxattr on symlink. */
-               saverr = EPERM;
-               goto out;
-       }
-
        sprintf(procname, "/proc/self/fd/%i", inode->fd);
 
        ret = removexattr(procname, name);
@@ -1274,7 +1213,6 @@ int main(int argc, char *argv[])
        } else {
                lo.source = "/";
        }
-       lo.root.is_symlink = false;
        if (!lo.timeout_set) {
                switch (lo.cache) {
                case CACHE_NEVER: