[PATCH] fuse: fix possible deadlock if rings are never initialized

[PATCH] fuse: fix possible deadlock if rings are never initialized

[Luis Henriques]

When mounting a user-space filesystem using io_uring, the initialization
of the rings is done separately in the server side.  If for some reason
(e.g. a server bug) this step is not performed it will be impossible to
unmount the filesystem if there are already requests waiting.

This issue is easily reproduced with the libfuse passthrough_ll example,
if the queue depth is set to '0' and a request is queued before trying to
unmount the filesystem.  When trying to force the unmount, fuse_abort_conn()
will try to wake up all tasks waiting in fc->blocked_waitq, but because the
rings were never initialized, fuse_uring_ready() will never return 'true'.

Fixes: 3393ff964e0f ("fuse: block request allocation until io-uring init is complete")
Signed-off-by: Luis Henriques <luis@igalia.com>
---
 fs/fuse/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 7edceecedfa5..2fe565e9b403 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -77,7 +77,7 @@ void fuse_set_initialized(struct fuse_conn *fc)
 static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
 {
 	return !fc->initialized || (for_background && fc->blocked) ||
-	       (fc->io_uring && !fuse_uring_ready(fc));
+	       (fc->io_uring && fc->connected && !fuse_uring_ready(fc));
 }
 
 static void fuse_drop_waiting(struct fuse_conn *fc)

Re: [PATCH] fuse: fix possible deadlock if rings are never initialized

[Bernd Schubert]

On 3/6/25 12:12, Luis Henriques wrote:
> When mounting a user-space filesystem using io_uring, the initialization
> of the rings is done separately in the server side.  If for some reason
> (e.g. a server bug) this step is not performed it will be impossible to
> unmount the filesystem if there are already requests waiting.
> 
> This issue is easily reproduced with the libfuse passthrough_ll example,
> if the queue depth is set to '0' and a request is queued before trying to
> unmount the filesystem.  When trying to force the unmount, fuse_abort_conn()
> will try to wake up all tasks waiting in fc->blocked_waitq, but because the
> rings were never initialized, fuse_uring_ready() will never return 'true'.
> 
> Fixes: 3393ff964e0f ("fuse: block request allocation until io-uring init is complete")
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
>  fs/fuse/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 7edceecedfa5..2fe565e9b403 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -77,7 +77,7 @@ void fuse_set_initialized(struct fuse_conn *fc)
>  static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
>  {
>  	return !fc->initialized || (for_background && fc->blocked) ||
> -	       (fc->io_uring && !fuse_uring_ready(fc));
> +	       (fc->io_uring && fc->connected && !fuse_uring_ready(fc));
>  }
>  
>  static void fuse_drop_waiting(struct fuse_conn *fc)
> 

Oh yes, I had missed that.

Reviewed-by: Bernd Schubert <bschubert@ddn.com>

Re: [PATCH] fuse: fix possible deadlock if rings are never initialized

[Luis Henriques]

On Thu, Mar 06 2025, Bernd Schubert wrote:

> On 3/6/25 12:12, Luis Henriques wrote:
>> When mounting a user-space filesystem using io_uring, the initialization
>> of the rings is done separately in the server side.  If for some reason
>> (e.g. a server bug) this step is not performed it will be impossible to
>> unmount the filesystem if there are already requests waiting.
>> 
>> This issue is easily reproduced with the libfuse passthrough_ll example,
>> if the queue depth is set to '0' and a request is queued before trying to
>> unmount the filesystem.  When trying to force the unmount, fuse_abort_conn()
>> will try to wake up all tasks waiting in fc->blocked_waitq, but because the
>> rings were never initialized, fuse_uring_ready() will never return 'true'.
>> 
>> Fixes: 3393ff964e0f ("fuse: block request allocation until io-uring init is complete")
>> Signed-off-by: Luis Henriques <luis@igalia.com>
>> ---
>>  fs/fuse/dev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index 7edceecedfa5..2fe565e9b403 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -77,7 +77,7 @@ void fuse_set_initialized(struct fuse_conn *fc)
>>  static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
>>  {
>>  	return !fc->initialized || (for_background && fc->blocked) ||
>> -	       (fc->io_uring && !fuse_uring_ready(fc));
>> +	       (fc->io_uring && fc->connected && !fuse_uring_ready(fc));
>>  }
>>  
>>  static void fuse_drop_waiting(struct fuse_conn *fc)
>> 
>
> Oh yes, I had missed that.
>
> Reviewed-by: Bernd Schubert <bschubert@ddn.com>

