Re: [PATCH v3 11/88] scsi: qla2xxx: Update IO path to use 128-byte IOCBs for 29xx

[sashiko-bot@kernel.org]

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

New issues:
- [High] Legacy non-MQ `.start_scsi` execution path builds 64-byte IOCB structures inside 128-byte ring slots on 29xx adapters, corrupting fields.
- [High] Task Management, Abort, and ELS LOGO commands are incorrectly formatted using legacy 64-byte builders on 29xx hardware.
- [High] Status continuation (STATUS_CONT_TYPE) payloads and PUREX/FPIN continuations are truncated to ~60 bytes on 29xx adapters.
- [High] `qla27xx_copy_multiple_pkt()` incorrectly advances the response queue ring pointer by 64 bytes instead of 128 bytes on 29xx hardware.
- [High] `qla2x00_status_entry()` parses the 128-byte 29xx extended status IOCB as a legacy 64-byte IOCB, reading garbage values for SCSI status and sense data.

Pre-existing issues:
- [High] Double free and cross-allocator mismatch of `crc_ctx` in `qla2x00_sp_free_dma()`.
- [High] Unprotected access to `ha->req_q_map` and its associated ring structures in `qla25xx_copy_mqueues` causes a Use-After-Free during concurrent queue teardown.
--

--- Patch [11]: [PATCH v3 11/88] scsi: qla2xxx: Update IO path to use 128-byte IOCBs for 29xx ---
commit b89f951ad304cc6622ae64928f42f9fb462d5532
Author: Anil Gurumurthy <agurumurthy@marvell.com>

scsi: qla2xxx: Update IO path to use 128-byte IOCBs for 29xx

This patch updates the QLogic qla2xxx driver IO path to support 128-byte
extended IOCBs for 29xx-class adapters. It introduces structure definitions
and extended fast-path helpers to manage the new 128-byte ring stride, while
falling back to 64-byte structures for older hardware.

> diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
> index a7e3ec9bba47f..acb58daacf358 100644
> --- a/drivers/scsi/qla2xxx/qla_dbg.c
> +++ b/drivers/scsi/qla2xxx/qla_dbg.c
[ ... ]
> @@ -623,19 +626,19 @@ qla25xx_copy_mqueues(struct qla_hw_data *ha, void *ptr, __be32 **last_chain)
>  		q->chain_size = htonl(
>  		    sizeof(struct qla2xxx_mqueue_chain) +
>  		    sizeof(struct qla2xxx_mqueue_header) +
> -		    (req->length * sizeof(request_t)));
> +		    (req->length * req_entry_size));
>  		ptr += sizeof(struct qla2xxx_mqueue_chain);
>  
>  		/* Add header. */
>  		qh = ptr;
>  		qh->queue = htonl(TYPE_REQUEST_QUEUE);
>  		qh->number = htonl(que);
> -		qh->size = htonl(req->length * sizeof(request_t));
> +		qh->size = htonl(req->length * req_entry_size);
>  		ptr += sizeof(struct qla2xxx_mqueue_header);
>  
>  		/* Add data. */
> -		memcpy(ptr, req->ring, req->length * sizeof(request_t));
> -		ptr += req->length * sizeof(request_t);
> +		memcpy(ptr, req->ring, req->length * req_entry_size);
> +		ptr += req->length * req_entry_size;

[Severity: High]
This is a pre-existing issue, but is it possible for this unprotected memcpy 
to trigger a Use-After-Free during concurrent queue teardown?

Looking at qla25xx_copy_mqueues(), it iterates over ha->req_q_map and 
copies req->ring without holding ha->mq_lock. Concurrently, 
qla25xx_free_req_que() calls dma_free_coherent() on req->ring prior to 
acquiring ha->mq_lock to clear the map entry. If a firmware dump races with 
queue teardown, it might access already-freed DMA memory.

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index 9038f67234448..13882a399fbc5 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
[ ... ]
> @@ -1620,13 +1640,13 @@ qla24xx_start_scsi(srb_t *sp)
>  	cmd->host_scribble = (unsigned char *)(unsigned long)handle;
>  	req->cnt -= req_cnt;
>  
> -	cmd_pkt = (struct cmd_type_7 *)req->ring_ptr;
> +	cmd_pkt = (struct cmd_type_7 *)qla_req_ring_slot(ha, req);
>  	cmd_pkt->handle = make_handle(req->id, handle);
>  
>  	/* Zero out remaining portion of packet. */
>  	/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */
>  	clr_ptr = (uint32_t *)cmd_pkt + 2;
> -	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> +	memset(clr_ptr, 0, qla_req_entry_size(ha) - 8);
>  	cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
>  
>  	/* Set NPORT-ID and LUN number*/

[Severity: High]
Might this format a 64-byte cmd_type_7 layout into a 128-byte slot on 29xx 
hardware?

Because this legacy non-MQ path doesn't branch to the extended builder, it 
writes the 8-bit vp_index to offset 51 instead of the 16-bit vp_index at 
offset 48, and places DSDs at offset 56 instead of 80. This could cause 
the firmware to read empty data segments resulting in DMA faults.

