[PATCH v2] fuse: virtio-fs: stop new requests before draining queues

[PATCH v2] fuse: virtio-fs: stop new requests before draining queues

[Qihang]

virtio_fs_conn_destroy() drains the virtqueues and then calls
fuse_conn_destroy(), which aborts the connection.  The abort ends every
request the fuse device still owns, and each ended request is freed as
soon as its waiter wakes up.

The drain only covers the requests that were already counted in
fsvq->in_flight when it started.  A request submitted after it returns is
not covered, but is still handed to the device before the abort runs:

  CPU 0 (umount)                     CPU 1
  ---------------------------------  --------------------------------
  virtio_fs_drain_all_queues()
    /* in_flight drops to 0 and
       the drain returns */
                                     virtio_fs_enqueue_req()
                                       virtqueue_add_sgs()
                                       inc_in_flight_req() -> 1
  fuse_conn_destroy()
    fuse_send_destroy()
    fuse_chan_abort()
      /* ends and frees the
         request above */
    fuse_chan_wait_aborted()
                                     /* the device still owns the
                                        virtqueue descriptors and
                                        DMA-writes into the freed
                                        request and its argbuf */

Stop admitting requests on each queue before that queue is drained.
The flag is set per queue, but that is sufficient: each queue is
drained after its own flag is set, and request admission,
virtqueue_add_sgs() and, on the submission path, the in_flight
increment are serialized by the same fsvq->lock.  A request is
therefore either rejected before the device can see it, or already
counted in fsvq->in_flight and covered by the drain.  There is no
window in between.

Rejected requests use the existing -ENOTCONN error path and are ended by
virtio_fs_request_dispatch_work() through end_reqs, so no new request
completion or ownership state is introduced.  Nothing has to be
discarded in the completion path, and fsvq->in_flight remains the only
state that has to be tracked.

The new state is not folded into fsvq->connected because the request
queues have to stay usable: fuse_conn_destroy() sends FUSE_DESTROY
through them, with args.force set so that fuse_simple_request() waits
for the reply.  Clearing fsvq->connected would make that request fail
with -ENOTCONN and the server would never see the destroy.

FUSE_DESTROY is the sole exception while aborting is set.  It is sent
only after the first drain, and its completion is waited for before
fuse_chan_abort() can end requests, so the destroy is the only request
the device sees after the drain.  virtio-fs exposes no userspace
/dev/fuse fd (and no_control disables the fusectl interface), so
userspace cannot inject another request.

virtio_fs_start_all_queues() clears the flag, so a mount that follows an
unmount starts with admission enabled again.

Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
V1 -> V2:
- prevent new requests from reaching the virtqueue at teardown time
  instead of skipping completions of aborted requests
- keep FUSE_DESTROY working: it is the teardown request and is waited
  for before fuse_chan_abort()
- drop the per-vq aborted flag and the completion-skip branch
 fs/fuse/virtio_fs.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index ba8d79c4bcac6..c6d2120157340 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 aborting; /* reject new requests except FUSE_DESTROY */
 	long in_flight;
 	struct completion in_flight_zero; /* No inflight requests */
 	struct kobject *kobj;
@@ -348,6 +349,7 @@ static void virtio_fs_start_all_queues(struct virtio_fs *fs)
 		fsvq = &fs->vqs[i];
 		spin_lock(&fsvq->lock);
 		fsvq->connected = true;
+		fsvq->aborting = false;
 		spin_unlock(&fsvq->lock);
 	}
 }
@@ -1226,6 +1228,28 @@ static void virtio_fs_stop_all_queues(struct virtio_fs *fs)
 	}
 }
 
