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

[misono.tomohiro at fujitsu.com]

> On Mon, Jan 13, 2020 at 7:42 PM Vivek Goyal <vgoyal at 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 at 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