Thanks!  And... by the way, Bernd:

I know io_uring support in libfuse isn't ready yet, but I think there's
some error handling missing in your uring branch.  In particular, the
return of fuse_uring_start() is never checked, and thus if the rings
initialization fails, the server will not get any error.

I found that out because I blindly tried the patch below, and I was
surprised that the server was started just fine.

Cheers,
-- 
Luís

diff --git a/lib/fuse_uring.c b/lib/fuse_uring.c
index 312aa5dbc735..2258cf0d4259 100644
--- a/lib/fuse_uring.c
+++ b/lib/fuse_uring.c
@@ -498,6 +498,11 @@ static struct fuse_ring_pool *fuse_create_ring(struct fuse_session *se)
 		fuse_log(FUSE_LOG_DEBUG, "starting io-uring q-depth=%d\n",
 			 se->uring.q_depth);
 
+	if (!se->uring.q_depth) {
+		fuse_log(FUSE_LOG_ERR, "Invalid ring queue depth value\n");
+		goto err;
+	}
+
 	fuse_ring = calloc(1, sizeof(*fuse_ring));
 	if (fuse_ring == NULL) {
 		fuse_log(FUSE_LOG_ERR, "Allocating the ring failed\n");

Re: [PATCH] fuse: fix possible deadlock if rings are never initialized

[Bernd Schubert]

On 3/6/25 14:16, Luis Henriques wrote:
> On Thu, Mar 06 2025, Bernd Schubert wrote:
> 
>> On 3/6/25 12:12, Luis Henriques wrote:
>>> When mounting a user-space filesystem using io_uring, the initialization
>>> of the rings is done separately in the server side.  If for some reason
>>> (e.g. a server bug) this step is not performed it will be impossible to
>>> unmount the filesystem if there are already requests waiting.
>>>
>>> This issue is easily reproduced with the libfuse passthrough_ll example,
>>> if the queue depth is set to '0' and a request is queued before trying to
>>> unmount the filesystem.  When trying to force the unmount, fuse_abort_conn()
>>> will try to wake up all tasks waiting in fc->blocked_waitq, but because the
>>> rings were never initialized, fuse_uring_ready() will never return 'true'.
>>>
>>> Fixes: 3393ff964e0f ("fuse: block request allocation until io-uring init is complete")
>>> Signed-off-by: Luis Henriques <luis@igalia.com>
>>> ---
>>>   fs/fuse/dev.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>>> index 7edceecedfa5..2fe565e9b403 100644
>>> --- a/fs/fuse/dev.c
>>> +++ b/fs/fuse/dev.c
>>> @@ -77,7 +77,7 @@ void fuse_set_initialized(struct fuse_conn *fc)
>>>   static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
>>>   {
>>>   	return !fc->initialized || (for_background && fc->blocked) ||
>>> -	       (fc->io_uring && !fuse_uring_ready(fc));
>>> +	       (fc->io_uring && fc->connected && !fuse_uring_ready(fc));
>>>   }
>>>   
>>>   static void fuse_drop_waiting(struct fuse_conn *fc)
>>>
>>
>> Oh yes, I had missed that.
>>
>> Reviewed-by: Bernd Schubert <bschubert@ddn.com>
> 
> Thanks!  And... by the way, Bernd:
> 
> I know io_uring support in libfuse isn't ready yet, but I think there's
> some error handling missing in your uring branch.  In particular, the
> return of fuse_uring_start() is never checked, and thus if the rings
> initialization fails, the server will not get any error.
> 
> I found that out because I blindly tried the patch below, and I was
> surprised that the server was started just fine.

Thank you! I will work a bit on splitting the uring branch into 
merge-able patches later today, but probably won't finish today (too 
many other things to do).


