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

[misono.tomohiro at fujitsu.com]

> On Thu, Jan 09, 2020 at 09:27:15AM +0000, misono.tomohiro at 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 at redhat.com>
> > >
> > > Looks good to me.
> > >
> > > Reviewed-by: Eryu Guan <eguan at 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].
> >   [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
> >
> > Attached patch adopts the above solution.
> 
> Hi Misono,
> 
> Can you post this patch again separately so that it is easier to give comments and discuss this patch. I have a quick look and
> this patch does look reasonable.

Thanks for the check, I will post it shortly.
Misono