[Virtio-fs] [PATCH v2 0/2] virtiofsd: Fix xattr and ACL

[Virtio-fs] [PATCH v2 0/2] virtiofsd: Fix xattr and ACL

[Misono Tomohiro]

Hi,

This is a second version of xattr fix for virtiofsd.
I included ACL fix (which introduces new option posix_acl) in this version
too as ACL mostly depends on xattr.

I run xfstests with XFS backend using "-o xattr -o posix_acl" option and
only new failure is generic/375 which checks if sgid bit is cleared after
setfacl. I'll try to investigate it.

change in v1 -> v2
 - rebased to current dev branch

 - Always chdir for xattr (1st patch)
   In v1, I keep current implementation for regular file/dir since it
   show better performance in my environment. But I notice opening file
   for xattr causes seek sanity test fails (xfstest generic/285, 436).

   I'm not sure what is the fundamental problem here but I believe
   performance can be improved by introducing some caching mechanism
   in general. So I change the code to always fchdir to avoid the
   problem for now. This results in simpler code too.

 - Add ACL fix (2nd patch)
   ACL mostly works if xattr option is enabled.
   To support default ACL behavior, add new option posix_acl which
   handles umasking in virtiofsd instead of guest kernel.

Thanks,
Misono

Misono Tomohiro (2):
  virtiofsd: Fix xattr operations
  virtiofsd: Add support of posix_acl

 tools/virtiofsd/fuse_virtio.c    |  13 +++
 tools/virtiofsd/helper.c         |   3 +
 tools/virtiofsd/passthrough_ll.c | 136 +++++++++++++++++++++----------
 tools/virtiofsd/seccomp.c        |  11 ++-
 4 files changed, 115 insertions(+), 48 deletions(-)

-- 
2.21.1

[Virtio-fs] [PATCH v2 1/2] 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) so that chdir
    would not affect other threads
 2. in xattr operations (i.e. lo_getxattr), use
    fchdir(proc_loot_fd) + ...xattr() + fchdir(root.fd)
    instead of openat() + f...xattr() to avoid open files

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>
---
 tools/virtiofsd/fuse_virtio.c    |  13 ++++
 tools/virtiofsd/passthrough_ll.c | 100 +++++++++++++++++++------------
 tools/virtiofsd/seccomp.c        |  10 ++--
 3 files changed, 81 insertions(+), 42 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 1aac6b8687..ad03a7dcc0 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -463,6 +463,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)
 {
@@ -478,6 +480,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 f0093ab84e..6bcc33e0eb 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2362,6 +2362,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) {
@@ -2377,17 +2378,14 @@ 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) {
+    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) {
         value = malloc(size);
@@ -2395,7 +2393,7 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
             goto out_err;
         }
 
-        ret = fgetxattr(fd, name, value, size);
+        ret = getxattr(procname, name, value, size);
         if (ret == -1) {
             goto out_err;
         }
@@ -2406,7 +2404,7 @@ 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);
+        ret = getxattr(procname, name, NULL, 0);
         if (ret == -1) {
             goto out_err;
         }
@@ -2420,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;
 
@@ -2440,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) {
@@ -2455,17 +2462,14 @@ 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) {
+    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) {
         value = malloc(size);
@@ -2473,7 +2477,7 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
             goto out_err;
         }
 
-        ret = flistxattr(fd, value, size);
+        ret = listxattr(procname, value, size);
         if (ret == -1) {
             goto out_err;
         }
@@ -2484,7 +2488,7 @@ 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);
+        ret = listxattr(procname, NULL, 0);
         if (ret == -1) {
             goto out_err;
         }
@@ -2498,6 +2502,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;
 
@@ -2518,6 +2530,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) {
@@ -2533,20 +2546,17 @@ 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 setxattr on symlink. */
-        saverr = EPERM;
-        goto out;
-    }
-
     sprintf(procname, "%i", inode->fd);
-    fd = openat(lo->proc_self_fd, procname, O_RDWR);
-    if (fd < 0) {
+    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 = fsetxattr(fd, name, value, size, flags);
+    ret = setxattr(procname, name, value, size, flags);
     saverr = ret == -1 ? errno : 0;
 
     if (!saverr) {
@@ -2557,6 +2567,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);
 }
@@ -2569,6 +2587,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) {
@@ -2584,20 +2603,17 @@ 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) {
+    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 = fremovexattr(fd, name);
+    ret = removexattr(procname, name);
     saverr = ret == -1 ? errno : 0;
 
     if (!saverr) {
@@ -2608,6 +2624,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);
 }
diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
index 2d9d4a7ec0..9ee99e4725 100644
--- a/tools/virtiofsd/seccomp.c
+++ b/tools/virtiofsd/seccomp.c
@@ -41,15 +41,12 @@ 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),
     SCMP_SYS(fdatasync),
-    SCMP_SYS(fgetxattr),
-    SCMP_SYS(flistxattr),
     SCMP_SYS(flock),
-    SCMP_SYS(fremovexattr),
-    SCMP_SYS(fsetxattr),
     SCMP_SYS(fstat),
     SCMP_SYS(fstatfs),
     SCMP_SYS(fsync),
@@ -62,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),
@@ -85,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),
@@ -98,10 +98,12 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(setresuid32),
 #endif
     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 v2 1/2] virtiofsd: Fix xattr operations

[Vivek Goyal]

On Tue, Jan 28, 2020 at 07:18:18PM +0900, Misono Tomohiro wrote:
> 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) so that chdir
>     would not affect other threads
>  2. in xattr operations (i.e. lo_getxattr), use
>     fchdir(proc_loot_fd) + ...xattr() + fchdir(root.fd)
>     instead of openat() + f...xattr() to avoid open files
> 
> 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>
> ---
>  tools/virtiofsd/fuse_virtio.c    |  13 ++++
>  tools/virtiofsd/passthrough_ll.c | 100 +++++++++++++++++++------------
>  tools/virtiofsd/seccomp.c        |  10 ++--
>  3 files changed, 81 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 1aac6b8687..ad03a7dcc0 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -463,6 +463,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)
>  {
> @@ -478,6 +480,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 f0093ab84e..6bcc33e0eb 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2362,6 +2362,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) {
> @@ -2377,17 +2378,14 @@ 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) {
> +    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) {
>          value = malloc(size);

I am wondering, is it better to allocate memory first (if need be) and
then do fchdir(). If memory allocation fails, we can return error right
away without having to do fchdir() twice.

Also do we need dir_changed variable? I think if we organize labels
properly, we should be able to get rid of this dir_changed variable.

Same comments apply to lo_listxattr() as well.

Thanks
Vivek

Re: [Virtio-fs] [PATCH v2 1/2] 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) so that chdir
> >     would not affect other threads
> >  2. in xattr operations (i.e. lo_getxattr), use
> >     fchdir(proc_loot_fd) + ...xattr() + fchdir(root.fd)
> >     instead of openat() + f...xattr() to avoid open files
> >
> > 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>
> > ---
> >  tools/virtiofsd/fuse_virtio.c    |  13 ++++
> >  tools/virtiofsd/passthrough_ll.c | 100 +++++++++++++++++++------------
> >  tools/virtiofsd/seccomp.c        |  10 ++--
> >  3 files changed, 81 insertions(+), 42 deletions(-)
> >
> > diff --git a/tools/virtiofsd/fuse_virtio.c
> > b/tools/virtiofsd/fuse_virtio.c index 1aac6b8687..ad03a7dcc0 100644
> > --- a/tools/virtiofsd/fuse_virtio.c
> > +++ b/tools/virtiofsd/fuse_virtio.c
> > @@ -463,6 +463,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)  { @@ -478,6
> > +480,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 f0093ab84e..6bcc33e0eb 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -2362,6 +2362,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) {
> > @@ -2377,17 +2378,14 @@ 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) {
> > +    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) {
> >          value = malloc(size);
> 
> I am wondering, is it better to allocate memory first (if need be) and then do fchdir(). If memory allocation fails, we can return
> error right away without having to do fchdir() twice.
> 
> Also do we need dir_changed variable? I think if we organize labels properly, we should be able to get rid of this dir_changed
> variable.
> 
> Same comments apply to lo_listxattr() as well.

Sounds reasonable, I will refactor the code.
Misono

[Virtio-fs] [PATCH v2 2/2] virtiofsd: Add support of posix_acl

[Misono Tomohiro]

This commit adds support of posix_acl which is enabled by 'posix_acl'
option (default: disabled).

If posix_acl is enabled, virtiofsd handles umask handling otherwise done
in guest kenrel in order to treat default ACL correctly.

With this, ACL-related xfstest generic/{099,237,319,444} passes on XFS
backend.

Implementation:
  virtiofsd adds FUSE_CAP_POSIX_ACL/FUSE_CAP_DONT_MASK to INIT replay if
 posix_acl is enabled. This results in skipping umask handling in guest
 kernel. In turn, virtiofsd sets umask before creating files by using
 umask value included in fuse_request and restore umask after that.
 Note that calling umask() is ok since now each worker thread has called
 unshare(CLONE_FS) when starting up.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
 tools/virtiofsd/helper.c         |  3 +++
 tools/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++++++++------
 tools/virtiofsd/seccomp.c        |  1 +
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index 0801cf752c..97968a9385 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -171,6 +171,9 @@ void fuse_cmdline_help(void)
            "                               default: no_writeback\n"
            "    -o xattr|no_xattr          enable/disable xattr\n"
            "                               default: no_xattr\n"
+           "    -o posix_acl|no_posxi_acl  enable/disable posix_acl\n"
+           "                               enabling this also enables xattr\n"
+           "                               default: no_posix_acl\n"
            );
 }
 
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 6bcc33e0eb..e1bb261b51 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -152,6 +152,7 @@ struct lo_data {
     int flock;
     int posix_lock;
     int xattr;
+    int posix_acl;
     char *source;
     double timeout;
     int cache;
@@ -182,6 +183,8 @@ static const struct fuse_opt lo_opts[] = {
     { "no_posix_lock", offsetof(struct lo_data, posix_lock), 0 },
     { "xattr", offsetof(struct lo_data, xattr), 1 },
     { "no_xattr", offsetof(struct lo_data, xattr), 0 },
+    { "posix_acl", offsetof(struct lo_data, posix_acl), 1 },
+    { "no_posix_acl", offsetof(struct lo_data, posix_acl), 0 },
     { "timeout=%lf", offsetof(struct lo_data, timeout), 0 },
     { "timeout=", offsetof(struct lo_data, timeout_set), 1 },
     { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE },
@@ -587,6 +590,17 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
         fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling readdirplus\n");
         conn->want &= ~FUSE_CAP_READDIRPLUS;
     }
+
+    if (conn->capable & FUSE_CAP_POSIX_ACL) {
+        if (lo->posix_acl) {
+            if (!lo->xattr)  {
+                fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling xattr for ACL\n");
+                lo->xattr = 1;
+            }
+            fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling ACL\n");
+            conn->want |= (FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK);
+        }
+    }
 }
 
 static int64_t *version_ptr(struct lo_data *lo, struct lo_inode *inode)
