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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The base queue (queue 0) allocation in `qla2x00_mem_alloc()` is not updated to allocate 128-byte entries for QLA29XX, leading to hardware DMA overruns.
- [Critical] The patch enlarges queue DMA buffers to 128-byte strides for 29xx but retains 64-byte pointer arithmetic, corrupting shadow pointers and fast-path iteration.

Pre-existing issues:
- [High] Inconsistent locking protecting the multiqueue mappings (`req_q_map`/`req_qid_map` and `rsp_q_map`/`rsp_qid_map`) leads to data races.
- [High] Pending response queue work (`qpair->q_work`) is not canceled before freeing the queue and its lock, leading to a Use-After-Free.
--

--- Patch [4]: [PATCH v3 04/88] scsi: qla2xxx: Add 29xx support in queue initialisation path ---
commit 4452b9447592dcc921780bd1cb74d843e0bd2ad1
Author: Manish Rangankar <mrangankar@marvell.com>
Subject: scsi: qla2xxx: Add 29xx support in queue initialisation path

This patch extends queue initialization to support the larger 128-byte
entries used by the 29xx adapters. It adds extended request and response
structures and updates the queue allocation sizes accordingly.

>     The enlarged entry stride doubles the DMA ring memory allocated for
>     both request and response queues on 29xx, and all ring pointer
>     arithmetic must account for the wider entries (handled by later
>     patches in this series).

[Severity: Critical]
Since the current patch enlarges the queue DMA buffers but leaves pointer
arithmetic for later patches, will shadow pointer calculations use the legacy
64-byte stride in the meantime?

For example, in qla25xx_create_req_que() and qla25xx_create_rsp_que():

    req->out_ptr = (uint16_t *)(req->ring + req->length);
    rsp->in_ptr = (uint16_t *)(rsp->ring + rsp->length);

Because req->ring is a 64-byte typed pointer, this will point to the middle
of the 128-byte stride array, which may miscalculate the hardware shadow
index. Additionally, fast-path iterations like rsp->ring_ptr++ in
qla24xx_process_response_queue() will implicitly advance by 64 bytes instead
of 128 bytes, which could cause the driver to interpret the zeroed padding
as the next response.

[Severity: Critical]
Also, it looks like the base queue allocation in qla2x00_mem_alloc() is not
updated to use the new 128-byte size:

    (*req)->ring = dma_alloc_coherent(&ha->pdev->dev,
        ((*req)->length + 1) * sizeof(request_t),
        &(*req)->dma, GFP_KERNEL);

Could this cause hardware DMA overruns during device probe if the base queue
is allocated at half the required size for a 29xx adapter? Is it safe to
separate these changes from the pointer arithmetic and base queue allocation
updates in the series?

> 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);
>  	req->ring = NULL;
>  	req->dma = 0;
>  	if (que_id) {

[Severity: High]
This is a pre-existing issue, but is there a data race when cleaning up
these queues?

In qla25xx_create_req_que(), the queue map allocations are protected by
ha->mq_lock. However, in qla25xx_free_req_que() and qla25xx_free_rsp_que(),
it looks like ha->req_q_map is modified without locks, and ha->req_qid_map
is cleared using ha->vport_lock:

    ha->req_q_map[que_id] = NULL;
    mutex_lock(&ha->vport_lock);
    clear_bit(que_id, ha->req_qid_map);
    mutex_unlock(&ha->vport_lock);

Could this mismatch allow concurrent queue creation and teardown to race,
potentially leading to array corruption or dual-allocation of the same
queue ID?

[ ... ]

> @@ -594,6 +598,10 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
>  {
>  	struct qla_hw_data *ha = vha->hw;
>  	uint16_t que_id = rsp->id;
> +	uint16_t rspsz;
> +
> +	rspsz = IS_QLA29XX(ha) ? sizeof(struct response_ext) :
> +				 sizeof(response_t);
>  
>  	if (rsp->msix && rsp->msix->have_irq) {
>  		free_irq(rsp->msix->vector, rsp->msix->handle);
> @@ -601,8 +609,9 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
>  		rsp->msix->in_use = 0;
>  		rsp->msix->handle = NULL;
>  	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this teardown path
need to cancel pending response queue work before freeing the DMA memory?

It looks like cancel_work_sync(&qpair->q_work) is not called before
dma_free_coherent() or before kfree(qpair) later on. If an interrupt fires
and queues the work just before free_irq(), could qla_do_work() execute
concurrently and try to access the freed qpair->rsp memory, resulting in a
use-after-free?

>  	dma_free_coherent(&ha->pdev->dev, (rsp->length + 1) *
> -		sizeof(response_t), rsp->ring, rsp->dma);
> +			  rspsz, rsp->ring, rsp->dma);
>  	rsp->ring = NULL;
>  	rsp->dma = 0;

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