[PATCH 2/6] nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

[Guixin Liu]

在 2026/11/6 15:38, Hannes Reinecke 写道:
> On 11/6/26 2:58 AM, Guixin Liu wrote:
>> Commit b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
>> switched the loop to list_for_each_entry_safe() to allow dropping the
>> rport lock for the nvme_fc_ctrl_put() of a non-matching controller.
>> But the safe iterator only protects the current entry within one
>> iteration: the next entry is cached before the body runs, and while
>> the lock is dropped that entry can run through its own final put into
>> nvme_fc_ctrl_free(), which removes it from the list and frees it.
>> The next iteration then dereferences freed memory.
>>
>> The safe variant is not needed here at all. The reference the loop
>> takes on the current entry pins it on the list until the iteration
>> moves on, so a plain list_for_each_entry() tolerates the lock drop:
>> while the lock is held the current entry cannot be unlinked, and once
>> the lock is dropped the cached ->next of the pinned entry still points
>> at a valid list member or the head.
>>
>> Switch back to the plain iterator.
>>
>> Fixes: b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Guixin Liu <kanie at linux.alibaba.com>
>>
>> ---
>>
>> Reproduced under KASAN in a VM through fcloop with three or more
>> controllers on one remote port, a tbkas force-reconnect racing
>> concurrent controller disconnects.  The lock-drop window was widened
>> with a test-only mdelay(200) inserted at the same place in both the
>> buggy and the fixed version so the iterator is the only variable; the
>> stock microsecond window did not trigger in 400 iterations of the
>> same workload.
>>
>>    BUG: KASAN: slab-use-after-free in nvme_fc_match_disconn_ls
>>    Read of size 8 by task kworker
>>    Workqueue: events nvme_fc_handle_ls_rqst_work
>>    Call Trace:
>>     nvme_fc_match_disconn_ls
>>     nvme_fc_ls_disconnect_assoc
>>     nvme_fc_handle_ls_rqst_work
>>     process_one_work
>>
>> The object was allocated by nvme_fc_alloc_ctrl() on connect and freed
>> by nvme_free_ctrl() on the concurrent teardown of the cached next
>> entry.  With the plain iterator the identical amplified sequence
>> reports nothing and survives; the read hits the pinned current entry,
>> which cannot be freed while the loop holds its reference.
>> ---
>>   drivers/nvme/host/fc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
>> index 0b9ff50d26ff..edf192055618 100644
>> --- a/drivers/nvme/host/fc.c
>> +++ b/drivers/nvme/host/fc.c
>> @@ -1470,14 +1470,14 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>>   {
>>       struct fcnvme_ls_disconnect_assoc_rqst *rqst =
>>                       &lsop->rqstbuf->rq_dis_assoc;
>> -    struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
>> +    struct nvme_fc_ctrl *ctrl, *ret = NULL;
>>       struct nvmefc_ls_rcv_op *oldls = NULL;
>>       u64 association_id = be64_to_cpu(rqst->associd.association_id);
>>       unsigned long flags;
>>         spin_lock_irqsave(&rport->lock, flags);
>>   -    list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
>> +    list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
>>           if (!nvme_fc_ctrl_get(ctrl))
>>               continue;
>>           spin_lock(&ctrl->lock);
>
> Not sure here. The implication of the function is that the any controller might've been deleted during each loop iteration
> (disconnect might be triggered from both sides, either host
> _or_ controller).
> So while it's true that a controller might not get deleted while
> a reference is held, that other process might drop a reference
> to the controller while the first process is executing the loop.
> Then the call to 'nvme_fc_ctrl_put()' at the end of the loop
> will be the last reference, the controller gets deleted,
> and the ->next element becomes invalid causing the loop
> to crash.
You are right, Hannes, the issue still exists.
>
> So, no.
>
> And the issue you've described should've caused the
> nvme_fc_ctrl_get() at the start of the loop to return
> 'false'; why didn't it trigger here?
The UAF happens earlier, inside the "for" loop,
list_for_each_entry_safe() expands to "pos = n, n = list_next_entry(n)",
so at that point it already dereferences the next pointer of that ctrl
that has been freed. That why KASAN reports "Read of size 8 by task kworker"
in nvme_fc_match_disconn_ls().

By the time execution reaches nvmet_fc_ctrl_get(), the memory state is
event less clear.

I will change a v2 fix, thanks.

Best Regards,
Guixin Liu
>  
>
> Cheers,
>
> Hannes