* [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr
@ 2020-01-08 20:24 Vivek Goyal
2020-01-09 2:23 ` Eryu Guan
0 siblings, 1 reply; 6+ messages in thread
From: Vivek Goyal @ 2020-01-08 20:24 UTC (permalink / raw)
To: virtio-fs-list
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>
---
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr
2020-01-08 20:24 [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr Vivek Goyal
@ 2020-01-09 2:23 ` Eryu Guan
2020-01-09 9:27 ` misono.tomohiro
0 siblings, 1 reply; 6+ messages in thread
From: Eryu Guan @ 2020-01-09 2:23 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs-list
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;
>
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr
2020-01-09 2:23 ` Eryu Guan
@ 2020-01-09 9:27 ` misono.tomohiro
2020-01-09 17:56 ` Vivek Goyal
2020-01-09 19:10 ` Vivek Goyal
0 siblings, 2 replies; 6+ messages in thread
From: misono.tomohiro @ 2020-01-09 9:27 UTC (permalink / raw)
To: 'Eryu Guan', Vivek Goyal; +Cc: virtio-fs-list
[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]
> 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].
[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
[-- Attachment #2: virtiofs-Fix-xattr-operations.patch --]
[-- Type: application/octet-stream, Size: 12736 bytes --]
From 4be65d24ab400aa4322116c0f9cc79a35e5e500f Mon Sep 17 00:00:00 2001
From: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
Date: Thu, 9 Jan 2020 14:10:53 +0900
Subject: [PATCH] virtiofs: Fix xattr operations
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)
2. in xattr operations (i.e. lo_getxattr), if inode is not a regular
file or directory, use fchdir(proc_loot_fd) + ...xattr() +
fchdir(root.fd) instead of openat() + f...xattr()
(Note: for a regular file/directory openat() + f...xattr()
is still used for performance reason)
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@jp.fujitsu.com>
---
tools/virtiofsd/fuse_virtio.c | 13 +++
tools/virtiofsd/passthrough_ll.c | 167 +++++++++++++++++++++++--------
tools/virtiofsd/seccomp.c | 6 ++
3 files changed, 142 insertions(+), 44 deletions(-)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 254b0a71cd..99339e17b2 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -460,6 +460,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)
{
@@ -475,6 +477,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 83dd0084b4..5b69048ddd 100644
--- 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);
/*
* 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;
- }
-
sprintf(procname, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, procname, O_RDONLY);
- if (fd < 0) {
- goto out_err;
+ if (inode->is_regular) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ goto out_err;
+ }
+ } else {
+ 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) {
@@ -2378,7 +2385,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
goto out_err;
}
- ret = fgetxattr(fd, name, value, size);
+ if (inode->is_regular) {
+ ret = fgetxattr(fd, name, value, size);
+ } else {
+ ret = getxattr(procname, name, value, size);
+ }
if (ret == -1) {
goto out_err;
}
@@ -2389,7 +2400,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
fuse_reply_buf(req, value, ret);
} else {
- ret = fgetxattr(fd, name, NULL, 0);
+ if (inode->is_regular) {
+ ret = fgetxattr(fd, name, NULL, 0);
+ } else {
+ ret = getxattr(procname, name, NULL, 0);
+ }
if (ret == -1) {
goto out_err;
}
@@ -2403,6 +2418,14 @@ out_free:
close(fd);
}
+ if (dir_changed) {
+ ret = fchdir(lo->root.fd);
+ if (ret < 0) {
+ /* should not happen */
+ fuse_log(FUSE_LOG_ERR, "lo_getxattr: fail fchdir to root.fd\n");
+ }
+ }
+
lo_inode_put(lo, &inode);
return;
@@ -2423,6 +2446,7 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
ssize_t ret;
int saverr;
int fd = -1;
+ bool dir_changed = false;
inode = lo_inode(req, ino);
if (!inode) {
@@ -2438,16 +2462,20 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
fuse_log(FUSE_LOG_DEBUG, "lo_listxattr(ino=%" PRIu64 ", size=%zd)\n", ino,
size);
- if (inode->is_symlink) {
- /* Sorry, no race free way to listxattr on symlink. */
- saverr = EPERM;
- goto out;
- }
-
sprintf(procname, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, procname, O_RDONLY);
- if (fd < 0) {
- goto out_err;
+ if (inode->is_regular) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ goto out_err;
+ }
+ } else {
+ ret = fchdir(lo->proc_self_fd);
+ if (ret < 0) {
+ /* should not happen */
+ fuse_log(FUSE_LOG_ERR, "lo_listxattr: fail fchdir to proc_self_fd\n");
+ goto out_err;
+ }
+ dir_changed = true;
}
if (size) {
@@ -2456,7 +2484,11 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
goto out_err;
}
- ret = flistxattr(fd, value, size);
+ if (inode->is_regular) {
+ ret = flistxattr(fd, value, size);
+ } else {
+ ret = listxattr(procname, value, size);
+ }
if (ret == -1) {
goto out_err;
}
@@ -2467,7 +2499,11 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
fuse_reply_buf(req, value, ret);
} else {
- ret = flistxattr(fd, NULL, 0);
+ if (inode->is_regular) {
+ ret = flistxattr(fd, NULL, 0);
+ } else {
+ ret = listxattr(procname, NULL, 0);
+ }
if (ret == -1) {
goto out_err;
}
@@ -2481,6 +2517,14 @@ out_free:
close(fd);
}
+ if (dir_changed) {
+ ret = fchdir(lo->root.fd);
+ if (ret < 0) {
+ /* should not happen */
+ fuse_log(FUSE_LOG_ERR, "lo_listxattr: fail fchdir to root.fd\n");
+ }
+ }
+
lo_inode_put(lo, &inode);
return;
@@ -2501,6 +2545,7 @@ static void lo_setxattr(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) {
@@ -2516,20 +2561,28 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
fuse_log(FUSE_LOG_DEBUG, "lo_setxattr(ino=%" PRIu64,
", name=%s value=%s size=%zd)\n", ino, name, value, size);
- if (inode->is_symlink) {
- /* Sorry, no race free way to removexattr on symlink. */
- saverr = EPERM;
- goto out;
- }
-
sprintf(procname, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, procname, O_RDWR);
- if (fd < 0) {
- saverr = errno;
- goto out;
+ if (inode->is_regular) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ saverr = errno;
+ goto out;
+ }
+
+ ret = fsetxattr(fd, name, value, size, flags);
+ } else {
+ ret = fchdir(lo->proc_self_fd);
+ if (ret < 0) {
+ /* should not happen */
+ fuse_log(FUSE_LOG_ERR, "lo_setxattr: fail fchdir to proc_self_fd\n");
+ saverr = errno;
+ goto out;
+ }
+ dir_changed = true;
+
+ ret = setxattr(procname, name, value, size, flags);
}
- ret = fsetxattr(fd, name, value, size, flags);
saverr = ret == -1 ? errno : 0;
if (!saverr) {
@@ -2540,6 +2593,14 @@ out:
close(fd);
}
+ if (dir_changed) {
+ ret = fchdir(lo->root.fd);
+ if (ret < 0) {
+ /* should not happen */
+ fuse_log(FUSE_LOG_ERR, "lo_setxattr: fail fchdir to root.fd\n");
+ }
+ }
+
lo_inode_put(lo, &inode);
fuse_reply_err(req, saverr);
}
@@ -2552,6 +2613,7 @@ static void lo_removexattr(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) {
@@ -2567,20 +2629,28 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *name)
fuse_log(FUSE_LOG_DEBUG, "lo_removexattr(ino=%" PRIu64 ", name=%s)\n", ino,
name);
- if (inode->is_symlink) {
- /* Sorry, no race free way to setxattr on symlink. */
- saverr = EPERM;
- goto out;
- }
-
sprintf(procname, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, procname, O_RDWR);
- if (fd < 0) {
- saverr = errno;
- goto out;
+ if (inode->is_regular) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ saverr = errno;
+ goto out;
+ }
+
+ ret = fremovexattr(fd, name);
+ } else {
+ ret = fchdir(lo->proc_self_fd);
+ if (ret < 0) {
+ /* should not happen */
+ fuse_log(FUSE_LOG_ERR, "lo_removexattr: fail fchdir to proc_self_fd\n");
+ saverr = errno;
+ goto out;
+ }
+ dir_changed = true;
+
+ ret = removexattr(procname, name);
}
- ret = fremovexattr(fd, name);
saverr = ret == -1 ? errno : 0;
if (!saverr) {
@@ -2591,6 +2661,14 @@ out:
close(fd);
}
+ if (dir_changed) {
+ ret = fchdir(lo->root.fd);
+ if (ret < 0) {
+ /* should not happen */
+ fuse_log(FUSE_LOG_ERR, "lo_removexattr: fail fchdir to root.fd\n");
+ }
+ }
+
lo_inode_put(lo, &inode);
fuse_reply_err(req, saverr);
}
@@ -3316,6 +3394,7 @@ int main(int argc, char *argv[])
lo.source = strdup("/");
}
lo.root.is_symlink = false;
+ lo.root.is_regular = true;
if (!lo.timeout_set) {
switch (lo.cache) {
case CACHE_NONE:
diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
index 33ec309fc8..87048c239e 100644
--- a/tools/virtiofsd/seccomp.c
+++ b/tools/virtiofsd/seccomp.c
@@ -37,6 +37,7 @@ static const int syscall_whitelist[] = {
SCMP_SYS(exit),
SCMP_SYS(exit_group),
SCMP_SYS(fallocate),
+ SCMP_SYS(fchdir),
SCMP_SYS(fchmodat),
SCMP_SYS(fchownat),
SCMP_SYS(fcntl),
@@ -58,7 +59,9 @@ static const int syscall_whitelist[] = {
SCMP_SYS(getpid),
SCMP_SYS(gettid),
SCMP_SYS(gettimeofday),
+ SCMP_SYS(getxattr),
SCMP_SYS(linkat),
+ SCMP_SYS(listxattr),
SCMP_SYS(lseek),
SCMP_SYS(madvise),
SCMP_SYS(mkdirat),
@@ -81,6 +84,7 @@ static const int syscall_whitelist[] = {
SCMP_SYS(recvmsg),
SCMP_SYS(renameat),
SCMP_SYS(renameat2),
+ SCMP_SYS(removexattr),
SCMP_SYS(rt_sigaction),
SCMP_SYS(rt_sigprocmask),
SCMP_SYS(rt_sigreturn),
@@ -88,10 +92,12 @@ static const int syscall_whitelist[] = {
SCMP_SYS(setresgid),
SCMP_SYS(setresuid),
SCMP_SYS(set_robust_list),
+ SCMP_SYS(setxattr),
SCMP_SYS(symlinkat),
SCMP_SYS(time), /* Rarely needed, except on static builds */
SCMP_SYS(tgkill),
SCMP_SYS(unlinkat),
+ SCMP_SYS(unshare),
SCMP_SYS(utimensat),
SCMP_SYS(write),
SCMP_SYS(writev),
--
2.21.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr
2020-01-09 9:27 ` misono.tomohiro
@ 2020-01-09 17:56 ` Vivek Goyal
2020-01-09 19:10 ` Vivek Goyal
1 sibling, 0 replies; 6+ messages in thread
From: Vivek Goyal @ 2020-01-09 17:56 UTC (permalink / raw)
To: misono.tomohiro; +Cc: virtio-fs-list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr
2020-01-09 9:27 ` misono.tomohiro
2020-01-09 17:56 ` Vivek Goyal
@ 2020-01-09 19:10 ` Vivek Goyal
2020-01-10 0:50 ` misono.tomohiro
1 sibling, 1 reply; 6+ messages in thread
From: Vivek Goyal @ 2020-01-09 19:10 UTC (permalink / raw)
To: misono.tomohiro; +Cc: virtio-fs-list
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].
> [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.
Only minor concern I have is that now each thread has done unshare(CLONE_FS).
Will we run into use cases where we want to enforce a particular change
aross all threads.
Thanks
Vivek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr
2020-01-09 19:10 ` Vivek Goyal
@ 2020-01-10 0:50 ` misono.tomohiro
0 siblings, 0 replies; 6+ messages in thread
From: misono.tomohiro @ 2020-01-10 0:50 UTC (permalink / raw)
To: 'Vivek Goyal'; +Cc: virtio-fs-list
> 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].
> > [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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-10 0:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-08 20:24 [Virtio-fs] [PATCH] virtiofsd: Open fd O_RDONLY in setxattr/removexattr Vivek Goyal
2020-01-09 2:23 ` Eryu Guan
2020-01-09 9:27 ` misono.tomohiro
2020-01-09 17:56 ` Vivek Goyal
2020-01-09 19:10 ` Vivek Goyal
2020-01-10 0:50 ` misono.tomohiro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.