Re: [PATCH v2 05/60] scsi: qla2xxx: Add 29xx support in queue initialisation path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free (UAF) in queue pair teardown due to missing work item cancellation.
--

commit a6fdb9faf346a1dfcb7285602fa98749fbabf7ba
Author: Manish Rangankar <mrangankar@marvell.com>

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

This commit extends the queue initialization and management to include 29xx
adapters. It introduces support for 128-byte request and response entries by
adding new extended structures and adjusting pointer arithmetic and allocations.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c

[ ... ]

> @@ -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 is a pre-existing issue, but can this lead to a use-after-free when
tearing down the queue pair?

The q_work item is initialized in qla25xx_create_rsp_que() to process
responses. During teardown, qla2xxx_delete_qpair() calls
qla25xx_free_rsp_que(), which unregisters the interrupt using free_irq()
as seen here.

While free_irq() waits for running hardirq handlers, it does not flush or
cancel work items already placed on the workqueue.

After qla25xx_free_rsp_que() returns, qla2xxx_delete_qpair() immediately
frees the qpair structure:

qla2xxx_delete_qpair() {
    ...
    mempool_destroy(qpair->srb_mempool);
    kfree(qpair);
    mutex_unlock(&ha->mq_lock);
}

If qpair->q_work was pending, it will later execute qla_do_work(),
dereferencing the freed qpair pointer to access vha and lock
qpair->qp_lock:

qla_do_work() {
    struct qla_qpair *qpair = container_of(work, struct qla_qpair, q_work);
    struct scsi_qla_host *vha = qpair->vha;

    spin_lock_irqsave(&qpair->qp_lock, flags);
    ...
}

This seems especially problematic in full adapter teardown, where
destroy_workqueue(ha->wq) forces pending work to run right after the queue
pairs are freed. Should a cancel_work_sync(&qpair->q_work) be added?

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

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