Re: [PATCH v6 3/8] 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:
- [High] scsi: lpfc: incomplete fix leaves DMA unmap oops window open during LUN reset
- [High] scsi: lpfc: data race on cmd_flag can lose LPFC_DRIVER_ABORTED
--

commit 675425a729cb7c6ac7d9c7e57d4ee3019d93591f
Author: Nigel Kirkland <nkirkland2304@gmail.com>

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

This commit attempts to fix a kernel oops at dma_unmap_sg_attrs by ensuring
inflight scsi I/Os are counted if either LPFC_IO_ON_TXCMPLQ or
LPFC_DRIVER_ABORTED is set. This is to keep the LLDD retaining an interest
in the scsi_cmnd and avoid premature unmapping.

> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index a167ad96d2a62..7a187d0efaa92 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -12739,8 +12739,12 @@ lpfc_sli_sum_iocb(struct lpfc_vport *vport, uint16_t tgt_id, uint64_t lun_id,
[ ... ]
> -		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]
Does this still leave a window open for the DMA unmap oops during LUN reset?

If a hardware completion arrives just before the SCSI error handler calls
lpfc_abort_handler(), the fast-path IRQ handler lpfc_sli_iocbq_lookup_by_tag()
removes the IOCB from TXCMPLQ and clears LPFC_IO_ON_TXCMPLQ.

Because the command is no longer on the TX queue, lpfc_abort_handler()
skips it and returns FAILED without ever setting LPFC_DRIVER_ABORTED:

drivers/scsi/lpfc/lpfc_scsi.c:lpfc_abort_handler() {
    ...
	/* the command is in process of being cancelled */
	if (!(iocb->cmd_flag & LPFC_IO_ON_TXCMPLQ)) {
		lpfc_printf_vlog(vport, KERN_WARNING, LOG_FCP,
			"3169 SCSI Layer abort requested I/O has been "
			"cancelled by LLD.\n");
		ret = FAILED;
		goto out_unlock_ring;
	}
    ...
}

If the SCSI error handler escalates to lpfc_device_reset_handler(), it
flushes I/Os by calling lpfc_sli_sum_iocb(). Since both LPFC_IO_ON_TXCMPLQ
and LPFC_DRIVER_ABORTED are 0, wouldn't lpfc_sli_sum_iocb() skip the
command and return 0?

This could allow the LUN reset handler to assume no commands are inflight
and finish, allowing the SCSI midlayer to free or repurpose the scsi_cmnd.
Finally, the delayed softirq completion handler lpfc_scsi_cmd_iocb_cmpl()
could execute and call lpfc_scsi_unprep_dma_buf() on the freed scsi_cmnd.


[Severity: High]
Can a data race on cmd_flag cause LPFC_DRIVER_ABORTED to be lost?

The cmd_flag is a 32-bit field updated using non-atomic bitwise operators.
The lpfc_sli_iocbq_lookup_by_tag() function clears LPFC_IO_ON_TXCMPLQ in
the completion handler holding only pring->ring_lock:

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

Concurrently, abort handlers like lpfc_sli_issue_abort_iotag() and
lpfc_sli4_issue_abort_iotag() set LPFC_DRIVER_ABORTED holding buf_lock
and hbalock, but explicitly without pring->ring_lock:

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

If LPFC_DRIVER_ABORTED is lost due to this race, lpfc_sli_sum_iocb()
could miss the command. This would bypass the logic introduced in this
patch and lead to early unload or unmap while hardware DMA is still active.

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