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

[PATCH v2] 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.

Also added is a comment why FR_PENDING is not cleared.

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>
---
Changes in v2:
- Removed patch 1 that unset FR_PENDING
- Added a comment as part of this patch why FR_PENDING
  is not cleared
- Replaced function pointer by direct call of
  fuse_remove_pending_req
---
 fs/fuse/dev.c         | 34 +++++++++++++++++++++++++---------
 fs/fuse/dev_uring.c   | 15 +++++++++++----
 fs/fuse/dev_uring_i.h |  6 ++++++
 fs/fuse/fuse_dev_i.h  |  1 +
 fs/fuse/fuse_i.h      |  3 +++
 5 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 2c3a4d09e500f98232d5d9412a012235af6bec2e..2645cd8accfd081c518d3e22127e899ad5a09127 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -407,6 +407,24 @@ static int queue_interrupt(struct fuse_req *req)
 	return 0;
 }
 
+bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock)
+{
+	spin_lock(lock);
+	if (test_bit(FR_PENDING, &req->flags)) {
+		/*
+		 * FR_PENDING does not get cleared as the request will end
+		 * up in destruction anyway.
+		 */
+		list_del(&req->list);
+		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,22 +446,20 @@ 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);
-			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);
+		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..add7273c8dc4a23a23e50b879db470fc06bd3d20 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,13 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req)
 	return true;
 }
 
