[PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Jian Huang Li]

This issue could be observed sometimes during libfuse xfstests, from
dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".

The cause is, if when fuse daemon just submitted
FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
this very early stage. After all uring queues stopped, might have one or
more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
new ring entities are created and added to ent_avail_queue, and
immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
get canceled. These ring entities will not be moved to ent_released, and
will stay in ent_in_userspace when fuse_uring_destruct is called, needed
be freed by the function.

Fixes: b6236c8407cb ("fuse: {io-uring} Prevent mount point hang on 
fuse-server termination")
Signed-off-by: Jian Huang Li <ali@ddn.com>
---
  fs/fuse/dev_uring.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index 249b210becb1..eed0fc6c8b05 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -201,7 +201,20 @@ void fuse_uring_destruct(struct fuse_conn *fc)
  		WARN_ON(!list_empty(&queue->ent_avail_queue));
  		WARN_ON(!list_empty(&queue->ent_w_req_queue));
  		WARN_ON(!list_empty(&queue->ent_commit_queue));
-		WARN_ON(!list_empty(&queue->ent_in_userspace));
+
+		/*
+		 * ent_in_userspace might not be empty, because
+		 * FUSE_IO_URING_CMD_REGISTER is not accounted yet
+		 * in ring->queue_refs and fuse_uring_wait_stopped_queues()
+		 * then passes too early. fuse_uring_cancel() adds these
+		 * commands to queue->ent_in_userspace - they need
+		 * to be freed here
+		 */
+		list_for_each_entry_safe(ent, next, &queue->ent_in_userspace,
+					 list) {
+			list_del_init(&ent->list);
+			kfree(ent);
+		}

  		list_for_each_entry_safe(ent, next, &queue->ent_released,
  					 list) {
-- 
2.47.1

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Jian Huang Li]

Changelog
---------
v1: 
https://lore.kernel.org/all/4a599306-5ef1-4531-b733-4984d09b97a1@ddn.com/
v1 -> v2:
* Instead of introducing a new list, keep to use ent_in_userspace to
   handle not-in-using entities when SQEs get canceled, and
   iterate/free on the ent_in_userspace list in fuse_uring_destruct.

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Joanne Koong]

On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
>
> This issue could be observed sometimes during libfuse xfstests, from
> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
>
> The cause is, if when fuse daemon just submitted
> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
> this very early stage. After all uring queues stopped, might have one or
> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
> new ring entities are created and added to ent_avail_queue, and
> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
> get canceled. These ring entities will not be moved to ent_released, and
> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
> be freed by the function.

Hi Jian,

Does it suffice to fix this race by tearing down the entries from the
available queue first before tearing down the entries in the userspace
queue? eg something like

 static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
 {
-       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
-                                    FRRS_USERSPACE);
        fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
                                     FRRS_AVAILABLE);
+       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
+                                    FRRS_USERSPACE);
 }

AFAICT, the race happens right now because when fuse_uring_cancel()
moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
ent_in_userspace queue, fuse_uring_teardown_entries() may have already
called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
thereby now missing the just-moved entries altogether, eg this logical
flow

-> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
    -> fuse_uring_cancel() moves entry from avail q to userspace q
-> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);

If instead fuse_uring_teardown_entries() stops the available queue first, then
-> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
    -> fuse_uring_cancel()
-> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);

seems fine now and fuse_uring_cancel() would basically be a no-op
since ent->state is now FRRS_TEARDOWN.


Thanks,
Joanne



