fuse: Don't use request timeouts on virtiofs
Subsystems (4)
PATCH 1/1 fuse: Don't use request timeouts on virtiofs
virtiofs cannot abort a request once it has been added to a virtqueue, because the device owns its buffers until it returns them on the used ring. This is why virtiofs sets no_control and no_force_umount; see commit 15c8e72e88e0 ("fuse: allow skipping control interface and forced unmount"), which says that "virtio-fs does not support aborting requests which are being processed".
The request timeout added by commit 0f6439f61a6e ("fuse: add kernel-enforced timeout option for requests") aborts them anyway. When a request takes longer than the timeout, fuse_check_timeout() calls fuse_chan_abort(), which moves the requests on each virtqueue's fpq->processing list to its end list and ends them, so they are freed. The device can then still write the reply into the freed request and its pages, and virtio_fs_requests_done_work() later gets the freed request back from virtqueue_get_buf().
The timeout is armed when the server sets FUSE_REQUEST_TIMEOUT in its INIT reply, or when the fs.fuse.default_request_timeout or fs.fuse.max_request_timeout sysctl is set, for example for other FUSE mounts on the same system. A slow host is then enough to hit it.
Don't arm the request timeout, and don't offer FUSE_REQUEST_TIMEOUT, on connections that set no_force_umount.
---
I used no_force_umount to tell these connections apart, since virtiofs sets it for the same reason. A separate flag would work as well if you prefer that.
Tested on x86_64 under KVM, on top of v7.4-rc1 (6628e7d4c6b2) with KASAN enabled, using QEMU's vhost-user-fs-pci and the C virtiofsd from QEMU 6.2:
1. In the guest, set fs.fuse.default_request_timeout=15 and mount virtiofs.
2. On the host, stop virtiofsd with SIGSTOP.
3. In the guest, cat a file on the mount.
4. After 50 seconds, resume virtiofsd with SIGCONT.
Without this patch, the cat fails with ECONNABORTED after about 26 seconds. Once virtiofsd resumes, the guest reports:
BUG: KASAN: slab-use-after-free in virtio_fs_requests_done_work+0x7be/0xbc0 Read of size 8 at addr ffff888005176020 by task kworker/1:1/29 Workqueue: events virtio_fs_requests_done_work ... Allocated by task 81: fuse_request_alloc+0x19/0x200 fuse_get_req+0x153/0x2d0 fuse_chan_send+0x4cf/0x840 fuse_do_getattr+0x23a/0x4b0 ... Freed by task 81: kmem_cache_free+0xbc/0x3e0 fuse_chan_send+0x4af/0x840 fuse_do_getattr+0x23a/0x4b0 ... Oops: general protection fault, probably for non-canonical address 0xe012fc11a000000a RIP: 0010:virtio_fs_requests_done_work+0x25a/0xbc0
Setting fs.fuse.max_request_timeout=15 instead gives the same result. With this patch, under either sysctl, the cat waits until virtiofsd resumes and then returns the file contents, and the mount keeps working.
I could not test the FUSE_REQUEST_TIMEOUT part, because this virtiofsd does not know about flags2.
The tests above were run with help from an AI agent.
fs/fuse/inode.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
fs/fuse/inode.c +12 -3
@@ -1451,7 +1451,14 @@ static void process_init_reply(struct fuse_args *args, int error) fc->no_flock =1 ; } -fuse_init_server_timeout (fc->chan, timeout); +/* +* A request timeout aborts the connection, which ends the +* requests that are still in flight. A transport that cannot +* take back requests it has handed to the device, such as +* virtiofs, sets no_force_umount for the same reason. +*/ +if (!fc->no_force_umount) +fuse_init_server_timeout (fc->chan, timeout); fm->sb->s_bdi->ra_pages =min (fm->sb->s_bdi->ra_pages, ra_pages);
@@ -1502,8 +1509,10 @@ static struct fuse_init_args *fuse_new_init(struct fuse_mount *fm)FUSE_HANDLE_KILLPRIV_V2 |FUSE_SETXATTR_EXT |FUSE_INIT_EXT |FUSE_SECURITY_CTX |FUSE_CREATE_SUPP_GROUP |FUSE_HAS_EXPIRE_ONLY |FUSE_DIRECT_IO_ALLOW_MMAP | -FUSE_NO_EXPORT_SUPPORT |FUSE_HAS_RESEND |FUSE_ALLOW_IDMAP | -FUSE_REQUEST_TIMEOUT ; +FUSE_NO_EXPORT_SUPPORT |FUSE_HAS_RESEND |FUSE_ALLOW_IDMAP ; +/* See process_init_reply() */ +if (!fm->fc->no_force_umount) + flags |=FUSE_REQUEST_TIMEOUT ;#ifdef CONFIG_FUSE_VDAX if (fm->fc->vdax) flags |=FUSE_MAP_ALIGNMENT ;
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
Signature · 1 line
2.43.0
[email protected]
AI reviewSashiko has reviewed this patch and found no issues.
Quoted 21 lines
virtiofs cannot abort a request once it has been added to a virtqueue, because the device owns its buffers until it returns them on the used ring. This is why virtiofs sets
no_controlandno_force_umount; see commit15c8e72e88e0("fuse: allow skipping control interface and forced unmount"), which says that "virtio-fs does not support aborting requests which are being processed".The request timeout added by commit
0f6439f61a6e("fuse: add kernel-enforced timeout option for requests") aborts them anyway. When a request takes longer than the timeout,fuse_check_timeout()callsfuse_chan_abort(), which moves the requests on each virtqueue'sfpq->processinglist to its end list and ends them, so they are freed. The device can then still write the reply into the freed request and its pages, andvirtio_fs_requests_done_work()later gets the freed request back fromvirtqueue_get_buf().The timeout is armed when the server sets
FUSE_REQUEST_TIMEOUTin its INIT reply, or when the fs.fuse.default_request_timeout or fs.fuse.max_request_timeout sysctl is set, for example for other FUSE mounts on the same system. A slow host is then enough to hit it.
Quoted 6 lines
Don't arm the request timeout, and don't offer
FUSE_REQUEST_TIMEOUT, on connections that setno_force_umount.Fixes 0f6439f61a6e ("fuse: add kernel-enforced timeout option for requests")Signed-off-by Yuho Choi <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Hi Yuho,
Quoted 51 lines · Yuho Choi
virtiofs cannot abort a request once it has been added to a virtqueue, because the device owns its buffers until it returns them on the used ring. This is why virtiofs sets
no_controlandno_force_umount; see commit15c8e72e88e0("fuse: allow skipping control interface and forced unmount"), which says that "virtio-fs does not support aborting requests which are being processed".The request timeout added by commit
0f6439f61a6e("fuse: add kernel-enforced timeout option for requests") aborts them anyway. When a request takes longer than the timeout,fuse_check_timeout()callsfuse_chan_abort(), which moves the requests on each virtqueue'sfpq->processinglist to its end list and ends them, so they are freed. The device can then still write the reply into the freed request and its pages, andvirtio_fs_requests_done_work()later gets the freed request back fromvirtqueue_get_buf().The timeout is armed when the server sets
FUSE_REQUEST_TIMEOUTin its INIT reply, or when the fs.fuse.default_request_timeout or fs.fuse.max_request_timeout sysctl is set, for example for other FUSE mounts on the same system. A slow host is then enough to hit it.Don't arm the request timeout, and don't offer
FUSE_REQUEST_TIMEOUT, on connections that setno_force_umount.Fixes 0f6439f61a6e ("fuse: add kernel-enforced timeout option for requests")Signed-off-by Yuho Choi <[email protected]>...
fs/fuse/inode.c +8
-1@@ -1451,7 +1451,14 @@ static void process_init_reply(struct fuse_args *args, int error) fc->no_flock =1 ; } -fuse_init_server_timeout (fc->chan, timeout); +/* +* A request timeout aborts the connection, which ends the +* requests that are still in flight. A transport that cannot +* take back requests it has handed to the device, such as +* virtiofs, sets no_force_umount for the same reason. +*/ +if (!fc->no_force_umount) +fuse_init_server_timeout (fc->chan, timeout); fm->sb->s_bdi->ra_pages =min (fm->sb->s_bdi->ra_pages, ra_pages);@@ -1502,8 +1509,10 @@ static struct fuse_init_args *fuse_new_init(struct fuse_mount *fm)FUSE_HANDLE_KILLPRIV_V2 |FUSE_SETXATTR_EXT |FUSE_INIT_EXT | FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
fs/fuse/inode.c static struct fuse_init_args *fuse_new_init(struct fuse_mount *fm) · line 1518
Quoted 10 lines
fs/fuse/inode.c +4
-2FUSE_HAS_EXPIRE_ONLY |FUSE_DIRECT_IO_ALLOW_MMAP | -FUSE_NO_EXPORT_SUPPORT |FUSE_HAS_RESEND |FUSE_ALLOW_IDMAP | -FUSE_REQUEST_TIMEOUT ; +FUSE_NO_EXPORT_SUPPORT |FUSE_HAS_RESEND |FUSE_ALLOW_IDMAP ; +/* See process_init_reply() */ +if (!fm->fc->no_force_umount) + flags |=FUSE_REQUEST_TIMEOUT ;#ifdef CONFIG_FUSE_VDAX if (fm->fc->vdax) flags |=FUSE_MAP_ALIGNMENT ;
Miklos has already posted a patch addressing the same issue [1]. Personally, I feel it would be better to introduce a separate parameter to avoid semantic confusion with whether forced unmount is allowed.
[1] https://lore.kernel.org/fuse-devel/16a0bf8f-8c30-415e-a35e-9551c78b8dcc@
kylinos.cn/T/#m34d62acd7887a86ec32e9091145e4b19fdbb3fc3
Thanks,
Li Wang