[Virtio-fs] [PATCH v2 1/2] virtiofsd: Fix xattr operations

[misono.tomohiro at fujitsu.com]

> > 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 at 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