>
> Fixes: b6236c8407cb ("fuse: {io-uring} Prevent mount point hang on
> fuse-server termination")
> Signed-off-by: Jian Huang Li <ali@ddn.com>
> ---
>   fs/fuse/dev_uring.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 249b210becb1..eed0fc6c8b05 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -201,7 +201,20 @@ void fuse_uring_destruct(struct fuse_conn *fc)
>                 WARN_ON(!list_empty(&queue->ent_avail_queue));
>                 WARN_ON(!list_empty(&queue->ent_w_req_queue));
>                 WARN_ON(!list_empty(&queue->ent_commit_queue));
> -               WARN_ON(!list_empty(&queue->ent_in_userspace));
> +
> +               /*
> +                * ent_in_userspace might not be empty, because
> +                * FUSE_IO_URING_CMD_REGISTER is not accounted yet
> +                * in ring->queue_refs and fuse_uring_wait_stopped_queues()
> +                * then passes too early. fuse_uring_cancel() adds these
> +                * commands to queue->ent_in_userspace - they need
> +                * to be freed here
> +                */
> +               list_for_each_entry_safe(ent, next, &queue->ent_in_userspace,
> +                                        list) {
> +                       list_del_init(&ent->list);
> +                       kfree(ent);
> +               }
>
>                 list_for_each_entry_safe(ent, next, &queue->ent_released,
>                                          list) {
> --
> 2.47.1
>

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Bernd Schubert]

Hi Joanne,

thanks for looking into this.

On 9/15/25 20:15, Joanne Koong wrote:
> On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
>>
>> This issue could be observed sometimes during libfuse xfstests, from
>> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
>> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
>>
>> The cause is, if when fuse daemon just submitted
>> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
>> this very early stage. After all uring queues stopped, might have one or
>> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
>> new ring entities are created and added to ent_avail_queue, and
>> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
>> get canceled. These ring entities will not be moved to ent_released, and
>> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
>> be freed by the function.
> 
> Hi Jian,
> 
> Does it suffice to fix this race by tearing down the entries from the
> available queue first before tearing down the entries in the userspace
> queue? eg something like
> 
>  static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
>  {
> -       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> -                                    FRRS_USERSPACE);
>         fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
>                                      FRRS_AVAILABLE);
> +       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> +                                    FRRS_USERSPACE);
>  }
> 
> AFAICT, the race happens right now because when fuse_uring_cancel()
> moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
> ent_in_userspace queue, fuse_uring_teardown_entries() may have already
> called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
> thereby now missing the just-moved entries altogether, eg this logical
> flow
> 
> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
>     -> fuse_uring_cancel() moves entry from avail q to userspace q
> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
> 
> If instead fuse_uring_teardown_entries() stops the available queue first, then
> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
>     -> fuse_uring_cancel()
> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
> 
> seems fine now and fuse_uring_cancel() would basically be a no-op
> since ent->state is now FRRS_TEARDOWN.
> 

I'm not sure. Let's say we have

task 1                                   task2
fuse_uring_cmd()
    fuse_uring_register()
         [slowness here]
					fuse_abort_conn()
                                          fuse_uring_teardown_entries()
	 [slowness continue]
         fuse_uring_do_register()
            fuse_uring_prepare_cancel()
            fuse_uring_ent_avail()


I.e. fuse_uring_teardown_entries() might be called before
the command gets marked cancel-able and before it is
moved to the avail queue. I think we should extend the patch
and actually not set the ring to ready when fc->connected
is set to 0.


Thanks,
Bernd

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Joanne Koong]

On Mon, Sep 15, 2025 at 1:15 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>
> Hi Joanne,
>
> thanks for looking into this.
>
> On 9/15/25 20:15, Joanne Koong wrote:
> > On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
> >>
> >> This issue could be observed sometimes during libfuse xfstests, from
> >> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
> >> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
> >>
> >> The cause is, if when fuse daemon just submitted
> >> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
> >> this very early stage. After all uring queues stopped, might have one or
> >> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
> >> new ring entities are created and added to ent_avail_queue, and
> >> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
> >> get canceled. These ring entities will not be moved to ent_released, and
> >> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
> >> be freed by the function.
> >
> > Hi Jian,
> >
> > Does it suffice to fix this race by tearing down the entries from the
> > available queue first before tearing down the entries in the userspace
> > queue? eg something like
> >
> >  static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
> >  {
> > -       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> > -                                    FRRS_USERSPACE);
> >         fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
> >                                      FRRS_AVAILABLE);
> > +       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> > +                                    FRRS_USERSPACE);
> >  }
> >
> > AFAICT, the race happens right now because when fuse_uring_cancel()
> > moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
> > ent_in_userspace queue, fuse_uring_teardown_entries() may have already
> > called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
> > thereby now missing the just-moved entries altogether, eg this logical
> > flow
> >
> > -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
> >     -> fuse_uring_cancel() moves entry from avail q to userspace q
> > -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
> >
> > If instead fuse_uring_teardown_entries() stops the available queue first, then
> > -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
> >     -> fuse_uring_cancel()
> > -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
> >
> > seems fine now and fuse_uring_cancel() would basically be a no-op
> > since ent->state is now FRRS_TEARDOWN.
> >
>
> I'm not sure. Let's say we have
>
> task 1                                   task2
> fuse_uring_cmd()
>     fuse_uring_register()
>          [slowness here]
>                                         fuse_abort_conn()
>                                           fuse_uring_teardown_entries()
>          [slowness continue]
>          fuse_uring_do_register()
>             fuse_uring_prepare_cancel()
>             fuse_uring_ent_avail()
>
>
> I.e. fuse_uring_teardown_entries() might be called before
> the command gets marked cancel-able and before it is
> moved to the avail queue. I think we should extend the patch
> and actually not set the ring to ready when fc->connected
> is set to 0.
>

