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

[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.

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?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare at suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich