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

[Bernd Schubert <bernd@bsbernd.com>]

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