Hi Bernd,

I think this is a separate race from the fuse_uring_cancel one.
afaics, this race can happen even if the user doesn't call
fuse_uring_cancel(). imo I think the cleanest solution to this
registration vs teardown race is to check queue->stopped in
fuse_uring_do_register() after we grab the queue spinlock, and if
queue->stopped is true, then just clean up the entry ourselves with
fuse_uring_entry_teardown()).

Thanks,
Joanne

>
> Thanks,
> Bernd

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Bernd Schubert]

On 9/15/25 23:23, Joanne Koong wrote:
> On Mon, Sep 15, 2025 at 1:15 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>>
>> Hi Joanne,
>>
>> thanks for looking into this.
>>
>> On 9/15/25 20:15, Joanne Koong wrote:
>>> On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
>>>>
>>>> This issue could be observed sometimes during libfuse xfstests, from
>>>> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
>>>> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
>>>>
>>>> The cause is, if when fuse daemon just submitted
>>>> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
>>>> this very early stage. After all uring queues stopped, might have one or
>>>> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
>>>> new ring entities are created and added to ent_avail_queue, and
>>>> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
>>>> get canceled. These ring entities will not be moved to ent_released, and
>>>> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
>>>> be freed by the function.
>>>
>>> Hi Jian,
>>>
>>> Does it suffice to fix this race by tearing down the entries from the
>>> available queue first before tearing down the entries in the userspace
>>> queue? eg something like
>>>
>>>  static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
>>>  {
>>> -       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
>>> -                                    FRRS_USERSPACE);
>>>         fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
>>>                                      FRRS_AVAILABLE);
>>> +       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
>>> +                                    FRRS_USERSPACE);
>>>  }
>>>
>>> AFAICT, the race happens right now because when fuse_uring_cancel()
>>> moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
>>> ent_in_userspace queue, fuse_uring_teardown_entries() may have already
>>> called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
>>> thereby now missing the just-moved entries altogether, eg this logical
>>> flow
>>>
>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
>>>     -> fuse_uring_cancel() moves entry from avail q to userspace q
>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
>>>
>>> If instead fuse_uring_teardown_entries() stops the available queue first, then
>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
>>>     -> fuse_uring_cancel()
>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
>>>
>>> seems fine now and fuse_uring_cancel() would basically be a no-op
>>> since ent->state is now FRRS_TEARDOWN.
>>>
>>
>> I'm not sure. Let's say we have
>>
>> task 1                                   task2
>> fuse_uring_cmd()
>>     fuse_uring_register()
>>          [slowness here]
>>                                         fuse_abort_conn()
>>                                           fuse_uring_teardown_entries()
>>          [slowness continue]
>>          fuse_uring_do_register()
>>             fuse_uring_prepare_cancel()
>>             fuse_uring_ent_avail()
>>
>>
>> I.e. fuse_uring_teardown_entries() might be called before
>> the command gets marked cancel-able and before it is
>> moved to the avail queue. I think we should extend the patch
>> and actually not set the ring to ready when fc->connected
>> is set to 0.
>>
> 
> Hi Bernd,
> 
> I think this is a separate race from the fuse_uring_cancel one.
> afaics, this race can happen even if the user doesn't call
> fuse_uring_cancel(). imo I think the cleanest solution to this
> registration vs teardown race is to check queue->stopped in
> fuse_uring_do_register() after we grab the queue spinlock, and if
> queue->stopped is true, then just clean up the entry ourselves with
> fuse_uring_entry_teardown()).

What speaks against just doing as in the existing patch and freeing
the ent_in_userspace entries fuse_uring_destruct()? 
IMO it covers both races, missing is just to avoid setting the ring
as ready.


Thanks,
Bernd

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Bernd Schubert]

