[PATCH 0/2] virtiofsd: Two fix for xattr operation

[PATCH 0/2] virtiofsd: Two fix for xattr operation

[Misono Tomohiro]

Hello,

I test xattr operation on virtiofs using xfstest generic/062
(with -o xattr option and XFS backend) and see some problems.

These patches fixes the two of the problems.

The remaining problems are:
 1. we cannot xattr to block device created by mknod
    which does not have actual device (since open in virtiofsd fails)
 2. we cannot xattr to symbolic link

I don't think 1 is a big problem but can we fix 2?

Misono Tomohiro (2):
  virtiofsd: Avoid process hang when doing xattr operation to FIFO
  virtiofsd: Allow setxattr operation to directry

 contrib/virtiofsd/passthrough_ll.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH 1/2] virtiofsd: Avoid process hang when doing xattr operation to FIFO

[Misono Tomohiro]

I see xfstest generic/062 causes process hang because of xattr operation to
FIFO created by mknod. The problem is that virtiofsd opens any files
with only O_RDWR or O_RDONLY flags for xattr operation, and therefore
if a file is FIFO, open may not return.

Since O_NONBLOCK flag has no effect to regular files, add it to
open flags to fix the problem.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
 contrib/virtiofsd/passthrough_ll.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 84b60d85bd..645324da58 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2251,7 +2251,7 @@ 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);
+	fd = openat(lo->proc_self_fd, procname, O_RDONLY|O_NONBLOCK);
 	if (fd < 0) {
 		goto out_err;
 	}
@@ -2323,7 +2323,7 @@ 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);
+	fd = openat(lo->proc_self_fd, procname, O_RDONLY|O_NONBLOCK);
 	if (fd < 0) {
 		goto out_err;
 	}
@@ -2397,7 +2397,7 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
 	}
 
 	sprintf(procname, "%i", inode->fd);
-	fd = openat(lo->proc_self_fd, procname, O_RDWR);
+	fd = openat(lo->proc_self_fd, procname, O_RDWR|O_NONBLOCK);
 	if (fd < 0) {
 		saverr = errno;
 		goto out;
@@ -2446,7 +2446,7 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *name)
 	}
 
 	sprintf(procname, "%i", inode->fd);
-	fd = openat(lo->proc_self_fd, procname, O_RDWR);
+	fd = openat(lo->proc_self_fd, procname, O_RDWR|O_NONBLOCK);
 	if (fd < 0) {
 		saverr = errno;
 		goto out;
-- 
2.21.0

[PATCH 2/2] virtiofsd: Allow setxattr operation to directry

[Misono Tomohiro]

setxattr to directry fails because lo_setxattr (and lo_remove_xattr)
tries to open any file with O_RDWR even if it is a directory.
Since O_RDONLY is enough for the operation, change O_RDWR flag to
O_RDONLY to fix the problem.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
 contrib/virtiofsd/passthrough_ll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 645324da58..1b439d58ed 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2397,7 +2397,7 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *name,
 	}
 
 	sprintf(procname, "%i", inode->fd);
-	fd = openat(lo->proc_self_fd, procname, O_RDWR|O_NONBLOCK);
+	fd = openat(lo->proc_self_fd, procname, O_RDONLY|O_NONBLOCK);
 	if (fd < 0) {
 		saverr = errno;
 		goto out;
@@ -2446,7 +2446,7 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *name)
 	}
 
 	sprintf(procname, "%i", inode->fd);
-	fd = openat(lo->proc_self_fd, procname, O_RDWR|O_NONBLOCK);
+	fd = openat(lo->proc_self_fd, procname, O_RDONLY|O_NONBLOCK);
 	if (fd < 0) {
 		saverr = errno;
 		goto out;
-- 
2.21.0

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

On Wed, Oct 16, 2019 at 07:37:52PM +0900, Misono Tomohiro wrote:
> Hello,
> 
> I test xattr operation on virtiofs using xfstest generic/062
> (with -o xattr option and XFS backend) and see some problems.
> 
> These patches fixes the two of the problems.
> 
> The remaining problems are:
>  1. we cannot xattr to block device created by mknod
>     which does not have actual device (since open in virtiofsd fails)
>  2. we cannot xattr to symbolic link
> 
> I don't think 1 is a big problem but can we fix 2?

Sorry, I don't know the answer.  Maybe it would be necessary to add a
new O_SYMLINK open flag to open(2) so that fgetxattr()/fsetxattr()
operations can be performed.  A kernel change like that would take some
time to get accepted upstream and shipped by distros, but it might be
the only way since the current syscall interface doesn't seem to offer a
way to do this.

> 
> Misono Tomohiro (2):
>   virtiofsd: Avoid process hang when doing xattr operation to FIFO
>   virtiofsd: Allow setxattr operation to directry
> 
>  contrib/virtiofsd/passthrough_ll.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> -- 
> 2.21.0
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Miklos Szeredi]

