Re: [PATCH v1] scsi: qla2xxx: Cancel qpair work before freeing queues

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch adds `cancel_work_sync(&qpair->q_work)` before unregistering the MSI-X interrupt, leaving a race window where the active interrupt handler can re-queue the work, leading to a Use-After-Free when the queue pair is subsequently freed.

Pre-existing issues:
- [High] If deleting the request or response queue fails, `qla2xxx_delete_qpair()` bails out early, leaking the queue pair's software resources.
- [High] `request_irq()` is called before `INIT_WORK(&qpair->q_work, qla_do_work)`, allowing an early interrupt to enqueue a zero-allocated (uninitialized) `work_struct`.
- [High] Software memory and DMA rings for queues are permanently leaked when `qpairs_req_created` is false.
--

--- Patch [1]: [PATCH v1] scsi: qla2xxx: Cancel qpair work before freeing queues ---
commit 6aefba2c9206876be8f5e675b0612f13ef6a77b5
Author: Yuho Choi <dbgh9129@gmail.com>

scsi: qla2xxx: Cancel qpair work before freeing queues

This commit fixes an issue where the qpair response interrupt handler could
queue work with a deleted qpair context. It does this by marking the qpair
for deletion and cancelling its work earlier in the teardown and error paths.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index e746c9274cded..d5cd33e84720e 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
[ ... ]
> @@ -10028,6 +10034,8 @@ int qla2xxx_delete_qpair(struct scsi_qla_host *vha, struct qla_qpair *qpair)
>  	struct qla_hw_data *ha = qpair->hw;
>  
>  	qpair->delete_in_progress = 1;
> +	if (qpair->hw->wq)
> +		cancel_work_sync(&qpair->q_work);

[Severity: High]
Does this leave a window where an interrupt can re-queue the work before the
interrupt is unregistered?

Because qla2xxx_msix_rsp_q() unconditionally queues the work without checking
delete_in_progress:

drivers/scsi/qla2xxx/qla_isr.c:qla2xxx_msix_rsp_q() {
    ...
    queue_work(ha->wq, &qpair->q_work);
    return IRQ_HANDLED;
}

And the interrupt isn't unregistered until later in the teardown when
qla25xx_delete_rsp_que() calls free_irq(), could a late interrupt fire right
after cancel_work_sync() and re-queue the work, leading to a use-after-free
when the qpair is eventually freed?

>  
>  	qla_free_buf_pool(qpair);
>  
>  	ret = qla25xx_delete_req_que(vha, qpair->req);
>  	if (ret != QLA_SUCCESS)
>  		goto fail;

[Severity: High]
This is a pre-existing issue, but could this early bailout leak the queue
pair's software resources?

If qla25xx_delete_req_que() times out or fails (e.g., due to unresponsive
hardware) and returns QLA_FUNCTION_FAILED, it takes the goto fail path and
returns early. Will this skip freeing the remaining DMA buffers, kernel
structures, and MSI-X vectors that are typically cleaned up unconditionally
at the bottom of qla2xxx_delete_qpair()?


I also noticed a few pre-existing issues in related code:

drivers/scsi/qla2xxx/qla_mid.c:qla25xx_create_rsp_que() {
    ...
	ret = qla25xx_request_irq(ha, qpair, qpair->msix);
	if (ret)
		goto que_failed;

	if (startqp) {
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but can an early interrupt
trigger a panic here?

Since the interrupt is enabled by request_irq() before INIT_WORK() initializes
qpair->q_work at the end of the function, could an interrupt fire immediately
and cause the handler to enqueue a zero-allocated (uninitialized) work_struct,
leading to a NULL pointer dereference in the workqueue worker thread?


drivers/scsi/qla2xxx/qla_mid.c:qla25xx_delete_req_que() {
	...
	if (req && vha->flags.qpairs_req_created) {
		req->options |= BIT_0;
		ret = qla25xx_init_req_que(vha, req);
		if (ret != QLA_SUCCESS)
			return QLA_FUNCTION_FAILED;

		qla25xx_free_req_que(vha, req);
	}

	return ret;
}

[Severity: High]
This is a pre-existing issue, but does this function permanently leak memory
when qpairs_req_created is false?

If a queue pair is created with startqp=false, the rings are allocated but
vha->flags.qpairs_req_created remains unset. During teardown, this function
evaluates the flag to false, returns QLA_SUCCESS early, and completely skips
qla25xx_free_req_que(). Will this leak the DMA rings (req->ring) and queue
structures upon driver unbind?

The same pattern also exists in qla25xx_delete_rsp_que() when
qpairs_rsp_created is false.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802230039.586918-1-dbgh9129@gmail.com?part=1