Re: [PATCH v5 1/1] scsi: lpfc: defer SCSI rport node put until devloss callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: lpfc: memory leak of old ndlp on rport reuse
- [Medium] scsi: lpfc: data races in lockless rport association handoff
--

commit 0e3550afaa0e5fec6d02f203dc40ee6302db8e5a
Author: Dai Ngo <dai.ngo@oracle.com>

scsi: lpfc: defer SCSI rport node put until devloss callback

This commit delays dropping the SCSI rport node reference until
lpfc_dev_loss_tmo_callbk() detaches the rport from the lpfc node. It
introduces an rport association generation number to verify the node,
generation, and rport before changing association state or dropping a
reference, closing use-after-free and rport-reuse races.

> 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
> @@ -187,6 +193,16 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
>  	    !test_bit(HBA_SETUP, &phba->hba_flag))) {
>  
>  		spin_lock_irqsave(&ndlp->lock, iflags);
> +		if (rdata->pnode != ndlp || rdata->generation != rport_gen) {

[Severity: Medium]
Does this plain read race with writers updating rdata->pnode and
rdata->generation?

The commit message mentions using READ_ONCE() / WRITE_ONCE() for the
lockless pnode handoff. While READ_ONCE() is used initially in
lpfc_dev_loss_tmo_callbk(), this read inside old_ndlp->lock is a plain
read. Since a concurrent writer for a reused rport holds a different lock
(new_ndlp->lock), this plain read races with the writer.

[ ... ]

> @@ -223,14 +237,13 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
[ ... ]
> -	if (ndlp->nlp_state == NLP_STE_MAPPED_NODE)
> -		return;
> -
>  	/* Ignore callback for a mismatched (stale) rport */
>  	if (ndlp->rport != rport) {

[Severity: Medium]
Could this plain read cause reader threads to observe torn stores or
out-of-order writes?

This read of ndlp->rport occurs outside the lock and races with concurrent
writers updating it. Without proper WRITE_ONCE() / READ_ONCE() pairing,
compiler optimizations can break the intended semantics.

[ ... ]

> @@ -4517,19 +4609,27 @@ lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
[ ... ]
>  	spin_lock_irqsave(&ndlp->lock, flags);
> +	/*
> +	 * A reused rport normally retains its old association reference.
> +	 * However, devloss may have completed while fc_remote_port_add()
> +	 * was running, in which case it cleared SCSI_XPT_REGD and released
> +	 * that reference. In the former case, transfer the old reference to
> +	 * the new association and release the provisional new reference.
> +	 */
> +	if (rport == old_rport &&
> +	    (ndlp->fc4_xpt_flags & SCSI_XPT_REGD))
> +		drop_new_scsi_ref = true;
> +	rdata->pnode = pnode;

[Severity: High]
Does this code leak the old lpfc_nodelist on rport reuse?

When a device logs out and logs back in (e.g., with a different NPort ID),
the transport's fc_remote_port_add() matches by WWPN, unblocks the
existing rport, and cancels its dev_loss_work timer.

In lpfc_register_remote_port(), rport == old_rport evaluates to false
because old_rport is new_ndlp->rport (usually NULL), so drop_new_scsi_ref
is false. Then rdata->pnode is overwritten with the new ndlp. The old ndlp
is permanently orphaned with SCSI_XPT_REGD set. Because its devloss timer
was cancelled and its association removed, the reference taken during its
registration is permanently leaked.

> +	rdata->generation = ++ndlp->rport_gen;
> +	ndlp->rport = rport;

[Severity: Medium]
Do these plain writes race with lockless readers?

As mentioned earlier, the commit message claims to use READ_ONCE() and
WRITE_ONCE() for the lockless pnode handoff, but plain writes are used
here for rdata->pnode, rdata->generation, and ndlp->rport.

>  	ndlp->fc4_xpt_flags |= SCSI_XPT_REGD;
>  	spin_unlock_irqrestore(&ndlp->lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929223133.2922017-1-dai.ngo@oracle.com?part=1