On Thu, Oct 17, 2019 at 12:05 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Wed, Oct 16, 2019 at 07:37:52PM +0900, Misono Tomohiro wrote:
> > Hello,
> >
> > I test xattr operation on virtiofs using xfstest generic/062
> > (with -o xattr option and XFS backend) and see some problems.
> >
> > These patches fixes the two of the problems.
> >
> > The remaining problems are:
> >  1. we cannot xattr to block device created by mknod
> >     which does not have actual device (since open in virtiofsd fails)
> >  2. we cannot xattr to symbolic link
> >
> > I don't think 1 is a big problem but can we fix 2?
>
> Sorry, I don't know the answer.  Maybe it would be necessary to add a
> new O_SYMLINK open flag to open(2) so that fgetxattr()/fsetxattr()
> operations can be performed.  A kernel change like that would take some
> time to get accepted upstream and shipped by distros, but it might be
> the only way since the current syscall interface doesn't seem to offer a
> way to do this.

The real problem is that open() on a non-regular, non-directory file
may have side effects (unless O_PATH is used).  These patches try to
paper over that, but the fact is: opening special files from a file
server is forbidden.

I see why this is being done, and it's not easy to fix properly
without the ..at() versions of these syscalls.  One idea is to fork()
+ fchdir(lo->proc_self_fd) + ..xattr().  Another related idea is to do
a unshare(CLONE_FS) after each thread's startup (will pthread library
balk?  I don't know) so that it's safe to do fchdir(lo->proc_self_fd)
+ ...xattr() + fchdir(lo->root_fd).

Thanks,
Miklos

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Miklos Szeredi]

On Thu, Oct 17, 2019 at 1:23 PM Miklos Szeredi <mszeredi@redhat.com> wrote:

> I see why this is being done, and it's not easy to fix properly
> without the ..at() versions of these syscalls.  One idea is to fork()
> + fchdir(lo->proc_self_fd) + ..xattr().  Another related idea is to do
> a unshare(CLONE_FS) after each thread's startup (will pthread library

Apparently Samba does this, so it should be safe.

Thanks,
Miklos

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Vivek Goyal]

On Thu, Oct 17, 2019 at 01:23:57PM +0200, Miklos Szeredi wrote:
> On Thu, Oct 17, 2019 at 12:05 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Wed, Oct 16, 2019 at 07:37:52PM +0900, Misono Tomohiro wrote:
> > > Hello,
> > >
> > > I test xattr operation on virtiofs using xfstest generic/062
> > > (with -o xattr option and XFS backend) and see some problems.
> > >
> > > These patches fixes the two of the problems.
> > >
> > > The remaining problems are:
> > >  1. we cannot xattr to block device created by mknod
> > >     which does not have actual device (since open in virtiofsd fails)
> > >  2. we cannot xattr to symbolic link
> > >
> > > I don't think 1 is a big problem but can we fix 2?
> >
> > Sorry, I don't know the answer.  Maybe it would be necessary to add a
> > new O_SYMLINK open flag to open(2) so that fgetxattr()/fsetxattr()
> > operations can be performed.  A kernel change like that would take some
> > time to get accepted upstream and shipped by distros, but it might be
> > the only way since the current syscall interface doesn't seem to offer a
> > way to do this.
> 
> The real problem is that open() on a non-regular, non-directory file
> may have side effects (unless O_PATH is used).  These patches try to
> paper over that, but the fact is: opening special files from a file
> server is forbidden.
> 
> I see why this is being done, and it's not easy to fix properly
> without the ..at() versions of these syscalls.  One idea is to fork()
> + fchdir(lo->proc_self_fd) + ..xattr().  Another related idea is to do
> a unshare(CLONE_FS) after each thread's startup (will pthread library
> balk?  I don't know) so that it's safe to do fchdir(lo->proc_self_fd)
> + ...xattr() + fchdir(lo->root_fd).

