[PATCH 0/2] fuse: {io-uring} Avoid possible FR_PENDING related list corruption

[PATCH 0/2] fuse: {io-uring} Avoid possible FR_PENDING related list corruption

[Bernd Schubert]

The issue was actually found by Joanne
https://lore.kernel.org/all/CAJnrk1ZgHNb78dz-yfNTpxmW7wtT88A=m-zF0ZoLXKLUHRjNTw@mail.gmail.com/

I had tried to test it by by writing from many processes at the same
time, but all my attempts to trigger an issue didn't succeed. Even
KCSAN never reported anything.

Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
Bernd Schubert (2):
      fuse: Clear FR_PENDING in request_wait_answer
      fuse: {io-uring} Fix a possible req cancellation race

 fs/fuse/dev.c         | 32 +++++++++++++++++++++++---------
 fs/fuse/dev_uring.c   | 17 +++++++++++++----
 fs/fuse/dev_uring_i.h | 10 ++++++++++
 fs/fuse/fuse_i.h      |  2 ++
 4 files changed, 48 insertions(+), 13 deletions(-)
---
base-commit: 81e4f8d68c66da301bb881862735bd74c6241a19
change-id: 20250218-fr_pending-race-3e362f22f319

Best regards,
-- 
Bernd Schubert <bschubert@ddn.com>

[PATCH 1/2] fuse: Clear FR_PENDING in request_wait_answer

[Bernd Schubert]

The request is removed from the list of pending requests,
directly after follows a __fuse_put_request() which is
likely going to destruct the request, but that is not
guaranteed, better if FR_PENDING gets cleared.

Suggested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 2c3a4d09e500f98232d5d9412a012235af6bec2e..124a6744e8088474efa014a483dc6d297cf321b7 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -438,6 +438,7 @@ static void request_wait_answer(struct fuse_req *req)
 		/* 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;

-- 
2.43.0

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

[Bernd Schubert]

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>
---
 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))
+{
+	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));
 
 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

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

[Joanne Koong]

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
>

Re: [PATCH 1/2] fuse: Clear FR_PENDING in request_wait_answer

[Joanne Koong]

On Wed, Mar 19, 2025 at 5:37 AM Bernd Schubert <bschubert@ddn.com> wrote:
>
> The request is removed from the list of pending requests,
> directly after follows a __fuse_put_request() which is
> likely going to destruct the request, but that is not
> guaranteed, better if FR_PENDING gets cleared.

I think it is guaranteed that the request will be freed. there's only
one call path for request_wait_answer():
__fuse_simple_request()
    fuse_get_req() -> request is allocated (req refcount is 1)
    __fuse_request_send()
        __fuse_get_request() -> req refcount is 2
        fuse_send_one()
        request_wait_answer()
   fuse_put_request() -> req refcount is dropped

if we hit that "if (test_bit(FR_PENDING, &req->flags))" case and
request_wait_answer() drops the ref, then the request will be freed
(or else it's leaked). imo we don't need this patch.


Thanks,
Joanne

>
> Suggested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/dev.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 2c3a4d09e500f98232d5d9412a012235af6bec2e..124a6744e8088474efa014a483dc6d297cf321b7 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -438,6 +438,7 @@ static void request_wait_answer(struct fuse_req *req)
>                 /* 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;
>
> --
> 2.43.0
>

Re: [PATCH 1/2] fuse: Clear FR_PENDING in request_wait_answer

[Bernd Schubert]

On 3/19/25 22:15, Joanne Koong wrote:
> On Wed, Mar 19, 2025 at 5:37 AM Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> The request is removed from the list of pending requests,
>> directly after follows a __fuse_put_request() which is
>> likely going to destruct the request, but that is not
>> guaranteed, better if FR_PENDING gets cleared.
> 
> I think it is guaranteed that the request will be freed. there's only
> one call path for request_wait_answer():
> __fuse_simple_request()
>     fuse_get_req() -> request is allocated (req refcount is 1)
>     __fuse_request_send()
>         __fuse_get_request() -> req refcount is 2
>         fuse_send_one()
>         request_wait_answer()
>    fuse_put_request() -> req refcount is dropped
> 
> if we hit that "if (test_bit(FR_PENDING, &req->flags))" case and
> request_wait_answer() drops the ref, then the request will be freed
> (or else it's leaked). imo we don't need this patch.
> 

I thought I had seen a path that was holding the request, but now I see
that every __fuse_get_request() actually also moves the request to
another list. I'm going to add a comments into the next patch explaining
why FR_PENDING is not cleared.


Thanks,
Bernd