Re: [PATCH v3 40/88] scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Queue ring buffer is freed before being removed from the global map, creating a race with asynchronous firmware dumps.
- [High] Pending queue pair worker (`q_work`) is not cancelled during a `qla2xxx_create_qpair()` failure, leading to a Use-After-Free of the `qpair` struct.
- [High] The queue pair `work_struct` (`q_work`) can be queued by the interrupt handler before it is initialized, causing a NULL pointer dereference or list corruption.
--

--- Patch [40]: [PATCH v3 40/88] scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking ---
commit 88d0670e7fb3084bb51e8aaca31241034a20bd22
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Fix queue teardown NULL dma_free and bitmap locking

This commit fixes queue teardown bugs in qla25xx_free_req_que() and
qla25xx_free_rsp_que(). It guards dma_free_coherent() with a NULL check
and safely locks map assignments under mq_lock. It also removes redundant
bitmap clears from the creation error paths.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index bb2521c052bf8..7072af5b42178 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -576,16 +576,17 @@ qla25xx_free_req_que(struct scsi_qla_host *vha, struct req_que *req)
>  	uint16_t que_id = req->id;
>  	size_t req_entry_size = qla_req_entry_size(ha);
>  
> -	dma_free_coherent(&ha->pdev->dev,
> -			  (req->length + 1) * req_entry_size,
> -			  req->ring, req->dma);
> +	if (req->ring)
> +		dma_free_coherent(&ha->pdev->dev,
> +				  (req->length + 1) * req_entry_size,
> +				  req->ring, req->dma);
>  	req->ring = NULL;

[Severity: High]
This is a pre-existing issue, but does clearing req->ring before removing the
queue from ha->req_q_map create a race condition?

If an asynchronous firmware error interrupt fires right after req->ring is
set to NULL:

ISR
ha->isp_ops->fw_dump()
    qla25xx_copy_mqueues()
        req = ha->req_q_map[que];
        if (!req)
            break;
        ...
        memcpy(ptr, req->ring, req->length * sizeof(request_t));
        
Could the firmware dump locklessly iterate ha->req_q_map, find the queue, and
dereference the newly NULL pointer or freed memory?

>  	req->dma = 0;
>  	if (que_id) {
> +		mutex_lock(&ha->mq_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);
> +		mutex_unlock(&ha->mq_lock);
>  	}

[ ... ]

> @@ -605,16 +606,17 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
>  		rsp->msix->handle = NULL;
>  	}
>  
> -	dma_free_coherent(&ha->pdev->dev,
> -			  (rsp->length + 1) * rsp_entry_size,
> -			  rsp->ring, rsp->dma);
> +	if (rsp->ring)
> +		dma_free_coherent(&ha->pdev->dev,
> +				  (rsp->length + 1) * rsp_entry_size,
> +				  rsp->ring, rsp->dma);
>  	rsp->ring = NULL;
>  	rsp->dma = 0;
>  	if (que_id) {
> +		mutex_lock(&ha->mq_lock);
>  		ha->rsp_q_map[que_id] = NULL;
> -		mutex_lock(&ha->vport_lock);
>  		clear_bit(que_id, ha->rsp_qid_map);
> -		mutex_unlock(&ha->vport_lock);
> +		mutex_unlock(&ha->mq_lock);
>  	}
>  	kfree(rsp);

[Severity: High]
This is a pre-existing issue, but could this teardown path lead to a
use-after-free of the qpair structure?

During a qla2xxx_create_qpair() failure where the response queue creation
succeeds but the request queue creation fails, the error path calls
qla25xx_free_rsp_que(). 

At this point, qpair->rsp->qpair is not yet assigned (it is assigned after
both queues are successfully created). Because of this, the check for
rsp->qpair evaluates to NULL, and cancel_work_sync(&rsp->qpair->q_work)
is bypassed.

If an early interrupt fired on the response queue's MSI-X vector before
teardown, the un-cancelled work item would remain active on ha->wq after
the qpair struct is freed.

[ ... ]

> @@ -942,9 +941,6 @@ qla25xx_create_rsp_que(struct qla_hw_data *ha, uint16_t options,
>  		if (ret != QLA_SUCCESS) {
>  			ql_log(ql_log_fatal, base_vha, 0x00e7,
>  			    "%s failed.\n", __func__);
> -			mutex_lock(&ha->mq_lock);
> -			clear_bit(que_id, ha->rsp_qid_map);
> -			mutex_unlock(&ha->mq_lock);
>  			goto que_failed;
>  		}
>  		vha->flags.qpairs_rsp_created = 1;

[Severity: High]
This is a pre-existing issue, but does this function safely initialize the
workqueue?

Earlier in qla25xx_create_rsp_que(), qla25xx_request_irq() is called, which
unmasks the MSI-X vector and activates the ISR qla2xxx_msix_rsp_q(). Then
qla25xx_init_rsp_que() activates the queue in firmware.

However, INIT_WORK(&qpair->q_work, qla_do_work) is not called until the
very end of the function.

If an interrupt fires immediately after IRQ registration and queue
activation, the ISR calls queue_work() on the work_struct which is completely
zeroed (from the kzalloc of qpair). Could this enqueueing of a
zero-initialized work_struct corrupt workqueue lists or cause a crash when
executing a NULL function pointer?

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