Re: [PATCH] fuse: virtio-fs: don't process completions of aborted requests

[Qihang <q.h.hack.winter@gmail.com>]

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