+bool fuse_uring_remove_pending_req(struct fuse_req *req)
+{
+	struct fuse_ring_queue *queue = req->ring_queue;
+
+	return fuse_remove_pending_req(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..e5b39a92b7ca0e371512e8071f15c89bb30caf59 100644
--- a/fs/fuse/dev_uring_i.h
+++ b/fs/fuse/dev_uring_i.h
@@ -142,6 +142,7 @@ 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);
 
 static inline void fuse_uring_abort(struct fuse_conn *fc)
 {
@@ -200,6 +201,11 @@ static inline bool fuse_uring_ready(struct fuse_conn *fc)
 	return false;
 }
 
+static inline bool fuse_uring_remove_pending_req(struct fuse_req *req)
+{
+	return false;
+}
+
 #endif /* CONFIG_FUSE_IO_URING */
 
 #endif /* _FS_FUSE_DEV_URING_I_H */
diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
index 3b2bfe1248d3573abe3b144a6d4bf6a502f56a40..2481da3388c5feec944143bfabb8d430a447d322 100644
--- a/fs/fuse/fuse_dev_i.h
+++ b/fs/fuse/fuse_dev_i.h
@@ -61,6 +61,7 @@ int fuse_copy_out_args(struct fuse_copy_state *cs, struct fuse_args *args,
 void fuse_dev_queue_forget(struct fuse_iqueue *fiq,
 			   struct fuse_forget_link *forget);
 void fuse_dev_queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req);
+bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock);
 
 #endif
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index fee96fe7887b30cd57b8a6bbda11447a228cf446..2086dac7243ba82e1ce6762e2d1406014566aaaa 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -378,6 +378,7 @@ struct fuse_io_priv {
  * FR_FINISHED:		request is finished
  * FR_PRIVATE:		request is on private list
  * FR_ASYNC:		request is asynchronous
+ * FR_URING:		request is handled through fuse-io-uring
  */
 enum fuse_req_flag {
 	FR_ISREPLY,
@@ -392,6 +393,7 @@ enum fuse_req_flag {
 	FR_FINISHED,
 	FR_PRIVATE,
 	FR_ASYNC,
+	FR_URING,
 };
 
 /**
@@ -441,6 +443,7 @@ struct fuse_req {
 
 #ifdef CONFIG_FUSE_IO_URING
 	void *ring_entry;
+	void *ring_queue;
 #endif
 };
 

---
base-commit: 81e4f8d68c66da301bb881862735bd74c6241a19
change-id: 20250218-fr_pending-race-3e362f22f319

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

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

[Luis Henriques]

Hi Bernd!

On Tue, Mar 25 2025, Bernd Schubert 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.
>
> Also added is a comment why FR_PENDING is not cleared.

At first, this patch looked OK to me.  However, after looking closer, I'm
not sure if this doesn't break fuse_abort_conn().  Because that function
assumes it is safe to walk through all the requests using fiq->lock, it
could race against fuse_uring_remove_pending_req(), which uses queue->lock
instead.  Am I missing something (quite likely!), or does fuse_abort_conn()
also needs to be modified?

[ Another scenario that is not problematic, but that could become messy in
  the future is if we want to add support for the FUSE_NOTIFY_RESEND
  operation through uring.  Obviously, that's not an issue right now, but
  this patch probably will make it harder to add that support. ]

Cheers,
-- 
Luís

> 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>
> ---
> Changes in v2:
> - Removed patch 1 that unset FR_PENDING
> - Added a comment as part of this patch why FR_PENDING
>   is not cleared
> - Replaced function pointer by direct call of
>   fuse_remove_pending_req
> ---
>  fs/fuse/dev.c         | 34 +++++++++++++++++++++++++---------
>  fs/fuse/dev_uring.c   | 15 +++++++++++----
>  fs/fuse/dev_uring_i.h |  6 ++++++
>  fs/fuse/fuse_dev_i.h  |  1 +
>  fs/fuse/fuse_i.h      |  3 +++
>  5 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 2c3a4d09e500f98232d5d9412a012235af6bec2e..2645cd8accfd081c518d3e22127e899ad5a09127 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -407,6 +407,24 @@ static int queue_interrupt(struct fuse_req *req)
>  	return 0;
>  }
>  
> +bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock)
> +{
> +	spin_lock(lock);
> +	if (test_bit(FR_PENDING, &req->flags)) {
> +		/*
> +		 * FR_PENDING does not get cleared as the request will end
> +		 * up in destruction anyway.
> +		 */
> +		list_del(&req->list);
> +		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,22 +446,20 @@ 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);
> -			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);
> +		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..add7273c8dc4a23a23e50b879db470fc06bd3d20 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,13 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req)
>  	return true;
>  }
>  
> +bool fuse_uring_remove_pending_req(struct fuse_req *req)
> +{
> +	struct fuse_ring_queue *queue = req->ring_queue;
> +
> +	return fuse_remove_pending_req(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..e5b39a92b7ca0e371512e8071f15c89bb30caf59 100644
> --- a/fs/fuse/dev_uring_i.h
> +++ b/fs/fuse/dev_uring_i.h
> @@ -142,6 +142,7 @@ 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);
>  
>  static inline void fuse_uring_abort(struct fuse_conn *fc)
>  {
> @@ -200,6 +201,11 @@ static inline bool fuse_uring_ready(struct fuse_conn *fc)
>  	return false;
>  }
>  
> +static inline bool fuse_uring_remove_pending_req(struct fuse_req *req)
> +{
> +	return false;
> +}
> +
>  #endif /* CONFIG_FUSE_IO_URING */
>  
>  #endif /* _FS_FUSE_DEV_URING_I_H */
> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> index 3b2bfe1248d3573abe3b144a6d4bf6a502f56a40..2481da3388c5feec944143bfabb8d430a447d322 100644
> --- a/fs/fuse/fuse_dev_i.h
> +++ b/fs/fuse/fuse_dev_i.h
> @@ -61,6 +61,7 @@ int fuse_copy_out_args(struct fuse_copy_state *cs, struct fuse_args *args,
>  void fuse_dev_queue_forget(struct fuse_iqueue *fiq,
>  			   struct fuse_forget_link *forget);
>  void fuse_dev_queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req);
> +bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock);
>  
>  #endif
>  
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index fee96fe7887b30cd57b8a6bbda11447a228cf446..2086dac7243ba82e1ce6762e2d1406014566aaaa 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -378,6 +378,7 @@ struct fuse_io_priv {
>   * FR_FINISHED:		request is finished
>   * FR_PRIVATE:		request is on private list
>   * FR_ASYNC:		request is asynchronous
> + * FR_URING:		request is handled through fuse-io-uring
>   */
>  enum fuse_req_flag {
>  	FR_ISREPLY,
> @@ -392,6 +393,7 @@ enum fuse_req_flag {
>  	FR_FINISHED,
>  	FR_PRIVATE,
>  	FR_ASYNC,
> +	FR_URING,
>  };
>  
>  /**
> @@ -441,6 +443,7 @@ struct fuse_req {
>  
>  #ifdef CONFIG_FUSE_IO_URING
>  	void *ring_entry;
> +	void *ring_queue;
>  #endif
>  };
>  
>
> ---
> base-commit: 81e4f8d68c66da301bb881862735bd74c6241a19
> change-id: 20250218-fr_pending-race-3e362f22f319
>
> Best regards,
> -- 
> Bernd Schubert <bschubert@ddn.com>
>

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

[Bernd Schubert]

On 3/25/25 22:38, Luis Henriques wrote:
> Hi Bernd!
> 
> On Tue, Mar 25 2025, Bernd Schubert 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.
>>
>> Also added is a comment why FR_PENDING is not cleared.

Hi Luis,

thanks for your review!

> 
> At first, this patch looked OK to me.  However, after looking closer, I'm
> not sure if this doesn't break fuse_abort_conn().  Because that function
> assumes it is safe to walk through all the requests using fiq->lock, it
> could race against fuse_uring_remove_pending_req(), which uses queue->lock
> instead.  Am I missing something (quite likely!), or does fuse_abort_conn()
> also needs to be modified?

I don't think there is an issue with abort

fuse_abort_conn()
   spin_lock(&fiq->lock);
   list_for_each_entry(req, &fiq->pending, list)
   ...
   spin_unlock(&fiq->lock);

   ...

   fuse_uring_abort(fc);

Iterating fiq->pending will not handle any uring request, as these are
in per queue lists. The per uring queues are then handled in
fuse_uring_abort().

I.e. I don't think this commit changes anything for abort.



> 
> [ Another scenario that is not problematic, but that could become messy in
>   the future is if we want to add support for the FUSE_NOTIFY_RESEND
>   operation through uring.  Obviously, that's not an issue right now, but
>   this patch probably will make it harder to add that support. ]

Oh yeah, this needs to be fixed. Though I don't think that this patch
changes much. We need to iterate through the per fpq and apply the
same logic?


Thanks,
Bernd

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

[Luis Henriques]

On Tue, Mar 25 2025, Bernd Schubert wrote:

> On 3/25/25 22:38, Luis Henriques wrote:
>> Hi Bernd!
>> 
>> On Tue, Mar 25 2025, Bernd Schubert 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.
>>>
>>> Also added is a comment why FR_PENDING is not cleared.
>
> Hi Luis,
>
> thanks for your review!
>
>> 
>> At first, this patch looked OK to me.  However, after looking closer, I'm
>> not sure if this doesn't break fuse_abort_conn().  Because that function
>> assumes it is safe to walk through all the requests using fiq->lock, it
>> could race against fuse_uring_remove_pending_req(), which uses queue->lock
>> instead.  Am I missing something (quite likely!), or does fuse_abort_conn()
>> also needs to be modified?
>
> I don't think there is an issue with abort
>
> fuse_abort_conn()
>    spin_lock(&fiq->lock);
>    list_for_each_entry(req, &fiq->pending, list)
>    ...
>    spin_unlock(&fiq->lock);
>
>    ...
>
>    fuse_uring_abort(fc);
>
> Iterating fiq->pending will not handle any uring request, as these are
> in per queue lists. The per uring queues are then handled in
> fuse_uring_abort().
>
> I.e. I don't think this commit changes anything for abort.

Yeah, you're right.  Thanks for looking, Bernd.

>> 
>> [ Another scenario that is not problematic, but that could become messy in
>>   the future is if we want to add support for the FUSE_NOTIFY_RESEND
>>   operation through uring.  Obviously, that's not an issue right now, but
>>   this patch probably will make it harder to add that support. ]
>
> Oh yeah, this needs to be fixed. Though I don't think that this patch
> changes much. We need to iterate through the per fpq and apply the
> same logic?

Right, I agree this patch doesn't change anything here.  And I guess I
also misunderstood the problem here as well -- I thought this would be an
issue when adding support for iouring, but in fact it is already a
problem.  The ideal solution would be to implement NOTIFY_RESEND over
iouring, but that would be a bit more evolving.

Cheers,
-- 
Luís

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

[Joanne Koong]

On Tue, Mar 25, 2025 at 10:30 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.
>
> Also added is a comment why FR_PENDING is not cleared.
>
> 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>

Reviewed-by: Joanne Koong <joannelkoong@gmail.com>

> ---
> Changes in v2:
> - Removed patch 1 that unset FR_PENDING
> - Added a comment as part of this patch why FR_PENDING
>   is not cleared
> - Replaced function pointer by direct call of
>   fuse_remove_pending_req
> ---
>  fs/fuse/dev.c         | 34 +++++++++++++++++++++++++---------
>  fs/fuse/dev_uring.c   | 15 +++++++++++----
>  fs/fuse/dev_uring_i.h |  6 ++++++
>  fs/fuse/fuse_dev_i.h  |  1 +
>  fs/fuse/fuse_i.h      |  3 +++
>  5 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 2c3a4d09e500f98232d5d9412a012235af6bec2e..2645cd8accfd081c518d3e22127e899ad5a09127 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -407,6 +407,24 @@ static int queue_interrupt(struct fuse_req *req)
>         return 0;
>  }
>
> +bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock)
> +{
> +       spin_lock(lock);
> +       if (test_bit(FR_PENDING, &req->flags)) {
> +               /*
> +                * FR_PENDING does not get cleared as the request will end
> +                * up in destruction anyway.
> +                */
> +               list_del(&req->list);
> +               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,22 +446,20 @@ 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);
> -                       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);
> +               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..add7273c8dc4a23a23e50b879db470fc06bd3d20 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,13 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req)
>         return true;
>  }
>
> +bool fuse_uring_remove_pending_req(struct fuse_req *req)
> +{
> +       struct fuse_ring_queue *queue = req->ring_queue;
> +
> +       return fuse_remove_pending_req(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..e5b39a92b7ca0e371512e8071f15c89bb30caf59 100644
> --- a/fs/fuse/dev_uring_i.h
> +++ b/fs/fuse/dev_uring_i.h
> @@ -142,6 +142,7 @@ 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);
>
>  static inline void fuse_uring_abort(struct fuse_conn *fc)
>  {
> @@ -200,6 +201,11 @@ static inline bool fuse_uring_ready(struct fuse_conn *fc)
>         return false;
>  }
>
> +static inline bool fuse_uring_remove_pending_req(struct fuse_req *req)
> +{
> +       return false;
> +}
> +
>  #endif /* CONFIG_FUSE_IO_URING */
>
>  #endif /* _FS_FUSE_DEV_URING_I_H */
> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> index 3b2bfe1248d3573abe3b144a6d4bf6a502f56a40..2481da3388c5feec944143bfabb8d430a447d322 100644
> --- a/fs/fuse/fuse_dev_i.h
> +++ b/fs/fuse/fuse_dev_i.h
> @@ -61,6 +61,7 @@ int fuse_copy_out_args(struct fuse_copy_state *cs, struct fuse_args *args,
>  void fuse_dev_queue_forget(struct fuse_iqueue *fiq,
>                            struct fuse_forget_link *forget);
>  void fuse_dev_queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req);
> +bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock);
>
>  #endif
>
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index fee96fe7887b30cd57b8a6bbda11447a228cf446..2086dac7243ba82e1ce6762e2d1406014566aaaa 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -378,6 +378,7 @@ struct fuse_io_priv {
>   * FR_FINISHED:                request is finished
>   * FR_PRIVATE:         request is on private list
>   * FR_ASYNC:           request is asynchronous
> + * FR_URING:           request is handled through fuse-io-uring
>   */
>  enum fuse_req_flag {
>         FR_ISREPLY,
> @@ -392,6 +393,7 @@ enum fuse_req_flag {
>         FR_FINISHED,
>         FR_PRIVATE,
>         FR_ASYNC,
> +       FR_URING,
>  };
>
>  /**
> @@ -441,6 +443,7 @@ struct fuse_req {
>
>  #ifdef CONFIG_FUSE_IO_URING
>         void *ring_entry;
> +       void *ring_queue;
>  #endif
>  };
>
>
> ---
> base-commit: 81e4f8d68c66da301bb881862735bd74c6241a19
> change-id: 20250218-fr_pending-race-3e362f22f319
>
> Best regards,
> --
> Bernd Schubert <bschubert@ddn.com>
>