Re: [PATCH v5 06/10] lpfc: Fix ndlp use-after-free during repeated RSCN and rediscovery sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] lpfc: UAF and rport leak in lpfc_register_remote_port() due to race with devloss
- [High] lpfc: Duplicate NVMe transport registrations in lpfc_nlp_reg_node() due to missing synchronization
--

commit 01b3084183c4a09b5084e2e3651c384b815d645a
Author: Nigel Kirkland <nkirkland2304@gmail.com>

lpfc: Fix ndlp use-after-free during repeated RSCN and rediscovery sequence

The commit refactors transport registration flags to separate SCSI and
NVME states, preventing premature release of node objects during transport
callbacks. It also introduces symmetric reference-count tracking for NVMET
target nodes.

> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c

[ ... ]

> @@ -4481,26 +4481,36 @@ lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  	struct lpfc_hba  *phba = vport->phba;
>  	unsigned long flags;
>  
> -	if (vport->cfg_enable_fc4_type == LPFC_ENABLE_NVME)
> +	/* Don't add the remote port if unloading. */
> +	if (test_bit(FC_UNLOADING, &vport->load_flag))
>  		return;
>  
> +	spin_lock_irqsave(&ndlp->lock, flags);
> +	if ((ndlp->fc4_xpt_flags & (SCSI_XPT_REGD | SCSI_XPT_UNREG_WAIT)) ==
> +	    SCSI_XPT_REGD) {
> +		/* Prevent racing registrations */
> +		spin_unlock_irqrestore(&ndlp->lock, flags);
> +		return;
> +	}
> +	ndlp->fc4_xpt_flags &= ~SCSI_XPT_UNREG_WAIT;
> +	ndlp->fc4_xpt_flags |= SCSI_XPT_REGD;
> +	spin_unlock_irqrestore(&ndlp->lock, flags);

[Severity: High]
Could this flag manipulation open a race window with devloss? By setting 
SCSI_XPT_REGD and dropping ndlp->lock before calling fc_remote_port_add(), 
ndlp->rport still points to the old rport.

If a concurrent dev_loss_work executes lpfc_dev_loss_tmo_callbk() on the
old rport before the new one is assigned, it will observe SCSI_XPT_REGD as
set. 

If the driver is unloading, lpfc_nlp_put() is called prematurely here,
causing a use-after-free when lpfc_register_remote_port() resumes.
Otherwise, the callback clears SCSI_XPT_REGD, leaving the new rport
permanently unregistered and leaked since subsequent unregister calls will
bypass removal.

>  	/* Remote port has reappeared. Re-register w/ FC transport */
>  	rport_ids.node_name = wwn_to_u64(ndlp->nlp_nodename.u.wwn);
>  	rport_ids.port_name = wwn_to_u64(ndlp->nlp_portname.u.wwn);

[ ... ]

> @@ -4623,7 +4640,10 @@ lpfc_nlp_reg_node(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  	lpfc_check_nlp_post_devloss(vport, ndlp);
>  
>  	spin_lock_irqsave(&ndlp->lock, iflags);
> -	if (ndlp->fc4_xpt_flags & NLP_XPT_REGD) {
> +	if (((ndlp->fc4_xpt_flags & SCSI_XPT_REGD) &&
> +	     !(ndlp->fc4_xpt_flags & SCSI_XPT_UNREG_WAIT)) ||
> +	    ((ndlp->fc4_xpt_flags & NVME_XPT_REGD) &&
> +	     !(ndlp->fc4_xpt_flags & NVME_XPT_UNREG_WAIT))) {
>  		/* Already registered with backend, trigger rescan */
>  		spin_unlock_irqrestore(&ndlp->lock, iflags);

[ ... ]

> -	ndlp->fc4_xpt_flags |= NLP_XPT_REGD;
>  	spin_unlock_irqrestore(&ndlp->lock, iflags);

[ ... ]

> -	if (vport->phba->sli_rev >= LPFC_SLI_REV4 &&
> -			ndlp->nlp_fc4_type & NLP_FC4_NVME) {
> -		if (vport->phba->nvmet_support == 0) {
> -			/* Register this rport with the transport.
> -			 * Only NVME Target Rports are registered with
> -			 * the transport.
> -			 */
> -			if (ndlp->nlp_type & NLP_NVME_TARGET) {
> -				vport->phba->nport_event_cnt++;
> -				lpfc_nvme_register_port(vport, ndlp);
> +	if (vport->phba->nvmet_support == 0) {
> +		/* Register this rport with the transport.
> +		 * Only NVME Target Rports are registered with
> +		 * the transport.
> +		 */
> +		if (ndlp->nlp_type & NLP_NVME_TARGET) {
> +			vport->phba->nport_event_cnt++;
> +			lpfc_nvme_register_port(vport, ndlp);

[Severity: High]
Does removing the unified NLP_XPT_REGD flag setting before the unlock
allow duplicate NVMe transport registrations?

By replacing the single check with separate SCSI and NVME flag checks and
dropping ndlp->lock immediately, concurrent executions of lpfc_nlp_reg_node()
(for instance, from RSCN spam) can both observe NVME_XPT_REGD as 0 and
proceed to call lpfc_nvme_register_port().

Since lpfc_nvme_register_port() does not internally synchronize and check
NVME_XPT_REGD before calling nvme_fc_register_remoteport(), both threads
will register a new remoteport and increment the ndlp reference count. The
first remoteport will be overwritten in ndlp->nrport, permanently leaking
the transport object and its reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928181757.21959-1-nkirkland2304@gmail.com?part=6