On Fri, Apr 16, 2021 at 07:49:12PM +0530, Pradeep P V K wrote:
> There is a minor race in setting the fuse out request error
> between fuse_abort_conn() and fuse_dev_do_read() as explained
> below.
>
> Thread-1 Thread-2
> ======== ========
> ->fuse_simple_request() ->shutdown
> ->__fuse_request_send()
> ->queue_request() ->fuse_abort_conn()
> ->fuse_dev_do_read() ->acquire(fpq->lock)
> ->wait_for(fpq->lock) ->set err to all req's in fpq->io
> ->release(fpq->lock)
> ->acquire(fpq->lock)
> ->add req to fpq->io
>
> The above scenario may cause Thread-1 request to add into
> fpq->io list after Thread-2 sets -ECONNABORTED err to all
> its requests in fpq->io list. This leaves Thread-1 request
> with unset err and this further misleads as a completed
> request without an err set upon request_end().
>
> Handle this by setting the err appropriately.
The fix looks good, but still allows the request to block during the copy phase
after being aborted, which the FR_LOCKED/FR_ABORT bits are meant to prevent.
Here's an updated fix. It does not allow the request to be queued on the
fpq->io list after fuse_abort_conn() has aborted requests on that list.
Can you verify that it fixes the race you reported?
Thanks,
Miklos
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1272,6 +1272,15 @@ static ssize_t fuse_dev_do_read(struct f
goto restart;
}
spin_lock(&fpq->lock);
+ /*
+ * Must not put request on fpq->io queue after having been shut down by
+ * fuse_abort_conn()
+ */
+ if (!fpq->connected) {
+ req->out.h.error = err = -ECONNABORTED;
+ goto out_end;
+
+ }
list_add(&req->list, &fpq->io);
spin_unlock(&fpq->lock);
cs->req = req;