Hi Miklos,

Trying to understand your proposal. So if we want to do an ..xattr()
operation on a special file (and we don't have _at() variants), how
will fchdir() help. Are you suggesting fchdir() to parent and then
do something special.

Can you please elaborate your proposal a bit more. I think I have
missed the core idea completely.

I understand that you want to do unshare(CLONE_FS) to make sure one thrad's
fchdir() does not impact other thread. But did not understand that how
fchdir() alone is enough to do getxattr()/setxattr() on special file.

Thanks
Vivek

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Miklos Szeredi]

On Thu, Oct 17, 2019 at 5:25 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Oct 17, 2019 at 01:23:57PM +0200, Miklos Szeredi wrote:
> > On Thu, Oct 17, 2019 at 12:05 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Wed, Oct 16, 2019 at 07:37:52PM +0900, Misono Tomohiro wrote:
> > > > Hello,
> > > >
> > > > I test xattr operation on virtiofs using xfstest generic/062
> > > > (with -o xattr option and XFS backend) and see some problems.
> > > >
> > > > These patches fixes the two of the problems.
> > > >
> > > > The remaining problems are:
> > > >  1. we cannot xattr to block device created by mknod
> > > >     which does not have actual device (since open in virtiofsd fails)
> > > >  2. we cannot xattr to symbolic link
> > > >
> > > > I don't think 1 is a big problem but can we fix 2?
> > >
> > > Sorry, I don't know the answer.  Maybe it would be necessary to add a
> > > new O_SYMLINK open flag to open(2) so that fgetxattr()/fsetxattr()
> > > operations can be performed.  A kernel change like that would take some
> > > time to get accepted upstream and shipped by distros, but it might be
> > > the only way since the current syscall interface doesn't seem to offer a
> > > way to do this.
> >
> > The real problem is that open() on a non-regular, non-directory file
> > may have side effects (unless O_PATH is used).  These patches try to
> > paper over that, but the fact is: opening special files from a file
> > server is forbidden.
> >
> > I see why this is being done, and it's not easy to fix properly
> > without the ..at() versions of these syscalls.  One idea is to fork()
> > + fchdir(lo->proc_self_fd) + ..xattr().  Another related idea is to do
> > a unshare(CLONE_FS) after each thread's startup (will pthread library
> > balk?  I don't know) so that it's safe to do fchdir(lo->proc_self_fd)
> > + ...xattr() + fchdir(lo->root_fd).
>
> Hi Miklos,
>
> Trying to understand your proposal. So if we want to do an ..xattr()
> operation on a special file (and we don't have _at() variants), how
> will fchdir() help. Are you suggesting fchdir() to parent and then
> do something special.
>
> Can you please elaborate your proposal a bit more. I think I have
> missed the core idea completely.
>
> I understand that you want to do unshare(CLONE_FS) to make sure one thrad's
> fchdir() does not impact other thread. But did not understand that how
> fchdir() alone is enough to do getxattr()/setxattr() on special file.

The fchdir() call is to avoid having to do openat().  The openat() was
needed because  /proc/self/fd/ is only accessible through a file
handle (lo->proc_self_fd).

Thanks,
Miklos

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]

