Re: [PATCH v5 04/10] lpfc: Check fc4_xpt_flags before decrementing ndlp kref on FDISC error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] lpfc: TOCTOU race in lpfc_cmpl_els_fdisc() leads to Use-After-Free
--

commit 46ec54ed5cbeb514e9e9362594a41ca2f9b3aa3d
Author: Nigel Kirkland <nkirkland2304@gmail.com>

lpfc: Check fc4_xpt_flags before decrementing ndlp kref on FDISC error

The current initial kref count drop logic for an ndlp that fails FDISC
assumes that the ndlp has never registered with transport layer and thus
the lpfc_dev_loss_tmo_callbk never called.  However, a failed FDISC can
occur after a successful transport layer registration too.  So,
lpfc_dev_loss_tmo_callbk can occur and there is a potential use-after-free
on the ndlp.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 6f6394a0047c2..45aad4cd2dc80 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -11427,11 +11426,22 @@ lpfc_cmpl_els_fdisc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
[ ... ]
> -		/* drop initial reference */
> -		if (!test_and_set_bit(NLP_DROPPED, &ndlp->nlp_flag))
> +		/* If have not previously registered with transport layer and no
> +		 * LPFC_EVT_DEV_LOSS work pending, then drop initial reference.
> +		 * Otherwise, let the dev_loss_tmo_callbk drop the initial
> +		 * reference.
> +		 */
> +		if (!(ndlp->fc4_xpt_flags & (SCSI_XPT_REGD | NVME_XPT_REGD)) &&
> +		    !test_bit(NLP_IN_DEV_LOSS, &ndlp->nlp_flag) &&
> +		    !test_and_set_bit(NLP_DROPPED, &ndlp->nlp_flag))
>  			lpfc_nlp_put(ndlp);

[Severity: High]
Does this lockless check introduce a time-of-check to time-of-use race
condition that could lead to a use-after-free?

If an FDISC error occurs concurrently with a devloss timeout for the same
ndlp, lpfc_dev_loss_tmo_callbk() could execute on another CPU:

CPU1 in lpfc_dev_loss_tmo_callbk():
    set_bit(NLP_IN_DEV_LOSS, &ndlp->nlp_flag);

    spin_lock_irqsave(&ndlp->lock, iflags);
    /* ... */
    ndlp->fc4_xpt_flags &= ~SCSI_XPT_REGD;

Because set_bit() lacks a full memory barrier and spin_lock() provides only
an ACQUIRE barrier, could the clearing of SCSI_XPT_REGD become visible to
CPU2 before NLP_IN_DEV_LOSS is seen?

If CPU2 is executing this new check in lpfc_cmpl_els_fdisc() locklessly and
without read barriers, it might see both SCSI_XPT_REGD == 0 and
NLP_IN_DEV_LOSS == 0.

Could this cause CPU2 to erroneously evaluate the condition as true, execute
lpfc_nlp_put(ndlp), and prematurely drop the initial reference, leading to a
use-after-free when CPU1 later tries to use the ndlp pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928181757.21959-1-nkirkland2304@gmail.com?part=4