Re: [PATCH v4 06/14] 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] scsi: lpfc: Use-after-free in NVMe unregistration due to missing serialization
- [High] scsi: lpfc: ndlp memory leak due to concurrent registration race
--

commit 6da5a3b652f8d5c08807e25e9db6e426dc7022ff
Author: Nigel Kirkland <nkirkland2304@gmail.com>

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

This commit refactors how registration state is tracked by separating the SCSI
and NVME transport registration flags. It introduces new wait flags to track
unregistration and a new flag for NVMET target nodes to address use-after-free
issues during target port failovers and RSCN storms.

> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
> index 4c673dffa671b..b4a5c7d5c2a0b 100644
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
[ ... ]
> @@ -4623,7 +4632,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);

[Severity: High]
Does removing the atomic test-and-set of the registration flag here introduce 
an ndlp memory leak?

If multiple threads execute lpfc_nlp_reg_node() concurrently (e.g., from
repeated RSCNs), they can all pass the registration check above while the flags
are still cleared. All threads will drop the lock and proceed to call
lpfc_register_remote_port(), which will unconditionally increment the ndlp
reference count via lpfc_nlp_get().

During unregistration, the SCSI path correctly prevents double unregistration
by checking SCSI_XPT_UNREG_WAIT, so it will only drop one reference. The extra
references taken during the concurrent registrations will permanently leak.

>  
>  	if (lpfc_valid_xpt_node(ndlp)) {

[ ... ]

> @@ -4706,16 +4728,11 @@ lpfc_nlp_unreg_node(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  				 kref_read(&ndlp->kref));
>  	}
>  
> -	if (ndlp->fc4_xpt_flags & NVME_XPT_REGD) {
> +	if ((ndlp->fc4_xpt_flags & (NVME_XPT_REGD | NVME_XPT_UNREG_WAIT)) ==
> +	    NVME_XPT_REGD) {

[Severity: High]
Can this lockless flag check cause a use-after-free on the ndlp object?

Because the flag check happens outside of ndlp->lock in lpfc_nlp_unreg_node(),
multiple threads can concurrently pass this check and call
lpfc_nvme_unregister_port().

Unlike the SCSI unregistration path, lpfc_nvme_unregister_port()
unconditionally sets the NVME_XPT_UNREG_WAIT flag under the lock without
verifying if a concurrent thread has already set it. Both threads will then
call nvme_fc_unregister_remoteport() and lpfc_nlp_put(). Since the ndlp
reference was only incremented once during NVMe registration, the double
lpfc_nlp_put() could result in a use-after-free.

>  		vport->phba->nport_event_cnt++;
> -		if (vport->phba->nvmet_support == 0) {
> -			lpfc_nvme_unregister_port(vport, ndlp);
> -		} else {
> -			/* NVMET has no upcall. */
> -			lpfc_nlp_put(ndlp);
> -		}
> +		lpfc_nvme_unregister_port(vport, ndlp);
>  	}
> -
>  }

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