On Wed, Mar 19, 2025 at 5:37 AM Bernd Schubert <bschubert@xxxxxxx> wrote:
>
> task-A (application) might be in request_wait_answer and
> try to remove the request when it has FR_PENDING set.
>
> task-B (a fuse-server io-uring task) might handle this
> request with FUSE_IO_URING_CMD_COMMIT_AND_FETCH, when
> fetching the next request and accessed the req from
> the pending list in fuse_uring_ent_assign_req().
> That code path was not protected by fiq->lock and so
> might race with task-A.
>
> For scaling reasons we better don't use fiq->lock, but
> add a handler to remove canceled requests from the queue.
>
> This also removes usage of fiq->lock from
> fuse_uring_add_req_to_ring_ent() altogether, as it was
> there just to protect against this race and incomplete.
>
> Fixes: c090c8abae4b ("fuse: Add io-uring sqe commit and fetch support")
> Reported-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> Closes: https://lore.kernel.org/all/CAJnrk1ZgHNb78dz-yfNTpxmW7wtT88A=m-zF0ZoLXKLUHRjNTw@xxxxxxxxxxxxxx/
> Signed-off-by: Bernd Schubert <bschubert@xxxxxxx>
LGTM but imo the code looks cleaner with
fuse_uring_remove_pending_req() just directly calling
fuse_remove_pending_req() instead of passing in "bool
(*remove_fn)(struct fuse_req *req, spinlock_t *lock))" as a function
arg.
Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> fs/fuse/dev.c | 33 +++++++++++++++++++++++----------
> fs/fuse/dev_uring.c | 17 +++++++++++++----
> fs/fuse/dev_uring_i.h | 10 ++++++++++
> fs/fuse/fuse_i.h | 2 ++
> 4 files changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 124a6744e8088474efa014a483dc6d297cf321b7..20c82bb2313b95cdc910808ee4968804077ed05b 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -407,6 +407,21 @@ static int queue_interrupt(struct fuse_req *req)
> return 0;
> }
>
> +static bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock)
> +{
> + spin_lock(lock);
> + if (test_bit(FR_PENDING, &req->flags)) {
> + list_del(&req->list);
> + clear_bit(FR_PENDING, &req->flags);
> + spin_unlock(lock);
> + __fuse_put_request(req);
> + req->out.h.error = -EINTR;
> + return true;
> + }
> + spin_unlock(lock);
> + return false;
> +}
> +
> static void request_wait_answer(struct fuse_req *req)
> {
> struct fuse_conn *fc = req->fm->fc;
> @@ -428,23 +443,21 @@ static void request_wait_answer(struct fuse_req *req)
> }
>
> if (!test_bit(FR_FORCE, &req->flags)) {
> + bool removed;
> +
> /* Only fatal signals may interrupt this */
> err = wait_event_killable(req->waitq,
> test_bit(FR_FINISHED, &req->flags));
> if (!err)
> return;
>
> - spin_lock(&fiq->lock);
> - /* Request is not yet in userspace, bail out */
> - if (test_bit(FR_PENDING, &req->flags)) {
> - list_del(&req->list);
> - clear_bit(FR_PENDING, &req->flags);
> - spin_unlock(&fiq->lock);
> - __fuse_put_request(req);
> - req->out.h.error = -EINTR;
> + if (test_bit(FR_URING, &req->flags))
> + removed = fuse_uring_remove_pending_req(
> + req, fuse_remove_pending_req);
> + else
> + removed = fuse_remove_pending_req(req, &fiq->lock);
> + if (removed)
> return;
> - }
> - spin_unlock(&fiq->lock);
> }
>
> /*
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index ebd2931b4f2acac461091b6b1f1176cde759e2d1..0d7fe8d6d2bf214b38bc90f6a7a9b4840219a81c 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -726,8 +726,6 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent,
> struct fuse_req *req)
> {
> struct fuse_ring_queue *queue = ent->queue;
> - struct fuse_conn *fc = req->fm->fc;
> - struct fuse_iqueue *fiq = &fc->iq;
>
> lockdep_assert_held(&queue->lock);
>
> @@ -737,9 +735,7 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent,
> ent->state);
> }
>
> - spin_lock(&fiq->lock);
> clear_bit(FR_PENDING, &req->flags);
> - spin_unlock(&fiq->lock);
> ent->fuse_req = req;
> ent->state = FRRS_FUSE_REQ;
> list_move(&ent->list, &queue->ent_w_req_queue);
> @@ -1238,6 +1234,8 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req)
> if (unlikely(queue->stopped))
> goto err_unlock;
>
> + set_bit(FR_URING, &req->flags);
> + req->ring_queue = queue;
> ent = list_first_entry_or_null(&queue->ent_avail_queue,
> struct fuse_ring_ent, list);
> if (ent)
> @@ -1276,6 +1274,8 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req)
> return false;
> }
>
> + set_bit(FR_URING, &req->flags);
> + req->ring_queue = queue;
> list_add_tail(&req->list, &queue->fuse_req_bg_queue);
>
> ent = list_first_entry_or_null(&queue->ent_avail_queue,
> @@ -1306,6 +1306,15 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req)
> return true;
> }
>
> +bool fuse_uring_remove_pending_req(struct fuse_req *req,
> + bool (*remove_fn)(struct fuse_req *req,
> + spinlock_t *lock))
nit: indentation should be aligned here?
> +{
> + struct fuse_ring_queue *queue = req->ring_queue;
> +
> + return remove_fn(req, &queue->lock);
> +}
> +
> static const struct fuse_iqueue_ops fuse_io_uring_ops = {
> /* should be send over io-uring as enhancement */
> .send_forget = fuse_dev_queue_forget,
> diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h
> index 2102b3d0c1aed1105e9c1200c91e1cb497b9a597..89a1da485b0e06fc6096f9b343dc0855c5df9c0b 100644
> --- a/fs/fuse/dev_uring_i.h
> +++ b/fs/fuse/dev_uring_i.h
> @@ -142,6 +142,9 @@ void fuse_uring_abort_end_requests(struct fuse_ring *ring);
> int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);
> void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req);
> bool fuse_uring_queue_bq_req(struct fuse_req *req);
> +bool fuse_uring_remove_pending_req(struct fuse_req *req,
> + bool (*remove_fn)(struct fuse_req *req,
> + spinlock_t *lock));
nit: indentation needs to be aligned here?
>
> static inline void fuse_uring_abort(struct fuse_conn *fc)
> {
> @@ -200,6 +203,13 @@ static inline bool fuse_uring_ready(struct fuse_conn *fc)
> return false;
> }
>
> +static inline bool fuse_uring_remove_pending_req(
> + struct fuse_req *req,
> + bool (*remove_fn)(struct fuse_req *req, spinlock_t *lock))
> +{
> + return false;
> +}
> +
> #endif /* CONFIG_FUSE_IO_URING */
>
> #endif /* _FS_FUSE_DEV_URING_I_H */
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index fee96fe7887b30cd57b8a6bbda11447a228cf446..5428a5b5e16a880894142f0ec1176a349c9469dc 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -392,6 +392,7 @@ enum fuse_req_flag {
> FR_FINISHED,
> FR_PRIVATE,
> FR_ASYNC,
> + FR_URING,
> };
>
> /**
> @@ -441,6 +442,7 @@ struct fuse_req {
>
> #ifdef CONFIG_FUSE_IO_URING
> void *ring_entry;
> + void *ring_queue;
> #endif
> };
>
>
> --
> 2.43.0
>
