Re: [PATCH 1/1] scsi: lpfc: Clear rport association before unregistering it

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditionally clearing `rdata->pnode` breaks the devloss timeout state machine, leading to permanent `lpfc_nodelist` memory leaks for SCSI targets.

Pre-existing issues:
- [High] A TOCTOU race condition from lockless checks of `ndlp->rport` leads to a NULL pointer dereference of `rport->dd_data` in `lpfc_unregister_remote_port()`.
- [High] The patch attempts to fix a Use-After-Free (UAF) by clearing `rdata->pnode` under `ndlp->lock`, but leaves a TOCTOU race window because the read side in `lpfc_dev_loss_tmo_callbk()` is completely lockless.
--

--- Patch [1]: [PATCH 1/1] scsi: lpfc: Clear rport association before unregistering it ---
commit 97fd261025f15460afb6d81b854ab25279c37bc0
Author: Dai Ngo <[email protected]>

scsi: lpfc: Clear rport association before unregistering it

System crashed when a Fibre Channel cable is removed from an Emulex/Broadcom
lpfc HBA. This crash occurs after the lpfc Link Down event, after dm-multipath
fails paths, and when the FC remote ports time out and are removed. This patch
clears rdata->pnode and ndlp->rport under ndlp->lock before calling
fc_remote_port_delete() to prevent stale rport-to-node references.

> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
> index c9b02d2c63054..a4e9a60ec162b 100644
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
[ ... ]
> @@ -4576,6 +4578,14 @@ lpfc_unregister_remote_port(struct lpfc_nodelist *ndlp)
>  			 ndlp->nlp_DID, rport, ndlp->fc4_xpt_flags,
>  			 kref_read(&ndlp->kref));
>  
> +	rdata = rport->dd_data;

[Severity: High]
This is a pre-existing issue, but won't this cause a NULL pointer dereference
if rport is NULL?

Looking at lpfc_nlp_unreg_node() in drivers/scsi/lpfc/lpfc_hbadisc.c,
ndlp->rport is checked outside of ndlp->lock:

    if (ndlp->rport &&
        ndlp->fc4_xpt_flags & SCSI_XPT_REGD) {

If a concurrent lpfc_dev_loss_tmo_callbk() acquires ndlp->lock and sets
ndlp->rport to NULL, lpfc_unregister_remote_port() will initialize its local
rport variable to NULL, leading to a crash here when dereferencing
rport->dd_data.

> +	spin_lock_irqsave(&ndlp->lock, iflags);
> +	if (rdata && rdata->pnode == ndlp)
> +		rdata->pnode = NULL;

[Severity: High]
Does unconditionally clearing rdata->pnode here break the devloss timeout
state machine and cause a permanent node leak?

Before this patch, fc_remote_port_delete() starts the SCSI transport's
devloss timer, which eventually calls lpfc_dev_loss_tmo_callbk(). That callback
relies on rdata->pnode to locate the ndlp and queue an LPFC_EVT_DEV_LOSS event,
which drops the initial node reference.

By setting rdata->pnode to NULL before deleting the rport, won't
lpfc_dev_loss_tmo_callbk() in drivers/scsi/lpfc/lpfc_hbadisc.c always see
a NULL pointer and return immediately:

    ndlp = ((struct lpfc_rport_data *)rport->dd_data)->pnode;
    if (!ndlp)
        return;

And thus never drop the initial reference, leaking the lpfc_nodelist memory?

[Severity: High]
This isn't a bug introduced by this patch, but does clearing rdata->pnode
under ndlp->lock actually prevent the use-after-free race in the devloss
callback?

The devloss callback in drivers/scsi/lpfc/lpfc_hbadisc.c reads the pointer
locklessly and doesn't take a reference:

    ndlp = ((struct lpfc_rport_data *)rport->dd_data)->pnode;

If lpfc_dev_loss_tmo_callbk() reads ndlp and then is preempted, a concurrent
lpfc_unregister_remote_port() could clear rdata->pnode and call
lpfc_nlp_put(ndlp), freeing the node. When the callback resumes, it would
access ndlp->vport and attempt to acquire ndlp->lock on a freed object.

> +	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