Re: [PATCH v5 09/10] lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: lpfc: memory leak of ndlp nodes on unsolicited PLOGI rejection
--

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

lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI

This commit updates the reference counting logic when rejecting an
unsolicited PLOGI to account for dynamically changing domain controller
FC_IDs. It modifies the cleanup path in lpfc_els_unsol_buffer to remove
references from an explicitly saved alloc_ndlp instead of ndlp.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 1793d9c6d8b80..0ca1659d5d102 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c

[ ... ]

> @@ -10580,7 +10582,9 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
>  			did, vport->port_state, ndlp->nlp_flag);
>  
>  		phba->fc_stat.elsRcvPLOGI++;
> +		alloc_ndlp = ndlp;
>  		ndlp = lpfc_plogi_confirm_nport(phba, payload, ndlp);
> +
>  		if (phba->sli_rev == LPFC_SLI_REV4 &&

[ ... ]

> @@ -10896,13 +10900,28 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
>  		stat.un.b.lsRjtRsnCodeExp = rjt_exp;
>  		lpfc_els_rsp_reject(vport, stat.un.lsRjtError, elsiocb, ndlp,
>  				    NULL);
> -		/* Remove the reference from above for new nodes. */
> -		if (newnode)
> -			lpfc_disc_state_machine(vport, ndlp, NULL,
> -					NLP_EVT_DEVICE_RM);
> +
> +		/* Remove the safety reference from routine start. */
> +		if (newnode) {
> +			/* alloc_ndlp is assigned for an unsolicited PLOGI from
> +			 * the ndlp allocated by this routine. The routine
> +			 * lpfc_plogi_confirm_nport may have swapped the initial
> +			 * ndlp to an existing ndlp. In that case, alloc_ndlp
> +			 * is the pre-swap ndlp that needs a DEVICE_RM because
> +			 * it is the newnode. For all other paths to lsrjt
> +			 * (non-PLOGI ELS) that were rejected by the early
> +			 * port_state check, alloc_ndlp is NULL and ndlp is
> +			 * still the originally allocated node.
> +			 */
> +			lpfc_disc_state_machine(vport,
> +						alloc_ndlp ? alloc_ndlp : ndlp,
> +						NULL, NLP_EVT_DEVICE_RM);

[Severity: High]
Does this code leak both struct lpfc_nodelist objects when rejecting an
unsolicited PLOGI from an unrecognized WWPN?

When the PLOGI is received, lpfc_els_unsol_buffer() allocates an initial
node (alloc_ndlp). Then lpfc_plogi_confirm_nport() allocates a new_ndlp:

drivers/scsi/lpfc/lpfc_els.c:lpfc_plogi_confirm_nport() {
	...
	new_ndlp = lpfc_nlp_init(vport, ndlp->nlp_DID);
	...
}

It swaps the states, places alloc_ndlp into NLP_STE_UNUSED_NODE, and returns
new_ndlp as ndlp.

If the PLOGI is then rejected (e.g., due to port state), this modified
cleanup path calls NLP_EVT_DEVICE_RM on alloc_ndlp instead of ndlp. This
seems to abandon new_ndlp (which is now assigned to ndlp) without dropping
its reference, causing it to leak.

Additionally, because alloc_ndlp is already in the NLP_STE_UNUSED_NODE state
after the swap, its cleanup handler (lpfc_device_rm_unused_node) might simply
return NLP_STE_FREED_NODE without dropping the reference, meaning alloc_ndlp
also leaks.

> +		}
>  	}
>

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