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

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

[Dai Ngo]

lpfc_register_remote_port() stores an lpfc_nodelist pointer in the
SCSI transport rport private data and takes a node reference for that
association.

On remote-port unregister, lpfc_unregister_remote_port() calls
fc_remote_port_delete(), which starts SCSI transport devloss handling.
The rport is not fully finished at that point.  The SCSI transport may
still call back into the driver through lpfc_terminate_rport_io() and
lpfc_dev_loss_tmo_callbk(), both of which use rport->dd_data->pnode to
find the lpfc node.

Dropping the node reference immediately after fc_remote_port_delete()
can therefore free the lpfc_nodelist while the rport still points to
it. A cable pull can hit this window and report a KASAN use-after-free
from lpfc_terminate_rport_io(), followed by stale-node access in
lpfc_dev_loss_tmo_callbk().

Keep the SCSI rport node reference until lpfc_dev_loss_tmo_callbk()
detaches the rport from the lpfc node.  Drop that reference after
clearing SCSI_XPT_REGD and removing the rport/dd_data association,
so the node remains valid for the SCSI transport devloss callbacks.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 drivers/scsi/lpfc/lpfc_hbadisc.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 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.

V3:
. Redo the patch based on Paul's review.
Delay dropping the SCSI rport node reference until
lpfc_dev_loss_tmo_callbk() detaches the rport from the lpfc node.

V4:
. Fix the node lead reference when the remote port reconnects before
dev_loss_tmo expires. If fc_remote_port_add() reuses the existing SCSI
rport, transfer the reference that is still held on the rport instead
of acquiring a new reference.

diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
index 4f68038789b5..582efb297c74 100644
--- a/drivers/scsi/lpfc/lpfc_hbadisc.c
+++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -162,6 +162,7 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 	struct lpfc_work_evt *evtp;
 	unsigned long iflags;
 	bool drop_initial_node_ref = false;
+	bool drop_scsi_node_ref = false;
 
 	ndlp = ((struct lpfc_rport_data *)rport->dd_data)->pnode;
 	if (!ndlp)
@@ -202,6 +203,7 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 		 */
 		if (ndlp->fc4_xpt_flags & SCSI_XPT_REGD) {
 			ndlp->fc4_xpt_flags &= ~SCSI_XPT_REGD;
+			drop_scsi_node_ref = true;
 
 			/* If NLP_XPT_REGD was cleared in lpfc_nlp_unreg_node,
 			 * unregister calls were made to the scsi and nvme
@@ -213,9 +215,6 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 				if (!(ndlp->fc4_xpt_flags & NVME_XPT_REGD))
 					ndlp->fc4_xpt_flags &= ~NLP_XPT_REGD;
 				spin_unlock_irqrestore(&ndlp->lock, iflags);
-
-				/* Release scsi transport reference */
-				lpfc_nlp_put(ndlp);
 			} else {
 				spin_unlock_irqrestore(&ndlp->lock, iflags);
 			}
@@ -223,6 +222,8 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 			spin_unlock_irqrestore(&ndlp->lock, iflags);
 		}
 
+		if (drop_scsi_node_ref)
+			lpfc_nlp_put(ndlp);
 		if (drop_initial_node_ref)
 			lpfc_nlp_put(ndlp);
 		return;
@@ -271,6 +272,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;
+	drop_scsi_node_ref = true;
 	((struct lpfc_rport_data *)rport->dd_data)->pnode = NULL;
 	ndlp->rport = NULL;
 	spin_unlock_irqrestore(&ndlp->lock, iflags);
@@ -287,6 +289,8 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 			list_add_tail(&evtp->evt_listp, &phba->work_list);
 			spin_unlock_irqrestore(&phba->hbalock, iflags);
 			lpfc_worker_wake_up(phba);
+			if (drop_scsi_node_ref)
+				lpfc_nlp_put(ndlp);
 			return;
 		}
 		spin_unlock_irqrestore(&phba->hbalock, iflags);
@@ -304,6 +308,8 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 						NLP_EVT_DEVICE_RM);
 		}
 	}
