[PATCH v1 1/3] fuse: fix race between ring creation and connection abortion

[PATCH v1 1/3] fuse: fix race between ring creation and connection abortion

[Joanne Koong]

This fixes this race:
- thread a: fuse_uring_cmd() gets called, passes fch->connected check
  (connection abortion not yet triggered)
- thread b: abort is called, calls fuse_uring_abort(),
fuse_uring_abort() is a no-op since ring == NULL right now
- thread a: creates ring, creates queue, creates entry

which results in
- leaked ring, queue, ent
- if thread a increments queue_refs before thread b calls
  fuse_chan_wait_aborted(), then fuse_chan_wait_aborted() calls
  "wait_event(ring->stop_waitq, atomic_read(&ring->queue_refs) == 0);"
  which will hang the abort/unmount thread indefinitely in unkillable
  state, as nothing will decrement queue_refs or wake stop_waitq.

Fix this by checking fch->connected under fch->lock in
fuse_uring_create() before publishing the ring via
smp_store_release(&fch->ring, ring) under the same lock scope.

Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands")
Cc: <stable@vger.kernel.org>
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev_uring.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index e467b23e6895..cd75f61018ec 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -244,6 +244,10 @@ static struct fuse_ring *fuse_uring_create(struct fuse_chan *fch)
 	max_payload_size = max(max_payload_size, fch->max_pages * PAGE_SIZE);
 
 	spin_lock(&fch->lock);
