Re: [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Jan 09, 2020 at 09:27:15AM +0000, misono.tomohiro@fujitsu.com wrote:
> > On Wed, Jan 08, 2020 at 03:24:22PM -0500, Vivek Goyal wrote:
> > > Do not open fd O_RDWR as it will fail for directories with EISDIR.
> > > This code can be called both for regular files as well as directories.
> > >
> > > I noticed this when I tried "setfattr -n user.foo -v test <dir>"
> > > inside the guest and got EISDIR.
> > >
> > > To write xattr, we don't have to open fd with write permissions. Looks
> > > like kernel will do permission checks on inode.
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Eryu Guan <eguan@linux.alibaba.com>
> > 
> > > ---
> > >  contrib/virtiofsd/passthrough_ll.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > Index: qemu/contrib/virtiofsd/passthrough_ll.c
> > > ===================================================================
> > > --- qemu.orig/contrib/virtiofsd/passthrough_ll.c	2020-01-08 15:01:03.821980889 -0500
> > > +++ qemu/contrib/virtiofsd/passthrough_ll.c	2020-01-08 15:05:15.209384352 -0500
> > > @@ -2424,7 +2424,7 @@ static void lo_setxattr(fuse_req_t req,
> > >  	}
> > >
> > >  	sprintf(procname, "%i", inode->fd);
> > > -	fd = openat(lo->proc_self_fd, procname, O_RDWR);
> > > +	fd = openat(lo->proc_self_fd, procname, O_RDONLY);
> > >  	if (fd < 0) {
> > >  		saverr = errno;
> > >  		goto out;
> > > @@ -2473,7 +2473,7 @@ static void lo_removexattr(fuse_req_t re
> > >  	}
> > >
> > >  	sprintf(procname, "%i", inode->fd);
> > > -	fd = openat(lo->proc_self_fd, procname, O_RDWR);
> > > +	fd = openat(lo->proc_self_fd, procname, O_RDONLY);
> > >  	if (fd < 0) {
> > >  		saverr = errno;
> > >  		goto out;
> > >
> 
> Hello,
> 
> So I sent the same patch last October[1] and got comments
> that it's would be better to fix the fundamental problem
> (do not call openat() to non regular file/directory) [2].

Right. So this patch is only solving the issue for directories (and not
for symlinks other special files). We will need more improvements in
this area.

I am trying to make overlayfs work on top of virtiofs so that it can
be tested and see if it works well or not. Overlayfs does bunch of
xattr operations on work/work directory and it fails. So this patch
will atleast fix that and allow me to make progress.


>   [1] https://www.redhat.com/archives/virtio-fs/2019-October/msg00030.html
>   [2] https://www.redhat.com/archives/virtio-fs/2019-October/msg00046.html
>  
> I tried the suggested approach which uses fchdir(proc_self_fd) + ...xattr() + fchdir(root.fd)
> combination instead of current openat() + f...xattr(). 
> However, it seems that always fchdir on xattr operation for regular files too incurs
> some performance overhead (~10% or more).
>  
> So I think hybrid approach is better: 
>   For regular file/directory: use f...xattr
>   For other file types: use fchdir + ...xattr + fchdir 

This sounds reasonable. I will give it a try.  In fact Miklos's patch
for allowing fsetxattr() on O_PATH is still being reviewed upstream. Once
that is merged, our approach should probably be.

1. Try fsetxattr(O_PATH fd).
2. If 1 fails, try openat() followed by fsetxattr() for regular files
   and dirs.
3. For other file types use fchdir + ...xattr() + fchdir. 

Thanks
Vivek

>  
> Attached patch adopts the above solution.
> So could you check the patch's approach? If it is fine, I will send it as a formal patch
> (and I apologize that my response is delayed).
> 
> Thanks,
> Misono