[PATCH v2 1/1] scsi: lpfc: Clear rport node association before unregister

[PATCH v2 1/1] scsi: lpfc: Clear rport node association before unregister

[Dai Ngo]

Pulling an FC cable can drive lpfc through remote-port devloss teardown.
In one observed crash, the SCSI transport workqueue called
lpfc_dev_loss_tmo_callbk() from fc_rport_final_delete(), and lpfc faulted
while dereferencing the ndlp-derived vport:

  Workqueue: fc_wq_19 fc_rport_final_delete [scsi_transport_fc]
  RIP: lpfc_dev_loss_tmo_callbk+0x58/0x48d [lpfc]

Call trace:

lpfc_dev_loss_tmo_callbk+0x58/0x48d [lpfc]
fc_rport_final_delete+0xeb/0x1b0 [scsi_transport_fc]
process_one_work+0x174/0x31a
worker_thread+0x191/0x300
kthread+0xcf/0x100
ret_from_fork+0x31/0x50
ret_from_fork_asm+0x1a/0x30

The rport private data still contained a non-NULL pnode, but that pointer
could refer to an ndlp whose lifetime had already ended. The callback then
treated the stale pnode as valid and dereferenced fields such as
ndlp->vport.

When lpfc unregisters a SCSI remote port, it calls fc_remote_port_delete()
and then drops the ndlp reference that was taken when rdata->pnode was set
during registration. However, the rport private pnode field was left intact
until the transport devloss callback path cleared it. This leaves a window
where a later transport callback can observe a stale pnode after the ndlp
reference has been released.

This patch detaches the lpfc node from the rport before calling
fc_remote_port_delete() and before dropping the transport-held ndlp
reference. Snapshot ndlp->rport under ndlp->lock, clear rdata->pnode,
clear ndlp->rport, and clear the SCSI transport registration flag while
holding the lock. Also make the devloss callback tolerate a NULL rport
or rport private data and use READ_ONCE() / WRITE_ONCE() for the lockless
pnode handoff.

This makes the unregister path match the callback teardown path: once lpfc
has severed the rport-to-ndlp association, later devloss callbacks see a
NULL pnode and return instead of touching a released node.

Assisted-by: Codex:gpt-5
Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 drivers/scsi/lpfc/lpfc_hbadisc.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

V2:
  . in lpfc_unregister_remote_port(), snapshot ndlp->rport under ndlp->lock,
    clear rdata->pnode, clear ndlp->rport, and clear the SCSI transport
    registration flag while holding the ndlp->lock.

  . in lpfc_dev_loss_tmo_callbk, check rport's private data before
    using it and and use READ_ONCE() / WRITE_ONCE() for the lockless
    pnode handoff.

diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
index 4f68038789b5..a04222ab4092 100644
--- a/drivers/scsi/lpfc/lpfc_hbadisc.c
+++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -157,13 +157,21 @@ void
 lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 {
 	struct lpfc_nodelist *ndlp;
+	struct lpfc_rport_data *rdata;
 	struct lpfc_vport *vport;
 	struct lpfc_hba   *phba;
 	struct lpfc_work_evt *evtp;
 	unsigned long iflags;
 	bool drop_initial_node_ref = false;
 
-	ndlp = ((struct lpfc_rport_data *)rport->dd_data)->pnode;
+	if (!rport)
+		return;
+
+	rdata = rport->dd_data;
+	if (!rdata)
+		return;
+
+	ndlp = READ_ONCE(rdata->pnode);
 	if (!ndlp)
 		return;
 
@@ -187,6 +195,7 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 	    !test_bit(HBA_SETUP, &phba->hba_flag))) {
 
 		spin_lock_irqsave(&ndlp->lock, iflags);
+		WRITE_ONCE(rdata->pnode, NULL);
 		ndlp->rport = NULL;
 
 		/* Only 1 thread can drop the initial node reference.
@@ -271,7 +280,7 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 	 * rport. Remove the association between rport and ndlp.
 	 */
 	ndlp->fc4_xpt_flags &= ~SCSI_XPT_REGD;
-	((struct lpfc_rport_data *)rport->dd_data)->pnode = NULL;
+	WRITE_ONCE(((struct lpfc_rport_data *)rport->dd_data)->pnode, NULL);
 	ndlp->rport = NULL;
 	spin_unlock_irqrestore(&ndlp->lock, iflags);
 
@@ -4553,8 +4562,10 @@ lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
 static void
 lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
 {
-	struct fc_rport *rport = ndlp->rport;
+	struct fc_rport *rport;
 	struct lpfc_vport *vport = ndlp->vport;
+	struct lpfc_rport_data *rdata;
+	unsigned long iflags;
 
 	if (vport->cfg_enable_fc4_type == LPFC_ENABLE_NVME)
 		return;
@@ -4566,9 +4577,22 @@ lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
 	lpfc_printf_vlog(vport, KERN_INFO, LOG_NODE,
 			 "3184 rport unregister x%06x, rport x%px "
 			 "xptflg x%x refcnt %d\n",
-			 ndlp->nlp_DID, rport, ndlp->fc4_xpt_flags,
+			 ndlp->nlp_DID, ndlp->rport, ndlp->fc4_xpt_flags,
 			 kref_read(&ndlp->kref));
 
+	spin_lock_irqsave(&ndlp->lock, iflags);
+	rport = ndlp->rport;
+	if (!rport) {
+		spin_unlock_irqrestore(&ndlp->lock, iflags);
+		return;
+	}
+	rdata = rport->dd_data;
+	if (rdata && READ_ONCE(rdata->pnode) == ndlp)
+		WRITE_ONCE(rdata->pnode, NULL);
+	ndlp->rport = NULL;
+	ndlp->fc4_xpt_flags &= ~SCSI_XPT_REGD;
+	spin_unlock_irqrestore(&ndlp->lock, iflags);
+
 	fc_remote_port_delete(rport);
 	lpfc_nlp_put(ndlp);
 }
-- 
2.47.3

Re: [PATCH v2 1/1] scsi: lpfc: Clear rport node association before unregister

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces a memory leak of `struct lpfc_nodelist` objects by short-circuiting the driver's normal devloss teardown mechanism.
--

--- Patch [1]: [PATCH v2 1/1] scsi: lpfc: Clear rport node association before unregister ---
commit b2533138432d8556bb87be05d21267317ff588d6
Author: Dai Ngo <dai.ngo@oracle.com>

scsi: lpfc: Clear rport node association before unregister

This patch detaches the lpfc node from the rport before calling
fc_remote_port_delete() and before dropping the transport-held ndlp reference.
It clears rdata->pnode and ndlp->rport under ndlp->lock in the unregister path,
and updates the devloss callback to tolerate a NULL rport or pnode.

> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
> index c9b02d2c63054..b737f976a789e 100644
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
> @@ -157,13 +157,21 @@ void
>  lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
>  {
>  	struct lpfc_nodelist *ndlp;
> +	struct lpfc_rport_data *rdata;
[ ... ]
> -	ndlp = ((struct lpfc_rport_data *)rport->dd_data)->pnode;
> +	if (!rport)
> +		return;
> +
> +	rdata = rport->dd_data;
> +	if (!rdata)
> +		return;
> +
> +	ndlp = READ_ONCE(rdata->pnode);
>  	if (!ndlp)
>  		return;

[Severity: High]
Does this early return lead to a leak of the struct lpfc_nodelist object?

If lpfc_unregister_remote_port() has already cleared rdata->pnode before the
devloss timer starts, this callback reads the cleared pnode, sees NULL, and
returns immediately without queueing the LPFC_EVT_DEV_LOSS event.

Because the event is never queued, the discovery state machine is never
notified via NLP_EVT_DEVICE_RM, and the initial node reference is never
dropped.

[ ... ]
> @@ -4560,8 +4569,10 @@ lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  static void
>  lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
>  {
[ ... ]
> +	spin_lock_irqsave(&ndlp->lock, iflags);
> +	rport = ndlp->rport;
> +	if (!rport) {
> +		spin_unlock_irqrestore(&ndlp->lock, iflags);
> +		return;
> +	}
> +	rdata = rport->dd_data;
> +	if (rdata && READ_ONCE(rdata->pnode) == ndlp)
> +		WRITE_ONCE(rdata->pnode, NULL);

[Severity: High]
Does this prematurely disconnect the node from the rport?

If an FC cable is pulled or a target LOGO is received, the node is
unregistered and moved to the NLP_STE_NPR_NODE state. Unconditionally clearing
rdata->pnode here appears to short-circuit the normal devloss teardown
mechanism by causing the early return in lpfc_dev_loss_tmo_callbk() mentioned
above.

> +	ndlp->rport = NULL;
> +	ndlp->fc4_xpt_flags &= ~SCSI_XPT_REGD;
> +	spin_unlock_irqrestore(&ndlp->lock, iflags);
> +
>  	fc_remote_port_delete(rport);
>  	lpfc_nlp_put(ndlp);
>  }

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

Re: [PATCH v2 1/1] scsi: lpfc: Clear rport node association before unregister

[Paul Ely]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

Hello Dai,

This patch is wrong.  The node is not fully unregistered until the the
scsi_transport_fc calls the driver back in lpfc_dev_loss_tmo_callbk
and the driver finishes with lpfc_dev_loss_tmo_handler.  This allows
for dev_loss_tmo seconds, 30 by default, recovery for the LLDD.

Broadcom has fixed this issue a few times now.  It might be better to
open a case with Broadcom support  so we can see what driver version
you are running when the issue occurs.

Thanks

Paul

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5453 bytes --]