Re: [PATCH v3 04/14] 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:

Pre-existing issues:
- [High] Pre-existing vulnerabilities in `lpfc_cmpl_els_flogi` and `lpfc_sli4_seq_abort_rsp` due to missing NVMe checks and non-atomic `NLP_DROPPED` flag operations.
--

--- Patch [4]: [PATCH v3 04/14] lpfc: Check fc4_xpt_flags before decrementing ndlp kref on FDISC error ---
commit 0f11d01d4dc626992ac78f74f8c706c25c1fb010
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Check fc4_xpt_flags before decrementing ndlp kref on FDISC error

This patch modifies FDISC error handling to ensure we check whether a node
has been registered with a transport layer before decrementing the initial
kref count. If previously registered, it delegates the kref drop to the
transport's respective dev_loss_tmo callback to avoid a potential
use-after-free.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 52fc5058976d9..b756de9986ebe 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,
>  			goto out;
>  		/* Warn FDISC status */
>  		lpfc_vlog_msg(vport, KERN_WARNING, LOG_ELS,
> -			      "0126 FDISC cmpl status: x%x/x%x)\n",
> -			      ulp_status, ulp_word4);
> +			      "0126 FDISC cmpl status: (x%x/x%x) ndlp x%px "
> +			      "Data: x%lx x%x x%x x%x x%x x%x x%x x%x x%x\n",
> +			      ulp_status, ulp_word4, ndlp, ndlp->nlp_flag,
> +			      ndlp->nlp_DID, ndlp->nlp_last_elscmd,
> +			      ndlp->nlp_type, ndlp->nlp_rpi, ndlp->nlp_state,
> +			      ndlp->nlp_prev_state, ndlp->fc4_xpt_flags,
> +			      kref_read(&ndlp->kref));
>  
> -		/* 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]
This is a pre-existing issue, but since this patch updates the logic here to
use an atomic test_and_set_bit() for the NLP_DROPPED flag and checks for both
SCSI and NVMe transport registrations, it highlights similar vulnerable code
blocks elsewhere.

For instance, in the same file, lpfc_cmpl_els_flogi() uses a non-atomic
check and set:

drivers/scsi/lpfc/lpfc_els.c:lpfc_cmpl_els_flogi() {
    ...
	if (!test_bit(NLP_IN_DEV_LOSS, &ndlp->nlp_flag) &&
	    !test_bit(NLP_DROPPED, &ndlp->nlp_flag) &&
	    !(ndlp->fc4_xpt_flags & SCSI_XPT_REGD)) {
		set_bit(NLP_DROPPED, &ndlp->nlp_flag);
		lpfc_nlp_put(ndlp);
	}
    ...
}

Can concurrent threads (like a link flap triggering lpfc_dev_loss_tmo_callbk)
evaluate test_bit(NLP_DROPPED, ...) as false and concurrently double-put the
ndlp kref here?

Also, does this FLOGI path miss the NVME_XPT_REGD check entirely, potentially
allowing premature kref drops for NVMe-only registrations?

A similar non-atomic pattern appears in sequence abort handling:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli4_seq_abort_rsp() {
    ...
	if (ndlp->nlp_state == NLP_STE_UNUSED_NODE &&
	    !test_bit(NLP_DROPPED, &ndlp->nlp_flag) &&
	    !(ndlp->fc4_xpt_flags & (NVME_XPT_REGD | SCSI_XPT_REGD))) {
		set_bit(NLP_DROPPED, &ndlp->nlp_flag);
		lpfc_nlp_put(ndlp);
	}
    ...
}

Would it be worth updating these other paths to use the atomic
test_and_set_bit() and the unified transport checks as you did in this patch?

>  
>  		goto fdisc_failed;

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