Re: [PATCH v3 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 3 potential issue(s) to consider:
- [High] Race condition between `lpfc_unregister_remote_port` and `lpfc_dev_loss_tmo_callbk` leading to double-put and Use-After-Free.
- [High] Unbounded reference count leak for NVMe Target (NVMET) nodes during repeated RSCN rediscovery.
- [Medium] Lockless check in `lpfc_nlp_unreg_node` introduces a TOCTOU race leading to double unregistration.
--

--- Patch [6]: [PATCH v3 06/14] lpfc: Fix ndlp use-after-free during repeated RSCN and rediscovery sequence ---
commit a4e515a68e8dcd0a05052708ad0dbafb1627b3e9
Author: Justin Tee <justintee8345@gmail.com>
Subject: lpfc: Fix ndlp use-after-free during repeated RSCN and rediscovery sequence

This commit refactors lpfc_fc4_xpt_flags to use separate flags for SCSI
and NVME registrations. It adds wait flags to track unregister requests
during device loss and transport callbacks.

[ ... ]
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
[ ... ]
> @@ -4562,6 +4556,7 @@ lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
>  {
>  	struct fc_rport *rport = ndlp->rport;
>  	struct lpfc_vport *vport = ndlp->vport;
> +	unsigned long flags;
>  
>  	if (vport->cfg_enable_fc4_type == LPFC_ENABLE_NVME)
>  		return;
> @@ -4577,6 +4572,11 @@ lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
>  			 kref_read(&ndlp->kref));
>  
>  	fc_remote_port_delete(rport);
> +
> +	/* Flag unreg pending and reference released */
> +	spin_lock_irqsave(&ndlp->lock, flags);
> +	ndlp->fc4_xpt_flags |= SCSI_XPT_UNREG_WAIT;
> +	spin_unlock_irqrestore(&ndlp->lock, flags);
>  	lpfc_nlp_put(ndlp);
>  }

[Severity: High]
Can this sequence cause a double-put of the ndlp structure if the transport
callback races with this function?

Because SCSI_XPT_UNREG_WAIT is set after fc_remote_port_delete() is called,
the queued dev_loss_work task can execute asynchronously before the lock is
acquired here to set the wait flag.

If the transport callback wins the race, lpfc_dev_loss_tmo_callbk() will read
!(ndlp->fc4_xpt_flags & SCSI_XPT_UNREG_WAIT) as true, clear SCSI_XPT_REGD,
and call lpfc_nlp_put(). When lpfc_unregister_remote_port() resumes, it sets
the wait flag and unconditionally calls lpfc_nlp_put() again, which could
trigger a use-after-free.

[ ... ]
> @@ -4623,7 +4623,9 @@ 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 | NVME_XPT_REGD)) &&
> +	    !(ndlp->fc4_xpt_flags & (SCSI_XPT_UNREG_WAIT |
> +				     NVME_XPT_UNREG_WAIT))) {
>  		/* Already registered with backend, trigger rescan */
>  		spin_unlock_irqrestore(&ndlp->lock, iflags);

[Severity: High]
Does this code leak an ndlp reference for NVMe targets during repeated RSCN
rediscovery events?

In an NVMe target setup (nvmet_support != 0), neither SCSI_XPT_REGD nor
NVME_XPT_REGD is ever set. Instead, the code falls through to the target
handling block at the end of lpfc_nlp_reg_node():

        } else {
                /* Just take an NDLP ref count since the
                 * target does not register rports.
                 */
                lpfc_nlp_get(ndlp);
        }

Without a registration flag set, every subsequent call to lpfc_nlp_reg_node()
during repeated RSCNs will bypass this early return check, falling through to
execute lpfc_nlp_get() again and permanently leaking a reference each time.

[ ... ]
> @@ -4678,7 +4675,7 @@ lpfc_nlp_unreg_node(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  	unsigned long iflags;
>  
>  	spin_lock_irqsave(&ndlp->lock, iflags);
> -	if (!(ndlp->fc4_xpt_flags & NLP_XPT_REGD)) {
> +	if (!(ndlp->fc4_xpt_flags & (SCSI_XPT_REGD | NVME_XPT_REGD))) {
>  		spin_unlock_irqrestore(&ndlp->lock, iflags);
[ ... ]
>  	}
> -
> -	ndlp->fc4_xpt_flags &= ~NLP_XPT_REGD;
>  	spin_unlock_irqrestore(&ndlp->lock, iflags);
>  
>  	if (ndlp->rport &&
> -	    ndlp->fc4_xpt_flags & SCSI_XPT_REGD) {
> +	    ((ndlp->fc4_xpt_flags & (SCSI_XPT_REGD | SCSI_XPT_UNREG_WAIT)) ==
> +	     SCSI_XPT_REGD)) {
>  		vport->phba->nport_event_cnt++;
>  		lpfc_unregister_remote_port(ndlp);

[Severity: Medium]
Could this lockless check introduce a time-of-check to time-of-use race?

The original code cleared NLP_XPT_REGD inside the ndlp->lock critical
section. By replacing it with a lockless check of the flag combination outside
the lock, two concurrent threads entering lpfc_nlp_unreg_node() could both
read the state as SCSI_XPT_REGD without SCSI_XPT_UNREG_WAIT.

If that happens, both threads would proceed to call
lpfc_unregister_remote_port(ndlp), leading to a double unregistration and
potential double-put of the ndlp structure.

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