Re: [PATCH v3 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 3 potential issue(s) to consider:

New issues:
- [High] Issuing an unconditional abort on an already-completed ELS command introduces a tag-reuse race condition that can inadvertently abort a newly allocated command.

Pre-existing issues:
- [High] Deadlock in `lpfc_sli_abort_iocb_ring` due to non-recursive double acquisition of `pring->ring_lock`.
- [High] Data race and list corruption from unlocked iteration of `pring->txcmplq` in `lpfc_sli_host_down`.
--

--- Patch [9]: [PATCH v3 09/14] lpfc: Send inhibited ABORT_WQE when PLOGI CQE SEQUENCE_TMO is received ---
commit faea8f503d6a5b239d2ea58d59fefef913f41aeb
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Send inhibited ABORT_WQE when PLOGI CQE SEQUENCE_TMO is received

This commit updates the driver to send an inhibited ABORT_WQE when a PLOGI
CQE SEQUENCE_TIMEOUT is received. This allows the aborted CQE to return
immediately instead of waiting for a BA_ACC.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 16b10377a0953..11c36968a2bed 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -4893,7 +4897,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) {
> +
> +				/* if invalid ndlp, do not retry */
> +				if (unlikely(!ndlp)) {
> +					retry = 0;
> +					break;
> +				}
> +
> +				lpfc_printf_vlog(vport, KERN_INFO, LOG_ELS,
> +						 "0159 PLOGI Sequence TMO for "
> +						 "ndlp x%px x%lx x%x x%x x%x "
> +						 "x%x x%x %u x%x\n",
> +						 ndlp, ndlp->nlp_flag,
> +						 ndlp->nlp_DID, ndlp->nlp_type,
> +						 ndlp->nlp_fc4_type,
> +						 ndlp->nlp_rpi, ndlp->nlp_state,
> +						 kref_read(&ndlp->kref),
> +						 ndlp->fc4_xpt_flags);
> +
> +				/* Abort all outstanding ELS and auto-ABTS.  If
> +				 * no response for E_D_TOV, then it is unlikely
> +				 * auto-ABTS would receive a response too.  So,
> +				 * inhibit the abort for faster XRI release.
> +				 * However, still proceed with delayed retry.
> +				 */
> +				pring = lpfc_phba_elsring(phba);
> +				spin_lock_irqsave(&phba->hbalock, iflags);
> +				lpfc_sli_issue_abort_iotag(phba, pring, cmdiocb,
> +							   true, NULL);

[Severity: High]
Could this unconditional abort introduce a tag reuse race condition?

When a PLOGI command fails with a sequence timeout, this new logic issues
an asynchronous abort and then returns 1 to retry.

Because lpfc_els_retry() returns 1, the caller lpfc_cmpl_els_plogi()
immediately jumps to out_freeiocb and calls lpfc_els_free_iocb(), freeing
the command.

The command is returned to the free list since the exchange busy flag is
only set on the response, not the command itself. Its tag then becomes
available for reallocation. Since the abort is processed asynchronously by
the HBA, could the firmware erroneously abort a newly allocated ELS command
if the tag is reused quickly?

> +				spin_unlock_irqrestore(&phba->hbalock, iflags);
>  			}
> +
>  			retry = 1;
>  			delay = 100;
>  			break;
[ ... ]
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 54bc5d8984fc2..0a300efc92401 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -4608,7 +4608,8 @@ lpfc_sli_abort_iocb_ring(struct lpfc_hba *phba, struct lpfc_sli_ring *pring)
>  	} else {
>  		/* 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);

[Severity: High]
This is a pre-existing issue, but does this lead to a deadlock on the ring
lock?

In lpfc_sli_abort_iocb_ring(), the ring lock is acquired before this loop:

spin_lock_irq(plock);
list_for_each_entry_safe(iocb, next_iocb, &pring->txcmplq, list)

Inside the loop, lpfc_sli_issue_abort_iotag() is called, which then
evaluates the ring and unconditionally acquires the exact same lock for
SLI-4:

spin_lock_irqsave(&pring->ring_lock, iflags);

Since these spinlocks are not recursive, won't this cause an immediate
deadlock?

>  	}
>  	spin_unlock_irq(plock);
>  
[ ... ]
> @@ -11998,7 +11999,7 @@ lpfc_sli_host_down(struct lpfc_vport *vport)
>  				if (iocb->vport != vport)
>  					continue;
>  				lpfc_sli_issue_abort_iotag(phba, pring, iocb,
> -							   NULL);
> +							   false, NULL);

[Severity: High]
This is a pre-existing issue, but does this unlocked traversal of the
completion queue invite data races or list corruption?

In lpfc_sli_host_down(), the code explicitly drops the ring lock for SLI-4
just before this loop:

spin_unlock(&pring->ring_lock);
list_for_each_entry_safe(iocb, next_iocb, &pring->txcmplq, list) {
    ...
}

However, the completion queue is actively modified by concurrent completion
handlers like lpfc_sli_sp_handle_rspiocb() which rely on the ring lock for
synchronization. Is it safe to iterate this list while it could be
modified concurrently?

>  			}
>  			pring->flag = prev_pring_flag;
>  		}

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