Re: [PATCH v4 09/14] lpfc: Send inhibited ABORT_WQE when PLOGI CQE SEQUENCE_TMO is received

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: lpfc: txcmplq list corruption in lpfc_sli_abort_iocb_ring()
- [High] scsi: lpfc: unintended abort of reused XRI/IoTag in lpfc_els_retry()
--

commit d8f6ae5839dc18257873d48c2e2262ffa79b7bec
Author: Nigel Kirkland <nkirkland2304@gmail.com>
Subject: lpfc: Send inhibited ABORT_WQE when PLOGI CQE SEQUENCE_TMO is received

This commit modifies the LPFC driver to explicitly set the Inhibit Abort (IA)
bit when sending an ABORT_WQE for a timed-out PLOGI CQE. It introduces a
boolean argument to lpfc_sli_issue_abort_iotag to control this behavior.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -4921,7 +4925,38 @@ lpfc_els_retry(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
>  				/* Reset the Link */
>  				link_reset = 1;
>  				break;
> +			} else if (cmd == ELS_CMD_PLOGI) {
[ ... ]
> +				pring = lpfc_phba_elsring(phba);
> +				spin_lock_irqsave(&phba->hbalock, iflags);
> +				lpfc_sli_issue_abort_iotag(phba, pring, cmdiocb,
> +							   true, NULL);
> +				spin_unlock_irqrestore(&phba->hbalock, iflags);

[Severity: High]
Could this inadvertently abort an unrelated command?

Because lpfc_els_retry() schedules a retry, the completion handler
lpfc_cmpl_els_plogi() will immediately free cmdiocb locally, returning the
XRI/IoTag to the free pool.

Since the abort is issued here without validating if the hardware actually
kept the exchange busy (LPFC_EXCHANGE_BUSY) or checking if this is SLI-4
hardware, another CPU could concurrently allocate the freed tag, issue a new
command, and have it erroneously aborted.

>  			}
> +
>  			retry = 1;
>  			delay = 100;
>  			break;
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
[ ... ]
> @@ -4620,12 +4621,19 @@ lpfc_sli_abort_iocb_ring(struct lpfc_hba *phba, struct lpfc_sli_ring *pring)
>  			iocb->cmd_flag &= ~LPFC_IO_ON_TXCMPLQ;
>  		list_splice_init(&pring->txcmplq, &tx_completions);
>  		pring->txcmplq_cnt = 0;
> +		spin_unlock_irqrestore(plock, iflag);
>  	} else {
> +		/* lpfc_sli_issue_abort_iotag expects the hba_lock held, but not
> +		 * the ring_lock.
> +		 */
> +		spin_unlock_irqrestore(plock, iflag);
> +		spin_lock_irqsave(&phba->hbalock, iflag);
>  		/* Issue ABTS for everything on the txcmplq */
>  		list_for_each_entry_safe(iocb, next_iocb, &pring->txcmplq, list)
> -			lpfc_sli_issue_abort_iotag(phba, pring, iocb, NULL);
> +			lpfc_sli_issue_abort_iotag(phba, pring, iocb, false,
> +						   NULL);
> +		spin_unlock_irqrestore(&phba->hbalock, iflag);

[Severity: High]
Does this code risk list corruption on the txcmplq list?

In lpfc_sli_abort_iocb_ring(), plock (which aliases ring_lock for SLI-4
hardware) is dropped, leaving txcmplq unprotected against concurrent removals
by completion handlers.

While list_for_each_entry_safe() protects against the current element being
removed, it is not thread-safe if a concurrent interrupt handler removes
next_iocb.

Could this lead to poisoned or corrupted list pointers during host reset or
link down events?

>  	}

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