On 9/15/25 23:46, Bernd Schubert wrote:
> 
> 
> On 9/15/25 23:23, Joanne Koong wrote:
>> On Mon, Sep 15, 2025 at 1:15 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>>>
>>> Hi Joanne,
>>>
>>> thanks for looking into this.
>>>
>>> On 9/15/25 20:15, Joanne Koong wrote:
>>>> On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
>>>>>
>>>>> This issue could be observed sometimes during libfuse xfstests, from
>>>>> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
>>>>> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
>>>>>
>>>>> The cause is, if when fuse daemon just submitted
>>>>> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
>>>>> this very early stage. After all uring queues stopped, might have one or
>>>>> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
>>>>> new ring entities are created and added to ent_avail_queue, and
>>>>> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
>>>>> get canceled. These ring entities will not be moved to ent_released, and
>>>>> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
>>>>> be freed by the function.
>>>>
>>>> Hi Jian,
>>>>
>>>> Does it suffice to fix this race by tearing down the entries from the
>>>> available queue first before tearing down the entries in the userspace
>>>> queue? eg something like
>>>>
>>>>  static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
>>>>  {
>>>> -       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
>>>> -                                    FRRS_USERSPACE);
>>>>         fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
>>>>                                      FRRS_AVAILABLE);
>>>> +       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
>>>> +                                    FRRS_USERSPACE);
>>>>  }
>>>>
>>>> AFAICT, the race happens right now because when fuse_uring_cancel()
>>>> moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
>>>> ent_in_userspace queue, fuse_uring_teardown_entries() may have already
>>>> called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
>>>> thereby now missing the just-moved entries altogether, eg this logical
>>>> flow
>>>>
>>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
>>>>     -> fuse_uring_cancel() moves entry from avail q to userspace q
>>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
>>>>
>>>> If instead fuse_uring_teardown_entries() stops the available queue first, then
>>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
>>>>     -> fuse_uring_cancel()
>>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
>>>>
>>>> seems fine now and fuse_uring_cancel() would basically be a no-op
>>>> since ent->state is now FRRS_TEARDOWN.
>>>>
>>>
>>> I'm not sure. Let's say we have
>>>
>>> task 1                                   task2
>>> fuse_uring_cmd()
>>>     fuse_uring_register()
>>>          [slowness here]
>>>                                         fuse_abort_conn()
>>>                                           fuse_uring_teardown_entries()
>>>          [slowness continue]
>>>          fuse_uring_do_register()
>>>             fuse_uring_prepare_cancel()
>>>             fuse_uring_ent_avail()
>>>
>>>
>>> I.e. fuse_uring_teardown_entries() might be called before
>>> the command gets marked cancel-able and before it is
>>> moved to the avail queue. I think we should extend the patch
>>> and actually not set the ring to ready when fc->connected
>>> is set to 0.
>>>
>>
>> Hi Bernd,
>>
>> I think this is a separate race from the fuse_uring_cancel one.
>> afaics, this race can happen even if the user doesn't call
>> fuse_uring_cancel(). imo I think the cleanest solution to this
>> registration vs teardown race is to check queue->stopped in
>> fuse_uring_do_register() after we grab the queue spinlock, and if
>> queue->stopped is true, then just clean up the entry ourselves with
>> fuse_uring_entry_teardown()).
> 
> What speaks against just doing as in the existing patch and freeing
> the ent_in_userspace entries fuse_uring_destruct()? 
> IMO it covers both races, missing is just to avoid setting the ring
> as ready.

Well, maybe cleaner, I don't have a strong opinion. We could skip the
comment and explanation with your approach.


Thanks,
Bernd

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Joanne Koong]

