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

[sashiko-bot@kernel.org]

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