[PATCH] fuse: allow skipping abort interface for virtiofs

[PATCH] fuse: allow skipping abort interface for virtiofs

[Xie Yongji]

The commit 15c8e72e88e0 ("fuse: allow skipping control
interface and forced unmount") tries to remove the control
interface for virtio-fs since it does not support aborting
requests which are being processed. But it doesn't work now.

This commit fixes the bug, but only remove the abort interface
instead since other interfaces should be useful.

Fixes: 15c8e72e88e0 ("fuse: allow skipping control interface and forced unmount")
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 fs/fuse/control.c   | 4 ++--
 fs/fuse/fuse_i.h    | 6 +++---
 fs/fuse/inode.c     | 2 +-
 fs/fuse/virtio_fs.c | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index 7cede9a3bc96..d93d8ea3a090 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -272,8 +272,8 @@ int fuse_ctl_add_conn(struct fuse_conn *fc)
 
 	if (!fuse_ctl_add_dentry(parent, fc, "waiting", S_IFREG | 0400, 1,
 				 NULL, &fuse_ctl_waiting_ops) ||
-	    !fuse_ctl_add_dentry(parent, fc, "abort", S_IFREG | 0200, 1,
-				 NULL, &fuse_ctl_abort_ops) ||
+	    (!fc->no_abort_control && !fuse_ctl_add_dentry(parent, fc, "abort",
+			S_IFREG | 0200, 1, NULL, &fuse_ctl_abort_ops)) ||
 	    !fuse_ctl_add_dentry(parent, fc, "max_background", S_IFREG | 0600,
 				 1, NULL, &fuse_conn_max_background_ops) ||
 	    !fuse_ctl_add_dentry(parent, fc, "congestion_threshold",
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 488b460e046f..e29a4e2f2b35 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -507,7 +507,7 @@ struct fuse_fs_context {
 	bool default_permissions:1;
 	bool allow_other:1;
 	bool destroy:1;
-	bool no_control:1;
+	bool no_abort_control:1;
 	bool no_force_umount:1;
 	bool legacy_opts_show:1;
 	enum fuse_dax_mode dax_mode;
@@ -766,8 +766,8 @@ struct fuse_conn {
 	/* Delete dentries that have gone stale */
 	unsigned int delete_stale:1;
 
-	/** Do not create entry in fusectl fs */
-	unsigned int no_control:1;
+	/** Do not create abort entry in fusectl fs */
+	unsigned int no_abort_control:1;
 
 	/** Do not allow MNT_FORCE umount */
 	unsigned int no_force_umount:1;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 8c0665c5dff8..02a16cd35f42 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1564,7 +1564,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
 	fc->legacy_opts_show = ctx->legacy_opts_show;
 	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
 	fc->destroy = ctx->destroy;
-	fc->no_control = ctx->no_control;
+	fc->no_abort_control = ctx->no_abort_control;
 	fc->no_force_umount = ctx->no_force_umount;
 
 	err = -ENOMEM;
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 8db53fa67359..af369bea6dbb 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1287,7 +1287,7 @@ static inline void virtio_fs_ctx_set_defaults(struct fuse_fs_context *ctx)
 	ctx->max_read = UINT_MAX;
 	ctx->blksize = 512;
 	ctx->destroy = true;
-	ctx->no_control = true;
+	ctx->no_abort_control = true;
 	ctx->no_force_umount = true;
 }
 
-- 
2.20.1

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Vivek Goyal]

On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> The commit 15c8e72e88e0 ("fuse: allow skipping control
> interface and forced unmount") tries to remove the control
> interface for virtio-fs since it does not support aborting
> requests which are being processed. But it doesn't work now.

Aha.., so "no_control" basically has no effect? I was looking at
the code and did not find anybody using "no_control" and I was
wondering who is making use of "no_control" variable.

I mounted virtiofs and noticed a directory named "40" showed up
under /sys/fs/fuse/connections/. That must be belonging to
virtiofs instance, I am assuming.

BTW, if there are multiple fuse connections, how will one figure
out which directory belongs to which instance. Because without knowing
that, one will be shooting in dark while trying to read/write any
of the control files.

So I think a separate patch should be sent which just gets rid of
"no_control" saying nobody uses. it.

> 
> This commit fixes the bug, but only remove the abort interface
> instead since other interfaces should be useful.

Hmm.., so writing to "abort" file is bad as it ultimately does.

fc->connected = 0;

So getting rid of this file till we support aborting the pending
requests properly, makes sense.

I think this probably should be a separate patch which explains
why adding "no_abort_control" is a good idea.

Thanks
Vivek

> 
> Fixes: 15c8e72e88e0 ("fuse: allow skipping control interface and forced unmount")
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  fs/fuse/control.c   | 4 ++--
>  fs/fuse/fuse_i.h    | 6 +++---
>  fs/fuse/inode.c     | 2 +-
>  fs/fuse/virtio_fs.c | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fuse/control.c b/fs/fuse/control.c
> index 7cede9a3bc96..d93d8ea3a090 100644
> --- a/fs/fuse/control.c
> +++ b/fs/fuse/control.c
> @@ -272,8 +272,8 @@ int fuse_ctl_add_conn(struct fuse_conn *fc)
>  
>  	if (!fuse_ctl_add_dentry(parent, fc, "waiting", S_IFREG | 0400, 1,
>  				 NULL, &fuse_ctl_waiting_ops) ||
> -	    !fuse_ctl_add_dentry(parent, fc, "abort", S_IFREG | 0200, 1,
> -				 NULL, &fuse_ctl_abort_ops) ||
> +	    (!fc->no_abort_control && !fuse_ctl_add_dentry(parent, fc, "abort",
> +			S_IFREG | 0200, 1, NULL, &fuse_ctl_abort_ops)) ||
>  	    !fuse_ctl_add_dentry(parent, fc, "max_background", S_IFREG | 0600,
>  				 1, NULL, &fuse_conn_max_background_ops) ||
>  	    !fuse_ctl_add_dentry(parent, fc, "congestion_threshold",
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 488b460e046f..e29a4e2f2b35 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -507,7 +507,7 @@ struct fuse_fs_context {
>  	bool default_permissions:1;
>  	bool allow_other:1;
>  	bool destroy:1;
> -	bool no_control:1;
> +	bool no_abort_control:1;
>  	bool no_force_umount:1;
>  	bool legacy_opts_show:1;
>  	enum fuse_dax_mode dax_mode;
> @@ -766,8 +766,8 @@ struct fuse_conn {
>  	/* Delete dentries that have gone stale */
>  	unsigned int delete_stale:1;
>  
> -	/** Do not create entry in fusectl fs */
> -	unsigned int no_control:1;
> +	/** Do not create abort entry in fusectl fs */
> +	unsigned int no_abort_control:1;
>  
>  	/** Do not allow MNT_FORCE umount */
>  	unsigned int no_force_umount:1;
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 8c0665c5dff8..02a16cd35f42 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1564,7 +1564,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	fc->legacy_opts_show = ctx->legacy_opts_show;
>  	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
>  	fc->destroy = ctx->destroy;
> -	fc->no_control = ctx->no_control;
> +	fc->no_abort_control = ctx->no_abort_control;
>  	fc->no_force_umount = ctx->no_force_umount;
>  
>  	err = -ENOMEM;
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 8db53fa67359..af369bea6dbb 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -1287,7 +1287,7 @@ static inline void virtio_fs_ctx_set_defaults(struct fuse_fs_context *ctx)
>  	ctx->max_read = UINT_MAX;
>  	ctx->blksize = 512;
>  	ctx->destroy = true;
> -	ctx->no_control = true;
> +	ctx->no_abort_control = true;
>  	ctx->no_force_umount = true;
>  }
>  
> -- 
> 2.20.1
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Vivek Goyal]

On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> The commit 15c8e72e88e0 ("fuse: allow skipping control
> interface and forced unmount") tries to remove the control
> interface for virtio-fs since it does not support aborting
> requests which are being processed. But it doesn't work now.

Aha.., so "no_control" basically has no effect? I was looking at
the code and did not find anybody using "no_control" and I was
wondering who is making use of "no_control" variable.

I mounted virtiofs and noticed a directory named "40" showed up
under /sys/fs/fuse/connections/. That must be belonging to
virtiofs instance, I am assuming.

BTW, if there are multiple fuse connections, how will one figure
out which directory belongs to which instance. Because without knowing
that, one will be shooting in dark while trying to read/write any
of the control files.

So I think a separate patch should be sent which just gets rid of
"no_control" saying nobody uses. it.

> 
> This commit fixes the bug, but only remove the abort interface
> instead since other interfaces should be useful.

Hmm.., so writing to "abort" file is bad as it ultimately does.

fc->connected = 0;

So getting rid of this file till we support aborting the pending
requests properly, makes sense.

I think this probably should be a separate patch which explains
why adding "no_abort_control" is a good idea.

Thanks
Vivek

> 
> Fixes: 15c8e72e88e0 ("fuse: allow skipping control interface and forced unmount")
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  fs/fuse/control.c   | 4 ++--
>  fs/fuse/fuse_i.h    | 6 +++---
>  fs/fuse/inode.c     | 2 +-
>  fs/fuse/virtio_fs.c | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fuse/control.c b/fs/fuse/control.c
> index 7cede9a3bc96..d93d8ea3a090 100644
> --- a/fs/fuse/control.c
> +++ b/fs/fuse/control.c
> @@ -272,8 +272,8 @@ int fuse_ctl_add_conn(struct fuse_conn *fc)
>  
>  	if (!fuse_ctl_add_dentry(parent, fc, "waiting", S_IFREG | 0400, 1,
>  				 NULL, &fuse_ctl_waiting_ops) ||
> -	    !fuse_ctl_add_dentry(parent, fc, "abort", S_IFREG | 0200, 1,
> -				 NULL, &fuse_ctl_abort_ops) ||
> +	    (!fc->no_abort_control && !fuse_ctl_add_dentry(parent, fc, "abort",
> +			S_IFREG | 0200, 1, NULL, &fuse_ctl_abort_ops)) ||
>  	    !fuse_ctl_add_dentry(parent, fc, "max_background", S_IFREG | 0600,
>  				 1, NULL, &fuse_conn_max_background_ops) ||
>  	    !fuse_ctl_add_dentry(parent, fc, "congestion_threshold",
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 488b460e046f..e29a4e2f2b35 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -507,7 +507,7 @@ struct fuse_fs_context {
>  	bool default_permissions:1;
>  	bool allow_other:1;
>  	bool destroy:1;
> -	bool no_control:1;
> +	bool no_abort_control:1;
>  	bool no_force_umount:1;
>  	bool legacy_opts_show:1;
>  	enum fuse_dax_mode dax_mode;
> @@ -766,8 +766,8 @@ struct fuse_conn {
>  	/* Delete dentries that have gone stale */
>  	unsigned int delete_stale:1;
>  
> -	/** Do not create entry in fusectl fs */
> -	unsigned int no_control:1;
> +	/** Do not create abort entry in fusectl fs */
> +	unsigned int no_abort_control:1;
>  
>  	/** Do not allow MNT_FORCE umount */
>  	unsigned int no_force_umount:1;
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 8c0665c5dff8..02a16cd35f42 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1564,7 +1564,7 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	fc->legacy_opts_show = ctx->legacy_opts_show;
>  	fc->max_read = max_t(unsigned int, 4096, ctx->max_read);
>  	fc->destroy = ctx->destroy;
> -	fc->no_control = ctx->no_control;
> +	fc->no_abort_control = ctx->no_abort_control;
>  	fc->no_force_umount = ctx->no_force_umount;
>  
>  	err = -ENOMEM;
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 8db53fa67359..af369bea6dbb 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -1287,7 +1287,7 @@ static inline void virtio_fs_ctx_set_defaults(struct fuse_fs_context *ctx)
>  	ctx->max_read = UINT_MAX;
>  	ctx->blksize = 512;
>  	ctx->destroy = true;
> -	ctx->no_control = true;
> +	ctx->no_abort_control = true;
>  	ctx->no_force_umount = true;
>  }
>  
> -- 
> 2.20.1
> 

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Yongji Xie]

On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> > The commit 15c8e72e88e0 ("fuse: allow skipping control
> > interface and forced unmount") tries to remove the control
> > interface for virtio-fs since it does not support aborting
> > requests which are being processed. But it doesn't work now.
>
> Aha.., so "no_control" basically has no effect? I was looking at
> the code and did not find anybody using "no_control" and I was
> wondering who is making use of "no_control" variable.
>
> I mounted virtiofs and noticed a directory named "40" showed up
> under /sys/fs/fuse/connections/. That must be belonging to
> virtiofs instance, I am assuming.
>

I think so.

> BTW, if there are multiple fuse connections, how will one figure
> out which directory belongs to which instance. Because without knowing
> that, one will be shooting in dark while trying to read/write any
> of the control files.
>

We can use "stat $mountpoint" to get the device minor ID which is the
name of the corresponding control directory.

> So I think a separate patch should be sent which just gets rid of
> "no_control" saying nobody uses. it.
>

OK.

> >
> > This commit fixes the bug, but only remove the abort interface
> > instead since other interfaces should be useful.
>
> Hmm.., so writing to "abort" file is bad as it ultimately does.
>
> fc->connected = 0;
>

Another problem is that it might trigger UAF since
virtio_fs_request_complete() doesn't know the requests are aborted.

> So getting rid of this file till we support aborting the pending
> requests properly, makes sense.
>
> I think this probably should be a separate patch which explains
> why adding "no_abort_control" is a good idea.
>

OK.

Thanks,
Yongji

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Vivek Goyal]

On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:
> On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> > > The commit 15c8e72e88e0 ("fuse: allow skipping control
> > > interface and forced unmount") tries to remove the control
> > > interface for virtio-fs since it does not support aborting
> > > requests which are being processed. But it doesn't work now.
> >
> > Aha.., so "no_control" basically has no effect? I was looking at
> > the code and did not find anybody using "no_control" and I was
> > wondering who is making use of "no_control" variable.
> >
> > I mounted virtiofs and noticed a directory named "40" showed up
> > under /sys/fs/fuse/connections/. That must be belonging to
> > virtiofs instance, I am assuming.
> >
> 
> I think so.
> 
> > BTW, if there are multiple fuse connections, how will one figure
> > out which directory belongs to which instance. Because without knowing
> > that, one will be shooting in dark while trying to read/write any
> > of the control files.
> >
> 
> We can use "stat $mountpoint" to get the device minor ID which is the
> name of the corresponding control directory.
> 
> > So I think a separate patch should be sent which just gets rid of
> > "no_control" saying nobody uses. it.
> >
> 
> OK.
> 
> > >
> > > This commit fixes the bug, but only remove the abort interface
> > > instead since other interfaces should be useful.
> >
> > Hmm.., so writing to "abort" file is bad as it ultimately does.
> >
> > fc->connected = 0;
> >
> 
> Another problem is that it might trigger UAF since
> virtio_fs_request_complete() doesn't know the requests are aborted.
> 
> > So getting rid of this file till we support aborting the pending
> > requests properly, makes sense.
> >
> > I think this probably should be a separate patch which explains
> > why adding "no_abort_control" is a good idea.
> >
> 
> OK.

BTW, which particular knob you are finding useful in control filesystem
for virtiofs. As you mentioned "abort" we will not implement. "waiting"
might not have much significance as well because requests are handed
over to virtiofs immidiately and if they can be sent to server (because
virtqueue is full) these are queued internally and fuse layer will not
have an idea.

That leaves us with "congestion_threshold" and "max_background".
max_background seems to control how many background requests can be
submitted at a time. That probably can be useful if server is overwhelemed
and we want to slow down the client a bit.

Not sure about congestion threshold.

So have you found some knob useful for your use case?

Thanks
Vivek

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Vivek Goyal]

On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:
> On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> > > The commit 15c8e72e88e0 ("fuse: allow skipping control
> > > interface and forced unmount") tries to remove the control
> > > interface for virtio-fs since it does not support aborting
> > > requests which are being processed. But it doesn't work now.
> >
> > Aha.., so "no_control" basically has no effect? I was looking at
> > the code and did not find anybody using "no_control" and I was
> > wondering who is making use of "no_control" variable.
> >
> > I mounted virtiofs and noticed a directory named "40" showed up
> > under /sys/fs/fuse/connections/. That must be belonging to
> > virtiofs instance, I am assuming.
> >
> 
> I think so.
> 
> > BTW, if there are multiple fuse connections, how will one figure
> > out which directory belongs to which instance. Because without knowing
> > that, one will be shooting in dark while trying to read/write any
> > of the control files.
> >
> 
> We can use "stat $mountpoint" to get the device minor ID which is the
> name of the corresponding control directory.
> 
> > So I think a separate patch should be sent which just gets rid of
> > "no_control" saying nobody uses. it.
> >
> 
> OK.
> 
> > >
> > > This commit fixes the bug, but only remove the abort interface
> > > instead since other interfaces should be useful.
> >
> > Hmm.., so writing to "abort" file is bad as it ultimately does.
> >
> > fc->connected = 0;
> >
> 
> Another problem is that it might trigger UAF since
> virtio_fs_request_complete() doesn't know the requests are aborted.
> 
> > So getting rid of this file till we support aborting the pending
> > requests properly, makes sense.
> >
> > I think this probably should be a separate patch which explains
> > why adding "no_abort_control" is a good idea.
> >
> 
> OK.

BTW, which particular knob you are finding useful in control filesystem
for virtiofs. As you mentioned "abort" we will not implement. "waiting"
might not have much significance as well because requests are handed
over to virtiofs immidiately and if they can be sent to server (because
virtqueue is full) these are queued internally and fuse layer will not
have an idea.

That leaves us with "congestion_threshold" and "max_background".
max_background seems to control how many background requests can be
submitted at a time. That probably can be useful if server is overwhelemed
and we want to slow down the client a bit.

Not sure about congestion threshold.

So have you found some knob useful for your use case?

Thanks
Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Yongji Xie]

On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:
> > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> > > > The commit 15c8e72e88e0 ("fuse: allow skipping control
> > > > interface and forced unmount") tries to remove the control
> > > > interface for virtio-fs since it does not support aborting
> > > > requests which are being processed. But it doesn't work now.
> > >
> > > Aha.., so "no_control" basically has no effect? I was looking at
> > > the code and did not find anybody using "no_control" and I was
> > > wondering who is making use of "no_control" variable.
> > >
> > > I mounted virtiofs and noticed a directory named "40" showed up
> > > under /sys/fs/fuse/connections/. That must be belonging to
> > > virtiofs instance, I am assuming.
> > >
> >
> > I think so.
> >
> > > BTW, if there are multiple fuse connections, how will one figure
> > > out which directory belongs to which instance. Because without knowing
> > > that, one will be shooting in dark while trying to read/write any
> > > of the control files.
> > >
> >
> > We can use "stat $mountpoint" to get the device minor ID which is the
> > name of the corresponding control directory.
> >
> > > So I think a separate patch should be sent which just gets rid of
> > > "no_control" saying nobody uses. it.
> > >
> >
> > OK.
> >
> > > >
> > > > This commit fixes the bug, but only remove the abort interface
> > > > instead since other interfaces should be useful.
> > >
> > > Hmm.., so writing to "abort" file is bad as it ultimately does.
> > >
> > > fc->connected = 0;
> > >
> >
> > Another problem is that it might trigger UAF since
> > virtio_fs_request_complete() doesn't know the requests are aborted.
> >
> > > So getting rid of this file till we support aborting the pending
> > > requests properly, makes sense.
> > >
> > > I think this probably should be a separate patch which explains
> > > why adding "no_abort_control" is a good idea.
> > >
> >
> > OK.
>
> BTW, which particular knob you are finding useful in control filesystem
> for virtiofs. As you mentioned "abort" we will not implement. "waiting"
> might not have much significance as well because requests are handed
> over to virtiofs immidiately and if they can be sent to server (because
> virtqueue is full) these are queued internally and fuse layer will not
> have an idea.
>

Couldn't it be used to check the inflight I/O for virtiofs?

> That leaves us with "congestion_threshold" and "max_background".
> max_background seems to control how many background requests can be
> submitted at a time. That probably can be useful if server is overwhelemed
> and we want to slow down the client a bit.
>
> Not sure about congestion threshold.
>
> So have you found some knob useful for your use case?
>

Since it doesn't do harm to the system, I think it would be better to
just keep it as it is. Maybe some fuse users can make use of it.

Thanks,
Yongji

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Vivek Goyal]

On Wed, Jun 08, 2022 at 09:57:51PM +0800, Yongji Xie wrote:
> On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:
> > > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> > > > > The commit 15c8e72e88e0 ("fuse: allow skipping control
> > > > > interface and forced unmount") tries to remove the control
> > > > > interface for virtio-fs since it does not support aborting
> > > > > requests which are being processed. But it doesn't work now.
> > > >
> > > > Aha.., so "no_control" basically has no effect? I was looking at
> > > > the code and did not find anybody using "no_control" and I was
> > > > wondering who is making use of "no_control" variable.
> > > >
> > > > I mounted virtiofs and noticed a directory named "40" showed up
> > > > under /sys/fs/fuse/connections/. That must be belonging to
> > > > virtiofs instance, I am assuming.
> > > >
> > >
> > > I think so.
> > >
> > > > BTW, if there are multiple fuse connections, how will one figure
> > > > out which directory belongs to which instance. Because without knowing
> > > > that, one will be shooting in dark while trying to read/write any
> > > > of the control files.
> > > >
> > >
> > > We can use "stat $mountpoint" to get the device minor ID which is the
> > > name of the corresponding control directory.
> > >
> > > > So I think a separate patch should be sent which just gets rid of
> > > > "no_control" saying nobody uses. it.
> > > >
> > >
> > > OK.
> > >
> > > > >
> > > > > This commit fixes the bug, but only remove the abort interface
> > > > > instead since other interfaces should be useful.
> > > >
> > > > Hmm.., so writing to "abort" file is bad as it ultimately does.
> > > >
> > > > fc->connected = 0;
> > > >
> > >
> > > Another problem is that it might trigger UAF since
> > > virtio_fs_request_complete() doesn't know the requests are aborted.
> > >
> > > > So getting rid of this file till we support aborting the pending
> > > > requests properly, makes sense.
> > > >
> > > > I think this probably should be a separate patch which explains
> > > > why adding "no_abort_control" is a good idea.
> > > >
> > >
> > > OK.
> >
> > BTW, which particular knob you are finding useful in control filesystem
> > for virtiofs. As you mentioned "abort" we will not implement. "waiting"
> > might not have much significance as well because requests are handed
> > over to virtiofs immidiately and if they can be sent to server (because
> > virtqueue is full) these are queued internally and fuse layer will not
> > have an idea.
> >
> 
> Couldn't it be used to check the inflight I/O for virtiofs?

Actually I might be wrong. It probably should work. Looking at
implementation.

fuse_conn_waiting_read() looks at fc->num_waiting to figure out
how many requests are in flight.

And either fuse_get_req()/fuse_simple_request() will bump up the
fc->num_request count and fuse_put_request() will drop that count
once request completes. And this seems to be independent of
virtiofs.

So looks like it should work even with virtiofs. Please give it a try.

> 
> > That leaves us with "congestion_threshold" and "max_background".
> > max_background seems to control how many background requests can be
> > submitted at a time. That probably can be useful if server is overwhelemed
> > and we want to slow down the client a bit.
> >
> > Not sure about congestion threshold.
> >
> > So have you found some knob useful for your use case?
> >
> 
> Since it doesn't do harm to the system, I think it would be better to
> just keep it as it is. Maybe some fuse users can make use of it.

I guess fair enough. I don't mind creating "control" file system for
virtiofs. Either we don't create "abort" file or may be somehow
writing to file returns error. I guess both the solutions should
work. 

Thanks
Vivek

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Vivek Goyal]

On Wed, Jun 08, 2022 at 09:57:51PM +0800, Yongji Xie wrote:
> On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:
> > > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> > > > > The commit 15c8e72e88e0 ("fuse: allow skipping control
> > > > > interface and forced unmount") tries to remove the control
> > > > > interface for virtio-fs since it does not support aborting
> > > > > requests which are being processed. But it doesn't work now.
> > > >
> > > > Aha.., so "no_control" basically has no effect? I was looking at
> > > > the code and did not find anybody using "no_control" and I was
> > > > wondering who is making use of "no_control" variable.
> > > >
> > > > I mounted virtiofs and noticed a directory named "40" showed up
> > > > under /sys/fs/fuse/connections/. That must be belonging to
> > > > virtiofs instance, I am assuming.
> > > >
> > >
> > > I think so.
> > >
> > > > BTW, if there are multiple fuse connections, how will one figure
> > > > out which directory belongs to which instance. Because without knowing
> > > > that, one will be shooting in dark while trying to read/write any
> > > > of the control files.
> > > >
> > >
> > > We can use "stat $mountpoint" to get the device minor ID which is the
> > > name of the corresponding control directory.
> > >
> > > > So I think a separate patch should be sent which just gets rid of
> > > > "no_control" saying nobody uses. it.
> > > >
> > >
> > > OK.
> > >
> > > > >
> > > > > This commit fixes the bug, but only remove the abort interface
> > > > > instead since other interfaces should be useful.
> > > >
> > > > Hmm.., so writing to "abort" file is bad as it ultimately does.
> > > >
> > > > fc->connected = 0;
> > > >
> > >
> > > Another problem is that it might trigger UAF since
> > > virtio_fs_request_complete() doesn't know the requests are aborted.
> > >
> > > > So getting rid of this file till we support aborting the pending
> > > > requests properly, makes sense.
> > > >
> > > > I think this probably should be a separate patch which explains
> > > > why adding "no_abort_control" is a good idea.
> > > >
> > >
> > > OK.
> >
> > BTW, which particular knob you are finding useful in control filesystem
> > for virtiofs. As you mentioned "abort" we will not implement. "waiting"
> > might not have much significance as well because requests are handed
> > over to virtiofs immidiately and if they can be sent to server (because
> > virtqueue is full) these are queued internally and fuse layer will not
> > have an idea.
> >
> 
> Couldn't it be used to check the inflight I/O for virtiofs?

Actually I might be wrong. It probably should work. Looking at
implementation.

fuse_conn_waiting_read() looks at fc->num_waiting to figure out
how many requests are in flight.

And either fuse_get_req()/fuse_simple_request() will bump up the
fc->num_request count and fuse_put_request() will drop that count
once request completes. And this seems to be independent of
virtiofs.

So looks like it should work even with virtiofs. Please give it a try.

> 
> > That leaves us with "congestion_threshold" and "max_background".
> > max_background seems to control how many background requests can be
> > submitted at a time. That probably can be useful if server is overwhelemed
> > and we want to slow down the client a bit.
> >
> > Not sure about congestion threshold.
> >
> > So have you found some knob useful for your use case?
> >
> 
> Since it doesn't do harm to the system, I think it would be better to
> just keep it as it is. Maybe some fuse users can make use of it.

I guess fair enough. I don't mind creating "control" file system for
virtiofs. Either we don't create "abort" file or may be somehow
writing to file returns error. I guess both the solutions should
work. 

Thanks
Vivek

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] fuse: allow skipping abort interface for virtiofs

[Yongji Xie]

On Thu, Jun 9, 2022 at 9:31 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Jun 08, 2022 at 09:57:51PM +0800, Yongji Xie wrote:
> > On Wed, Jun 8, 2022 at 8:44 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:
> > > > On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > >
> > > > > On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:
> > > > > > The commit 15c8e72e88e0 ("fuse: allow skipping control
> > > > > > interface and forced unmount") tries to remove the control
> > > > > > interface for virtio-fs since it does not support aborting
> > > > > > requests which are being processed. But it doesn't work now.
> > > > >
> > > > > Aha.., so "no_control" basically has no effect? I was looking at
> > > > > the code and did not find anybody using "no_control" and I was
> > > > > wondering who is making use of "no_control" variable.
> > > > >
> > > > > I mounted virtiofs and noticed a directory named "40" showed up
> > > > > under /sys/fs/fuse/connections/. That must be belonging to
> > > > > virtiofs instance, I am assuming.
> > > > >
> > > >
> > > > I think so.
> > > >
> > > > > BTW, if there are multiple fuse connections, how will one figure
> > > > > out which directory belongs to which instance. Because without knowing
> > > > > that, one will be shooting in dark while trying to read/write any
> > > > > of the control files.
> > > > >
> > > >
> > > > We can use "stat $mountpoint" to get the device minor ID which is the
> > > > name of the corresponding control directory.
> > > >
> > > > > So I think a separate patch should be sent which just gets rid of
> > > > > "no_control" saying nobody uses. it.
> > > > >
> > > >
> > > > OK.
> > > >
> > > > > >
> > > > > > This commit fixes the bug, but only remove the abort interface
> > > > > > instead since other interfaces should be useful.
> > > > >
> > > > > Hmm.., so writing to "abort" file is bad as it ultimately does.
> > > > >
> > > > > fc->connected = 0;
> > > > >
> > > >
> > > > Another problem is that it might trigger UAF since
> > > > virtio_fs_request_complete() doesn't know the requests are aborted.
> > > >
> > > > > So getting rid of this file till we support aborting the pending
> > > > > requests properly, makes sense.
> > > > >
> > > > > I think this probably should be a separate patch which explains
> > > > > why adding "no_abort_control" is a good idea.
> > > > >
> > > >
> > > > OK.
> > >
> > > BTW, which particular knob you are finding useful in control filesystem
> > > for virtiofs. As you mentioned "abort" we will not implement. "waiting"
> > > might not have much significance as well because requests are handed
> > > over to virtiofs immidiately and if they can be sent to server (because
> > > virtqueue is full) these are queued internally and fuse layer will not
> > > have an idea.
> > >
> >
> > Couldn't it be used to check the inflight I/O for virtiofs?
>
> Actually I might be wrong. It probably should work. Looking at
> implementation.
>
> fuse_conn_waiting_read() looks at fc->num_waiting to figure out
> how many requests are in flight.
>
> And either fuse_get_req()/fuse_simple_request() will bump up the
> fc->num_request count and fuse_put_request() will drop that count
> once request completes. And this seems to be independent of
> virtiofs.
>
> So looks like it should work even with virtiofs. Please give it a try.
>

OK.

> >
> > > That leaves us with "congestion_threshold" and "max_background".
> > > max_background seems to control how many background requests can be
> > > submitted at a time. That probably can be useful if server is overwhelemed
> > > and we want to slow down the client a bit.
> > >
> > > Not sure about congestion threshold.
> > >
> > > So have you found some knob useful for your use case?
> > >
> >
> > Since it doesn't do harm to the system, I think it would be better to
> > just keep it as it is. Maybe some fuse users can make use of it.
>
> I guess fair enough. I don't mind creating "control" file system for
> virtiofs. Either we don't create "abort" file or may be somehow
> writing to file returns error. I guess both the solutions should
> work.
>

I think so.

Thanks,
Yongji