Re: [PATCH v5 04/56] scsi: qla2xxx: Add 29xx support in queue initialisation path

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch intentionally introduces broken pointer arithmetic and structure size mismatches for QLA29XX hardware, severely breaking bisectability.
- [Critical] Base queues are allocated with 64-byte entries, but initialized as 128-byte entries on 29xx hardware, causing severe DMA out-of-bounds access.

Pre-existing issues:
- [High] Unconditional call to dma_free_coherent() with a NULL virtual address in the queue allocation error paths.
- [High] A queue ID is cleared twice during initialization failure and its mapping is nullified without locks, causing a race condition that silently destroys a concurrently allocated queue.
- [High] If the firmware is unresponsive during teardown, queue deletion aborts and permanently leaks DMA memory and host structs.
--

--- Patch [4]: [PATCH v5 04/56] scsi: qla2xxx: Add 29xx support in queue initialisation path ---
commit eb9f04cb3a041d8d7db2e1662875c85a2485be0c
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add 29xx support in queue initialisation path

Extend the queue initialisation and multi-queue management mailbox
commands to include IS_QLA29XX() checks, following the same mailbox
interface as 27xx/28xx. Unlike earlier adapters that use 64-byte
request/response ring entries, 29xx uses 128-byte entries.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index c563133f751e8..e75b7ae22bc52 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -574,9 +574,13 @@ qla25xx_free_req_que(struct scsi_qla_host *vha, struct req_que *req)
>  {
>  	struct qla_hw_data *ha = vha->hw;
>  	uint16_t que_id = req->id;
> +	uint16_t reqsz;
> +
> +	reqsz = IS_QLA29XX(ha) ? sizeof(struct request_ext) :
> +				 sizeof(request_t);
>  
>  	dma_free_coherent(&ha->pdev->dev, (req->length + 1) *
> -		sizeof(request_t), req->ring, req->dma);
> +			  reqsz, req->ring, req->dma);

[Severity: High]
This isn't a bug introduced by this patch, but if dma_alloc_coherent()
fails in qla25xx_create_req_que() or qla25xx_create_rsp_que(), the code
branches to the que_failed label and calls qla25xx_free_req_que() or
qla25xx_free_rsp_que().

Will this unconditionally call dma_free_coherent() with a NULL virtual
address for req->ring or rsp->ring? Could this trigger a kernel oops or
WARN during low memory conditions?

[ ... ]

> @@ -706,6 +715,7 @@ qla25xx_create_req_que(struct qla_hw_data *ha, uint16_t options,
>  	uint16_t que_id = 0;
>  	device_reg_t *reg;
>  	uint32_t cnt;
> +	uint16_t reqsz;
>  
>  	req = kzalloc_obj(struct req_que);
>  	if (req == NULL) {
> @@ -714,9 +724,12 @@ qla25xx_create_req_que(struct qla_hw_data *ha, uint16_t options,
>  		goto failed;
>  	}
>  
> +	reqsz = IS_QLA29XX(ha) ? sizeof(struct request_ext) :
> +				 sizeof(request_t);
> +
>  	req->length = REQUEST_ENTRY_CNT_24XX;
>  	req->ring = dma_alloc_coherent(&ha->pdev->dev,
> -			(req->length + 1) * sizeof(request_t),
> +			(req->length + 1) * reqsz,
>  			&req->dma, GFP_KERNEL);

[Severity: Critical]
The commit message notes that pointer arithmetic for the wider entries is
handled by later patches in the series.

Does this mean this commit leaves req->out_ptr and rsp->in_ptr using 64-byte
pointer arithmetic on a 128-byte stride ring during initialization?

Could this place the DMA pointers exactly halfway through the allocated
ring buffer instead of at the end, causing the hardware to read and write
out of bounds, and potentially breaking bisectability if this specific commit
is tested on QLA29XX adapters?

[Severity: Critical]
While this allocates 128-byte stride entries for multi-queues on 29xx
hardware, what happens to the primary base queues (queue 0)?

Looking at qla2x00_mem_alloc() in qla_os.c, the base queues still appear to
be allocated with a hardcoded sizeof(request_t) which is 64 bytes.

When the driver probes the 29xx hardware and initializes these base queues
with a 128-byte stride, could the hardware read and write past the end of the
allocated DMA buffers?

[Severity: High]
This is a pre-existing issue, but looking at the error path in
qla25xx_create_req_que():

If qla25xx_init_req_que() fails, the code clears que_id from ha->req_qid_map
while holding ha->mq_lock, and then calls qla25xx_free_req_que().

Inside qla25xx_free_req_que():

    if (que_id) {
        ha->req_q_map[que_id] = NULL;
        mutex_lock(&ha->vport_lock);
        clear_bit(que_id, ha->req_qid_map);
        ...
    }

Could a concurrent thread re-allocate this same que_id before
qla25xx_free_req_que() sets ha->req_q_map[que_id] = NULL without holding
mq_lock? Would this silently destroy the concurrent allocation's queue pointer
and clear the bit again using the wrong lock (vport_lock)?

[Severity: High]
This is also a pre-existing issue, but looking at queue teardown:

In qla25xx_delete_req_que(), the driver attempts to notify the firmware by
calling qla25xx_init_req_que().

If the firmware is unresponsive (e.g. due to a crash or PCI error), does
this function return early and skip calling qla25xx_free_req_que()?

Furthermore, in qla25xx_delete_queues(), the loop processing all queues
aborts on the first failure. Could this permanently leak all subsequent
struct req_que allocations and their large DMA rings, leading to DMA pool
exhaustion after device resets?

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