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

[Bernd Schubert <bernd@bsbernd.com>]

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