Re: [PATCH v1 3/3] fuse: fix moving cancelled entry to ent_in_userspace list

[Bernd Schubert <bernd@bsbernd.com>]

On 5/15/26 23:11, Joanne Koong wrote:
> On Fri, May 15, 2026 at 4:10 AM Bernd Schubert <bernd@bsbernd.com> wrote:
>>
>>
>> On 5/15/26 06:55, Joanne Koong wrote:
>>> fuse_uring_cancel() moves entries that are available (these have no reqs
>>> attached) to the ent_in_userspace list. ent_list_request_expired()
>>> checks the first entry on ent_in_userspace and dereferences
>>> ent->fuse_req unconditionally, which will crash on a cancelled entry
>>> that was moved to this list.
>>>
>>> Fix this by freeing the entry and dropping queue_refs directly in
>>> fuse_uring_cancel(). This is safe because cancel is the cancel handler
>>> itself - after io_uring_cmd_done(), no more cancels will be dispatched
>>> for this command, and teardown serializes with cancel via queue->lock.
>>>
>>> Since cancel now decrements queue_refs, fuse_uring_abort() must no
>>> longer gate fuse_uring_abort_end_requests() on queue_refs > 0, as
>>> cancelled entries may have already dropped queue_refs while requests are
>>> still queued. Remove the gate so abort always flushes requests and stops
>>> queues.
>>>
>>> Reported-by: Heechan Kang <gganji11@naver.com>
>>> Fixes: 4fea593e625c ("fuse: optimize over-io-uring request expiration check")
>>> Cc: stable@vger.kernel.org
>>> Co-developed-by: Jian Huang Li <ali@ddn.com>
>>> Co-developed-by: Horst Birthelmer <horst@birthelmer.de>
>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>> ---
>>>  fs/fuse/dev_uring.c   | 6 ++++--
>>>  fs/fuse/dev_uring_i.h | 6 +++---
>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
>>> index d9108b5b5db8..f4ba64a1796a 100644
>>> --- a/fs/fuse/dev_uring.c
>>> +++ b/fs/fuse/dev_uring.c
>>> @@ -511,8 +511,7 @@ static void fuse_uring_cancel(struct io_uring_cmd *cmd,
>>>       queue = ent->queue;
>>>       spin_lock(&queue->lock);
>>>       if (ent->state == FRRS_AVAILABLE) {
>>> -             ent->state = FRRS_USERSPACE;
>>> -             list_move_tail(&ent->list, &queue->ent_in_userspace);
>>> +             list_del_init(&ent->list);
>>>               need_cmd_done = true;
>>>               ent->cmd = NULL;
>>>       }
>>> @@ -521,6 +520,9 @@ static void fuse_uring_cancel(struct io_uring_cmd *cmd,
>>>       if (need_cmd_done) {
>>>               /* no queue lock to avoid lock order issues */
>>>               io_uring_cmd_done(cmd, -ENOTCONN, issue_flags);
>>> +             kfree(ent);
>>> +             if (atomic_dec_and_test(&queue->ring->queue_refs))
>>> +                     wake_up_all(&queue->ring->stop_waitq);
>>>       }
>>>  }
>>
>> Hmm, ok, I had done that via fuse_uring_entry_teardown(), but this way
>> is also fine.
>>
>> While thinking about it over night, I wonder if we should abort the
>> connection here. Calls for fuse_uring_cancel() / IO_URING_F_CANCEL
>> happen when
>>
>> a) The daemon dies - that is what I had written the function for
>> b) When one calls
>>
>> With reduced rings queues we would actually need to have per queue refs
>> and if a single queue reaches 0, it would need to re-calculate the
>> queue. In general this gets complex and from my point of view, if
>> fuse-server wants to re-initialize queues, fuse-server should:
>>
>> a) wake up the ring thread with an eventfd (libfuse already has that)
>> b) we need a reconfig SQE (like FUSE_IO_URING_CMD_RECONFIG) that
>> requests to re-configure things
>>
>> Right now that is all not supported, from my point of view we should
>> call fuse_abort_conn() when we call into fuse_uring_cancel()
>>
> 
> From what I see, I don't think it's safe to call the abort in
> fuse_uring_cancel() since the cancel runs in the io_uring submitter's
> task context and the uring lock is held when it gets called. The abort
> logic can trigger calls to io_uring_cmd_done(cmd, -ENOTCONN,
> IO_URING_F_UNLOCKED) (through fuse_uring_stop_queues() ->
> fuse_uring_entry_teardown()) which will lead to a deadlock in trying
> to acquire the lock that's already held. I like the idea of keeping
> things simple with just aborting, but I think in actuality it might
> lead to more trouble.
> 

Yeah agreed, this lock is a persistent root of trouble :) I'm still
tempted to create an async task to tear down the connection here.


Thanks,
Bernd