* Re: [PATCH] fuse: virtio-fs: don't process completions of aborted requests
2026-10-20 13:53 [PATCH] fuse: virtio-fs: don't process completions of aborted requests Qihang
@ 2026-10-20 14:09 ` sashiko-bot
2026-11-08 14:08 ` Qihang
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-10-20 14:09 UTC (permalink / raw)
To: Qihang; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] fuse: virtio-fs: DMA use-after-free on aborted requests in fuse_chan_abort()
- [High] fuse: virtio-fs: fsvq->aborted never cleared in virtio_fs_start_all_queues()
- [High] fuse: virtio-fs: queued_reqs not aborted or drained in virtio_fs_abort()
- [Medium] fuse: virtio-fs: req->argbuf leak on aborted requests in done_work
--
commit 0a788f32822f61f6e1ab378f337037e0c6397f67
Author: Qihang <q.h.hack.winter@gmail.com>
fuse: virtio-fs: don't process completions of aborted requests
This commit adds an abort callback to struct fuse_iqueue_ops so virtio-fs can
mark its virtqueues as aborted when fuse_chan_abort() runs. In
virtio_fs_requests_done_work(), completions on an aborted virtqueue are
skipped after decrementing in_flight to avoid dereferencing requests already
freed by fuse_chan_abort().
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index ba8d79c4bcac6..ae4a14aaaba98 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -55,6 +55,7 @@ struct virtio_fs_vq {
> struct work_struct dispatch_work;
> struct fuse_dev *fud;
> bool connected;
> + bool aborted;
> long in_flight;
> struct completion in_flight_zero; /* No inflight requests */
> struct kobject *kobj;
> @@ -871,6 +872,15 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
> virtqueue_disable_cb(vq);
>
> while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> + if (fsvq->aborted) {
> + /*
> + * fuse_chan_abort() ended every request this
> + * queue still owns; the token may already be
> + * freed, do not touch it.
> + */
> + dec_in_flight_req(fsvq);
> + continue;
> + }
[Severity: Critical]
Can this still result in a DMA use-after-free on struct fuse_req and
ap->folios before virtqueue_get_buf() even returns?
In virtio_fs_enqueue_req(), the scatter-gather list passed to
virtqueue_add_sgs() maps &req->in.h, req->argbuf, and input ap->folios as
device-readable buffers, and maps &req->out.h (embedded inside struct
fuse_req), req->argbuf + argbuf_used, and output ap->folios as
device-writable buffers:
fs/fuse/virtio_fs.c:virtio_fs_enqueue_req() {
...
/* Request elements */
sg_init_one(&sg[out_sgs++], &req->in.h, sizeof(req->in.h));
out_sgs += sg_init_fuse_args(&sg[out_sgs], req,
(struct fuse_arg *)args->in_args,
args->in_numargs, args->in_pages,
req->argbuf, &argbuf_used);
/* Reply elements */
if (test_bit(FR_ISREPLY, &req->flags)) {
sg_init_one(&sg[out_sgs + in_sgs++],
&req->out.h, sizeof(req->out.h));
in_sgs += sg_init_fuse_args(&sg[out_sgs + in_sgs], req,
args->out_args, args->out_numargs,
args->out_pages,
req->argbuf + argbuf_used, NULL);
}
...
}
When fuse_chan_abort() runs while a request is in flight in the virtqueue,
virtio_fs_abort() sets fsvq->aborted = true without resetting the virtio
device, and fuse_chan_abort() immediately splices fpq->processing onto
to_end and calls fuse_dev_end_requests() -> fuse_request_end(). That invokes
req->args->end() (unlocking and releasing ap->folios) and frees struct
fuse_req back to fuse_req_cachep via fuse_request_free() while the
descriptor chain remains active in the virtqueue.
When the host later completes the stalled descriptor, won't the device (and
any DMA unmapping in virtqueue_get_buf()) write into &req->out.h inside the
already-freed struct fuse_req and into the released ap->folios before
reaching the fsvq->aborted check here?
[Severity: Medium]
Does skipping virtio_fs_request_complete() here also leak req->argbuf for
every aborted in-flight request?
When virtio_fs_enqueue_req() prepares a request, copy_args_to_argbuf()
allocates req->argbuf:
fs/fuse/virtio_fs.c:copy_args_to_argbuf() {
...
req->argbuf = kmalloc(len, gfp);
if (!req->argbuf)
return -ENOMEM;
...
}
Normally req->argbuf is freed in copy_args_from_argbuf() called from
virtio_fs_request_complete(). When fuse_chan_abort() ends requests on
fpq->processing via fuse_dev_end_requests() -> fuse_request_end() ->
fuse_put_request() -> fuse_request_free(), struct fuse_req is freed to
fuse_req_cachep without freeing req->argbuf:
fs/fuse/dev.c:fuse_request_free() {
WARN_ON(!list_empty(&req->intr_entry));
kmem_cache_free(fuse_req_cachep, req);
}
Since virtio_fs_requests_done_work() skips virtio_fs_request_complete() when
fsvq->aborted is true (and req is already freed at that point), where is
req->argbuf freed for aborted requests?
> if (!virtio_fs_verify_response(req, len)) {
> req->out.h.error = -EIO;
> req->out.h.len = sizeof(struct fuse_out_header);
> @@ -1570,10 +1580,26 @@ static void virtio_fs_send_req(struct fuse_iqueue *fiq, struct fuse_req *req)
> }
> }
>
> +static void virtio_fs_abort(struct fuse_iqueue *fiq)
> +{
> + struct virtio_fs *fs = fiq->priv;
> + int i;
> +
> + for (i = 0; i < fs->nvqs; i++) {
> + struct virtio_fs_vq *fsvq = &fs->vqs[i];
> +
> + spin_lock(&fsvq->lock);
> + fsvq->connected = false;
> + fsvq->aborted = true;
> + spin_unlock(&fsvq->lock);
> + }
> +}
[Severity: High]
Will setting fsvq->aborted = true here without clearing it when queues are
restarted break subsequent mounts of the same virtio-fs filesystem?
A struct virtio_fs instance and its fs->vqs[] array persist across mounts
for the lifetime of the virtio_device. On a normal unmount,
virtio_kill_sb() -> virtio_fs_conn_destroy() -> fuse_conn_destroy() calls
fuse_chan_abort(fc->chan, false):
fs/fuse/inode.c:fuse_conn_destroy() {
...
fuse_chan_abort(fc->chan, false);
fuse_chan_wait_aborted(fc->chan);
...
}
Because fch->connected is still set during a normal unmount,
fuse_chan_abort() invokes fiq->ops->abort(fiq) -> virtio_fs_abort(fiq),
which sets fsvq->aborted = true on every queue in fs->vqs[].
When the same virtio-fs tag is mounted again, virtio_fs_get_tree() ->
virtio_fs_fill_super() calls virtio_fs_start_all_queues(fs), which sets
fsvq->connected = true but never clears fsvq->aborted:
fs/fuse/virtio_fs.c:virtio_fs_start_all_queues() {
struct virtio_fs_vq *fsvq;
int i;
for (i = 0; i < fs->nvqs; i++) {
fsvq = &fs->vqs[i];
spin_lock(&fsvq->lock);
fsvq->connected = true;
spin_unlock(&fsvq->lock);
}
}
On the new mount, won't virtio_fs_requests_done_work() see fsvq->aborted ==
true and drop all completions (starting with FUSE_INIT from
fuse_send_init()), leaking req->argbuf and hanging the mount?
[Severity: High]
What happens to requests on fsvq->queued_reqs when virtio_fs_abort() runs?
When a virtqueue is full, virtio_fs_enqueue_req() drops fsvq->lock and
returns -ENOSPC without adding req to fpq->processing. Both
virtio_fs_send_req() and virtio_fs_request_dispatch_work() then re-acquire
fsvq->lock and place req onto fsvq->queued_reqs without checking
fsvq->connected:
fs/fuse/virtio_fs.c:virtio_fs_send_req() {
...
fsvq = &fs->vqs[queue_id];
ret = virtio_fs_enqueue_req(fsvq, req, false, GFP_ATOMIC);
if (ret < 0) {
if (ret == -ENOSPC) {
/*
* Virtqueue full. Retry submission from worker
* context as we might be holding fc->chan->bg_lock.
*/
spin_lock(&fsvq->lock);
list_add_tail(&req->list, &fsvq->queued_reqs);
inc_in_flight_req(fsvq);
spin_unlock(&fsvq->lock);
return;
}
...
}
Because FR_PENDING was already cleared in virtio_fs_send_req() and req is
not on fpq->processing, fuse_chan_abort() will not find these requests. In
addition, a racing virtio_fs_send_req() that saw fsvq->connected == true
inside virtio_fs_enqueue_req() and got -ENOSPC can acquire fsvq->lock after
virtio_fs_abort() has finished and still add req to fsvq->queued_reqs.
Since virtio_fs_abort() does not drain fsvq->queued_reqs or schedule
fsvq->dispatch_work (which is otherwise only scheduled from
virtio_fs_requests_done_work() when completions arrive), won't requests left
on fsvq->queued_reqs when the host stalls never be ended, hanging tasks in
request_wait_answer(), fuse_chan_wait_aborted(), and
virtio_fs_drain_queue()?
[ ... ]
விரும்பியபடி, இيمெயில் பதிலை மேலே உருவாக்கியுள்ளேன்.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261020135350.7751-1-q.h.hack.winter@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] fuse: virtio-fs: don't process completions of aborted requests
2026-10-20 13:53 [PATCH] fuse: virtio-fs: don't process completions of aborted requests Qihang
2026-10-20 14:09 ` sashiko-bot
@ 2026-11-08 14:08 ` Qihang
1 sibling, 0 replies; 3+ messages in thread
From: Qihang @ 2026-11-08 14:08 UTC (permalink / raw)
To: sashiko-reviews; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
Hi Sashiko,
Thanks for the review. I did not extend that patch: tagging
completions was the wrong layer, and it introduced the sticky-state
problem you found in #2. I replaced it with a v2 patch, which
prevents the request from becoming device-visible in the first place:
[PATCH v2] fuse: virtio-fs: stop new requests before draining queues
One new per-vq flag, fsvq->aborting, is set by
virtio_fs_stop_new_requests() before the first drain in
virtio_fs_conn_destroy(). It is set per queue, not with a global
barrier, and that is enough: each queue is drained after its own flag
is set, and for a request that reaches the device, admission,
virtqueue_add_sgs() and the in_flight increment happen under the same
fsvq->lock. During teardown a request therefore has only two
possible states:
1. it observes aborting under fsvq->lock and is rejected with
-ENOTCONN before virtqueue_add_sgs(); or
2. it passes admission, is added to the virtqueue and is covered by
in_flight - either incremented under the same lock on first
submission, or already counted when it was queued - so the
drain waits for it.
There is no state observable by the drain in which the request is
device-visible but not accounted for by in_flight.
FUSE_DESTROY is the one request intentionally submitted after the
first drain: it is the teardown request itself, issued with
args.force, and fuse_simple_request() waits for its completion before
fuse_chan_abort() is called, so its lifetime cannot overlap the
abort.
#1 (post-drain request -> DMA UAF; Sashiko's "in-flight at abort")
I agree the normal entry points cannot reach fuse_chan_abort() with an
in-flight request: virtio-fs has no userspace /dev/fuse fd, disables
fusectl (no_control), and sets no_force_umount. The only path is
fuse_conn_destroy(), preceded by a drain.
That drain only covers requests counted when it starts. A request
submitted after it returns is added to the virtqueue and counted, and
is then freed by fuse_chan_abort() while the device still owns the
descriptors:
CPU0 (umount) CPU1
drain returns (in_flight == 0)
enqueue_req()
virtqueue_add_sgs()
inc_in_flight_req() -> 1
fuse_conn_destroy()
fuse_send_destroy()
fuse_chan_abort()
ends and frees the request
device still DMA-writes into the
freed request and argbuf
This is the reachable variant of #1, and what the patch closes.
Unpatched, I could land the post-drain submission in the virtqueue
and get a KASAN slab-use-after-free in
virtio_fs_requests_done_work(), with umount hanging afterwards;
patched, the same submission returns -ENOTCONN and umount completes.
(Full dmesg is in the reproducer.)
#2 (aborted never cleared, second mount's FUSE_INIT dropped)
The v2 patch has no aborted/completion-suppression state. aborting is
reinitialized by virtio_fs_start_all_queues() on every mount, so it
cannot leak into a subsequent mount. I verified mount/read/umount
across repeated remounts; FUSE_INIT completed normally.
#3 (queued_reqs not handled by abort/drain)
Requests on the dispatch queue are already accounted for by
in_flight, so the drain covers them even though they have not reached
the virtqueue. If the dispatch worker retries after teardown starts,
aborting makes virtio_fs_enqueue_req() return -ENOTCONN before
virtqueue_add_sgs(); the retry passes in_flight=true, so it is not
counted twice, and flush_work(dispatch_work) then waits for the
worker to finish.
Measured with a forced -ENOSPC so the request really takes the queued
path:
in_flight seen=1 after 2 ms
VF_INV: q=requests.0 in_flight=0 queued=0 connected=1 aborting=1
injected STATFS returned -107
VF_INV: q=requests.0 in_flight=0 queued=0 connected=0 aborting=1
#4 (argbuf leak on aborted requests)
There is no skipped completion, so there is no new argbuf owner.
Rejected requests free argbuf in the existing error path of
virtio_fs_enqueue_req(), the same way the !connected path does, and
the stock abort path is untouched.
I also checked the two teardown interactions. remove() only narrows
admission: connected=false is set under fsvq->lock before the
virtqueues are torn down, and enqueue_req() checks connected before
dereferencing fsvq->vq, so it cannot race into a cleaned-up
virtqueue. For a new mount, sget_fc() keeps finding the extant
superblock until SB_DEAD is set, and SB_DEAD is set after kill_sb()
returns; conn_destroy() runs inside kill_sb(), so start_all_queues()
cannot clear aborting during teardown. Both were confirmed by
running umount against unbind and against a concurrent mount.
Tested with KASAN, prove-locking, DEBUG_LIST, DEBUG_SPINLOCK and
DEBUG_MUTEXES: 23 mount/umount cycles with read/write load, the A/B
above, and the queued/remove/mount window tests; no WARN or KASAN
reports. The tests used one normal request queue plus the
high-priority queue (nvqs=2); the multi-queue loop is the same
per-queue code but was not exercised. The test kernels are
instrumented (injected STATFS, widened teardown window), so their
timings only show ordering. A FUSE_DESTROY that is never answered
still blocks umount; that is pre-existing.
v2 also words the /dev/fuse point precisely: virtio-fs exposes no
userspace fd at all, and no_control disables fusectl.
Thanks,
Qihang
^ permalink raw reply [flat|nested] 3+ messages in thread