[Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Misono Tomohiro]

Current virtiofsd has problems about xattr operations and
they does not work properly for directory/symlink/special file.

The fundamental cause is that virtiofsd uses openat() + f...xattr()
systemcalls for xattr operation but we should not open symlink/special
file in the daemon. Therefore the function is restricted.

Fix this problem by:
 1. during setup of each thread, call unshare(CLONE_FS)
 2. in xattr operations (i.e. lo_getxattr), if inode is not a regular
    file or directory, use fchdir(proc_loot_fd) + ...xattr() +
    fchdir(root.fd) instead of openat() + f...xattr()

 Note: for a regular file/directory openat() + f...xattr()
       is still used for performance reason. Also we use
       O_RDONLY flag instead of O_RDWR in set/removexattr
       so that direcotry can be opened.

With this patch, xfstests generic/062 passes on virtiofs.
This fix is suggested by Miklos Szeredi and Stefan Hajnoczi.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
Hello,

This aproach is suggested in
https://www.redhat.com/archives/virtio-fs/2019-October/msg00046.html

Thanks,
Misono

 tools/virtiofsd/fuse_virtio.c    |  13 +++
 tools/virtiofsd/passthrough_ll.c | 167 +++++++++++++++++++++++--------
 tools/virtiofsd/seccomp.c        |   6 ++
 3 files changed, 142 insertions(+), 44 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 254b0a71cd..99339e17b2 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -460,6 +460,8 @@ err:
     return ret;
 }
 
+static __thread bool clone_fs_called;
+
 /* Process one FVRequest in a thread pool */
 static void fv_queue_worker(gpointer data, gpointer user_data)
 {
@@ -475,6 +477,17 @@ static void fv_queue_worker(gpointer data, gpointer user_data)
 
     assert(se->bufsize > sizeof(struct fuse_in_header));
 
+    if (!clone_fs_called) {
+        int ret;
+
+        /* unshare FS for xattr operation */
+        ret = unshare(CLONE_FS);
+        /* should not fail */
+        assert(ret == 0);
+
+        clone_fs_called = true;
+    }
+
     /*
      * An element contains one request and the space to send our response
      * They're spread over multiple descriptors in a scatter/gather set
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 83dd0084b4..5b69048ddd 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -133,6 +133,7 @@ struct lo_inode {
     GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */
 
     bool is_symlink;
+    bool is_regular;
 };
 
 struct lo_cred {
@@ -1038,6 +1039,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
         }
 
         inode->is_symlink = S_ISLNK(e->attr.st_mode);
+        inode->is_regular = S_ISREG(e->attr.st_mode) + S_ISDIR(e->attr.st_mode);
 
         /*
          * One for the caller and one for nlookup (released in
@@ -2345,6 +2347,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
     ssize_t ret;
     int saverr;
     int fd = -1;
+    bool dir_changed = false;
 
     inode = lo_inode(req, ino);
     if (!inode) {
@@ -2360,16 +2363,20 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
     fuse_log(FUSE_LOG_DEBUG, "lo_getxattr(ino=%" PRIu64 ", name=%s size=%zd)\n",
              ino, name, size);
 
-    if (inode->is_symlink) {
-        /* Sorry, no race free way to getxattr on symlink. */
-        saverr = EPERM;
-        goto out;
-    }
-
     sprintf(procname, "%i", inode->fd);
-    fd = openat(lo->proc_self_fd, procname, O_RDONLY);
-    if (fd < 0) {
-        goto out_err;
+    if (inode->is_regular) {
+        fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+        if (fd < 0) {
+          goto out_err;
+        }
+    } else {
+        ret = fchdir(lo->proc_self_fd);
+        if (ret < 0) {
+            /* should not happen */
+            fuse_log(FUSE_LOG_ERR, "lo_getxattr: fail fchdir to proc_self_fd\n");
+            goto out_err;
+        }
+        dir_changed = true;
     }
 
     if (size) {
@@ -2378,7 +2385,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
             goto out_err;
         }
 
-        ret = fgetxattr(fd, name, value, size);
+        if (inode->is_regular) {
+            ret = fgetxattr(fd, name, value, size);
+        } else {
+            ret = getxattr(procname, name, value, size);
+        }
         if (ret == -1) {
             goto out_err;
         }
@@ -2389,7 +2400,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
 
         fuse_reply_buf(req, value, ret);
     } else {
-        ret = fgetxattr(fd, name, NULL, 0);
+        if (inode->is_regular) {
+            ret = fgetxattr(fd, name, NULL, 0);
+        } else {
+            ret = getxattr(procname, name, NULL, 0);
+        }
         if (ret == -1) {
             goto out_err;
         }
@@ -2403,6 +2418,14 @@ out_free:
         close(fd);
     }
 