+	if (!fch->connected) {
+		spin_unlock(&fch->lock);
+		goto out_err;
+	}
 	if (fch->ring) {
 		/* race, another thread created the ring in the meantime */
 		spin_unlock(&fch->lock);
-- 
2.52.0

Re: [PATCH v1 1/3] fuse: fix race between ring creation and connection abortion

[Bernd Schubert]

On 5/15/26 06:55, Joanne Koong wrote:
> This fixes this race:
> - thread a: fuse_uring_cmd() gets called, passes fch->connected check
>   (connection abortion not yet triggered)
> - thread b: abort is called, calls fuse_uring_abort(),
> fuse_uring_abort() is a no-op since ring == NULL right now
> - thread a: creates ring, creates queue, creates entry
> 
> which results in
> - leaked ring, queue, ent
> - if thread a increments queue_refs before thread b calls
>   fuse_chan_wait_aborted(), then fuse_chan_wait_aborted() calls
>   "wait_event(ring->stop_waitq, atomic_read(&ring->queue_refs) == 0);"
>   which will hang the abort/unmount thread indefinitely in unkillable
>   state, as nothing will decrement queue_refs or wake stop_waitq.
> 
> Fix this by checking fch->connected under fch->lock in
> fuse_uring_create() before publishing the ring via
> smp_store_release(&fch->ring, ring) under the same lock scope.

We had this discussion before, I still think it is covered by 2nd patch
"fuse: fix race between registration and connection abortion", because

fuse_uring_destruct() is called by delayed_release() in inode.c and that
is only called when there is nothing accessing /dev/fuse anymore.

The follow-up patch also handles ring->queue_refs going to 0.

From my point of view, what this patch really does is to avoid ring and
queue creation overhead when the connection is going down anyway, so
just the commit message is a bit confusing.

> 
> Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/dev_uring.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index e467b23e6895..cd75f61018ec 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -244,6 +244,10 @@ static struct fuse_ring *fuse_uring_create(struct fuse_chan *fch)
>  	max_payload_size = max(max_payload_size, fch->max_pages * PAGE_SIZE);
>  
>  	spin_lock(&fch->lock);
> +	if (!fch->connected) {
> +		spin_unlock(&fch->lock);
> +		goto out_err;
> +	}
>  	if (fch->ring) {
>  		/* race, another thread created the ring in the meantime */
>  		spin_unlock(&fch->lock);


Reviewed-by: Bernd Schubert <bernd@bsbernd.com>

[PATCH v1 2/3] fuse: fix race between registration and connection abortion

[Joanne Koong]

This fixes this race:
- thread a: io_uring_enter -> register sqe ->
  fuse_uring_create_ring_ent -> allocate ent but doesn't grab queue_ref
  yet
- thread b: fuse_conn_destroy() -> fuse_chan_abort() ->
  fuse_uring_abort() is a no-op due to queue ref being 0
- thread a: grabs the queue_ref, queue_ref is now 1, rest of
  fuse_uring_do_register() logic executes
- thread b: fuse_chan_abort() returns, fuse_chan_wait_aborted() now runs
  and calls
  "wait_event(ring->stop_waitq, atomic_read(&ring->queue_refs) == 0);"
The abort/unmount thread will hang indefinitely in unkillable state as
nothing will decrement queue_refs or wake stop_waitq, and the ring,
queue, and ent are leaked.

Fix this by checking fch->connected under fch->lock after the created
ent has grabbed a ref count on the queue. This ensures that in the
scenario above, it is guaranteed that we either release the queue ref
and wake up stop_waitq (in case fuse_chan_wait_aborted() is already
waiting) in fuse_uring_do_register() when we detect !fch->connected, or
if the connection is aborted after the check, it is guaranteed that the
async teardown worker will be running in the background cleaning up ents
and decrementing the ent's ref on the queue, which will unblock the
eventual queue and ring teardown.

Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands")
Cc: <stable@vger.kernel.org>
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev_uring.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index cd75f61018ec..d9108b5b5db8 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -977,15 +977,26 @@ static bool is_ring_ready(struct fuse_ring *ring, int current_qid)
 /*
  * fuse_uring_req_fetch command handling
  */
-static void fuse_uring_do_register(struct fuse_ring_ent *ent,
-				   struct io_uring_cmd *cmd,
-				   unsigned int issue_flags)
+static int fuse_uring_do_register(struct fuse_ring_ent *ent,
+				  struct io_uring_cmd *cmd,
+				  unsigned int issue_flags)
 {
 	struct fuse_ring_queue *queue = ent->queue;
 	struct fuse_ring *ring = queue->ring;
 	struct fuse_chan *fch = ring->chan;
 	struct fuse_iqueue *fiq = &fch->iq;
 
+	spin_lock(&fch->lock);
+	/* abort teardown path is running or has run */
+	if (!fch->connected) {
+		spin_unlock(&fch->lock);
+		if (atomic_dec_and_test(&ring->queue_refs))
+			wake_up_all(&ring->stop_waitq);
+		kfree(ent);
+		return -ECONNABORTED;
+	}
+	spin_unlock(&fch->lock);
+
 	fuse_uring_prepare_cancel(cmd, issue_flags, ent);
 
 	spin_lock(&queue->lock);
@@ -1002,6 +1013,7 @@ static void fuse_uring_do_register(struct fuse_ring_ent *ent,
 			wake_up_all(&fch->blocked_waitq);
 		}
 	}
+	return 0;
 }
 
 /*
@@ -1118,9 +1130,7 @@ static int fuse_uring_register(struct io_uring_cmd *cmd,
 	if (IS_ERR(ent))
 		return PTR_ERR(ent);
 
-	fuse_uring_do_register(ent, cmd, issue_flags);
-
-	return 0;
+	return fuse_uring_do_register(ent, cmd, issue_flags);
 }
 
 /*
-- 
2.52.0

Re: [PATCH v1 2/3] fuse: fix race between registration and connection abortion

[Bernd Schubert]

On 5/15/26 06:55, Joanne Koong wrote:
> This fixes this race:
> - thread a: io_uring_enter -> register sqe ->
>   fuse_uring_create_ring_ent -> allocate ent but doesn't grab queue_ref
>   yet
> - thread b: fuse_conn_destroy() -> fuse_chan_abort() ->
>   fuse_uring_abort() is a no-op due to queue ref being 0
> - thread a: grabs the queue_ref, queue_ref is now 1, rest of
>   fuse_uring_do_register() logic executes
> - thread b: fuse_chan_abort() returns, fuse_chan_wait_aborted() now runs
>   and calls
>   "wait_event(ring->stop_waitq, atomic_read(&ring->queue_refs) == 0);"
> The abort/unmount thread will hang indefinitely in unkillable state as
> nothing will decrement queue_refs or wake stop_waitq, and the ring,
> queue, and ent are leaked.
> 
> Fix this by checking fch->connected under fch->lock after the created
> ent has grabbed a ref count on the queue. This ensures that in the
> scenario above, it is guaranteed that we either release the queue ref
> and wake up stop_waitq (in case fuse_chan_wait_aborted() is already
> waiting) in fuse_uring_do_register() when we detect !fch->connected, or
> if the connection is aborted after the check, it is guaranteed that the
> async teardown worker will be running in the background cleaning up ents
> and decrementing the ent's ref on the queue, which will unblock the
> eventual queue and ring teardown.
> 
> Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/dev_uring.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index cd75f61018ec..d9108b5b5db8 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -977,15 +977,26 @@ static bool is_ring_ready(struct fuse_ring *ring, int current_qid)
>  /*
>   * fuse_uring_req_fetch command handling
>   */
> -static void fuse_uring_do_register(struct fuse_ring_ent *ent,
> -				   struct io_uring_cmd *cmd,
> -				   unsigned int issue_flags)
> +static int fuse_uring_do_register(struct fuse_ring_ent *ent,
> +				  struct io_uring_cmd *cmd,
> +				  unsigned int issue_flags)
>  {
>  	struct fuse_ring_queue *queue = ent->queue;
>  	struct fuse_ring *ring = queue->ring;
>  	struct fuse_chan *fch = ring->chan;
>  	struct fuse_iqueue *fiq = &fch->iq;
>  
> +	spin_lock(&fch->lock);
> +	/* abort teardown path is running or has run */
> +	if (!fch->connected) {
> +		spin_unlock(&fch->lock);
> +		if (atomic_dec_and_test(&ring->queue_refs))
> +			wake_up_all(&ring->stop_waitq);
> +		kfree(ent);
> +		return -ECONNABORTED;
> +	}
> +	spin_unlock(&fch->lock);
> +
>  	fuse_uring_prepare_cancel(cmd, issue_flags, ent);
>  
>  	spin_lock(&queue->lock);
> @@ -1002,6 +1013,7 @@ static void fuse_uring_do_register(struct fuse_ring_ent *ent,
>  			wake_up_all(&fch->blocked_waitq);
>  		}
>  	}
> +	return 0;
>  }
>  
>  /*
> @@ -1118,9 +1130,7 @@ static int fuse_uring_register(struct io_uring_cmd *cmd,
>  	if (IS_ERR(ent))
>  		return PTR_ERR(ent);
>  
> -	fuse_uring_do_register(ent, cmd, issue_flags);
> -
> -	return 0;
> +	return fuse_uring_do_register(ent, cmd, issue_flags);
>  }
>  
>  /*

Reviewed-by: Bernd Schubert <bernd@bsbernd.com>

[PATCH v1 3/3] fuse: fix moving cancelled entry to ent_in_userspace list

[Joanne Koong]

fuse_uring_cancel() moves entries that are available (these have no reqs
attached) to the ent_in_userspace list. ent_list_request_expired()
checks the first entry on ent_in_userspace and dereferences
ent->fuse_req unconditionally, which will crash on a cancelled entry
that was moved to this list.

Fix this by freeing the entry and dropping queue_refs directly in
fuse_uring_cancel(). This is safe because cancel is the cancel handler
itself - after io_uring_cmd_done(), no more cancels will be dispatched
for this command, and teardown serializes with cancel via queue->lock.

Since cancel now decrements queue_refs, fuse_uring_abort() must no
longer gate fuse_uring_abort_end_requests() on queue_refs > 0, as
cancelled entries may have already dropped queue_refs while requests are
still queued. Remove the gate so abort always flushes requests and stops
queues.

Reported-by: Heechan Kang <gganji11@naver.com>
Fixes: 4fea593e625c ("fuse: optimize over-io-uring request expiration check")
Cc: stable@vger.kernel.org
Co-developed-by: Jian Huang Li <ali@ddn.com>
Co-developed-by: Horst Birthelmer <horst@birthelmer.de>
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev_uring.c   | 6 ++++--
 fs/fuse/dev_uring_i.h | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index d9108b5b5db8..f4ba64a1796a 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -511,8 +511,7 @@ static void fuse_uring_cancel(struct io_uring_cmd *cmd,
 	queue = ent->queue;
 	spin_lock(&queue->lock);
 	if (ent->state == FRRS_AVAILABLE) {
-		ent->state = FRRS_USERSPACE;
-		list_move_tail(&ent->list, &queue->ent_in_userspace);
+		list_del_init(&ent->list);
 		need_cmd_done = true;
 		ent->cmd = NULL;
 	}
@@ -521,6 +520,9 @@ static void fuse_uring_cancel(struct io_uring_cmd *cmd,
 	if (need_cmd_done) {
 		/* no queue lock to avoid lock order issues */
 		io_uring_cmd_done(cmd, -ENOTCONN, issue_flags);
