Re: [PATCH v4 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: 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