+    if (dir_changed) {
+        ret = fchdir(lo->root.fd);
+        if (ret < 0) {
+            /* should not happen */
+            fuse_log(FUSE_LOG_ERR, "lo_getxattr: fail fchdir to root.fd\n");
+        }
+    }
+
     lo_inode_put(lo, &inode);
     return;
 
@@ -2423,6 +2446,7 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
     ssize_t ret;
     int saverr;
     int fd = -1;
+    bool dir_changed = false;
 
     inode = lo_inode(req, ino);
     if (!inode) {
@@ -2438,16 +2462,20 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
     fuse_log(FUSE_LOG_DEBUG, "lo_listxattr(ino=%" PRIu64 ", size=%zd)\n", ino,
              size);
 
-    if (inode->is_symlink) {
-        /* Sorry, no race free way to listxattr on symlink. */
-        saverr = EPERM;
-        goto out;
-    }
-
     sprintf(procname, "%i", inode->fd);
-    fd = openat(lo->proc_self_fd, procname, O_RDONLY);
-    if (fd < 0) {
-        goto out_err;
+    if (inode->is_regular) {
+        fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+        if (fd < 0) {
+          goto out_err;
+        }
+    } else {
+        ret = fchdir(lo->proc_self_fd);
+        if (ret < 0) {
+            /* should not happen */
+            fuse_log(FUSE_LOG_ERR, "lo_listxattr: fail fchdir to proc_self_fd\n");
+            goto out_err;
+        }
+        dir_changed = true;
     }
 
     if (size) {
@@ -2456,7 +2484,11 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
             goto out_err;
         }
 
-        ret = flistxattr(fd, value, size);
+        if (inode->is_regular) {
+            ret = flistxattr(fd, value, size);
+        } else {
+            ret = listxattr(procname, value, size);
+        }
         if (ret == -1) {
             goto out_err;
         }
@@ -2467,7 +2499,11 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
 
         fuse_reply_buf(req, value, ret);
     } else {
-        ret = flistxattr(fd, NULL, 0);
+        if (inode->is_regular) {
+            ret = flistxattr(fd, NULL, 0);
+        } else {
+            ret = listxattr(procname, NULL, 0);
+        }
         if (ret == -1) {
             goto out_err;
         }
@@ -2481,6 +2517,14 @@ out_free:
         close(fd);
     }
 
+    if (dir_changed) {
+        ret = fchdir(lo->root.fd);
+        if (ret < 0) {
+            /* should not happen */
+            fuse_log(FUSE_LOG_ERR, "lo_listxattr: fail fchdir to root.fd\n");
+        }
+    }
+
     lo_inode_put(lo, &inode);
     return;
 
@@ -2501,6 +2545,7 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
     ssize_t ret;
     int saverr;
     int fd = -1;
+    bool dir_changed = false;
 
     inode = lo_inode(req, ino);
     if (!inode) {
@@ -2516,20 +2561,28 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
     fuse_log(FUSE_LOG_DEBUG, "lo_setxattr(ino=%" PRIu64,
              ", name=%s value=%s size=%zd)\n", ino, name, value, size);
 
-    if (inode->is_symlink) {
-        /* Sorry, no race free way to removexattr on symlink. */
-        saverr = EPERM;
-        goto out;
-    }
-
     sprintf(procname, "%i", inode->fd);
-    fd = openat(lo->proc_self_fd, procname, O_RDWR);
-    if (fd < 0) {
-        saverr = errno;
-        goto out;
+    if (inode->is_regular) {
+        fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+        if (fd < 0) {
+            saverr = errno;
+            goto out;
+        }
+
+        ret = fsetxattr(fd, name, value, size, flags);
+    } else {
+        ret = fchdir(lo->proc_self_fd);
+        if (ret < 0) {
+            /* should not happen */
+            fuse_log(FUSE_LOG_ERR, "lo_setxattr: fail fchdir to proc_self_fd\n");
+            saverr = errno;
+            goto out;
+        }
+        dir_changed = true;
+
+        ret = setxattr(procname, name, value, size, flags);
     }
 
-    ret = fsetxattr(fd, name, value, size, flags);
     saverr = ret == -1 ? errno : 0;
 
     if (!saverr) {
@@ -2540,6 +2593,14 @@ out:
         close(fd);
     }
 
+    if (dir_changed) {
+        ret = fchdir(lo->root.fd);
+        if (ret < 0) {
+          /* should not happen */
+          fuse_log(FUSE_LOG_ERR, "lo_setxattr: fail fchdir to root.fd\n");
+        }
+    }
+
     lo_inode_put(lo, &inode);
     fuse_reply_err(req, saverr);
 }
@@ -2552,6 +2613,7 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *name)
     ssize_t ret;
     int saverr;
     int fd = -1;
+    bool dir_changed = false;
 
     inode = lo_inode(req, ino);
     if (!inode) {
@@ -2567,20 +2629,28 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *name)
     fuse_log(FUSE_LOG_DEBUG, "lo_removexattr(ino=%" PRIu64 ", name=%s)\n", ino,
              name);
 
-    if (inode->is_symlink) {
-        /* Sorry, no race free way to setxattr on symlink. */
-        saverr = EPERM;
-        goto out;
-    }
-
     sprintf(procname, "%i", inode->fd);
-    fd = openat(lo->proc_self_fd, procname, O_RDWR);
-    if (fd < 0) {
-        saverr = errno;
-        goto out;
+    if (inode->is_regular) {
+        fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+        if (fd < 0) {
+            saverr = errno;
+            goto out;
+        }
+
+        ret = fremovexattr(fd, name);
+    } else {
+        ret = fchdir(lo->proc_self_fd);
+        if (ret < 0) {
+            /* should not happen */
+            fuse_log(FUSE_LOG_ERR, "lo_removexattr: fail fchdir to proc_self_fd\n");
+            saverr = errno;
+            goto out;
+        }
+        dir_changed = true;
+
+        ret = removexattr(procname, name);
     }
 
-    ret = fremovexattr(fd, name);
     saverr = ret == -1 ? errno : 0;
 
     if (!saverr) {
@@ -2591,6 +2661,14 @@ out:
         close(fd);
     }
 
+    if (dir_changed) {
+        ret = fchdir(lo->root.fd);
+        if (ret < 0) {
+            /* should not happen */
+            fuse_log(FUSE_LOG_ERR, "lo_removexattr: fail fchdir to root.fd\n");
+        }
+    }
+
     lo_inode_put(lo, &inode);
     fuse_reply_err(req, saverr);
 }
@@ -3316,6 +3394,7 @@ int main(int argc, char *argv[])
         lo.source = strdup("/");
     }
     lo.root.is_symlink = false;
