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

[Bernd Schubert <bernd@bsbernd.com>]

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