* [PATCH v4 0/2] virtiofsd: Fix xattr operations
@ 2020-02-27 5:59 Misono Tomohiro
2020-02-27 5:59 ` [PATCH v4 1/2] virtiofsd: passthrough_ll: cleanup getxattr/listxattr Misono Tomohiro
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Misono Tomohiro @ 2020-02-27 5:59 UTC (permalink / raw)
To: virtio-fs; +Cc: qemu-devel, vgoyal
Currently xattr operations on virtiofs does not work properly in some case:
- directory ... cannot set
- special files (pipe) ... cause hang
This fixes these problems and now xfstests generic/062 passes on virtiofs
with -o xattr option (I tested with xfs).
v3 -> v4:
- No logic change
- Some code style fix/update comments and commit log as suggested by Vivek
- CC qemu-devel ML too
Previous versions can be found in virtiofs ML:
v3: https://www.redhat.com/archives/virtio-fs/2020-February/msg00032.html
Thanks,
Misono Tomohiro (2):
virtiofsd: passthrough_ll: cleanup getxattr/listxattr
virtiofsd: Fix xattr operations
tools/virtiofsd/fuse_virtio.c | 13 +++
tools/virtiofsd/passthrough_ll.c | 139 ++++++++++++++++---------------
tools/virtiofsd/seccomp.c | 6 ++
3 files changed, 89 insertions(+), 69 deletions(-)
--
2.21.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v4 1/2] virtiofsd: passthrough_ll: cleanup getxattr/listxattr
2020-02-27 5:59 [PATCH v4 0/2] virtiofsd: Fix xattr operations Misono Tomohiro
@ 2020-02-27 5:59 ` Misono Tomohiro
2020-02-27 5:59 ` [PATCH v4 2/2] virtiofsd: Fix xattr operations Misono Tomohiro
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Misono Tomohiro @ 2020-02-27 5:59 UTC (permalink / raw)
To: virtio-fs; +Cc: qemu-devel, vgoyal
This is a cleanup patch to simplify the following xattr fix and
there is no functional changes.
- Move memory allocation to head of the function
- Unify fgetxattr/flistxattr call for both size == 0 and
size != 0 case
- Remove redundant lo_inode_put call in error path
(Note: second call is ignored now since @inode is already NULL)
Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
tools/virtiofsd/passthrough_ll.c | 54 +++++++++++++-------------------
1 file changed, 22 insertions(+), 32 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 9772823066..7b94300ae0 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2384,34 +2384,30 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
goto out;
}
+ if (size) {
+ value = malloc(size);
+ if (!value) {
+ goto out_err;
+ }
+ }
+
sprintf(procname, "%i", inode->fd);
fd = openat(lo->proc_self_fd, procname, O_RDONLY);
if (fd < 0) {
goto out_err;
}
+ ret = fgetxattr(fd, name, value, size);
+ if (ret == -1) {
+ goto out_err;
+ }
if (size) {
- value = malloc(size);
- if (!value) {
- goto out_err;
- }
-
- ret = fgetxattr(fd, name, value, size);
- if (ret == -1) {
- goto out_err;
- }
saverr = 0;
if (ret == 0) {
goto out;
}
-
fuse_reply_buf(req, value, ret);
} else {
- ret = fgetxattr(fd, name, NULL, 0);
- if (ret == -1) {
- goto out_err;
- }
-
fuse_reply_xattr(req, ret);
}
out_free:
@@ -2427,7 +2423,6 @@ out_free:
out_err:
saverr = errno;
out:
- lo_inode_put(lo, &inode);
fuse_reply_err(req, saverr);
goto out_free;
}
@@ -2462,34 +2457,30 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
goto out;
}
+ if (size) {
+ value = malloc(size);
+ if (!value) {
+ goto out_err;
+ }
+ }
+
sprintf(procname, "%i", inode->fd);
fd = openat(lo->proc_self_fd, procname, O_RDONLY);
if (fd < 0) {
goto out_err;
}
+ ret = flistxattr(fd, value, size);
+ if (ret == -1) {
+ goto out_err;
+ }
if (size) {
- value = malloc(size);
- if (!value) {
- goto out_err;
- }
-
- ret = flistxattr(fd, value, size);
- if (ret == -1) {
- goto out_err;
- }
saverr = 0;
if (ret == 0) {
goto out;
}
-
fuse_reply_buf(req, value, ret);
} else {
- ret = flistxattr(fd, NULL, 0);
- if (ret == -1) {
- goto out_err;
- }
-
fuse_reply_xattr(req, ret);
}
out_free:
@@ -2505,7 +2496,6 @@ out_free:
out_err:
saverr = errno;
out:
- lo_inode_put(lo, &inode);
fuse_reply_err(req, saverr);
goto out_free;
}
--
2.21.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 2/2] virtiofsd: Fix xattr operations
2020-02-27 5:59 [PATCH v4 0/2] virtiofsd: Fix xattr operations Misono Tomohiro
2020-02-27 5:59 ` [PATCH v4 1/2] virtiofsd: passthrough_ll: cleanup getxattr/listxattr Misono Tomohiro
@ 2020-02-27 5:59 ` Misono Tomohiro
2020-02-28 12:50 ` [PATCH v4 0/2] " Vivek Goyal
2020-03-03 11:37 ` [Virtio-fs] " Dr. David Alan Gilbert
3 siblings, 0 replies; 5+ messages in thread
From: Misono Tomohiro @ 2020-02-27 5:59 UTC (permalink / raw)
To: virtio-fs; +Cc: qemu-devel, vgoyal
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.
The original discussion can be found here:
https://www.redhat.com/archives/virtio-fs/2019-October/msg00046.html
Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
tools/virtiofsd/fuse_virtio.c | 13 ++++
tools/virtiofsd/passthrough_ll.c | 105 +++++++++++++++++--------------
tools/virtiofsd/seccomp.c | 6 ++
3 files changed, 77 insertions(+), 47 deletions(-)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 655b9a1413..21c5d76d58 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -463,6 +463,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)
{
@@ -478,6 +480,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 7b94300ae0..9d325be8a5 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -130,7 +130,7 @@ struct lo_inode {
pthread_mutex_t plock_mutex;
GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */
- bool is_symlink;
+ mode_t filetype;
};
struct lo_cred {
@@ -734,7 +734,7 @@ static int utimensat_empty(struct lo_data *lo, struct lo_inode *inode,
struct lo_inode *parent;
char path[PATH_MAX];
- if (inode->is_symlink) {
+ if (S_ISLNK(inode->filetype)) {
res = utimensat(inode->fd, "", tv, AT_EMPTY_PATH);
if (res == -1 && errno == EINVAL) {
/* Sorry, no race free way to set times on symlink. */
@@ -1037,7 +1037,8 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
goto out_err;
}
- inode->is_symlink = S_ISLNK(e->attr.st_mode);
+ /* cache only filetype */
+ inode->filetype = (e->attr.st_mode & S_IFMT);
/*
* One for the caller and one for nlookup (released in
@@ -1264,7 +1265,7 @@ static int linkat_empty_nofollow(struct lo_data *lo, struct lo_inode *inode,
struct lo_inode *parent;
char path[PATH_MAX];
- if (inode->is_symlink) {
+ if (S_ISLNK(inode->filetype)) {
res = linkat(inode->fd, "", dfd, name, AT_EMPTY_PATH);
if (res == -1 && (errno == ENOENT || errno == EINVAL)) {
/* Sorry, no race free way to hard-link a symlink. */
@@ -2378,12 +2379,6 @@ 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;
- }
-
if (size) {
value = malloc(size);
if (!value) {
@@ -2392,12 +2387,25 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
}
sprintf(procname, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, procname, O_RDONLY);
- if (fd < 0) {
- goto out_err;
+ /*
+ * It is not safe to open() non-regular/non-dir files in file server
+ * unless O_PATH is used, so use that method for regular files/dir
+ * only (as it seems giving less performance overhead).
+ * Otherwise, call fchdir() to avoid open().
+ */
+ if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ goto out_err;
+ }
+ ret = fgetxattr(fd, name, value, size);
+ } else {
+ /* fchdir should not fail here */
+ assert(fchdir(lo->proc_self_fd) == 0);
+ ret = getxattr(procname, name, value, size);
+ assert(fchdir(lo->root.fd) == 0);
}
- ret = fgetxattr(fd, name, value, size);
if (ret == -1) {
goto out_err;
}
@@ -2451,12 +2459,6 @@ 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;
- }
-
if (size) {
value = malloc(size);
if (!value) {
@@ -2465,12 +2467,19 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size)
}
sprintf(procname, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, procname, O_RDONLY);
- if (fd < 0) {
- goto out_err;
+ if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ goto out_err;
+ }
+ ret = flistxattr(fd, value, size);
+ } else {
+ /* fchdir should not fail here */
+ assert(fchdir(lo->proc_self_fd) == 0);
+ ret = listxattr(procname, value, size);
+ assert(fchdir(lo->root.fd) == 0);
}
- ret = flistxattr(fd, value, size);
if (ret == -1) {
goto out_err;
}
@@ -2524,20 +2533,21 @@ 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 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 (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ saverr = errno;
+ goto out;
+ }
+ ret = fsetxattr(fd, name, value, size, flags);
+ } else {
+ /* fchdir should not fail here */
+ assert(fchdir(lo->proc_self_fd) == 0);
+ ret = setxattr(procname, name, value, size, flags);
+ assert(fchdir(lo->root.fd) == 0);
}
- ret = fsetxattr(fd, name, value, size, flags);
saverr = ret == -1 ? errno : 0;
if (!saverr) {
@@ -2575,20 +2585,21 @@ 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 (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
+ fd = openat(lo->proc_self_fd, procname, O_RDONLY);
+ if (fd < 0) {
+ saverr = errno;
+ goto out;
+ }
+ ret = fremovexattr(fd, name);
+ } else {
+ /* fchdir should not fail here */
+ assert(fchdir(lo->proc_self_fd) == 0);
+ ret = removexattr(procname, name);
+ assert(fchdir(lo->root.fd) == 0);
}
- ret = fremovexattr(fd, name);
saverr = ret == -1 ? errno : 0;
if (!saverr) {
@@ -3185,7 +3196,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
exit(1);
}
- root->is_symlink = false;
+ root->filetype = S_IFDIR;
root->fd = fd;
root->key.ino = stat.st_ino;
root->key.dev = stat.st_dev;
diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
index 2d9d4a7ec0..bd9e7b083c 100644
--- a/tools/virtiofsd/seccomp.c
+++ b/tools/virtiofsd/seccomp.c
@@ -41,6 +41,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),
@@ -62,7 +63,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),
@@ -85,6 +88,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),
@@ -98,10 +102,12 @@ static const int syscall_whitelist[] = {
SCMP_SYS(setresuid32),
#endif
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] 5+ messages in thread
* Re: [PATCH v4 0/2] virtiofsd: Fix xattr operations
2020-02-27 5:59 [PATCH v4 0/2] virtiofsd: Fix xattr operations Misono Tomohiro
2020-02-27 5:59 ` [PATCH v4 1/2] virtiofsd: passthrough_ll: cleanup getxattr/listxattr Misono Tomohiro
2020-02-27 5:59 ` [PATCH v4 2/2] virtiofsd: Fix xattr operations Misono Tomohiro
@ 2020-02-28 12:50 ` Vivek Goyal
2020-03-03 11:37 ` [Virtio-fs] " Dr. David Alan Gilbert
3 siblings, 0 replies; 5+ messages in thread
From: Vivek Goyal @ 2020-02-28 12:50 UTC (permalink / raw)
To: Misono Tomohiro; +Cc: virtio-fs, qemu-devel
On Thu, Feb 27, 2020 at 02:59:25PM +0900, Misono Tomohiro wrote:
> Currently xattr operations on virtiofs does not work properly in some case:
> - directory ... cannot set
> - special files (pipe) ... cause hang
>
> This fixes these problems and now xfstests generic/062 passes on virtiofs
> with -o xattr option (I tested with xfs).
>
> v3 -> v4:
> - No logic change
> - Some code style fix/update comments and commit log as suggested by Vivek
> - CC qemu-devel ML too
>
> Previous versions can be found in virtiofs ML:
> v3: https://www.redhat.com/archives/virtio-fs/2020-February/msg00032.html
>
> Thanks,
> Misono Tomohiro (2):
> virtiofsd: passthrough_ll: cleanup getxattr/listxattr
> virtiofsd: Fix xattr operations
Thanks Misono for this work. I am fine with this patch series.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Thanks
Vivek
>
> tools/virtiofsd/fuse_virtio.c | 13 +++
> tools/virtiofsd/passthrough_ll.c | 139 ++++++++++++++++---------------
> tools/virtiofsd/seccomp.c | 6 ++
> 3 files changed, 89 insertions(+), 69 deletions(-)
>
> --
> 2.21.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH v4 0/2] virtiofsd: Fix xattr operations
2020-02-27 5:59 [PATCH v4 0/2] virtiofsd: Fix xattr operations Misono Tomohiro
` (2 preceding siblings ...)
2020-02-28 12:50 ` [PATCH v4 0/2] " Vivek Goyal
@ 2020-03-03 11:37 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2020-03-03 11:37 UTC (permalink / raw)
To: Misono Tomohiro; +Cc: virtio-fs, qemu-devel, vgoyal
* Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote:
> Currently xattr operations on virtiofs does not work properly in some case:
> - directory ... cannot set
> - special files (pipe) ... cause hang
>
> This fixes these problems and now xfstests generic/062 passes on virtiofs
> with -o xattr option (I tested with xfs).
>
> v3 -> v4:
> - No logic change
> - Some code style fix/update comments and commit log as suggested by Vivek
> - CC qemu-devel ML too
>
> Previous versions can be found in virtiofs ML:
> v3: https://www.redhat.com/archives/virtio-fs/2020-February/msg00032.html
>
> Thanks,
Thanks,
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
and queued.
(I suspect there's some more cleanup involved in the error paths in
those functions; I'll have a look another time).
Dave
> Misono Tomohiro (2):
> virtiofsd: passthrough_ll: cleanup getxattr/listxattr
> virtiofsd: Fix xattr operations
>
> tools/virtiofsd/fuse_virtio.c | 13 +++
> tools/virtiofsd/passthrough_ll.c | 139 ++++++++++++++++---------------
> tools/virtiofsd/seccomp.c | 6 ++
> 3 files changed, 89 insertions(+), 69 deletions(-)
>
> --
> 2.21.1
>
>
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-03 11:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-27 5:59 [PATCH v4 0/2] virtiofsd: Fix xattr operations Misono Tomohiro
2020-02-27 5:59 ` [PATCH v4 1/2] virtiofsd: passthrough_ll: cleanup getxattr/listxattr Misono Tomohiro
2020-02-27 5:59 ` [PATCH v4 2/2] virtiofsd: Fix xattr operations Misono Tomohiro
2020-02-28 12:50 ` [PATCH v4 0/2] " Vivek Goyal
2020-03-03 11:37 ` [Virtio-fs] " Dr. David Alan Gilbert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).