+    lo.root.is_regular = true;
     if (!lo.timeout_set) {
         switch (lo.cache) {
         case CACHE_NONE:
diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
index 33ec309fc8..87048c239e 100644
--- a/tools/virtiofsd/seccomp.c
+++ b/tools/virtiofsd/seccomp.c
@@ -37,6 +37,7 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(exit),
     SCMP_SYS(exit_group),
     SCMP_SYS(fallocate),
+    SCMP_SYS(fchdir),
     SCMP_SYS(fchmodat),
     SCMP_SYS(fchownat),
     SCMP_SYS(fcntl),
@@ -58,7 +59,9 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(getpid),
     SCMP_SYS(gettid),
     SCMP_SYS(gettimeofday),
+    SCMP_SYS(getxattr),
     SCMP_SYS(linkat),
+    SCMP_SYS(listxattr),
     SCMP_SYS(lseek),
     SCMP_SYS(madvise),
     SCMP_SYS(mkdirat),
@@ -81,6 +84,7 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(recvmsg),
     SCMP_SYS(renameat),
     SCMP_SYS(renameat2),
+    SCMP_SYS(removexattr),
     SCMP_SYS(rt_sigaction),
     SCMP_SYS(rt_sigprocmask),
     SCMP_SYS(rt_sigreturn),
@@ -88,10 +92,12 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(setresgid),
     SCMP_SYS(setresuid),
     SCMP_SYS(set_robust_list),
+    SCMP_SYS(setxattr),
     SCMP_SYS(symlinkat),
     SCMP_SYS(time), /* Rarely needed, except on static builds */
     SCMP_SYS(tgkill),
     SCMP_SYS(unlinkat),
+    SCMP_SYS(unshare),
     SCMP_SYS(utimensat),
     SCMP_SYS(write),
     SCMP_SYS(writev),
-- 
2.21.1

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Vivek Goyal]

On Fri, Jan 10, 2020 at 10:07:22AM +0900, Misono Tomohiro wrote:
[..]
> @@ -2345,6 +2347,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
>      ssize_t ret;
>      int saverr;
>      int fd = -1;
> +    bool dir_changed = false;
>  
>      inode = lo_inode(req, ino);
>      if (!inode) {

> @@ -2360,16 +2363,20 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
>      fuse_log(FUSE_LOG_DEBUG, "lo_getxattr(ino=%" PRIu64 ", name=%s size=%zd)\n",
>               ino, name, size);
>  
> -    if (inode->is_symlink) {
> -        /* Sorry, no race free way to getxattr on symlink. */
> -        saverr = EPERM;
> -        goto out;
> -    }
> -

What was the race previously on symlinks and how does this code get rid
of that race?

>      sprintf(procname, "%i", inode->fd);
> -    fd = openat(lo->proc_self_fd, procname, O_RDONLY);
> -    if (fd < 0) {
> -        goto out_err;
> +    if (inode->is_regular) {
> +        fd = openat(lo->proc_self_fd, procname, O_RDONLY);
> +        if (fd < 0) {
> +          goto out_err;
> +        }
> +    } else {
> +        ret = fchdir(lo->proc_self_fd);
> +        if (ret < 0) {
> +            /* should not happen */
> +            fuse_log(FUSE_LOG_ERR, "lo_getxattr: fail fchdir to proc_self_fd\n");
> +            goto out_err;
> +        }
> +        dir_changed = true;
>      }
>  
>      if (size) {
> @@ -2378,7 +2385,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
>              goto out_err;
>          }
>  
> -        ret = fgetxattr(fd, name, value, size);
> +        if (inode->is_regular) {
> +            ret = fgetxattr(fd, name, value, size);
> +        } else {
> +            ret = getxattr(procname, name, value, size);
> +        }

Do we need "lgetxattr()" instead of "getxattr()" for symlinks?

Say I have following setup.

foo-symlink -> foo.txt

Now lo_lookup(foo-symlink.txt, O_PATH | O_NOFOLLOW), will get fd on
symlink, say 3. And proc should show something similar.

And /proc/self/fd/3 -> /foo-symlink.txt

Now if we do getxattr(3), will it not resolve symlinks and get xattr value
from "foo.txt" instead?

I think in practice that's not happening. So I am misunderstanding
something. What's that?

Thanks
Vivek

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Vivek Goyal]

On Fri, Jan 10, 2020 at 11:13:34AM -0500, Vivek Goyal wrote:

[..]
> >      if (size) {
> > @@ -2378,7 +2385,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> >              goto out_err;
> >          }
> >  
> > -        ret = fgetxattr(fd, name, value, size);
> > +        if (inode->is_regular) {
> > +            ret = fgetxattr(fd, name, value, size);
> > +        } else {
> > +            ret = getxattr(procname, name, value, size);
> > +        }
> 
> Do we need "lgetxattr()" instead of "getxattr()" for symlinks?

Well, lgetxattr() will not work as that will try to work on
/proc/pid/fd/<fdN>" and that's just a /proc file.

> 
> Say I have following setup.
> 
> foo-symlink -> foo.txt
> 
> Now lo_lookup(foo-symlink.txt, O_PATH | O_NOFOLLOW), will get fd on
> symlink, say 3. And proc should show something similar.
> 
> And /proc/self/fd/3 -> /foo-symlink.txt
> 
> Now if we do getxattr(3), will it not resolve symlinks and get xattr value
> from "foo.txt" instead?

That does not happen. We had opened 3, with O_PATH and O_NOFOLLOW. So
looks like all the operations now happen on actual symbolic file and
if this is path to a symlink file, it does not get resolved further.
Is it because of O_NOFOLLOW flag?

Vivek

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Vivek Goyal]

On Fri, Jan 10, 2020 at 10:07:22AM +0900, Misono Tomohiro wrote:

[..]
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -133,6 +133,7 @@ struct lo_inode {
>      GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */
>  
>      bool is_symlink;
> +    bool is_regular;
>  };
>  
>  struct lo_cred {
> @@ -1038,6 +1039,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>          }
>  
>          inode->is_symlink = S_ISLNK(e->attr.st_mode);
> +        inode->is_regular = S_ISREG(e->attr.st_mode) + S_ISDIR(e->attr.st_mode);

How about having two variables. One for regular files and one for
directories. Say ->is_regular and ->is_dir.  That way if there are other
users later, these can cleary differentiate between regular files and
directories.

>  
>          /*
>           * One for the caller and one for nlookup (released in
> @@ -2345,6 +2347,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
>      ssize_t ret;
>      int saverr;
>      int fd = -1;
> +    bool dir_changed = false;
>  
>      inode = lo_inode(req, ino);
>      if (!inode) {
> @@ -2360,16 +2363,20 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
>      fuse_log(FUSE_LOG_DEBUG, "lo_getxattr(ino=%" PRIu64 ", name=%s size=%zd)\n",
>               ino, name, size);
>  
> -    if (inode->is_symlink) {
> -        /* Sorry, no race free way to getxattr on symlink. */
> -        saverr = EPERM;
> -        goto out;
> -    }
> -

This code came from Miklos as part of original commit in libfuse.

commit fc9f632a219decea427271dc5a77654f6aaa9610
Author: Miklos Szeredi <mszeredi@redhat.com>
Date:   Tue Aug 14 21:37:02 2018 +0200

    passthrough_ll: add *xattr() operations

Miklos, can you please help us understand what are the races you are
concerned about on symlink and whether this patch fixes those races
or not. If not, then we probably need to retain this code and not
allow xattr operations on symlink yet.

Thanks
Vivek

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Miklos Szeredi]

On Mon, Jan 13, 2020 at 7:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Jan 10, 2020 at 10:07:22AM +0900, Misono Tomohiro wrote:
>
> [..]
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -133,6 +133,7 @@ struct lo_inode {
> >      GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */
> >
> >      bool is_symlink;
> > +    bool is_regular;
> >  };
> >
> >  struct lo_cred {
> > @@ -1038,6 +1039,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> >          }
> >
> >          inode->is_symlink = S_ISLNK(e->attr.st_mode);
> > +        inode->is_regular = S_ISREG(e->attr.st_mode) + S_ISDIR(e->attr.st_mode);
>
> How about having two variables. One for regular files and one for
> directories. Say ->is_regular and ->is_dir.  That way if there are other
> users later, these can cleary differentiate between regular files and
> directories.
>
> >
> >          /*
> >           * One for the caller and one for nlookup (released in
> > @@ -2345,6 +2347,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> >      ssize_t ret;
> >      int saverr;
> >      int fd = -1;
> > +    bool dir_changed = false;
> >
> >      inode = lo_inode(req, ino);
> >      if (!inode) {
> > @@ -2360,16 +2363,20 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> >      fuse_log(FUSE_LOG_DEBUG, "lo_getxattr(ino=%" PRIu64 ", name=%s size=%zd)\n",
> >               ino, name, size);
> >
> > -    if (inode->is_symlink) {
> > -        /* Sorry, no race free way to getxattr on symlink. */
> > -        saverr = EPERM;
> > -        goto out;
> > -    }
> > -
>
> This code came from Miklos as part of original commit in libfuse.
>
> commit fc9f632a219decea427271dc5a77654f6aaa9610
> Author: Miklos Szeredi <mszeredi@redhat.com>
> Date:   Tue Aug 14 21:37:02 2018 +0200
>
>     passthrough_ll: add *xattr() operations
>
> Miklos, can you please help us understand what are the races you are
> concerned about on symlink and whether this patch fixes those races
> or not. If not, then we probably need to retain this code and not
> allow xattr operations on symlink yet.

