[PATCH] scsi: lpfc: Add NULL check for vport in lpfc_dev_loss_tmo_callbk

[PATCH] scsi: lpfc: Add NULL check for vport in lpfc_dev_loss_tmo_callbk

[Vaibhav Nagare]

Fix a kernel NULL pointer dereference in lpfc_dev_loss_tmo_callbk()
when ndlp->vport is NULL during FC remote port deletion.

The crash occurs during fc_rport_final_delete() when the vport has
already been cleared on the ndlp structure, but the dev_loss_tmo
callback is still invoked. This is a race condition during port
removal where the vport is destroyed before the rport cleanup completes.

The existing code checks if ndlp is NULL but does not verify that
ndlp->vport is valid before dereferencing it to access vport->phba.

Crash signature observed on RHEL 8.10 (4.18.0-553.125.1.el8_10.x86_64):
  BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
  RIP: lpfc_dev_loss_tmo_callbk+0x54
  Call Trace:
   fc_rport_final_delete+0xea/0x1d0 [scsi_transport_fc]
   process_one_work+0x1d3/0x390
   worker_thread+0x30/0x390

Add a NULL check for vport after retrieving it from ndlp. When vport
is NULL, the port is in a teardown state, so we log the condition,
clear the pnode reference to prevent stale pointers, and return early.

Note: Commit 1cced5779e7a ("scsi: lpfc: Ensure HBA_SETUP flag is used
only for SLI4 in dev_loss_tmo_callbk") addressed a different issue
further down in the function and does not prevent this crash.

Signed-off-by: Vaibhav Nagare <vnagare@redhat.com>
---
 drivers/scsi/lpfc/lpfc_hbadisc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
index f3a85f6c796e..b8649c40b537 100644
--- a/drivers/scsi/lpfc/lpfc_hbadisc.c
+++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -168,6 +168,22 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
 		return;
 
 	vport = ndlp->vport;
+	if (!vport) {
+		/*
+		 * Vport is NULL - this can happen during teardown when the
+		 * vport has been destroyed but the rport final delete is
+		 * still processing. Clear the association and return.
+		 */
+		pr_err("lpfc: Null vport on ndlp %p, DID x%06x rport %p\n",
+		       ndlp, ndlp->nlp_DID, rport);
+
+		spin_lock_irqsave(&ndlp->lock, iflags);
+		((struct lpfc_rport_data *)rport->dd_data)->pnode = NULL;
+		ndlp->rport = NULL;
+		spin_unlock_irqrestore(&ndlp->lock, iflags);
+		return;
+	}
+
 	phba  = vport->phba;
 
 	lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_RPORT,
-- 
2.54.0

Re: [PATCH] scsi: lpfc: Add NULL check for vport in lpfc_dev_loss_tmo_callbk

[sashiko-bot]

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

New issues:
- [High] The patch causes a reference count leak for the `ndlp` node list entry and the remote port device `rport` by clearing their association pointers without dropping their respective references.
- [Low] The patch introduces build failures due to an undeclared variable `iflags` and a non-existent struct member `ndlp->lock`.

Pre-existing issues:
- [High] The patch misdiagnoses the root cause of a crash as a simple NULL pointer dereference and worsens an existing Use-After-Free race condition by attempting to lock and read from an already-freed `ndlp` object.
--

commit 26e4dbf45e788bb3e83d872e9f277d137a11dde1
Author: Vaibhav Nagare <nagarevaibhav@gmail.com>
Subject: scsi: lpfc: Add NULL check for vport in lpfc_dev_loss_tmo_callbk

This commit attempts to fix a kernel NULL pointer dereference in
lpfc_dev_loss_tmo_callbk() when ndlp->vport is NULL during FC remote
port deletion. It adds a NULL check, logs the teardown state, clears
the node pointers, and returns early.

> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
> index bfc2442dd74a5..7c487508916e3 100644
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
> @@ -118,6 +118,22 @@ lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
>  		return;
>  
>  	vport = ndlp->vport;
> +	if (!vport) {
> +		/*
> +		 * Vport is NULL - this can happen during teardown when the
> +		 * vport has been destroyed but the rport final delete is
> +		 * still processing. Clear the association and return.
> +		 */
> +		pr_err("lpfc: Null vport on ndlp %p, DID x%06x rport %p\n",
> +		       ndlp, ndlp->nlp_DID, rport);

[Severity: High]
This isn't a bug introduced by this patch, but if ndlp->vport is NULL, does
that indicate the ndlp object has already been freed?

Since vport is typically set during node allocation and never legitimately
cleared, wouldn't evaluating it as NULL mean we are dealing with freed
memory? If so, does dereferencing ndlp->nlp_DID here log from already-freed
memory, exacerbating the use-after-free race condition?

> +
> +		spin_lock_irqsave(&ndlp->lock, iflags);

[Severity: Low]
Are iflags and ndlp->lock defined in this context?

Looking at lpfc_dev_loss_tmo_callbk(), iflags does not appear to be
declared. Additionally, looking at struct lpfc_nodelist, it doesn't seem to
have a lock member. Could this cause a build failure?

> +		((struct lpfc_rport_data *)rport->dd_data)->pnode = NULL;
> +		ndlp->rport = NULL;
> +		spin_unlock_irqrestore(&ndlp->lock, iflags);
> +		return;

[Severity: High]
Does returning here leak the references for the ndlp and rport objects?

The existing teardown code lower in this function handles unloading like
this:

    if (vport->load_flag & FC_UNLOADING) {
        put_node = rdata->pnode != NULL;
        ...
        rdata->pnode = NULL;
        ndlp->rport = NULL;
        if (put_node)
            lpfc_nlp_put(ndlp);
        ...
    }

By clearing the association pointers here without invoking
lpfc_nlp_put(ndlp) and put_device(&rport->dev), will we permanently leak
these references?

> +	}
> +
>  	phba  = vport->phba;
>  
>  	lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_RPORT,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629160557.586208-1-vnagare@redhat.com?part=1