Re: [PATCH] scsi: lpfc: Fix ndlp reference leak in lpfc_nlp_reg_node()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: lpfc: missing NVME_XPT_REGD clear in NVMET unregister causes leak
- [Medium] scsi: lpfc: TOCTOU race in lpfc_nlp_reg_node() leaks reference
--

commit 8a787af86823c5598e09493f24aaa14226c3f5da
Author: Wentao Liang <vulab@iscas.ac.cn>

scsi: lpfc: Fix ndlp reference leak in lpfc_nlp_reg_node()

This commit attempts to fix a node reference leak for NVMET nodes by
setting the NVME_XPT_REGD flag when the reference is taken in
lpfc_nlp_reg_node(). This aims to ensure that lpfc_nlp_unreg_node()
balances the reference count during teardown.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
> index 4c673dffa671b..f61026e3b48ea 100644
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
> @@ -4664,9 +4664,15 @@ lpfc_nlp_reg_node(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  			}
>  		} else {
>  			/* Just take an NDLP ref count since the
> -			 * target does not register rports.
> +			 * target does not register rports. Mark it as
> +			 * an NVME transport ref so that
> +			 * lpfc_nlp_unreg_node() releases it.
>  			 */
>  			lpfc_nlp_get(ndlp);
> +
> +			spin_lock_irqsave(&ndlp->lock, iflags);
> +			ndlp->fc4_xpt_flags |= NVME_XPT_REGD;
> +			spin_unlock_irqrestore(&ndlp->lock, iflags);

[Severity: Medium]
Does acquiring the lock here to set NVME_XPT_REGD introduce a time-of-check
to time-of-use race that can leak the reference?

Earlier in lpfc_nlp_reg_node(), NLP_XPT_REGD is set and ndlp->lock is
dropped to call lpfc_register_remote_port().

If a concurrent lpfc_nlp_unreg_node() call sees NLP_XPT_REGD set, it clears
it, and then locklessly evaluates NVME_XPT_REGD as 0, skipping the
lpfc_nlp_put().

When lpfc_nlp_reg_node() resumes, it increments the refcount and sets
NVME_XPT_REGD. Because NLP_XPT_REGD was already cleared, subsequent
unregisters will return early, permanently leaking this reference.

[Severity: High]
Does setting NVME_XPT_REGD for NVMET nodes here strand the node during
unregistration?

While lpfc_nlp_unreg_node() drops the reference for NVMET nodes, it fails
to clear the NVME_XPT_REGD flag:

lpfc_nlp_unreg_node() {
    ...
    /* NVMET has no upcall. */
    lpfc_nlp_put(ndlp);
    // missing: ndlp->fc4_xpt_flags &= ~NVME_XPT_REGD;
    ...
}

Leaving this flag set permanently prevents the driver from evaluating this
to true in lpfc_dev_loss_tmo_handler():

    if (!(ndlp->fc4_xpt_flags & NVME_XPT_REGD))
        lpfc_disc_state_machine(vport, ndlp, NULL, NLP_EVT_DEVICE_RM);

This strands the node, blocking device removal and potentially deadlocking
vport teardown.

Additionally, if the node is rediscovered as FCP-only, the sticky flag
triggers an unmatched lpfc_nlp_put(), causing an underflow and
use-after-free.

>  		}
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917144543.2157614-1-vulab@iscas.ac.cn?part=1