I missed the fact that setattr(2), getattr(2) etc. on a proc symlink
(A) pointing to a real symlink (B) will resolve A but not B.

The patch looks good.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[misono.tomohiro]

> On Mon, Jan 13, 2020 at 7:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, Jan 10, 2020 at 10:07:22AM +0900, Misono Tomohiro wrote:
> >
> > [..]
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -133,6 +133,7 @@ struct lo_inode {
> > >      GHashTable *posix_locks; /* protected by lo_inode->plock_mutex
> > > */
> > >
> > >      bool is_symlink;
> > > +    bool is_regular;
> > >  };
> > >
> > >  struct lo_cred {
> > > @@ -1038,6 +1039,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > >          }
> > >
> > >          inode->is_symlink = S_ISLNK(e->attr.st_mode);
> > > +        inode->is_regular = S_ISREG(e->attr.st_mode) +
> > > + S_ISDIR(e->attr.st_mode);
> >
> > How about having two variables. One for regular files and one for
> > directories. Say ->is_regular and ->is_dir.  That way if there are
> > other users later, these can cleary differentiate between regular
> > files and directories.

Thanks for the comments
Maybe we should just hold st_mode in lo_inode? I'm fine with either approach and will update.

> >
> > >
> > >          /*
> > >           * One for the caller and one for nlookup (released in @@
> > > -2345,6 +2347,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> > >      ssize_t ret;
> > >      int saverr;
> > >      int fd = -1;
> > > +    bool dir_changed = false;
> > >
> > >      inode = lo_inode(req, ino);
> > >      if (!inode) {
> > > @@ -2360,16 +2363,20 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> > >      fuse_log(FUSE_LOG_DEBUG, "lo_getxattr(ino=%" PRIu64 ", name=%s size=%zd)\n",
> > >               ino, name, size);
> > >
> > > -    if (inode->is_symlink) {
> > > -        /* Sorry, no race free way to getxattr on symlink. */
> > > -        saverr = EPERM;
> > > -        goto out;
> > > -    }
> > > -
> >
> > This code came from Miklos as part of original commit in libfuse.
> >
> > commit fc9f632a219decea427271dc5a77654f6aaa9610
> > Author: Miklos Szeredi <mszeredi@redhat.com>
> > Date:   Tue Aug 14 21:37:02 2018 +0200
> >
> >     passthrough_ll: add *xattr() operations
> >
> > Miklos, can you please help us understand what are the races you are
> > concerned about on symlink and whether this patch fixes those races or
> > not. If not, then we probably need to retain this code and not allow
> > xattr operations on symlink yet.
> 
> I missed the fact that setattr(2), getattr(2) etc. on a proc symlink
> (A) pointing to a real symlink (B) will resolve A but not B.
> 
> The patch looks good.
> 
> Thanks,
> Miklos

