Re: [PATCH v6 7/8] lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] lpfc: Data race and list corruption in lpfc_findnode_did() due to lock mismatch
- [High] scsi: lpfc: NVMe remote port leak in PLOGI confirm swap
- [Medium] scsi: lpfc: memory leak of alloc_ndlp on PLOGI with existing non-NPR node
--

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

    lpfc: Update correct ndlp refcnt when rejecting an unsolicited PLOGI
    
    The domain controller FC_ID can change whenever a fabric switch with
    an ISL link is replaced or the admin reconfigures the fabric switch
    with active links. In such situations, reference count updates should
    be applied to the correct nodelist structure.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 5691af515b432..0e5038feec9a7 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c

[ ... ]

> @@ -10457,6 +10458,7 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
>  		      struct lpfc_vport *vport, struct lpfc_iocbq *elsiocb)
>  {
>  	struct lpfc_nodelist *ndlp;

[Severity: High]
This problem wasn't introduced by this patch, but can this lead to a data race
and list corruption when finding the remote node in lpfc_findnode_did()?

lpfc_findnode_did() protects the traversal of vport->fc_nodes using
shost->host_lock:

drivers/scsi/lpfc/lpfc_hbadisc.c:lpfc_findnode_did() {
	spin_lock_irqsave(&shost->host_lock, iflags);
	ndlp = __lpfc_findnode_did(vport, did);
	spin_unlock_irqrestore(&shost->host_lock, iflags);
}

However, the list is modified in lpfc_enqueue_node() and lpfc_dequeue_node()
using vport->fc_nodes_list_lock instead:

drivers/scsi/lpfc/lpfc_hbadisc.c:lpfc_enqueue_node() {
	spin_lock_irqsave(&vport->fc_nodes_list_lock, iflags);
	list_add_tail(&ndlp->nlp_listp, &vport->fc_nodes);
	spin_unlock_irqrestore(&vport->fc_nodes_list_lock, iflags);
}

Since callers of lpfc_enqueue_node() and lpfc_dequeue_node() do not hold
shost->host_lock, modifying the list concurrently with lpfc_findnode_did()
can corrupt list pointers and trigger a kernel panic during external ELS
command processing.

> +	struct lpfc_nodelist *alloc_ndlp = NULL;
>  	struct ls_rjt stat;
>  	u32 *payload, payload_len;
>  	u32 cmd = 0, did = 0, newnode, status = 0;

[ ... ]

> @@ -10555,7 +10557,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);

[Severity: High]
Does calling lpfc_plogi_confirm_nport() here leak the NVMe remote port tracking
structure?

When an unsolicited PLOGI arrives from an existing NVMe target or initiator,
the driver creates a new ndlp with a zeroed WWPN, so memcmp against zero
evaluates to true. In lpfc_plogi_confirm_nport(), it finds the existing
new_ndlp and swaps their nrport fields, storing the original in keep_nrport:

	keep_nrport = new_ndlp->nrport;
	new_ndlp->nrport = ndlp->nrport;

	/* Move this back to NPR state */
	if (memcmp(&ndlp->nlp_portname, name, sizeof(struct lpfc_name)) == 0) {
		/* The ndlp doesn't have a portname yet... */

In this branch, keep_nrport is bypassed. It is never restored to new_ndlp or
ndlp, nor is it freed, permanently leaking the pointer and leaving the NVMe
transport with orphaned state that can hang recovery or shutdown.

> +
>  		if (phba->sli_rev == LPFC_SLI_REV4 &&
>  		    test_bit(FC_PT2PT, &phba->pport->fc_flag)) {
>  			vport->fc_prevDID = vport->fc_myDID;

[ ... ]

> @@ -10871,13 +10875,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);
> +		}
>  	}
>  
> -	/* Release the reference on this elsiocb, not the ndlp. */
> +	/* This elsiocb is not the IOCB issued for a response. Remove
> +	 * the safety reference allocated earlier.
> +	 */
>  	lpfc_nlp_put(elsiocb->ndlp);
>  	elsiocb->ndlp = NULL;

[Severity: Medium]
Does this bypass the node drop and leak alloc_ndlp when the existing node is
not in NLP_STE_NPR_NODE?

When lpfc_plogi_confirm_nport() finds an existing node, alloc_ndlp takes the
pre-swap state of the existing node (e.g., NLP_STE_MAPPED_NODE). The inline
cleanup check at the end of lpfc_plogi_confirm_nport() evaluates to false
because the state is no longer NPR_NODE, bypassing the NLP_EVT_DEVICE_RM
event.

On the success path (rjt_err == 0), the routine exits after dropping the safety
reference here, leaving the un-dropped initial reference of alloc_ndlp leaked,
accumulating as a garbage node on the vport->fc_nodes list. Furthermore, on
the reject path, this patch calls NLP_EVT_DEVICE_RM on alloc_ndlp, but since
its state is no longer NPR_NODE, the event maps to lpfc_disc_illegal (a no-op)
and the node still leaks.

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