Re: [PATCH 2/2] fuse: {io-uring} Fix a possible req cancellation race

[Joanne Koong <joannelkoong@gmail.com>]

On Wed, Mar 19, 2025 at 5:37 AM Bernd Schubert <bschubert@ddn.com> 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@gmail.com>
> Closes: https://lore.kernel.org/all/CAJnrk1ZgHNb78dz-yfNTpxmW7wtT88A=m-zF0ZoLXKLUHRjNTw@mail.gmail.com/
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>

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@gmail.com>

> ---
>  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
>