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

[sashiko-bot@kernel.org]

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