On Mon, Sep 15, 2025 at 2:57 PM Bernd Schubert <bernd@bsbernd.com> wrote:
> On 9/15/25 23:46, Bernd Schubert wrote:
> > On 9/15/25 23:23, Joanne Koong wrote:
> >> On Mon, Sep 15, 2025 at 1:15 PM Bernd Schubert <bernd@bsbernd.com> wrote:
> >>>
> >>> Hi Joanne,
> >>>
> >>> thanks for looking into this.
> >>>
> >>> On 9/15/25 20:15, Joanne Koong wrote:
> >>>> On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
> >>>>>
> >>>>> This issue could be observed sometimes during libfuse xfstests, from
> >>>>> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
> >>>>> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
> >>>>>
> >>>>> The cause is, if when fuse daemon just submitted
> >>>>> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
> >>>>> this very early stage. After all uring queues stopped, might have one or
> >>>>> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
> >>>>> new ring entities are created and added to ent_avail_queue, and
> >>>>> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
> >>>>> get canceled. These ring entities will not be moved to ent_released, and
> >>>>> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
> >>>>> be freed by the function.
> >>>>
> >>>> Hi Jian,
> >>>>
> >>>> Does it suffice to fix this race by tearing down the entries from the
> >>>> available queue first before tearing down the entries in the userspace
> >>>> queue? eg something like
> >>>>
> >>>>  static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
> >>>>  {
> >>>> -       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> >>>> -                                    FRRS_USERSPACE);
> >>>>         fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
> >>>>                                      FRRS_AVAILABLE);
> >>>> +       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> >>>> +                                    FRRS_USERSPACE);
> >>>>  }
> >>>>
> >>>> AFAICT, the race happens right now because when fuse_uring_cancel()
> >>>> moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
> >>>> ent_in_userspace queue, fuse_uring_teardown_entries() may have already
> >>>> called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
> >>>> thereby now missing the just-moved entries altogether, eg this logical
> >>>> flow
> >>>>
> >>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
> >>>>     -> fuse_uring_cancel() moves entry from avail q to userspace q
> >>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
> >>>>
> >>>> If instead fuse_uring_teardown_entries() stops the available queue first, then
> >>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
> >>>>     -> fuse_uring_cancel()
> >>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
> >>>>
> >>>> seems fine now and fuse_uring_cancel() would basically be a no-op
> >>>> since ent->state is now FRRS_TEARDOWN.
> >>>>
> >>>
> >>> I'm not sure. Let's say we have
> >>>
> >>> task 1                                   task2
> >>> fuse_uring_cmd()
> >>>     fuse_uring_register()
> >>>          [slowness here]
> >>>                                         fuse_abort_conn()
> >>>                                           fuse_uring_teardown_entries()
> >>>          [slowness continue]
> >>>          fuse_uring_do_register()
> >>>             fuse_uring_prepare_cancel()
> >>>             fuse_uring_ent_avail()
> >>>
> >>>
> >>> I.e. fuse_uring_teardown_entries() might be called before
> >>> the command gets marked cancel-able and before it is
> >>> moved to the avail queue. I think we should extend the patch
> >>> and actually not set the ring to ready when fc->connected
> >>> is set to 0.
> >>>
> >>
> >> Hi Bernd,
> >>
> >> I think this is a separate race from the fuse_uring_cancel one.
> >> afaics, this race can happen even if the user doesn't call
> >> fuse_uring_cancel(). imo I think the cleanest solution to this
> >> registration vs teardown race is to check queue->stopped in
> >> fuse_uring_do_register() after we grab the queue spinlock, and if
> >> queue->stopped is true, then just clean up the entry ourselves with
> >> fuse_uring_entry_teardown()).
> >
> > What speaks against just doing as in the existing patch and freeing
> > the ent_in_userspace entries fuse_uring_destruct()?
> > IMO it covers both races, missing is just to avoid setting the ring
> > as ready.

Couldn't the entry in fuse_uring_do_register() be in the available
queue when we get to fuse_uring_destruct() which means the existing
patch would also have to iterate through the available queue too? eg
i'm imagining something like

fuse_uring_do_register()
   -> fuse_uring_prepare_cancel()
         *** fuse_uring_cancel() + teardown run on other threads
   -> fuse_uring_ent_avail()

imo, I think this scenario is its own separate race (between
registration and cancellation, that can happen irregardless of
teardown) that should be fixed by calling fuse_uring_prepare_cancel()
only after the fuse_uring_ent_avail() call, but I think this
underscores a bit that explicitly checking against torn down entries
is more robust when dealing with these races.

>
> Well, maybe cleaner, I don't have a strong opinion. We could skip the
> comment and explanation with your approach.

I don't really have a strong opinion on this either, just wanted to
share my thoughts. If you'd rather go with the existing patch, then we
should do that.

Thanks,
Joanne

>
>
> Thanks,
> Bernd

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Bernd Schubert]