@@ -1137,9 +1151,11 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name)
 /*
  * Change to uid/gid of caller so that file is created with
  * ownership of caller.
+ * Also update umask if posix_acl is enabled.
  * TODO: What about selinux context?
  */
-static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
+static int lo_change_cred_and_umask(fuse_req_t req, struct lo_cred *old,
+                                    struct lo_data *lo)
 {
     int res;
 
@@ -1159,11 +1175,15 @@ static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
         return errno_save;
     }
 
+    if (lo->posix_acl) {
+        umask(req->ctx.umask);
+    }
+
     return 0;
 }
 
 /* Regain Privileges */
-static void lo_restore_cred(struct lo_cred *old)
+static void lo_restore_cred_and_umask(struct lo_cred *old, struct lo_data *lo)
 {
     int res;
 
@@ -1178,6 +1198,10 @@ static void lo_restore_cred(struct lo_cred *old)
         fuse_log(FUSE_LOG_ERR, "setegid(%u): %m\n", old->egid);
         exit(1);
     }
+
+    if (lo->posix_acl) {
+        umask(0);
+    }
 }
 
 static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
@@ -1204,7 +1228,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
 
     saverr = ENOMEM;
 
-    saverr = lo_change_cred(req, &old);
+    saverr = lo_change_cred_and_umask(req, &old, lo);
     if (saverr) {
         goto out;
     }
@@ -1213,7 +1237,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
 
     saverr = errno;
 
-    lo_restore_cred(&old);
+    lo_restore_cred_and_umask(&old, lo);
 
     if (res == -1) {
         goto out;
@@ -1898,7 +1922,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
         return;
     }
 
-    err = lo_change_cred(req, &old);
+    err = lo_change_cred_and_umask(req, &old, lo);
     if (err) {
         goto out;
     }
@@ -1908,7 +1932,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
     fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
                 mode);
     err = fd == -1 ? errno : 0;
