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 <[email protected]> > 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 <[email protected]> [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/[email protected]?part=1
