Re: [PATCH v3 03/14] lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted cmd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fails to fully resolve the dma_unmap_sg_attrs oops race because the validation logic skips counting the I/O during the critical window before scsi_done() finishes.
- [High] Unprotected concurrent modifications to the bitfield `cmd_flag` cause a Read-Modify-Write (RMW) data race, which can lead to the loss of the `LPFC_DRIVER_ABORTED` flag and completely break the synchronization fix.
--

--- Patch [3]: [PATCH v3 03/14] lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted cmd ---
commit 6bc9dc760f9e39b87bc318946c28662acdff4498
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Fix kernel oops when unmapping scsi dma buffers for an aborted cmd

This commit updates the inflight I/O check in lpfc_sli_sum_iocb() to count
I/Os that have either LPFC_IO_ON_TXCMPLQ or LPFC_DRIVER_ABORTED set. This
is intended to prevent a race with scsi_done() during aborts.

> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 62a30a92b7927..10db07771ccff 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -12725,8 +12725,12 @@ lpfc_sli_sum_iocb(struct lpfc_vport *vport, uint16_t tgt_id, uint64_t lun_id,
>  
>  		if (!iocbq || iocbq->vport != vport)
>  			continue;
> -		if (!(iocbq->cmd_flag & LPFC_IO_FCP) ||
> -		    !(iocbq->cmd_flag & LPFC_IO_ON_TXCMPLQ))
> +		/* Only count FCP i/o */
> +		if (!(iocbq->cmd_flag & LPFC_IO_FCP))
> +			continue;
> +		/* Count i/o whilst LLDD retains an interest in the scsi_cmnd */
> +		if (!(iocbq->cmd_flag &
> +				(LPFC_IO_ON_TXCMPLQ | LPFC_DRIVER_ABORTED)))
>  			continue;

[Severity: High]
This isn't a bug introduced by this patch, but does this fully resolve the
dma_unmap_sg_attrs oops if the validation logic still skips the I/O when
pCmd is NULL?

In the completion paths, pCmd is set to NULL prior to calling scsi_done():

drivers/scsi/lpfc/lpfc_scsi.c:lpfc_scsi_cmd_iocb_cmpl() {
    ...
	lpfc_cmd->pCmd = NULL;
	spin_unlock(&lpfc_cmd->buf_lock);
    ...
	scsi_done(cmd);
}

When the error handling path calls lpfc_sli_sum_iocb(), it appears to rely
on lpfc_sli_validate_fcp_iocb() which excludes the I/O if pCmd is NULL:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli_validate_fcp_iocb() {
    ...
	if (lpfc_cmd->pCmd == NULL)
		return rc;
    ...
}

Could this cause the I/O to be ignored during the critical window before
scsi_done() finishes, allowing the counter to reach zero prematurely?

[Severity: High]
This is also a pre-existing issue, but can the LPFC_DRIVER_ABORTED flag be
lost due to a read-modify-write data race on cmd_flag?

The abort path sets LPFC_DRIVER_ABORTED non-atomically under hbalock, but
explicitly drops ring_lock:

drivers/scsi/lpfc/lpfc_scsi.c:lpfc_abort_handler() {
    ...
		spin_unlock(&pring_s4->ring_lock);
		ret_val = lpfc_sli4_issue_abort_iotag(phba, iocb,
						      lpfc_sli_abort_fcp_cmpl);
    ...
}

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli4_issue_abort_iotag() {
    ...
	cmdiocb->cmd_flag |= LPFC_DRIVER_ABORTED;
    ...
}

Meanwhile, the completion path clears LPFC_IO_ON_TXCMPLQ non-atomically
under ring_lock but without hbalock:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli_iocbq_lookup_by_tag() {
    ...
			cmd_iocb->cmd_flag &= ~LPFC_IO_ON_TXCMPLQ;
    ...
}

If an I/O completes exactly as the error handler issues an abort, could one
update overwrite the other, causing LPFC_DRIVER_ABORTED to be unset here
and breaking the synchronization fix?

>  
>  		/* Include counting outstanding aborts */

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