[ ... ]
> @@ -2343,10 +2447,20 @@ __qla2x00_alloc_iocbs(struct qla_qpair *qpair, srb_t *sp)
>  		sp->handle = handle;
>  	}
>  
> -	/* Prep packet */
> +	/*
> +	 * Prep packet.  29xx posts into a 128-byte-strided ring via
> +	 * ring_ext_ptr; the first 8 bytes of struct request_ext overlay the
> +	 * common request_t header, so entry_count/handle writes are
> +	 * layout-compatible once we return the pkt as request_t *.
> +	 */
>  	req->cnt -= req_cnt;
> -	pkt = req->ring_ptr;
> -	memset(pkt, 0, REQUEST_ENTRY_SIZE);
> +	if (IS_QLA29XX(ha)) {
> +		pkt = (request_t *)req->ring_ext_ptr;
> +		memset(pkt, 0, REQUEST_ENTRY_SIZE_EXT);
> +	} else {

[Severity: High]
Does returning a 128-byte slot here for all commands lead to memory 
corruption in the legacy dispatch paths?

Functions like qla2x00_start_sp() dispatch Task Management, Abort, and ELS 
LOGO commands to legacy 24xx builders (e.g., qla24xx_els_logo_iocb) which 
cast this 128-byte slot to the 64-byte structs. This causes critical fields 
like vp_index to be written as 8-bit values at incorrect offsets, which 
could misroute the commands to the physical port instead of the intended 
NPIV virtual port.

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index c47c38e099ff8..e95fb0e59f387 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
[ ... ]
> @@ -287,13 +281,7 @@ int __qla_copy_purex_to_buffer(struct scsi_qla_host *vha,
>  				break;
>  			}
>  
> -			rsp_q->ring_index++;
> -			if (rsp_q->ring_index == rsp_q->length) {
> -				rsp_q->ring_index = 0;
> -				rsp_q->ring_ptr = rsp_q->ring;
> -			} else {
> -				rsp_q->ring_ptr++;
> -			}
> +			qla_rsp_ring_advance(rsp_q);
>  			no_bytes = (pending_bytes > sizeof(new_pkt->data)) ?
>  			    sizeof(new_pkt->data) : pending_bytes;

[Severity: High]
Could this truncate the extended PUREX payload continuations?

The 29xx hardware places up to ~120 bytes of payload per 128-byte extended 
IOCB slot. However, bounding the copy length against sizeof(new_pkt->data) 
uses the legacy 64-byte limit. Because qla_rsp_ring_advance() steps by 128 
bytes, this skips over and permanently drops the remaining ~70 bytes of 
payload in each extended continuation IOCB. This same truncation issue 
appears in qla27xx_copy_fpin_pkt().

[ ... ]
> @@ -4003,13 +3980,7 @@ void qla24xx_process_response_queue(struct scsi_qla_host *vha,
>  		pkt = (struct sts_entry_24xx *)rsp->ring_ptr;
>  		cur_ring_index = rsp->ring_index;
>  
> -		rsp->ring_index++;
> -		if (rsp->ring_index == rsp->length) {
> -			rsp->ring_index = 0;
> -			rsp->ring_ptr = rsp->ring;
> -		} else {
> -			rsp->ring_ptr++;
> -		}
> +		qla_rsp_ring_advance(rsp);
>  
>  		if (pkt->entry_status != 0) {
>  			if (qla2x00_error_entry(vha, rsp, (sts_entry_t *) pkt))

[Severity: High]
Are we parsing the 128-byte 29xx extended status IOCB as a legacy 64-byte 
IOCB here?

For standard SCSI completions on 29xx hardware, the firmware generates 
sts_entry_24xx_ext where fields like scsi_status are at offset 48. Because 
qla2x00_status_entry() was not updated with an IS_QLA29XX() branch, it 
casts to the 64-byte sts_entry_24xx and reads scsi_status from offset 22. 
On 29xx, this offset holds the wr_sa_index, causing random SA index values 
to be interpreted as SCSI errors.

Additionally, did we miss updating qla27xx_copy_multiple_pkt() to use the 
new stride-aware qla_rsp_ring_advance() macro? It appears to still use 
rsp_q->ring_ptr++ which only increments by 64 bytes on 29xx hardware, 
desynchronizing the consumer pointer.

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 948242f0088e7..146701445485f 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
[ ... ]
> @@ -2693,6 +2697,7 @@ static struct isp_operations qla29xx_isp_ops = {
>  	.read_optrom_region	= qla29xx_read_optrom_data,
>  	.write_optrom_region	= qla29xx_write_optrom_data,
>  	.get_flash_version	= qla24xx_get_flash_version,
> +	.start_scsi		= qla24xx_dif_start_scsi,
>  	.start_scsi_mq		= qla2xxx_dif_start_scsi_mq,
>  	.abort_isp		= qla2x00_abort_isp,
>  	.iospace_config		= qla83xx_iospace_config,

[Severity: High]
Will assigning the legacy qla24xx_dif_start_scsi handler here corrupt 
the extended 29xx hardware rings?

If multiqueue is disabled or the block layer invokes this non-MQ hook, it 
formats 64-byte layouts (cmd_type_7, cmd_type_crc_2) into the 128-byte 
slots fetched via qla_req_ring_slot(). This writes the 8-bit vp_index to 
offset 51, corrupting the 16-bit vp_index and ctrl_flags_2 expected by 
29xx hardware.

[Severity: High]
This is a pre-existing issue, but can qla2x00_sp_free_dma() trigger a double 
free of the crc_ctx buffer?

When cleaning up a T10-DIF protected SCSI command, it frees the context:

qla2x00_sp_free_dma() {
    ...
    if (sp->u.scmd.crc_ctx) {
        dma_pool_free(ha->dl_dma_pool, sp->u.scmd.crc_ctx,
                      sp->u.scmd.crc_ctx->crc_ctx_dma);
    }
    ...
}

The pointer sp->u.scmd.crc_ctx is not set to NULL after being freed to the 
DMA pool. A subsequent check for the same pointer evaluates to true and 
attempts to free it again via mempool_free(), corrupting the ctx_mempool 
slab pool.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707055435.2680300-1-njavali@marvell.com?part=11