Thanks for review and confirming.
sidenote: so can we fix libfuse/passthrough_ll as well?

Thanks,
Misono

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Miklos Szeredi]

On Tue, Jan 14, 2020 at 2:17 AM misono.tomohiro@fujitsu.com
<misono.tomohiro@fujitsu.com> wrote:
>
> > On Mon, Jan 13, 2020 at 7:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Fri, Jan 10, 2020 at 10:07:22AM +0900, Misono Tomohiro wrote:
> > >
> > > [..]
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -133,6 +133,7 @@ struct lo_inode {
> > > >      GHashTable *posix_locks; /* protected by lo_inode->plock_mutex
> > > > */
> > > >
> > > >      bool is_symlink;
> > > > +    bool is_regular;
> > > >  };
> > > >
> > > >  struct lo_cred {
> > > > @@ -1038,6 +1039,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > >          }
> > > >
> > > >          inode->is_symlink = S_ISLNK(e->attr.st_mode);
> > > > +        inode->is_regular = S_ISREG(e->attr.st_mode) +
> > > > + S_ISDIR(e->attr.st_mode);
> > >
> > > How about having two variables. One for regular files and one for
> > > directories. Say ->is_regular and ->is_dir.  That way if there are
> > > other users later, these can cleary differentiate between regular
> > > files and directories.
>
> Thanks for the comments
> Maybe we should just hold st_mode in lo_inode? I'm fine with either approach and will update.
>
> > >
> > > >
> > > >          /*
> > > >           * One for the caller and one for nlookup (released in @@
> > > > -2345,6 +2347,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> > > >      ssize_t ret;
> > > >      int saverr;
> > > >      int fd = -1;
> > > > +    bool dir_changed = false;
> > > >
> > > >      inode = lo_inode(req, ino);
> > > >      if (!inode) {
> > > > @@ -2360,16 +2363,20 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> > > >      fuse_log(FUSE_LOG_DEBUG, "lo_getxattr(ino=%" PRIu64 ", name=%s size=%zd)\n",
> > > >               ino, name, size);
> > > >
> > > > -    if (inode->is_symlink) {
> > > > -        /* Sorry, no race free way to getxattr on symlink. */
> > > > -        saverr = EPERM;
> > > > -        goto out;
> > > > -    }
> > > > -
> > >
> > > This code came from Miklos as part of original commit in libfuse.
> > >
> > > commit fc9f632a219decea427271dc5a77654f6aaa9610
> > > Author: Miklos Szeredi <mszeredi@redhat.com>
> > > Date:   Tue Aug 14 21:37:02 2018 +0200
> > >
> > >     passthrough_ll: add *xattr() operations
> > >
> > > Miklos, can you please help us understand what are the races you are
> > > concerned about on symlink and whether this patch fixes those races or
> > > not. If not, then we probably need to retain this code and not allow
> > > xattr operations on symlink yet.
> >
> > I missed the fact that setattr(2), getattr(2) etc. on a proc symlink
> > (A) pointing to a real symlink (B) will resolve A but not B.
> >
> > The patch looks good.
> >
> > Thanks,
> > Miklos
>
> Thanks for review and confirming.
> sidenote: so can we fix libfuse/passthrough_ll as well?

