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

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 <[email protected]>

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/[email protected]?part=1