On Thu, Oct 17, 2019 at 01:23:57PM +0200, Miklos Szeredi wrote:
> On Thu, Oct 17, 2019 at 12:05 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Wed, Oct 16, 2019 at 07:37:52PM +0900, Misono Tomohiro wrote:
> > > Hello,
> > >
> > > I test xattr operation on virtiofs using xfstest generic/062
> > > (with -o xattr option and XFS backend) and see some problems.
> > >
> > > These patches fixes the two of the problems.
> > >
> > > The remaining problems are:
> > >  1. we cannot xattr to block device created by mknod
> > >     which does not have actual device (since open in virtiofsd fails)
> > >  2. we cannot xattr to symbolic link
> > >
> > > I don't think 1 is a big problem but can we fix 2?
> >
> > Sorry, I don't know the answer.  Maybe it would be necessary to add a
> > new O_SYMLINK open flag to open(2) so that fgetxattr()/fsetxattr()
> > operations can be performed.  A kernel change like that would take some
> > time to get accepted upstream and shipped by distros, but it might be
> > the only way since the current syscall interface doesn't seem to offer a
> > way to do this.
> 
> The real problem is that open() on a non-regular, non-directory file
> may have side effects (unless O_PATH is used).  These patches try to
> paper over that, but the fact is: opening special files from a file
> server is forbidden.
> 
> I see why this is being done, and it's not easy to fix properly
> without the ..at() versions of these syscalls.  One idea is to fork()
> + fchdir(lo->proc_self_fd) + ..xattr().  Another related idea is to do
> a unshare(CLONE_FS) after each thread's startup (will pthread library
> balk?  I don't know) so that it's safe to do fchdir(lo->proc_self_fd)
> + ...xattr() + fchdir(lo->root_fd).

Looking at the f*xattr() code in the kernel, it doesn't really care
about the file descriptor, it wants the inode instead.  So the O_SYMLINK
idea I mentioned could also be called O_XATTR and be similar to O_PATH,
except that only f*xattr() calls are allowed on this file descriptor.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Miklos Szeredi]

On Thu, Oct 17, 2019 at 6:09 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Oct 17, 2019 at 01:23:57PM +0200, Miklos Szeredi wrote:
> > On Thu, Oct 17, 2019 at 12:05 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Wed, Oct 16, 2019 at 07:37:52PM +0900, Misono Tomohiro wrote:
> > > > Hello,
> > > >
> > > > I test xattr operation on virtiofs using xfstest generic/062
> > > > (with -o xattr option and XFS backend) and see some problems.
> > > >
> > > > These patches fixes the two of the problems.
> > > >
> > > > The remaining problems are:
> > > >  1. we cannot xattr to block device created by mknod
> > > >     which does not have actual device (since open in virtiofsd fails)
> > > >  2. we cannot xattr to symbolic link
> > > >
> > > > I don't think 1 is a big problem but can we fix 2?
> > >
> > > Sorry, I don't know the answer.  Maybe it would be necessary to add a
> > > new O_SYMLINK open flag to open(2) so that fgetxattr()/fsetxattr()
> > > operations can be performed.  A kernel change like that would take some
> > > time to get accepted upstream and shipped by distros, but it might be
> > > the only way since the current syscall interface doesn't seem to offer a
> > > way to do this.
> >
> > The real problem is that open() on a non-regular, non-directory file
> > may have side effects (unless O_PATH is used).  These patches try to
> > paper over that, but the fact is: opening special files from a file
> > server is forbidden.
> >
> > I see why this is being done, and it's not easy to fix properly
> > without the ..at() versions of these syscalls.  One idea is to fork()
> > + fchdir(lo->proc_self_fd) + ..xattr().  Another related idea is to do
> > a unshare(CLONE_FS) after each thread's startup (will pthread library
> > balk?  I don't know) so that it's safe to do fchdir(lo->proc_self_fd)
> > + ...xattr() + fchdir(lo->root_fd).
>
> Looking at the f*xattr() code in the kernel, it doesn't really care
> about the file descriptor, it wants the inode instead.  So the O_SYMLINK
> idea I mentioned could also be called O_XATTR and be similar to O_PATH,
> except that only f*xattr() calls are allowed on this file descriptor.

Even simpler: allow O_PATH descriptors for f*xattr().

Thanks,
Miklos

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Thu, Oct 17, 2019 at 6:48 PM Miklos Szeredi <mszeredi@redhat.com> wrote:

> Even simpler: allow O_PATH descriptors for f*xattr().

Attached patch.  Will post shortly.

However, I think it would make sense to fix virtiofsd as well, as this
will take time to percolate down, even if Al doesn't find anything
wrong with it.

Doing unshare(CLONE_FS) after thread startup seems safe, though must
be careful to change the working directory to the root of the mount
*before* starting any threads.

Thanks,
Miklos