+/*
+ * Stop admitting new requests before draining the queues.  Admission and
+ * virtqueue submission are serialized by fsvq->lock, so a request is
+ * either rejected before reaching the device or accounted in in_flight
+ * and covered by the drain.
+ *
+ * FUSE_DESTROY is exempt because fuse_conn_destroy() sends it after the
+ * first drain and waits for its completion before aborting the channel.
+ */
+static void virtio_fs_stop_new_requests(struct virtio_fs *fs)
+{
+	struct virtio_fs_vq *fsvq;
+	int i;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		fsvq = &fs->vqs[i];
+		spin_lock(&fsvq->lock);
+		fsvq->aborting = true;
+		spin_unlock(&fsvq->lock);
+	}
+}
+
 static void virtio_fs_remove(struct virtio_device *vdev)
 {
 	struct virtio_fs *fs = vdev->priv;
@@ -1479,7 +1503,8 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 
 	spin_lock(&fsvq->lock);
 
-	if (!fsvq->connected) {
+	if (!fsvq->connected ||
+	    (fsvq->aborting && req->in.h.opcode != FUSE_DESTROY)) {
 		spin_unlock(&fsvq->lock);
 		ret = -ENOTCONN;
 		goto out;
@@ -1669,6 +1694,9 @@ static void virtio_fs_conn_destroy(struct fuse_mount *fm)
 	spin_lock(&fsvq->lock);
 	fsvq->connected = false;
 	spin_unlock(&fsvq->lock);
+
+	/* Nothing may be queued after the drain below */
+	virtio_fs_stop_new_requests(vfs);
 	virtio_fs_drain_all_queues(vfs);
 
 	fuse_conn_destroy(fm);
-- 
2.54.0 (Apple Git-157)

Re: [PATCH v2] fuse: virtio-fs: stop new requests before draining queues

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] fuse: virtio-fs: race on -ENOSPC in virtio_fs_send_req()
  bypasses aborting
- [Medium] fuse: virtio-fs: self-deadlock in
  virtio_fs_request_dispatch_work()
- [Low] fuse: virtio-fs: missing Fixes trailer for unmount teardown race
  fix
--

commit 183d70093cd447a74de7ef9f20f4895fc3aef144
Author: Qihang <q.h.hack.winter@gmail.com>

> fuse: virtio-fs: stop new requests before draining queues
>
> virtio_fs_conn_destroy() drains the virtqueues and then calls
> fuse_conn_destroy(), which aborts the connection.  The abort ends every
> request the fuse device still owns, and each ended request is freed as
> soon as its waiter wakes up.
>
> The drain only covers the requests that were already counted in
> fsvq->in_flight when it started.  A request submitted after it returns is
> not covered, but is still handed to the device before the abort runs:
>
>   CPU 0 (umount)                     CPU 1
>   ---------------------------------  --------------------------------
>   virtio_fs_drain_all_queues()
>     /* in_flight drops to 0 and
>        the drain returns */
>                                      virtio_fs_enqueue_req()
>                                        virtqueue_add_sgs()
>                                        inc_in_flight_req() -> 1
>   fuse_conn_destroy()
>     fuse_send_destroy()
>     fuse_chan_abort()
>       /* ends and frees the
>          request above */
>     fuse_chan_wait_aborted()
>                                      /* the device still owns the
>                                         virtqueue descriptors and
>                                         DMA-writes into the freed
>                                         request and its argbuf */
>
> Stop admitting requests on each queue before that queue is drained.
> The flag is set per queue, but that is sufficient: each queue is
> drained after its own flag is set, and request admission,
> virtqueue_add_sgs() and, on the submission path, the in_flight
> increment are serialized by the same fsvq->lock.  A request is
> therefore either rejected before the device can see it, or already
> counted in fsvq->in_flight and covered by the drain.  There is no
> window in between.
>
> Rejected requests use the existing -ENOTCONN error path and are ended by
> virtio_fs_request_dispatch_work() through end_reqs, so no new request
> completion or ownership state is introduced.  Nothing has to be
> discarded in the completion path, and fsvq->in_flight remains the only
> state that has to be tracked.
>
> The new state is not folded into fsvq->connected because the request
> queues have to stay usable: fuse_conn_destroy() sends FUSE_DESTROY
> through them, with args.force set so that fuse_simple_request() waits
> for the reply.  Clearing fsvq->connected would make that request fail
> with -ENOTCONN and the server would never see the destroy.
>
> FUSE_DESTROY is the sole exception while aborting is set.  It is sent
> only after the first drain, and its completion is waited for before
> fuse_chan_abort() can end requests, so the destroy is the only request
> the device sees after the drain.  virtio-fs exposes no userspace
> /dev/fuse fd (and no_control disables the fusectl interface), so
> userspace cannot inject another request.
>
> virtio_fs_start_all_queues() clears the flag, so a mount that follows an
> unmount starts with admission enabled again.
>
> Signed-off-by: Qihang <q.h.hack.winter@gmail.com>

[Severity: Low]
Since this fixes the unmount teardown race present since virtio-fs was
introduced, should it include a Fixes: tag?

  Fixes: a62a8ef9d97d ("virtio-fs: add virtiofs filesystem")

> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index ba8d79c4bcac6..c6d2120157340 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c

[ ... ]

> @@ -1479,7 +1503,8 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>  
>  	spin_lock(&fsvq->lock);
>  
> -	if (!fsvq->connected) {
> +	if (!fsvq->connected ||
> +	    (fsvq->aborting && req->in.h.opcode != FUSE_DESTROY)) {
>  		spin_unlock(&fsvq->lock);
>  		ret = -ENOTCONN;
>  		goto out;

[Severity: Medium]
Can a request that hits -ENOSPC in virtqueue_add_sgs() bypass fsvq->aborting
and hang unmount?

When virtqueue_add_sgs() returns -ENOSPC, virtio_fs_enqueue_req() drops
fsvq->lock without incrementing fsvq->in_flight or queueing req onto
fsvq->queued_reqs, and virtio_fs_send_req() re-acquires fsvq->lock in a
separate critical section without re-checking fsvq->aborting:

fs/fuse/virtio_fs.c:virtio_fs_send_req() {
    ...
	ret = virtio_fs_enqueue_req(fsvq, req, false, GFP_ATOMIC);
	if (ret < 0) {
		if (ret == -ENOSPC) {
			spin_lock(&fsvq->lock);
			list_add_tail(&req->list, &fsvq->queued_reqs);
			inc_in_flight_req(fsvq);
			spin_unlock(&fsvq->lock);
			return;
		}
    ...
}

If the remaining in-flight requests complete and the queue is drained in
that unlocked window, req is stranded on fsvq->queued_reqs:

CPU0                              CPU1 (umount)
virtio_fs_send_req()
  virtio_fs_enqueue_req()
    spin_lock(&fsvq->lock)
    /* fsvq->aborting is false */
    virtqueue_add_sgs() -> -ENOSPC
    spin_unlock(&fsvq->lock)
                                  /* in-flight reqs complete;
                                     done_work sees queued_reqs
                                     empty, in_flight == 0 */
                                  virtio_fs_stop_new_requests()
                                    fsvq->aborting = true
                                  virtio_fs_drain_all_queues()
  spin_lock(&fsvq->lock)
  list_add_tail(&req->list,
                &fsvq->queued_reqs)
  inc_in_flight_req(fsvq) -> 1
  spin_unlock(&fsvq->lock)
                                  fuse_conn_destroy()
                                    fuse_chan_wait_aborted()
                                      <-- hangs waiting for req

Because fuse_chan_abort() does not walk fsvq->queued_reqs and nothing
schedules fsvq->dispatch_work again on that queue, req is never ended,
hanging fuse_chan_wait_aborted() or the second virtio_fs_drain_all_queues().
A similar unlocked window exists in virtio_fs_request_dispatch_work() when
virtio_fs_enqueue_req() returns -ENOSPC and req is re-added to
fsvq->queued_reqs without re-checking fsvq->aborting.

> @@ -1669,6 +1694,9 @@ static void virtio_fs_conn_destroy(struct fuse_mount *fm)
>  	spin_lock(&fsvq->lock);
>  	fsvq->connected = false;
>  	spin_unlock(&fsvq->lock);
> +
> +	/* Nothing may be queued after the drain below */
> +	virtio_fs_stop_new_requests(vfs);
>  	virtio_fs_drain_all_queues(vfs);
>  
>  	fuse_conn_destroy(fm);

[Severity: Medium]
Can setting fsvq->aborting before fuse_chan_abort() clears fch->connected
cause virtio_fs_request_dispatch_work() to deadlock on itself?

When virtio_fs_enqueue_req() rejects an asynchronous FUSE_RELEASE for an
unlinked DAX inode with -ENOTCONN while SB_ACTIVE and fch->connected are
still set, virtio_fs_request_dispatch_work() ends it directly instead of
offloading req->args->may_block requests to a separate worker the way
virtio_fs_requests_done_work() does:

virtio_fs_request_dispatch_work()
  fuse_request_end(req)
    fuse_release_end()
      iput()
        ...
          fuse_evict_inode()
            fuse_vdax_inode_cleanup()
              ...
                fuse_send_removemapping()
                  fuse_simple_request()
                    virtio_fs_send_req()
                      virtio_fs_enqueue_req() -> -ENOTCONN
                      list_add_tail(&req->list, &fsvq->end_reqs)
                      schedule_work(&fsvq->dispatch_work)
                    request_wait_answer()  <-- waits for dispatch_work

Because fuse_simple_request() blocks inside virtio_fs_request_dispatch_work()
waiting for fsvq->dispatch_work to process fsvq->end_reqs, the work item can
never run again and unmount deadlocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108135830.29924-1-q.h.hack.winter@gmail.com?part=1

Re: [PATCH v2] fuse: virtio-fs: stop new requests before draining queues

[Qihang]

Hi Sashiko,

Thanks for the review.  Both Medium findings are valid; v3 fixes them
and adds the missing Fixes tag.  The v3 patch is a separate thread.

#1 (-ENOSPC can bypass aborting)

Agreed.  virtio_fs_enqueue_req() drops fsvq->lock before returning
-ENOSPC, and virtio_fs_send_req() queues the request in a second
critical section.  Teardown can complete in between and leave the
request stranded on queued_reqs, which fuse_chan_abort() does not walk.

v3 re-checks connected/aborting in that second critical section, and in
the dispatch worker's -ENOSPC requeue path.  A request now either joins
queued_reqs before aborting is set (and is counted in in_flight), or is
rejected with -ENOTCONN and ended through end_reqs.  After aborting is
set, no request can newly enter queued_reqs or the virtqueue, except
FUSE_DESTROY.

I instrumented the window: enqueue returns -ENOSPC for a FUSE_STATFS,
send_req() waits before re-checking, and conn_destroy() sets aborting
and drains in that window.  Three runs:

  VF_INV: q=requests.0 in_flight=0 queued=0 connected=1 aborting=1
  VF_ENOSPC: rejected
  VF_ENOSPC: STATFS returned -107
  VF_INV: q=requests.0 in_flight=0 queued=0 connected=0 aborting=1
  umount rc=0

No stranded request, no warning.

#2 (dispatch worker may_block self-deadlock)

Agreed as well.  Rejecting requests earlier means a DAX release can be
ended by virtio_fs_request_dispatch_work(); its end callback issues a
synchronous removemapping request, which is rejected and needs the same
worker.

v3 offloads endings with req->args->may_block to a separate work item
that only calls fuse_request_end().  virtio_fs_request_complete()
cannot be reused: it also consumes argbuf and drops in_flight, while a
rejected request has neither.

I verified the path with a synthetic callback: a background STATFS with
may_block=true and an end callback that issues a synchronous STATFS.

  VF_DEADLOCK: queue outer
  VF_DEADLOCK: offload may_block end
  VF_DEADLOCK: outer end error=-107
  VF_DEADLOCK: inner returned -107
  VF_DEADLOCK: outer end done
  umount rc=0

The old inline fuse_request_end() would stop at "outer end error" and
wait for itself.

#3 (missing Fixes)

Added:

  Fixes: a62a8ef9d97d ("virtio-fs: add virtiofs filesystem")
  Cc: stable@vger.kernel.org

Thanks,
Qihang