+	if (drop_scsi_node_ref)
+		lpfc_nlp_put(ndlp);
 }
 
 /**
@@ -4475,6 +4481,7 @@ static void
 lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
 {
 	struct Scsi_Host *shost = lpfc_shost_from_vport(vport);
+	struct fc_rport  *old_rport = ndlp->rport;
 	struct fc_rport  *rport;
 	struct lpfc_rport_data *rdata;
 	struct fc_rport_identifiers rport_ids;
@@ -4510,7 +4517,10 @@ lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
 	rport->maxframe_size = ndlp->nlp_maxframe;
 	rport->supported_classes = ndlp->nlp_class_sup;
 	rdata = rport->dd_data;
-	rdata->pnode = lpfc_nlp_get(ndlp);
+	if (rport == old_rport)
+		rdata->pnode = ndlp;
+	else
+		rdata->pnode = lpfc_nlp_get(ndlp);
 	if (!rdata->pnode) {
 		dev_warn(&phba->pcidev->dev,
 			 "Warning - node ref failed. Unreg rport\n");
@@ -4570,7 +4580,6 @@ lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
 			 kref_read(&ndlp->kref));
 
 	fc_remote_port_delete(rport);
-	lpfc_nlp_put(ndlp);
 }
 
 static void
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: lpfc: Use-After-Free of lpfc_nodelist via TOCTOU race during rport registration
- [Medium] scsi: lpfc: Memory leak of lpfc_nodelist on early return in devloss callback
--

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

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

This patch defers dropping the SCSI rport node reference from remote-port 
unregister time to the devloss callback to prevent a use-after-free window.

> 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
> @@ -4482,6 +4488,7 @@ static void
>  lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  {
>  	struct Scsi_Host *shost = lpfc_shost_from_vport(vport);
> +	struct fc_rport  *old_rport = ndlp->rport;
>  	struct fc_rport  *rport;
>  	struct lpfc_rport_data *rdata;
[ ... ]
> @@ -4517,7 +4524,10 @@ lpfc_register_remote_port(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp)
>  	rport->maxframe_size = ndlp->nlp_maxframe;
>  	rport->supported_classes = ndlp->nlp_class_sup;
>  	rdata = rport->dd_data;
> -	rdata->pnode = lpfc_nlp_get(ndlp);
> +	if (rport == old_rport)
> +		rdata->pnode = ndlp;
> +	else
> +		rdata->pnode = lpfc_nlp_get(ndlp);

[Severity: High]
Can this lead to a use-after-free via a time-of-check to time-of-use race if
the remote port's devloss timer expires concurrently?

If lpfc_register_remote_port() caches old_rport locklessly before calling
fc_remote_port_add(), and the devloss worker thread concurrently executes
lpfc_dev_loss_tmo_callbk(), the callback clears the SCSI_XPT_REGD flag and
successfully drops the original node reference via lpfc_nlp_put(ndlp).

When the registering thread resumes and finishes fc_remote_port_add(), if the
transport reuses the same port structure, the check (rport == old_rport)
evaluates to true. This would leave the node tracked without acquiring a new
reference, even though the original reference was already dropped by the
devloss callback.

[ ... ]
> @@ -4577,7 +4587,6 @@ lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
>  			 kref_read(&ndlp->kref));
>  
>  	fc_remote_port_delete(rport);
> -	lpfc_nlp_put(ndlp);
>  }

[Severity: Medium]
Does removing this unconditional lpfc_nlp_put() introduce a memory leak if
lpfc_dev_loss_tmo_callbk() exits early?

If a remote port drops and reconnects, obtaining a different rport structure
from the transport layer, lpfc_register_remote_port() updates ndlp->rport
to the new pointer.

Later, when the devloss timer for the old rport expires,
lpfc_dev_loss_tmo_callbk() evaluates this condition:

	/* Ignore callback for a mismatched (stale) rport */
	if (ndlp->rport != rport) {
		lpfc_vlog_msg(vport, KERN_WARNING, LOG_NODE,
			      "6788 fc rport mismatch...");
		return;
	}

Since the pointer was updated, the condition evaluates to true and the function
returns early. Because lpfc_nlp_put(ndlp) was deferred to the end of the
devloss callback, it is completely bypassed here, permanently leaking the
lpfc_nodelist object.

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