I think we should.

BTW, you earlier wrote that there was a performance problem with the
fchdir() + getxattr() + fchdir() sequence.   Can you quantify how much
it is worse than the openat() + fgetxattr()  + close() sequence?
Because I'm wondering whether the added complexity is really worth it.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[misono.tomohiro]

> On Tue, Jan 14, 2020 at 2:17 AM misono.tomohiro@fujitsu.com <misono.tomohiro@fujitsu.com> wrote:
> >
> > > On Mon, Jan 13, 2020 at 7:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > On Fri, Jan 10, 2020 at 10:07:22AM +0900, Misono Tomohiro wrote:
> > > >
> > > > [..]
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -133,6 +133,7 @@ struct lo_inode {
> > > > >      GHashTable *posix_locks; /* protected by
> > > > > lo_inode->plock_mutex */
> > > > >
> > > > >      bool is_symlink;
> > > > > +    bool is_regular;
> > > > >  };
> > > > >
> > > > >  struct lo_cred {
> > > > > @@ -1038,6 +1039,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > >          }
> > > > >
> > > > >          inode->is_symlink = S_ISLNK(e->attr.st_mode);
> > > > > +        inode->is_regular = S_ISREG(e->attr.st_mode) +
> > > > > + S_ISDIR(e->attr.st_mode);
> > > >
> > > > How about having two variables. One for regular files and one for
> > > > directories. Say ->is_regular and ->is_dir.  That way if there are
> > > > other users later, these can cleary differentiate between regular
> > > > files and directories.
> >
> > Thanks for the comments
> > Maybe we should just hold st_mode in lo_inode? I'm fine with either approach and will update.
> >
> > > >
> > > > >
> > > > >          /*
> > > > >           * One for the caller and one for nlookup (released in
> > > > > @@
> > > > > -2345,6 +2347,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> > > > >      ssize_t ret;
> > > > >      int saverr;
> > > > >      int fd = -1;
> > > > > +    bool dir_changed = false;
> > > > >
> > > > >      inode = lo_inode(req, ino);
> > > > >      if (!inode) {
> > > > > @@ -2360,16 +2363,20 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
> > > > >      fuse_log(FUSE_LOG_DEBUG, "lo_getxattr(ino=%" PRIu64 ", name=%s size=%zd)\n",
> > > > >               ino, name, size);
> > > > >
> > > > > -    if (inode->is_symlink) {
> > > > > -        /* Sorry, no race free way to getxattr on symlink. */
> > > > > -        saverr = EPERM;
> > > > > -        goto out;
> > > > > -    }
> > > > > -
> > > >
> > > > This code came from Miklos as part of original commit in libfuse.
> > > >
> > > > commit fc9f632a219decea427271dc5a77654f6aaa9610
> > > > Author: Miklos Szeredi <mszeredi@redhat.com>
> > > > Date:   Tue Aug 14 21:37:02 2018 +0200
> > > >
> > > >     passthrough_ll: add *xattr() operations
> > > >
> > > > Miklos, can you please help us understand what are the races you
> > > > are concerned about on symlink and whether this patch fixes those
> > > > races or not. If not, then we probably need to retain this code
> > > > and not allow xattr operations on symlink yet.
> > >
> > > I missed the fact that setattr(2), getattr(2) etc. on a proc symlink
> > > (A) pointing to a real symlink (B) will resolve A but not B.
> > >
> > > The patch looks good.
> > >
> > > Thanks,
> > > Miklos
> >
> > Thanks for review and confirming.
> > sidenote: so can we fix libfuse/passthrough_ll as well?
> 
> I think we should.