-    lo_restore_cred(&old);
+    lo_restore_cred_and_umask(&old, lo);
 
     if (!err) {
         ssize_t fh;
diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
index 9ee99e4725..1b6f579452 100644
--- a/tools/virtiofsd/seccomp.c
+++ b/tools/virtiofsd/seccomp.c
@@ -102,6 +102,7 @@ static const int syscall_whitelist[] = {
     SCMP_SYS(symlinkat),
     SCMP_SYS(time), /* Rarely needed, except on static builds */
     SCMP_SYS(tgkill),
+    SCMP_SYS(umask),
     SCMP_SYS(unlinkat),
     SCMP_SYS(unshare),
     SCMP_SYS(utimensat),
-- 
2.21.1

Re: [Virtio-fs] [PATCH v2 2/2] virtiofsd: Add support of posix_acl

[Vivek Goyal]

On Tue, Jan 28, 2020 at 07:18:19PM +0900, Misono Tomohiro wrote:
> This commit adds support of posix_acl which is enabled by 'posix_acl'
> option (default: disabled).
> 
> If posix_acl is enabled, virtiofsd handles umask handling otherwise done
> in guest kenrel in order to treat default ACL correctly.
> 
> With this, ACL-related xfstest generic/{099,237,319,444} passes on XFS
> backend.

Can you please explain what's the problem with current code.

Thanks
Vivek
> 
> Implementation:
>   virtiofsd adds FUSE_CAP_POSIX_ACL/FUSE_CAP_DONT_MASK to INIT replay if
>  posix_acl is enabled. This results in skipping umask handling in guest
>  kernel. In turn, virtiofsd sets umask before creating files by using
>  umask value included in fuse_request and restore umask after that.
>  Note that calling umask() is ok since now each worker thread has called
>  unshare(CLONE_FS) when starting up.
> 
> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> ---
>  tools/virtiofsd/helper.c         |  3 +++
>  tools/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++++++++------
>  tools/virtiofsd/seccomp.c        |  1 +
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index 0801cf752c..97968a9385 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -171,6 +171,9 @@ void fuse_cmdline_help(void)
>             "                               default: no_writeback\n"
>             "    -o xattr|no_xattr          enable/disable xattr\n"
>             "                               default: no_xattr\n"
> +           "    -o posix_acl|no_posxi_acl  enable/disable posix_acl\n"
> +           "                               enabling this also enables xattr\n"
> +           "                               default: no_posix_acl\n"
>             );
>  }
>  
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 6bcc33e0eb..e1bb261b51 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -152,6 +152,7 @@ struct lo_data {
>      int flock;
>      int posix_lock;
>      int xattr;
> +    int posix_acl;
>      char *source;
>      double timeout;
>      int cache;
> @@ -182,6 +183,8 @@ static const struct fuse_opt lo_opts[] = {
>      { "no_posix_lock", offsetof(struct lo_data, posix_lock), 0 },
>      { "xattr", offsetof(struct lo_data, xattr), 1 },
>      { "no_xattr", offsetof(struct lo_data, xattr), 0 },
> +    { "posix_acl", offsetof(struct lo_data, posix_acl), 1 },
> +    { "no_posix_acl", offsetof(struct lo_data, posix_acl), 0 },
>      { "timeout=%lf", offsetof(struct lo_data, timeout), 0 },
>      { "timeout=", offsetof(struct lo_data, timeout_set), 1 },
>      { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE },
> @@ -587,6 +590,17 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
>          fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling readdirplus\n");
>          conn->want &= ~FUSE_CAP_READDIRPLUS;
>      }
> +
> +    if (conn->capable & FUSE_CAP_POSIX_ACL) {
> +        if (lo->posix_acl) {
> +            if (!lo->xattr)  {
> +                fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling xattr for ACL\n");
> +                lo->xattr = 1;
> +            }
> +            fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling ACL\n");
> +            conn->want |= (FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK);
> +        }
> +    }
>  }
>  
>  static int64_t *version_ptr(struct lo_data *lo, struct lo_inode *inode)
> @@ -1137,9 +1151,11 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name)
>  /*
>   * Change to uid/gid of caller so that file is created with
>   * ownership of caller.
> + * Also update umask if posix_acl is enabled.
>   * TODO: What about selinux context?
>   */
> -static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
> +static int lo_change_cred_and_umask(fuse_req_t req, struct lo_cred *old,
> +                                    struct lo_data *lo)
>  {
>      int res;
>  
> @@ -1159,11 +1175,15 @@ static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
>          return errno_save;
>      }
>  
> +    if (lo->posix_acl) {
> +        umask(req->ctx.umask);
> +    }
> +
>      return 0;
>  }
>  
>  /* Regain Privileges */
> -static void lo_restore_cred(struct lo_cred *old)
> +static void lo_restore_cred_and_umask(struct lo_cred *old, struct lo_data *lo)
>  {
>      int res;
>  
> @@ -1178,6 +1198,10 @@ static void lo_restore_cred(struct lo_cred *old)
>          fuse_log(FUSE_LOG_ERR, "setegid(%u): %m\n", old->egid);
>          exit(1);
>      }
> +
> +    if (lo->posix_acl) {
> +        umask(0);
> +    }
>  }
>  
>  static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
> @@ -1204,7 +1228,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
>  
>      saverr = ENOMEM;
>  
> -    saverr = lo_change_cred(req, &old);
> +    saverr = lo_change_cred_and_umask(req, &old, lo);
>      if (saverr) {
>          goto out;
>      }
> @@ -1213,7 +1237,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
>  
>      saverr = errno;
>  
> -    lo_restore_cred(&old);
> +    lo_restore_cred_and_umask(&old, lo);
>  
>      if (res == -1) {
>          goto out;
> @@ -1898,7 +1922,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>          return;
>      }
>  
> -    err = lo_change_cred(req, &old);
> +    err = lo_change_cred_and_umask(req, &old, lo);
>      if (err) {
>          goto out;
>      }
> @@ -1908,7 +1932,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>      fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
>                  mode);
>      err = fd == -1 ? errno : 0;
> -    lo_restore_cred(&old);
> +    lo_restore_cred_and_umask(&old, lo);
>  
>      if (!err) {
>          ssize_t fh;
> diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
> index 9ee99e4725..1b6f579452 100644
> --- a/tools/virtiofsd/seccomp.c
> +++ b/tools/virtiofsd/seccomp.c
> @@ -102,6 +102,7 @@ static const int syscall_whitelist[] = {
>      SCMP_SYS(symlinkat),
>      SCMP_SYS(time), /* Rarely needed, except on static builds */
>      SCMP_SYS(tgkill),
> +    SCMP_SYS(umask),
>      SCMP_SYS(unlinkat),
>      SCMP_SYS(unshare),
>      SCMP_SYS(utimensat),
> -- 
> 2.21.1
> 

Re: [Virtio-fs] [PATCH v2 2/2] virtiofsd: Add support of posix_acl

[misono.tomohiro]

> On Tue, Jan 28, 2020 at 07:18:19PM +0900, Misono Tomohiro wrote:
> > This commit adds support of posix_acl which is enabled by 'posix_acl'
> > option (default: disabled).
> >
> > If posix_acl is enabled, virtiofsd handles umask handling otherwise
> > done in guest kernel in order to treat default ACL correctly.
> >
> > With this, ACL-related xfstest generic/{099,237,319,444} passes on XFS
> > backend.
> 
> Can you please explain what's the problem with current code.

Default ACL can be set to directory only.
Usually default permission when creating files is masked by current umask value.
If directory has default ACL value, however, its value takes priority over umask
(i.e. default ACL value is used and umask value is ignored).

Without this patch (without specifying FUSE_CAP_DONT_MASK), 
fuse kernel module always performs umasking before sending requests regardless of ACL settings: 
 https://github.com/torvalds/linux/blob/master/fs/fuse/dir.c#L459-L460

So, default ACL does not work properly with current code,
I believe FUSE_CAP_DONT_MASK was actually introduced to fix this problem: 
 https://github.com/torvalds/linux/commit/e0a43ddcc08c34dbd666d93600fd23914505f4aa

Thanks,
Misono

> 
> Thanks
> Vivek
> >
> > Implementation:
> >   virtiofsd adds FUSE_CAP_POSIX_ACL/FUSE_CAP_DONT_MASK to INIT replay
> > if  posix_acl is enabled. This results in skipping umask handling in
> > guest  kernel. In turn, virtiofsd sets umask before creating files by
> > using  umask value included in fuse_request and restore umask after that.
> >  Note that calling umask() is ok since now each worker thread has
> > called
> >  unshare(CLONE_FS) when starting up.
> >
> > Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> > ---
> >  tools/virtiofsd/helper.c         |  3 +++
> >  tools/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++++++++------
> >  tools/virtiofsd/seccomp.c        |  1 +
> >  3 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c index
> > 0801cf752c..97968a9385 100644
> > --- a/tools/virtiofsd/helper.c
> > +++ b/tools/virtiofsd/helper.c
> > @@ -171,6 +171,9 @@ void fuse_cmdline_help(void)
> >             "                               default: no_writeback\n"
> >             "    -o xattr|no_xattr          enable/disable xattr\n"
> >             "                               default: no_xattr\n"
> > +           "    -o posix_acl|no_posxi_acl  enable/disable posix_acl\n"
> > +           "                               enabling this also enables xattr\n"
> > +           "                               default: no_posix_acl\n"
> >             );
> >  }
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c
> > b/tools/virtiofsd/passthrough_ll.c
> > index 6bcc33e0eb..e1bb261b51 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -152,6 +152,7 @@ struct lo_data {
> >      int flock;
> >      int posix_lock;
> >      int xattr;
> > +    int posix_acl;
> >      char *source;
> >      double timeout;
> >      int cache;
> > @@ -182,6 +183,8 @@ static const struct fuse_opt lo_opts[] = {
> >      { "no_posix_lock", offsetof(struct lo_data, posix_lock), 0 },
> >      { "xattr", offsetof(struct lo_data, xattr), 1 },
> >      { "no_xattr", offsetof(struct lo_data, xattr), 0 },
> > +    { "posix_acl", offsetof(struct lo_data, posix_acl), 1 },
> > +    { "no_posix_acl", offsetof(struct lo_data, posix_acl), 0 },
> >      { "timeout=%lf", offsetof(struct lo_data, timeout), 0 },
> >      { "timeout=", offsetof(struct lo_data, timeout_set), 1 },
> >      { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE }, @@
> > -587,6 +590,17 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn)
> >          fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling readdirplus\n");
> >          conn->want &= ~FUSE_CAP_READDIRPLUS;
> >      }
> > +
> > +    if (conn->capable & FUSE_CAP_POSIX_ACL) {
> > +        if (lo->posix_acl) {
> > +            if (!lo->xattr)  {
> > +                fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling xattr for ACL\n");
> > +                lo->xattr = 1;
> > +            }
> > +            fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling ACL\n");
> > +            conn->want |= (FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK);
> > +        }
> > +    }
> >  }
> >
> >  static int64_t *version_ptr(struct lo_data *lo, struct lo_inode
> > *inode) @@ -1137,9 +1151,11 @@ static void lo_lookup(fuse_req_t req,
> > fuse_ino_t parent, const char *name)
> >  /*
> >   * Change to uid/gid of caller so that file is created with
> >   * ownership of caller.
> > + * Also update umask if posix_acl is enabled.
> >   * TODO: What about selinux context?
> >   */
> > -static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
> > +static int lo_change_cred_and_umask(fuse_req_t req, struct lo_cred *old,
> > +                                    struct lo_data *lo)
> >  {
> >      int res;
> >
> > @@ -1159,11 +1175,15 @@ static int lo_change_cred(fuse_req_t req, struct lo_cred *old)
> >          return errno_save;
> >      }
> >
> > +    if (lo->posix_acl) {
> > +        umask(req->ctx.umask);
> > +    }
> > +
> >      return 0;
> >  }
> >
> >  /* Regain Privileges */
> > -static void lo_restore_cred(struct lo_cred *old)
> > +static void lo_restore_cred_and_umask(struct lo_cred *old, struct
> > +lo_data *lo)
> >  {
> >      int res;
> >
> > @@ -1178,6 +1198,10 @@ static void lo_restore_cred(struct lo_cred *old)
> >          fuse_log(FUSE_LOG_ERR, "setegid(%u): %m\n", old->egid);
> >          exit(1);
> >      }
> > +
> > +    if (lo->posix_acl) {
> > +        umask(0);
> > +    }
> >  }
> >
> >  static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, @@
> > -1204,7 +1228,7 @@ static void lo_mknod_symlink(fuse_req_t req,
> > fuse_ino_t parent,
> >
> >      saverr = ENOMEM;
> >
> > -    saverr = lo_change_cred(req, &old);
> > +    saverr = lo_change_cred_and_umask(req, &old, lo);
> >      if (saverr) {
> >          goto out;
> >      }
> > @@ -1213,7 +1237,7 @@ static void lo_mknod_symlink(fuse_req_t req,
> > fuse_ino_t parent,
> >
> >      saverr = errno;
> >
> > -    lo_restore_cred(&old);
> > +    lo_restore_cred_and_umask(&old, lo);
> >
> >      if (res == -1) {
> >          goto out;
> > @@ -1898,7 +1922,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> >          return;
> >      }
> >
> > -    err = lo_change_cred(req, &old);
> > +    err = lo_change_cred_and_umask(req, &old, lo);
> >      if (err) {
> >          goto out;
> >      }
> > @@ -1908,7 +1932,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> >      fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> >                  mode);
> >      err = fd == -1 ? errno : 0;
> > -    lo_restore_cred(&old);
> > +    lo_restore_cred_and_umask(&old, lo);
> >
> >      if (!err) {
> >          ssize_t fh;
> > diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
> > index 9ee99e4725..1b6f579452 100644
> > --- a/tools/virtiofsd/seccomp.c
> > +++ b/tools/virtiofsd/seccomp.c
> > @@ -102,6 +102,7 @@ static const int syscall_whitelist[] = {
> >      SCMP_SYS(symlinkat),
> >      SCMP_SYS(time), /* Rarely needed, except on static builds */
> >      SCMP_SYS(tgkill),
> > +    SCMP_SYS(umask),
> >      SCMP_SYS(unlinkat),
> >      SCMP_SYS(unshare),
> >      SCMP_SYS(utimensat),
> > --
> > 2.21.1
> >

Re: [Virtio-fs] [PATCH v2 0/2] virtiofsd: Fix xattr and ACL

[Vivek Goyal]

On Tue, Jan 28, 2020 at 07:18:17PM +0900, Misono Tomohiro wrote:
> Hi,
> 
> This is a second version of xattr fix for virtiofsd.
> I included ACL fix (which introduces new option posix_acl) in this version
> too as ACL mostly depends on xattr.
> 
> I run xfstests with XFS backend using "-o xattr -o posix_acl" option and
> only new failure is generic/375 which checks if sgid bit is cleared after
> setfacl. I'll try to investigate it.
> 
> change in v1 -> v2
>  - rebased to current dev branch
> 
>  - Always chdir for xattr (1st patch)
>    In v1, I keep current implementation for regular file/dir since it
>    show better performance in my environment. But I notice opening file
>    for xattr causes seek sanity test fails (xfstest generic/285, 436).
> 
>    I'm not sure what is the fundamental problem here but I believe
>    performance can be improved by introducing some caching mechanism
>    in general.

Hi Misono,

How much is performance degradation due to fchdir(). If it is significant,
then I will be inclined to keep original code for dir/file till some
other mechanism is introduced to offset the perofrmance loss.

> So I change the code to always fchdir to avoid the
>    problem for now. This results in simpler code too.
> 
>  - Add ACL fix (2nd patch)
>    ACL mostly works if xattr option is enabled.
>    To support default ACL behavior, add new option posix_acl which
>    handles umasking in virtiofsd instead of guest kernel.

Ideally I would preferred ACL changes in a separapte patch series. We had
been discussing xattr related changes and it will be easier to test and
review xattr changes and then followed by ACL changes. Anyway, now you
have already posted these changes in smae patch series, I will review it.

But if you happen to post another patch series, then lets separate out
xattr changes and ACL changes in separate patch series.

Thanks
Vivek

> 
> Thanks,
> Misono
> 
> Misono Tomohiro (2):
>   virtiofsd: Fix xattr operations
>   virtiofsd: Add support of posix_acl
> 
>  tools/virtiofsd/fuse_virtio.c    |  13 +++
>  tools/virtiofsd/helper.c         |   3 +
>  tools/virtiofsd/passthrough_ll.c | 136 +++++++++++++++++++++----------
>  tools/virtiofsd/seccomp.c        |  11 ++-
>  4 files changed, 115 insertions(+), 48 deletions(-)
> 
> -- 
> 2.21.1
> 

Re: [Virtio-fs] [PATCH v2 0/2] virtiofsd: Fix xattr and ACL

[misono.tomohiro]

> On Tue, Jan 28, 2020 at 07:18:17PM +0900, Misono Tomohiro wrote:
> > Hi,
> >
> > This is a second version of xattr fix for virtiofsd.
> > I included ACL fix (which introduces new option posix_acl) in this
> > version too as ACL mostly depends on xattr.
> >
> > I run xfstests with XFS backend using "-o xattr -o posix_acl" option
> > and only new failure is generic/375 which checks if sgid bit is
> > cleared after setfacl. I'll try to investigate it.
> >
> > change in v1 -> v2
> >  - rebased to current dev branch
> >
> >  - Always chdir for xattr (1st patch)
> >    In v1, I keep current implementation for regular file/dir since it
> >    show better performance in my environment. But I notice opening file
> >    for xattr causes seek sanity test fails (xfstest generic/285, 436).
> >
> >    I'm not sure what is the fundamental problem here but I believe
> >    performance can be improved by introducing some caching mechanism
> >    in general.
> 
> Hi Misono,
> 
> How much is performance degradation due to fchdir(). If it is significant, then I will be inclined to keep original code for dir/file
> till some other mechanism is introduced to offset the perofrmance loss.

Please refer this replay: https://www.redhat.com/archives/virtio-fs/2020-January/msg00063.html

> 
> > So I change the code to always fchdir to avoid the
> >    problem for now. This results in simpler code too.
> >
> >  - Add ACL fix (2nd patch)
> >    ACL mostly works if xattr option is enabled.
> >    To support default ACL behavior, add new option posix_acl which
> >    handles umasking in virtiofsd instead of guest kernel.
> 
> Ideally I would preferred ACL changes in a separapte patch series. We had been discussing xattr related changes and it will be
> easier to test and review xattr changes and then followed by ACL changes. 

One motivation I included ACL fix comes from the fact that -o xattr option also enables ACL.

> Anyway, now you have already posted these changes in smae patch series, I will review it.
> But if you happen to post another patch series, then lets separate out xattr changes and ACL changes in separate patch series.

Thanks, If it is better I will do it next version.
(btw, I take a leave next week so my response will be delayed) 

Thanks for all the comments!
Misono

> 
> Thanks
> Vivek
> 
> >
> > Thanks,
> > Misono
> >
> > Misono Tomohiro (2):
> >   virtiofsd: Fix xattr operations
> >   virtiofsd: Add support of posix_acl
> >
> >  tools/virtiofsd/fuse_virtio.c    |  13 +++
> >  tools/virtiofsd/helper.c         |   3 +
> >  tools/virtiofsd/passthrough_ll.c | 136 +++++++++++++++++++++----------
> >  tools/virtiofsd/seccomp.c        |  11 ++-
> >  4 files changed, 115 insertions(+), 48 deletions(-)
> >
> > --
> > 2.21.1
> >

Re: [Virtio-fs] [PATCH v2 0/2] virtiofsd: Fix xattr and ACL

[Vivek Goyal]

On Fri, Jan 31, 2020 at 02:06:51AM +0000, misono.tomohiro@fujitsu.com wrote:
> > On Tue, Jan 28, 2020 at 07:18:17PM +0900, Misono Tomohiro wrote:
> > > Hi,
> > >
> > > This is a second version of xattr fix for virtiofsd.
> > > I included ACL fix (which introduces new option posix_acl) in this
> > > version too as ACL mostly depends on xattr.
> > >
> > > I run xfstests with XFS backend using "-o xattr -o posix_acl" option
> > > and only new failure is generic/375 which checks if sgid bit is
> > > cleared after setfacl. I'll try to investigate it.
> > >
> > > change in v1 -> v2
> > >  - rebased to current dev branch
> > >
> > >  - Always chdir for xattr (1st patch)
> > >    In v1, I keep current implementation for regular file/dir since it
> > >    show better performance in my environment. But I notice opening file
> > >    for xattr causes seek sanity test fails (xfstest generic/285, 436).
> > >
> > >    I'm not sure what is the fundamental problem here but I believe
> > >    performance can be improved by introducing some caching mechanism
> > >    in general.
> > 
> > Hi Misono,
> > 
> > How much is performance degradation due to fchdir(). If it is significant, then I will be inclined to keep original code for dir/file
> > till some other mechanism is introduced to offset the perofrmance loss.
> 
> Please refer this replay: https://www.redhat.com/archives/virtio-fs/2020-January/msg00063.html

As per your email, regression due to fchdir() seems to be in the range of
5% to 10%. It is not trivial, IMO. May be its a good idea to keep original
logic and use fchdir() only when need be.

Thanks
Vivek