On 9/16/25 01:04, Joanne Koong wrote:
> On Mon, Sep 15, 2025 at 2:57 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>> On 9/15/25 23:46, Bernd Schubert wrote:
>>> On 9/15/25 23:23, Joanne Koong wrote:
>>>> On Mon, Sep 15, 2025 at 1:15 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>>>>>
>>>>> Hi Joanne,
>>>>>
>>>>> thanks for looking into this.
>>>>>
>>>>> On 9/15/25 20:15, Joanne Koong wrote:
>>>>>> On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
>>>>>>>
>>>>>>> This issue could be observed sometimes during libfuse xfstests, from
>>>>>>> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
>>>>>>> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
>>>>>>>
>>>>>>> The cause is, if when fuse daemon just submitted
>>>>>>> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
>>>>>>> this very early stage. After all uring queues stopped, might have one or
>>>>>>> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
>>>>>>> new ring entities are created and added to ent_avail_queue, and
>>>>>>> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
>>>>>>> get canceled. These ring entities will not be moved to ent_released, and
>>>>>>> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
>>>>>>> be freed by the function.
>>>>>>
>>>>>> Hi Jian,
>>>>>>
>>>>>> Does it suffice to fix this race by tearing down the entries from the
>>>>>> available queue first before tearing down the entries in the userspace
>>>>>> queue? eg something like
>>>>>>
>>>>>>  static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
>>>>>>  {
>>>>>> -       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
>>>>>> -                                    FRRS_USERSPACE);
>>>>>>         fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
>>>>>>                                      FRRS_AVAILABLE);
>>>>>> +       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
>>>>>> +                                    FRRS_USERSPACE);
>>>>>>  }
>>>>>>
>>>>>> AFAICT, the race happens right now because when fuse_uring_cancel()
>>>>>> moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
>>>>>> ent_in_userspace queue, fuse_uring_teardown_entries() may have already
>>>>>> called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
>>>>>> thereby now missing the just-moved entries altogether, eg this logical
>>>>>> flow
>>>>>>
>>>>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
>>>>>>     -> fuse_uring_cancel() moves entry from avail q to userspace q
>>>>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
>>>>>>
>>>>>> If instead fuse_uring_teardown_entries() stops the available queue first, then
>>>>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
>>>>>>     -> fuse_uring_cancel()
>>>>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
>>>>>>
>>>>>> seems fine now and fuse_uring_cancel() would basically be a no-op
>>>>>> since ent->state is now FRRS_TEARDOWN.
>>>>>>
>>>>>
>>>>> I'm not sure. Let's say we have
>>>>>
>>>>> task 1                                   task2
>>>>> fuse_uring_cmd()
>>>>>     fuse_uring_register()
>>>>>          [slowness here]
>>>>>                                         fuse_abort_conn()
>>>>>                                           fuse_uring_teardown_entries()
>>>>>          [slowness continue]
>>>>>          fuse_uring_do_register()
>>>>>             fuse_uring_prepare_cancel()
>>>>>             fuse_uring_ent_avail()
>>>>>
>>>>>
>>>>> I.e. fuse_uring_teardown_entries() might be called before
>>>>> the command gets marked cancel-able and before it is
>>>>> moved to the avail queue. I think we should extend the patch
>>>>> and actually not set the ring to ready when fc->connected
>>>>> is set to 0.
>>>>>
>>>>
>>>> Hi Bernd,
>>>>
>>>> I think this is a separate race from the fuse_uring_cancel one.
>>>> afaics, this race can happen even if the user doesn't call
>>>> fuse_uring_cancel(). imo I think the cleanest solution to this
>>>> registration vs teardown race is to check queue->stopped in
>>>> fuse_uring_do_register() after we grab the queue spinlock, and if
>>>> queue->stopped is true, then just clean up the entry ourselves with
>>>> fuse_uring_entry_teardown()).
>>>
>>> What speaks against just doing as in the existing patch and freeing
>>> the ent_in_userspace entries fuse_uring_destruct()?
>>> IMO it covers both races, missing is just to avoid setting the ring
>>> as ready.
> 
> Couldn't the entry in fuse_uring_do_register() be in the available
> queue when we get to fuse_uring_destruct() which means the existing

We would never go into fuse_uring_destruct(), because io-uring would
still hold references on the fuse device / fuse_conn.

> patch would also have to iterate through the available queue too? eg
> i'm imagining something like
> 
> fuse_uring_do_register()
>    -> fuse_uring_prepare_cancel()
>          *** fuse_uring_cancel() + teardown run on other threads
>    -> fuse_uring_ent_avail()

Up to hear it is right, but then see io_ring_exit_work() in io_uring.c.
I.e. it will run io_uring_try_cancel_requests() in a loop.
fuse_uring_cancel() is a noop as long as ent->state != FRRS_AVAILABLE

If commands are not cancelled at all, io_ring_exit_work() will print
warnings in intervals - initially I hadn't figured out about
IO_URING_F_CANCEL (and maybe it also didn't exist in earlier kernel
versions) and then had added workarounds into fuse_dev_release(),
because of the io-uring references.