+		kfree(ent);
+		if (atomic_dec_and_test(&queue->ring->queue_refs))
+			wake_up_all(&queue->ring->stop_waitq);
 	}
 }
 
diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h
index 368f4d0790eb..22ec67e39ee0 100644
--- a/fs/fuse/dev_uring_i.h
+++ b/fs/fuse/dev_uring_i.h
@@ -150,10 +150,10 @@ static inline void fuse_uring_abort(struct fuse_chan *fch)
 	if (ring == NULL)
 		return;
 
-	if (atomic_read(&ring->queue_refs) > 0) {
-		fuse_uring_abort_end_requests(ring);
+	fuse_uring_abort_end_requests(ring);
+
+	if (atomic_read(&ring->queue_refs) > 0)
 		fuse_uring_stop_queues(ring);
-	}
 }
 
 static inline void fuse_uring_wait_stopped_queues(struct fuse_chan *fch)
-- 
2.52.0

Re: [PATCH v1 3/3] fuse: fix moving cancelled entry to ent_in_userspace list

[Bernd Schubert]

On 5/15/26 06:55, Joanne Koong wrote:
> fuse_uring_cancel() moves entries that are available (these have no reqs
> attached) to the ent_in_userspace list. ent_list_request_expired()
> checks the first entry on ent_in_userspace and dereferences
> ent->fuse_req unconditionally, which will crash on a cancelled entry
> that was moved to this list.
> 
> Fix this by freeing the entry and dropping queue_refs directly in
> fuse_uring_cancel(). This is safe because cancel is the cancel handler
> itself - after io_uring_cmd_done(), no more cancels will be dispatched
> for this command, and teardown serializes with cancel via queue->lock.
> 
> Since cancel now decrements queue_refs, fuse_uring_abort() must no
> longer gate fuse_uring_abort_end_requests() on queue_refs > 0, as
> cancelled entries may have already dropped queue_refs while requests are
> still queued. Remove the gate so abort always flushes requests and stops
> queues.
> 
> Reported-by: Heechan Kang <gganji11@naver.com>
> Fixes: 4fea593e625c ("fuse: optimize over-io-uring request expiration check")
> Cc: stable@vger.kernel.org
> Co-developed-by: Jian Huang Li <ali@ddn.com>
> Co-developed-by: Horst Birthelmer <horst@birthelmer.de>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/dev_uring.c   | 6 ++++--
>  fs/fuse/dev_uring_i.h | 6 +++---
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index d9108b5b5db8..f4ba64a1796a 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -511,8 +511,7 @@ static void fuse_uring_cancel(struct io_uring_cmd *cmd,
>  	queue = ent->queue;
>  	spin_lock(&queue->lock);
>  	if (ent->state == FRRS_AVAILABLE) {
> -		ent->state = FRRS_USERSPACE;
> -		list_move_tail(&ent->list, &queue->ent_in_userspace);
> +		list_del_init(&ent->list);
>  		need_cmd_done = true;
>  		ent->cmd = NULL;
>  	}
> @@ -521,6 +520,9 @@ static void fuse_uring_cancel(struct io_uring_cmd *cmd,
>  	if (need_cmd_done) {
>  		/* no queue lock to avoid lock order issues */
>  		io_uring_cmd_done(cmd, -ENOTCONN, issue_flags);
> +		kfree(ent);
> +		if (atomic_dec_and_test(&queue->ring->queue_refs))
> +			wake_up_all(&queue->ring->stop_waitq);
>  	}
>  }

Hmm, ok, I had done that via fuse_uring_entry_teardown(), but this way
is also fine.

While thinking about it over night, I wonder if we should abort the
connection here. Calls for fuse_uring_cancel() / IO_URING_F_CANCEL
happen when

a) The daemon dies - that is what I had written the function for
b) When one calls

