Re: [PATCH v4 08/14] lpfc: Improve PLOGI retry handling for large SAN configurations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] lpfc: Deferred PLOGIs are erroneously rejected in lpfc_nlp_logo_unreg due to state check
- [Medium] lpfc: TOCTOU race in NLP_PLOGI_SND flag tracking allows multiple PLOGIs in flight
--

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

lpfc: Improve PLOGI retry handling for large SAN configurations

In large SAN configurations with link perturbations, rediscovery of target
ports is problematic due to PLOGI retry race conditions. This patch improves
target rediscovery by ensuring PLOGI retries are serialized in unregistration
and retry handler paths.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index cd431c7bd9f08..3e26654a8286a 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -2329,17 +2331,43 @@ lpfc_issue_els_plogi(struct lpfc_vport *vport, uint32_t did, uint8_t retry)
[ ... ]
> +	if (test_bit(NLP_PLOGI_SND, &ndlp->nlp_flag)) {
> +		lpfc_printf_vlog(vport, KERN_INFO,
> +				 LOG_ELS | LOG_NODE | LOG_DISCOVERY,
> +				 "4113 Reject PLOGI issue, PLOGI in-flight "
> +				 "x%px, DID x%x nflag x%lx\n",
> +				 ndlp, ndlp->nlp_DID, ndlp->nlp_flag);
> +		return 1;
> +	}

[Severity: Medium]
Can concurrent worker threads race on this lockless flag check?

If two threads execute lpfc_issue_els_plogi() simultaneously (for example,
from an unregistration completion and a timer), both could evaluate
test_bit(NLP_PLOGI_SND, &ndlp->nlp_flag) as false.

Both threads would then continue executing, allocate their IOCBs, and reach
the set_bit(NLP_PLOGI_SND, &ndlp->nlp_flag) call at the end of the function.
This sequence would issue duplicate PLOGI commands to the same NPort.

> +
> +	if (ndlp->nlp_state > NLP_STE_PLOGI_ISSUE &&
> +	    ndlp->nlp_state <= NLP_STE_MAPPED_NODE) {
> +		lpfc_printf_vlog(vport, KERN_INFO,
> +				 LOG_ELS | LOG_NODE | LOG_DISCOVERY,
> +				 "4114 Reject PLOGI issue, Node in "
> +				 "unexpected state x%px, DID x%x nflag x%lx "
> +				 "in State x%x\n",
> +				 ndlp, ndlp->nlp_DID,
> +				 ndlp->nlp_flag, ndlp->nlp_state);
> +		return 1;
>  	}

[Severity: High]
Will this strict state check inadvertently drop deferred PLOGIs during LOGO
unregistration?

During target recovery after a LOGO storm or link toggle, the unregistration
handler lpfc_nlp_logo_unreg() uses lpfc_issue_els_logo() to transition the
node state to NLP_STE_LOGO_ISSUE. 

When the deferred PLOGI is subsequently issued via lpfc_issue_els_plogi(),
this new check (ndlp->nlp_state > NLP_STE_PLOGI_ISSUE &&
ndlp->nlp_state <= NLP_STE_MAPPED_NODE) evaluates to true because
NLP_STE_LOGO_ISSUE (0x5) falls between 0x1 and 0x7. This causes the deferred
PLOGI to be erroneously rejected, potentially stalling the target discovery
process.

[ ... ]
> @@ -2410,11 +2438,26 @@ lpfc_issue_els_plogi(struct lpfc_vport *vport, uint32_t did, uint8_t retry)
[ ... ]
>  		lpfc_els_free_iocb(phba, elsiocb);
>  		lpfc_nlp_put(ndlp);
>  		return 1;
>  	}
>  
> +	set_bit(NLP_PLOGI_SND, &ndlp->nlp_flag);
>  	return 0;
>  }

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