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

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>

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

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

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

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

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

> > 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

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

> -----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

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

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

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

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