[Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr
misono.tomohiro at fujitsu.com
misono.tomohiro at fujitsu.com
Thu Jan 9 09:27:15 UTC 2020
> 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.
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: virtiofs-Fix-xattr-operations.patch
Type: application/octet-stream
Size: 12736 bytes
Desc: virtiofs-Fix-xattr-operations.patch
URL: <http://listman.redhat.com/archives/virtio-fs/attachments/20200109/65627daf/attachment.obj>
More information about the Virtio-fs
mailing list