> 
> imo, I think this scenario is its own separate race (between
> registration and cancellation, that can happen irregardless of
> teardown) that should be fixed by calling fuse_uring_prepare_cancel()
> only after the fuse_uring_ent_avail() call, but I think this
> underscores a bit that explicitly checking against torn down entries
> is more robust when dealing with these races.
> 
>>
>> Well, maybe cleaner, I don't have a strong opinion. We could skip the
>> comment and explanation with your approach.
> 
> I don't really have a strong opinion on this either, just wanted to
> share my thoughts. If you'd rather go with the existing patch, then we
> should do that.


I still think Jians patch works as it is, although there is a bit magic
behind it. The mere fact that we have the discussion above it probably
reason enough to at least try to find a way to make easier to read.


Thanks,
Bernd

Re: [PATCH v2] fs/fuse: fix potential memory leak from fuse_uring_cancel

[Joanne Koong]

On Tue, Sep 16, 2025 at 2:17 AM Bernd Schubert <bernd@bsbernd.com> wrote:
>
> On 9/16/25 01:04, Joanne Koong wrote:
> > On Mon, Sep 15, 2025 at 2:57 PM Bernd Schubert <bernd@bsbernd.com> wrote:
> >> On 9/15/25 23:46, Bernd Schubert wrote:
> >>> On 9/15/25 23:23, Joanne Koong wrote:
> >>>> On Mon, Sep 15, 2025 at 1:15 PM Bernd Schubert <bernd@bsbernd.com> wrote:
> >>>>>
> >>>>> Hi Joanne,
> >>>>>
> >>>>> thanks for looking into this.
> >>>>>
> >>>>> On 9/15/25 20:15, Joanne Koong wrote:
> >>>>>> On Thu, Sep 11, 2025 at 3:34 AM Jian Huang Li <ali@ddn.com> wrote:
> >>>>>>>
> >>>>>>> This issue could be observed sometimes during libfuse xfstests, from
> >>>>>>> dmseg prints some like "kernel: WARNING: CPU: 4 PID: 0 at
> >>>>>>> fs/fuse/dev_uring.c:204 fuse_uring_destruct+0x1f5/0x200 [fuse]".
> >>>>>>>
> >>>>>>> The cause is, if when fuse daemon just submitted
> >>>>>>> FUSE_IO_URING_CMD_REGISTER SQEs, then umount or fuse daemon quits at
> >>>>>>> this very early stage. After all uring queues stopped, might have one or
> >>>>>>> more unprocessed FUSE_IO_URING_CMD_REGISTER SQEs get processed then some
> >>>>>>> new ring entities are created and added to ent_avail_queue, and
> >>>>>>> immediately fuse_uring_cancel moves them to ent_in_userspace after SQEs
> >>>>>>> get canceled. These ring entities will not be moved to ent_released, and
> >>>>>>> will stay in ent_in_userspace when fuse_uring_destruct is called, needed
> >>>>>>> be freed by the function.
> >>>>>>
> >>>>>> Hi Jian,
> >>>>>>
> >>>>>> Does it suffice to fix this race by tearing down the entries from the
> >>>>>> available queue first before tearing down the entries in the userspace
> >>>>>> queue? eg something like
> >>>>>>
> >>>>>>  static void fuse_uring_teardown_entries(struct fuse_ring_queue *queue)
> >>>>>>  {
> >>>>>> -       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> >>>>>> -                                    FRRS_USERSPACE);
> >>>>>>         fuse_uring_stop_list_entries(&queue->ent_avail_queue, queue,
> >>>>>>                                      FRRS_AVAILABLE);
> >>>>>> +       fuse_uring_stop_list_entries(&queue->ent_in_userspace, queue,
> >>>>>> +                                    FRRS_USERSPACE);
> >>>>>>  }
> >>>>>>
> >>>>>> AFAICT, the race happens right now because when fuse_uring_cancel()
> >>>>>> moves the FRRS_AVAILABLE entries on the ent_avail_queue to the
> >>>>>> ent_in_userspace queue, fuse_uring_teardown_entries() may have already
> >>>>>> called fuse_uring_stop_list_entries() on the ent_in_userspace queue,
> >>>>>> thereby now missing the just-moved entries altogether, eg this logical
> >>>>>> flow
> >>>>>>
> >>>>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
> >>>>>>     -> fuse_uring_cancel() moves entry from avail q to userspace q
> >>>>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
> >>>>>>
> >>>>>> If instead fuse_uring_teardown_entries() stops the available queue first, then
> >>>>>> -> fuse_uring_stop_list_entries(&queue->ent_avail_queue, ...);
> >>>>>>     -> fuse_uring_cancel()
> >>>>>> -> fuse_uring_stop_list_entries(&queue->ent_in_userspace, ...);
> >>>>>>
> >>>>>> seems fine now and fuse_uring_cancel() would basically be a no-op
> >>>>>> since ent->state is now FRRS_TEARDOWN.
> >>>>>>
> >>>>>
> >>>>> I'm not sure. Let's say we have
> >>>>>
> >>>>> task 1                                   task2
> >>>>> fuse_uring_cmd()
> >>>>>     fuse_uring_register()
> >>>>>          [slowness here]
> >>>>>                                         fuse_abort_conn()
> >>>>>                                           fuse_uring_teardown_entries()
> >>>>>          [slowness continue]
> >>>>>          fuse_uring_do_register()
> >>>>>             fuse_uring_prepare_cancel()
> >>>>>             fuse_uring_ent_avail()
> >>>>>
> >>>>>
> >>>>> I.e. fuse_uring_teardown_entries() might be called before
> >>>>> the command gets marked cancel-able and before it is
> >>>>> moved to the avail queue. I think we should extend the patch
> >>>>> and actually not set the ring to ready when fc->connected
> >>>>> is set to 0.
> >>>>>
> >>>>
> >>>> Hi Bernd,
> >>>>
> >>>> I think this is a separate race from the fuse_uring_cancel one.
> >>>> afaics, this race can happen even if the user doesn't call
> >>>> fuse_uring_cancel(). imo I think the cleanest solution to this
> >>>> registration vs teardown race is to check queue->stopped in
> >>>> fuse_uring_do_register() after we grab the queue spinlock, and if
> >>>> queue->stopped is true, then just clean up the entry ourselves with
> >>>> fuse_uring_entry_teardown()).
> >>>
> >>> What speaks against just doing as in the existing patch and freeing
> >>> the ent_in_userspace entries fuse_uring_destruct()?
> >>> IMO it covers both races, missing is just to avoid setting the ring
> >>> as ready.
> >
> > Couldn't the entry in fuse_uring_do_register() be in the available
> > queue when we get to fuse_uring_destruct() which means the existing
>
> We would never go into fuse_uring_destruct(), because io-uring would
> still hold references on the fuse device / fuse_conn.
>
> > patch would also have to iterate through the available queue too? eg
> > i'm imagining something like
> >
> > fuse_uring_do_register()
> >    -> fuse_uring_prepare_cancel()
> >          *** fuse_uring_cancel() + teardown run on other threads
> >    -> fuse_uring_ent_avail()
>
> Up to hear it is right, but then see io_ring_exit_work() in io_uring.c.
> I.e. it will run io_uring_try_cancel_requests() in a loop.
> fuse_uring_cancel() is a noop as long as ent->state != FRRS_AVAILABLE
>
> If commands are not cancelled at all, io_ring_exit_work() will print
> warnings in intervals - initially I hadn't figured out about
> IO_URING_F_CANCEL (and maybe it also didn't exist in earlier kernel
> versions) and then had added workarounds into fuse_dev_release(),
> because of the io-uring references.