Ok, I will send a patch there too in my spare time.

> 
> BTW, you earlier wrote that there was a performance problem with the
> fchdir() + getxattr() + fchdir() sequence.   Can you quantify how much
> it is worse than the openat() + fgetxattr()  + close() sequence?
> Because I'm wondering whether the added complexity is really worth it.

I see some performance hit in xfstests. For example, the runtimes of generic/127 (using fsx) are:
 no xattr ≒ 390s
 xattr (this patch) ≒ 440s
 xattr (always fchdir) ≒ 460s~480s
(kernel 5.4, backend XFS, default virtiofsd option other than xattr)

I don't know if there is a better way to measure xattr performance.

Thanks,
Misono

Re: [Virtio-fs] [PATCH] virtiofsd: Fix xattr operations

[Vivek Goyal]

On Tue, Jan 14, 2020 at 01:17:37AM +0000, misono.tomohiro@fujitsu.com wrote:
> > On Mon, Jan 13, 2020 at 7:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Fri, Jan 10, 2020 at 10:07:22AM +0900, Misono Tomohiro wrote:
> > >
> > > [..]
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -133,6 +133,7 @@ struct lo_inode {
> > > >      GHashTable *posix_locks; /* protected by lo_inode->plock_mutex
> > > > */
> > > >
> > > >      bool is_symlink;
> > > > +    bool is_regular;
> > > >  };
> > > >
> > > >  struct lo_cred {
> > > > @@ -1038,6 +1039,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > >          }
> > > >
> > > >          inode->is_symlink = S_ISLNK(e->attr.st_mode);
> > > > +        inode->is_regular = S_ISREG(e->attr.st_mode) +
> > > > + S_ISDIR(e->attr.st_mode);
> > >
> > > How about having two variables. One for regular files and one for
> > > directories. Say ->is_regular and ->is_dir.  That way if there are
> > > other users later, these can cleary differentiate between regular
> > > files and directories.
> 
> Thanks for the comments
> Maybe we should just hold st_mode in lo_inode? I'm fine with either approach and will update.

Caching st_mode (only file type and not mode) sounds good to me. That's
even more generic and more usable for future use cases.

Thanks
Vivek