With reduced rings queues we would actually need to have per queue refs
and if a single queue reaches 0, it would need to re-calculate the
queue. In general this gets complex and from my point of view, if
fuse-server wants to re-initialize queues, fuse-server should:

a) wake up the ring thread with an eventfd (libfuse already has that)
b) we need a reconfig SQE (like FUSE_IO_URING_CMD_RECONFIG) that
requests to re-configure things

Right now that is all not supported, from my point of view we should
call fuse_abort_conn() when we call into fuse_uring_cancel()


Thanks,
Bernd

>  
> diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h
> index 368f4d0790eb..22ec67e39ee0 100644
> --- a/fs/fuse/dev_uring_i.h
> +++ b/fs/fuse/dev_uring_i.h
> @@ -150,10 +150,10 @@ static inline void fuse_uring_abort(struct fuse_chan *fch)
>  	if (ring == NULL)
>  		return;
>  
> -	if (atomic_read(&ring->queue_refs) > 0) {
> -		fuse_uring_abort_end_requests(ring);
> +	fuse_uring_abort_end_requests(ring);
> +
> +	if (atomic_read(&ring->queue_refs) > 0)
>  		fuse_uring_stop_queues(ring);
> -	}
>  }
>  
>  static inline void fuse_uring_wait_stopped_queues(struct fuse_chan *fch)