[-- Attachment #2: xattr-allow-o_path-descriptors.patch --]
[-- Type: text/x-patch, Size: 1223 bytes --]

diff --git a/fs/xattr.c b/fs/xattr.c
index 90dd78f0eb27..fd1335b86e60 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -495,7 +495,7 @@ SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
 SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
 		const void __user *,value, size_t, size, int, flags)
 {
-	struct fd f = fdget(fd);
+	struct fd f = fdget_raw(fd);
 	int error = -EBADF;
 
 	if (!f.file)
@@ -587,7 +587,7 @@ SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
 SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
 		void __user *, value, size_t, size)
 {
-	struct fd f = fdget(fd);
+	struct fd f = fdget_raw(fd);
 	ssize_t error = -EBADF;
 
 	if (!f.file)
@@ -662,7 +662,7 @@ SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
 
 SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
 {
-	struct fd f = fdget(fd);
+	struct fd f = fdget_raw(fd);
 	ssize_t error = -EBADF;
 
 	if (!f.file)
@@ -727,7 +727,7 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
 
 SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
 {
-	struct fd f = fdget(fd);
+	struct fd f = fdget_raw(fd);
 	int error = -EBADF;
 
 	if (!f.file)

RE: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[misono.tomohiro]

> > Even simpler: allow O_PATH descriptors for f*xattr().
> 
> Attached patch.  Will post shortly.
> 
> However, I think it would make sense to fix virtiofsd as well, as this will take time to percolate down, even if Al doesn't find
> anything wrong with it.

Thanks for you comments.

Though I'm still learning virtiofsd code, if nobody will try I'm willing to work on this.

> Doing unshare(CLONE_FS) after thread startup seems safe, though must be careful to change the working directory to the root of
> the mount
> *before* starting any threads.

I think working directry is changed in setup_sandbox() -> setup_mount_namespace() -> setup_pivot_root().
So, can we just add unshare(CLONE_FS) in fv_queue_worker()?

Sorry if I'm totally misunderstood the situation.

Thanks,
Misono

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

On Fri, Oct 18, 2019 at 09:16:36AM +0200, Miklos Szeredi wrote:
> On Thu, Oct 17, 2019 at 6:48 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> 
> > Even simpler: allow O_PATH descriptors for f*xattr().
> 
> Attached patch.  Will post shortly.
> 
> However, I think it would make sense to fix virtiofsd as well, as this
> will take time to percolate down, even if Al doesn't find anything
> wrong with it.
> 
> Doing unshare(CLONE_FS) after thread startup seems safe, though must
> be careful to change the working directory to the root of the mount
> *before* starting any threads.

Thank you for extending O_PATH, that's great!  This will be the cleanest
way to perform xattr operations.

If your patch is accepted I will send a man-pages.git patch to update
the open(2) O_PATH documentation (with a minimum kernel version).

I've added the unshare(CLONE_FS) task to my todo list in case no one
else gets to it first.  I may not have time to work on it before
Novemeber though.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

On Fri, Oct 18, 2019 at 08:51:20AM +0000, misono.tomohiro@fujitsu.com wrote:
> > Doing unshare(CLONE_FS) after thread startup seems safe, though must be careful to change the working directory to the root of
> > the mount
> > *before* starting any threads.
> 
> I think working directry is changed in setup_sandbox() -> setup_mount_namespace() -> setup_pivot_root().
> So, can we just add unshare(CLONE_FS) in fv_queue_worker()?

fv_queue_worker() is the thread pool worker function that is called for
each request.  Calling unshare(CLONE_FS) for each request is not
necessary and will reduce performance.

A thread-local variable can be used to avoid repeated calls to
unshare(CLONE_FS) from the same worker thread:

  static __thread bool clone_fs_called;

  static void fv_queue_worker(gpointer data, gpointer user_data)
  {
      ...
      if (!clone_fs_called) {
          int ret;

	  ret = unshare(CLONE_FS);
	  assert(ret == 0); /* errors not expected according to man page */

	  clone_fs_called = true;
      }

Another issue is the seccomp policy.  Since worker threads are spawned
at runtime it is necessary to add the unshare(2) syscall to the seccomp
whitelist in contrib/virtiofsd/seccomp.c.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[misono.tomohiro]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Monday, October 21, 2019 6:41 PM
> To: Misono, Tomohiro/味曽野 智礼 <misono.tomohiro@fujitsu.com>
> Cc: 'Miklos Szeredi' <mszeredi@redhat.com>; virtio-fs@redhat.com; qemu-devel@nongnu.org
> Subject: Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation
> 
> On Fri, Oct 18, 2019 at 08:51:20AM +0000, misono.tomohiro@fujitsu.com wrote:
> > > Doing unshare(CLONE_FS) after thread startup seems safe, though must
> > > be careful to change the working directory to the root of the mount
> > > *before* starting any threads.
> >
> > I think working directry is changed in setup_sandbox() -> setup_mount_namespace() -> setup_pivot_root().
> > So, can we just add unshare(CLONE_FS) in fv_queue_worker()?
> 
> fv_queue_worker() is the thread pool worker function that is called for each request.  Calling unshare(CLONE_FS) for each request
> is not necessary and will reduce performance.
> 
> A thread-local variable can be used to avoid repeated calls to
> unshare(CLONE_FS) from the same worker thread:
> 
>   static __thread bool clone_fs_called;
> 
>   static void fv_queue_worker(gpointer data, gpointer user_data)
>   {
>       ...
>       if (!clone_fs_called) {
>           int ret;
> 
> 	  ret = unshare(CLONE_FS);
> 	  assert(ret == 0); /* errors not expected according to man page */
> 
> 	  clone_fs_called = true;
>       }
> 
> Another issue is the seccomp policy.  Since worker threads are spawned at runtime it is necessary to add the unshare(2) syscall
> to the seccomp whitelist in contrib/virtiofsd/seccomp.c.
> 

Thanks for suggesting.

I tried above code with fchdir() + ...xattr() + fchdir() in lo_...xattr
and it solves all the problem about xattr I see.

However, it seems the fix causes some performance issue in stress test
as ACL check issues getxattr() and a lot of fchdir() happens. So, I may
try to combine the old method for regular file and this method for special
files.

Thanks,
Misono

Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]

On Wed, Oct 23, 2019 at 11:42:50AM +0000, misono.tomohiro@fujitsu.com wrote:
> > -----Original Message-----
> > From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> > Sent: Monday, October 21, 2019 6:41 PM
> > To: Misono, Tomohiro/味曽野 智礼 <misono.tomohiro@fujitsu.com>
> > Cc: 'Miklos Szeredi' <mszeredi@redhat.com>; virtio-fs@redhat.com; qemu-devel@nongnu.org
> > Subject: Re: [Virtio-fs] [PATCH 0/2] virtiofsd: Two fix for xattr operation
> > 
> > On Fri, Oct 18, 2019 at 08:51:20AM +0000, misono.tomohiro@fujitsu.com wrote:
> > > > Doing unshare(CLONE_FS) after thread startup seems safe, though must
> > > > be careful to change the working directory to the root of the mount
> > > > *before* starting any threads.
> > >
> > > I think working directry is changed in setup_sandbox() -> setup_mount_namespace() -> setup_pivot_root().
> > > So, can we just add unshare(CLONE_FS) in fv_queue_worker()?
> > 
> > fv_queue_worker() is the thread pool worker function that is called for each request.  Calling unshare(CLONE_FS) for each request
> > is not necessary and will reduce performance.
> > 
> > A thread-local variable can be used to avoid repeated calls to
> > unshare(CLONE_FS) from the same worker thread:
> > 
> >   static __thread bool clone_fs_called;
> > 
> >   static void fv_queue_worker(gpointer data, gpointer user_data)
> >   {
> >       ...
> >       if (!clone_fs_called) {
> >           int ret;
> > 
> > 	  ret = unshare(CLONE_FS);
> > 	  assert(ret == 0); /* errors not expected according to man page */
> > 
> > 	  clone_fs_called = true;
> >       }
> > 
> > Another issue is the seccomp policy.  Since worker threads are spawned at runtime it is necessary to add the unshare(2) syscall
> > to the seccomp whitelist in contrib/virtiofsd/seccomp.c.
> > 
> 
> Thanks for suggesting.
> 
> I tried above code with fchdir() + ...xattr() + fchdir() in lo_...xattr
> and it solves all the problem about xattr I see.
> 
> However, it seems the fix causes some performance issue in stress test
> as ACL check issues getxattr() and a lot of fchdir() happens. So, I may
> try to combine the old method for regular file and this method for special
> files.

Sounds good.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]