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

On Wed, Jun 08, 2022 at 04:42:46PM +0800, Yongji Xie wrote:

On Wed, Jun 8, 2022 at 3:34 AM Vivek Goyal [off-list ref] wrote:

quoted

On Tue, Jun 07, 2022 at 07:05:04PM +0800, Xie Yongji wrote:

quoted

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.

quoted

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.

quoted

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

OK.

quoted

quoted

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.

quoted

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