Re: [PATCH] scsi: smartpqi: cancel pending workers before freeing controller

[sashiko-bot@kernel.org]

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

New issues:
- [High] Canceling event_work during controller teardown introduces an unrecoverable deadlock if the worker is blocked in pqi_wait_if_ctrl_blocked().

Pre-existing issues:
- [Critical] Concurrent execution of `pqi_fail_all_outstanding_requests` can lead to double completion of SCSI commands, causing a use-after-free and double-free vulnerability.
- [High] Incomplete teardown ordering allows `event_work` to re-arm `rescan_work` after it has been cancelled, leading to a Use-After-Free when `ctrl_info` is freed.
- [High] `pqi_suspend_or_freeze()` races with `ctrl_offline_work`, causing a double free of IRQ vectors.
--

commit 038afb1987e7de8ef32f28b104faf6296b50e850
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: smartpqi: cancel pending workers before freeing controller

The smartpqi driver does not cancel pending workers during controller
teardown, leading to use-after-free bugs when queued workers dereference a
freed controller. This patch drains the workers in pqi_free_ctrl_resources()
using disable_work_sync() and cancel_work_sync() in an order that prevents
arming chains from surviving the teardown.

> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
> index 5ec583dc2e7df..b16c50761f1ca 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -8897,7 +8897,16 @@ static void pqi_free_interrupts(struct pqi_ctrl_info *ctrl_info)
>  
>  static void pqi_free_ctrl_resources(struct pqi_ctrl_info *ctrl_info)
>  {
> +	/*
> +	 * Disable, not cancel: the callback also frees IRQs, and
> +	 * pqi_take_ctrl_offline() can re-queue it after a cancel.
> +	 */
> +	disable_work_sync(&ctrl_info->ctrl_offline_work);

[Severity: Critical]
This is a pre-existing issue, but can concurrent execution of
pqi_fail_all_outstanding_requests() lead to double completion of SCSI
commands?

pqi_fail_all_outstanding_requests() iterates over the I/O request pool and
completes requests using a non-atomic check:

smartpqi_init.c:pqi_fail_all_outstanding_requests() {
    ...
    if (atomic_read(&io_request->refcount) == 0)
        continue;
    ...
}

If a hardware error triggers ctrl_offline_work while a surprise removal
triggers pqi_remove_ctrl(), both threads can execute
pqi_fail_all_outstanding_requests() concurrently.

Both threads might see a non-zero refcount and invoke io_complete_callback
for the same request, leading to double frees and data corruption.

[Severity: High]
This is a pre-existing issue, but can pqi_suspend_or_freeze() race with
ctrl_offline_work and cause a double free of IRQ vectors?

pqi_suspend_or_freeze() calls pqi_free_irqs() without draining
ctrl_offline_work. If a hardware error queues ctrl_offline_work just before
suspend, the worker runs concurrently and also calls pqi_free_irqs().

Because pqi_free_irqs() lacks locking:

smartpqi_init.c:pqi_free_irqs() {
    for (i = 0; i < ctrl_info->num_msix_vectors_initialized; i++)
        free_irq(pci_irq_vector(ctrl_info->pci_dev, i),
                 &ctrl_info->queue_groups[i]);
    ...
}

Both threads can simultaneously read a positive num_msix_vectors_initialized
and call free_irq() on the same MSI-X vectors.

>  	pqi_free_interrupts(ctrl_info);
> +	/* IRQs queue event_work, which can queue OFA work. */
> +	cancel_work_sync(&ctrl_info->event_work);

[Severity: High]
Does this cancellation introduce a deadlock if the event worker is currently
blocked?

In pqi_remove_ctrl(), pqi_ctrl_block_requests() is called which sets
block_requests to true. 

If event_work is executing, its callback pqi_event_worker() unconditionally
waits for block_requests to become false:

smartpqi_init.c:pqi_event_worker() {
    ...
    pqi_wait_if_ctrl_blocked(ctrl_info);
    ...
}

Since block_requests is never cleared during teardown, the worker blocks
forever. cancel_work_sync() will then wait forever for the worker to finish,
causing a deadlock.

[Severity: High]
This is a pre-existing issue, but does the teardown order allow event_work
to re-arm rescan_work after it was cancelled?

In pqi_remove_ctrl(), pqi_cancel_rescan_worker() cancels rescan_work before
pqi_free_ctrl_resources() drains event_work here.

However, event_work can arm rescan_work via
pqi_schedule_rescan_worker_with_delay():

smartpqi_init.c:pqi_schedule_rescan_worker_with_delay() {
    if (pqi_ctrl_offline(ctrl_info))
        return;
    schedule_delayed_work(&ctrl_info->rescan_work, delay);
}

If event_work checks pqi_ctrl_offline() and sees false, but is preempted
while pqi_remove_ctrl() sets the controller offline and cancels rescan_work,
the resumed event_work will queue rescan_work again.

This newly queued work is leaked and could cause a use-after-free when the
controller is freed.

> +	cancel_work_sync(&ctrl_info->ofa_memory_alloc_work);
> +	cancel_work_sync(&ctrl_info->ofa_quiesce_work);
>  	if (ctrl_info->queue_memory_base)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701142757.8447-1-fanwu01@zju.edu.cn?part=1