I took a look at the io-uring code (thanks for the pointer to
io_ring_exit_work()) and now imo I think fuse_uring_cancel() should
tear down the entry directly instead of moving it to the userspace
queue. If I'm understanding it correctly, IO_URING_F_CANCEL is only
sent on io-uring or process teardown (eg when the last refcount on the
io-uring fd is dropped, or when a process exits) so the intended
behavior with the cancel is to tear down the entry. imo the cleanest
approach here is to just tear down the entry on cancel instead of
moving it to the userspace queue to have fuse_uring_teardown_entries()
try to handle it. Unless I'm missing some other reason it needs to be
moved to the userspace queue?

Thanks,
Joanne

>
> >
> > imo, I think this scenario is its own separate race (between
> > registration and cancellation, that can happen irregardless of
> > teardown) that should be fixed by calling fuse_uring_prepare_cancel()
> > only after the fuse_uring_ent_avail() call, but I think this
> > underscores a bit that explicitly checking against torn down entries
> > is more robust when dealing with these races.
> >
> >>
> >> Well, maybe cleaner, I don't have a strong opinion. We could skip the
> >> comment and explanation with your approach.
> >
> > I don't really have a strong opinion on this either, just wanted to
> > share my thoughts. If you'd rather go with the existing patch, then we
> > should do that.
>
>
> I still think Jians patch works as it is, although there is a bit magic
> behind it. The mere fact that we have the discussion above it probably
> reason enough to at least try to find a way to make easier to read.
>
>
> Thanks,
> Bernd