Thanks,
Bernd

Re: [PATCH] fuse: fix possible deadlock if rings are never initialized

[Luis Henriques]

On Thu, Mar 06 2025, Luis Henriques wrote:

> When mounting a user-space filesystem using io_uring, the initialization
> of the rings is done separately in the server side.  If for some reason
> (e.g. a server bug) this step is not performed it will be impossible to
> unmount the filesystem if there are already requests waiting.
>
> This issue is easily reproduced with the libfuse passthrough_ll example,
> if the queue depth is set to '0' and a request is queued before trying to
> unmount the filesystem.  When trying to force the unmount, fuse_abort_conn()
> will try to wake up all tasks waiting in fc->blocked_waitq, but because the
> rings were never initialized, fuse_uring_ready() will never return 'true'.
>
> Fixes: 3393ff964e0f ("fuse: block request allocation until io-uring init is complete")
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
>  fs/fuse/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 7edceecedfa5..2fe565e9b403 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -77,7 +77,7 @@ void fuse_set_initialized(struct fuse_conn *fc)
>  static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
>  {
>  	return !fc->initialized || (for_background && fc->blocked) ||
> -	       (fc->io_uring && !fuse_uring_ready(fc));
> +	       (fc->io_uring && fc->connected && !fuse_uring_ready(fc));
>  }
>  
>  static void fuse_drop_waiting(struct fuse_conn *fc)
>

Gentle ping.  I was wondering if this would be worth picking before 6.14
is out.

Cheers,
-- 
Luís

Re: [PATCH] fuse: fix possible deadlock if rings are never initialized

[Miklos Szeredi]

On Thu, 6 Mar 2025 at 12:12, Luis Henriques <luis@igalia.com> wrote:
>
> When mounting a user-space filesystem using io_uring, the initialization
> of the rings is done separately in the server side.  If for some reason
> (e.g. a server bug) this step is not performed it will be impossible to
> unmount the filesystem if there are already requests waiting.
>
> This issue is easily reproduced with the libfuse passthrough_ll example,
> if the queue depth is set to '0' and a request is queued before trying to
> unmount the filesystem.  When trying to force the unmount, fuse_abort_conn()
> will try to wake up all tasks waiting in fc->blocked_waitq, but because the
> rings were never initialized, fuse_uring_ready() will never return 'true'.
>
> Fixes: 3393ff964e0f ("fuse: block request allocation until io-uring init is complete")
> Signed-off-by: Luis Henriques <luis@igalia.com>

Acked-by: Miklos Szeredi <mszeredi@redhat.com>

Christian, can you please pick this up as well for 6.14?

Thanks,
Miklos


> ---
>  fs/fuse/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 7edceecedfa5..2fe565e9b403 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -77,7 +77,7 @@ void fuse_set_initialized(struct fuse_conn *fc)
>  static bool fuse_block_alloc(struct fuse_conn *fc, bool for_background)
>  {
>         return !fc->initialized || (for_background && fc->blocked) ||
> -              (fc->io_uring && !fuse_uring_ready(fc));
> +              (fc->io_uring && fc->connected && !fuse_uring_ready(fc));
>  }
>
>  static void fuse_drop_waiting(struct fuse_conn *fc)

Re: [PATCH] fuse: fix possible deadlock if rings are never initialized

[Christian Brauner]

On Thu, 06 Mar 2025 11:12:18 +0000, Luis Henriques wrote:
> When mounting a user-space filesystem using io_uring, the initialization
> of the rings is done separately in the server side.  If for some reason
> (e.g. a server bug) this step is not performed it will be impossible to
> unmount the filesystem if there are already requests waiting.
> 
> This issue is easily reproduced with the libfuse passthrough_ll example,
> if the queue depth is set to '0' and a request is queued before trying to
> unmount the filesystem.  When trying to force the unmount, fuse_abort_conn()
> will try to wake up all tasks waiting in fc->blocked_waitq, but because the
> rings were never initialized, fuse_uring_ready() will never return 'true'.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fuse: fix possible deadlock if rings are never initialized
      https://git.kernel.org/vfs/vfs/c/d55011469b41