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

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

[Fan Wu]

The smartpqi driver schedules four struct work_struct members of
struct pqi_ctrl_info on the system workqueue: ctrl_offline_work,
event_work, ofa_memory_alloc_work and ofa_quiesce_work.
ctrl_offline_work is armed by pqi_take_ctrl_offline(), reached from the
heartbeat timer handler and the error paths; event_work is queued by the
event-interrupt handler, and the event worker can in turn queue the two
OFA workers while processing OFA events.

None of these workers is cancelled during controller teardown.
pqi_remove_ctrl() stops the heartbeat timer and cancels the rescan and
update_time delayed workers, then calls pqi_free_ctrl_resources(), which
frees the interrupts and finally the controller in pqi_free_ctrl_info()
(kfree).  A worker queued by the timer or the event handler can therefore
run after the controller has been freed:

    CPU0 (teardown)                 CPU1 (system_wq)
    heartbeat timer / event IRQ
    pqi_take_ctrl_offline()
      schedule_work(ctrl_offline_work)
    pqi_remove_ctrl()
      ...
      pqi_free_ctrl_resources()
        free_irq()
        kfree(ctrl_info)            pqi_ctrl_offline_worker()
                                      container_of(work) -> freed ctrl_info
                                      pqi_take_ctrl_offline_deferred()
                                        dereferences ctrl_info  UAF

Clearing controller_online does not stop an already-queued
ctrl_offline_work: pqi_ctrl_offline_worker() and its callee
pqi_take_ctrl_offline_deferred() never check it and dereference the
controller unconditionally.  pqi_event_worker() and the two OFA workers
recover the controller with container_of() and dereference it the same way.

Drain the workers in pqi_free_ctrl_resources(), in an order that drains
each layer of the arming chain.

ctrl_offline_work is disabled with disable_work_sync() rather than
cancelled.  Its armer, pqi_take_ctrl_offline(), checks controller_online
and then, still in the same call, clears it and calls schedule_work(); an
interrupt or error handler that has already passed that check will queue
ctrl_offline_work even after pqi_remove_ctrl() has cleared
controller_online, so a plain cancel_work_sync() can return before that
in-flight armer queues the work.  The worker's callback,
pqi_take_ctrl_offline_deferred(), also calls pqi_free_interrupts(), so an
instance running during teardown would race the
num_msix_vectors_initialized counter in pqi_free_irqs() and call
free_irq() twice for the same vectors.  disable_work_sync() drains any
queued or running instance and makes later schedule_work() calls on it
no-ops, so the worker can neither run during the teardown's own
pqi_free_interrupts() nor survive until the controller is freed.

The remaining three workers are cancelled after pqi_free_interrupts().
Freeing the IRQs first stops the handler from arming event_work, draining
event_work next stops the event worker from arming the OFA workers, and
the OFA workers are drained last: each cancel is a final drain with no
re-arm race.

cancel_work_sync() is the correct primitive for the remaining work_struct
items: it cancels a pending instance and waits for a running callback to
return.  pqi_free_ctrl_resources() is reached only through the
pqi_remove_ctrl() chokepoint shared by pqi_pci_remove() and the probe
error path, and the four work_structs are initialised in
pqi_alloc_ctrl_info(), so draining here always operates on initialised
work.

Fixes: 5f310425c8ea ("scsi: smartpqi: update rescan worker")
Cc: stable@vger.kernel.org # needs adjustments for <= 6.6: use cancel_work_sync()
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/scsi/smartpqi/smartpqi_init.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -8891,7 +8891,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);
 	pqi_free_interrupts(ctrl_info);
+	/* IRQs queue event_work, which can queue OFA work. */
+	cancel_work_sync(&ctrl_info->event_work);
+	cancel_work_sync(&ctrl_info->ofa_memory_alloc_work);
+	cancel_work_sync(&ctrl_info->ofa_quiesce_work);
 	if (ctrl_info->queue_memory_base)
 		dma_free_coherent(&ctrl_info->pci_dev->dev,
 			ctrl_info->queue_memory_length,

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

[sashiko-bot]

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

[PATCH v2] scsi: smartpqi: drain controller workers before freeing controller

[Fan Wu]

pqi_remove_ctrl() stops the heartbeat timer and cancels the rescan and
update_time delayed workers, but the controller can still have
ctrl_offline_work, event_work, ofa_memory_alloc_work and ofa_quiesce_work
queued (armed from the heartbeat timer and IRQ/error paths). These recover
struct pqi_ctrl_info through container_of and dereference it, so a worker
that runs after pqi_free_ctrl_info() frees the controller is a
use-after-free.

Drain them in pqi_free_ctrl_resources(). pqi_event_worker() starts by
wait_event()ing on block_requests, which pqi_remove_ctrl() sets and never
clears in the remove path, so cancelling it while blocked would wait
forever; unblock first. The controller is already offline by then
(controller_online was cleared earlier in pqi_remove_ctrl()), so the
released worker returns without rearming rescan or OFA work.

ctrl_offline_work is disabled rather than cancelled because
pqi_take_ctrl_offline() can re-queue it. Its callback also calls
pqi_free_interrupts(), so it must be drained before the teardown's own
pqi_free_interrupts() to avoid a concurrent double free_irq() on the same
MSIX vectors.

After the IRQs are freed and event_work is drained, cancel the rescan
worker again: an event worker that passed the pqi_ctrl_offline() check
before the controller went offline may still have scheduled rescan via
pqi_schedule_rescan_worker_with_delay(). The OFA workers are cancelled
last; their sole producer is event_work, already drained.

This bug was found by static analysis.

Fixes: 5f310425c8ea ("scsi: smartpqi: update rescan worker")
Cc: stable@vger.kernel.org # needs adjustments for <= 6.6: use cancel_work_sync()
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

---

Changes from v1:
- Fixed deadlock: cancel_work_sync(event_work) while block_requests was
  set could wait forever; now unblocks first.
- Moved disable_work_sync(ctrl_offline_work) before pqi_free_interrupts()
  to avoid concurrent double free_irq() with the offline worker's own
  interrupt freeing.
- Cancel rescan_work again after draining event_work, to catch a rescan
  armed by an event worker that passed the offline check before the
  controller went offline.
---
 drivers/scsi/smartpqi/smartpqi_init.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index fe549e2b7c94..14f85fb95dfa 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -8891,7 +8891,19 @@ static void pqi_free_interrupts(struct pqi_ctrl_info *ctrl_info)
 
 static void pqi_free_ctrl_resources(struct pqi_ctrl_info *ctrl_info)
 {
+	/*
+	 * Release blocked workers first.  Disable ctrl_offline_work before
+	 * freeing IRQs because its callback can also free them.  event_work
+	 * can requeue rescan_work, so drain event_work before cancelling
+	 * rescan again.
+	 */
+	pqi_ctrl_unblock_requests(ctrl_info);
+	disable_work_sync(&ctrl_info->ctrl_offline_work);
 	pqi_free_interrupts(ctrl_info);
+	cancel_work_sync(&ctrl_info->event_work);
+	pqi_cancel_rescan_worker(ctrl_info);
+	cancel_work_sync(&ctrl_info->ofa_memory_alloc_work);
+	cancel_work_sync(&ctrl_info->ofa_quiesce_work);
 	if (ctrl_info->queue_memory_base)
 		dma_free_coherent(&ctrl_info->pci_dev->dev,
 			ctrl_info->queue_memory_length,
-- 
2.34.1

Re: [PATCH v2] scsi: smartpqi: drain controller workers before freeing controller

[sashiko-bot]

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

New issues:
- [High] Waking up all threads on `block_requests_wait` causes a Use-After-Free for synchronous user threads (e.g., ioctls) because `ctrl_info` is freed without waiting for them to complete.

Pre-existing issues:
- [High] The heartbeat timer can be re-armed by the OFA quiesce worker during teardown and is never stopped again, leading to a Use-After-Free when the timer expires.
--

commit 753a17ed3e0060de6ddd2fedeade46717222e86f
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: smartpqi: drain controller workers before freeing controller

This commit addresses use-after-free bugs by draining several delayed workers
and event workers in pqi_free_ctrl_resources(). It also unblocks requests
before cancelling the event worker to prevent it from waiting indefinitely
during controller removal.

> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
> index 5ec583dc2e7df..707693d2eb80a 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -8897,7 +8897,19 @@ static void pqi_free_interrupts(struct pqi_ctrl_info *ctrl_info)
>  
>  static void pqi_free_ctrl_resources(struct pqi_ctrl_info *ctrl_info)
>  {
> +	/*
> +	 * Release blocked workers first.  Disable ctrl_offline_work before
> +	 * freeing IRQs because its callback can also free them.  event_work
> +	 * can requeue rescan_work, so drain event_work before cancelling
> +	 * rescan again.
> +	 */
> +	pqi_ctrl_unblock_requests(ctrl_info);

[Severity: High]
Does waking up all threads on block_requests_wait here cause a
use-after-free for synchronous user threads?

By calling pqi_ctrl_unblock_requests() (which wakes up all threads on
ctrl_info->block_requests_wait), user threads blocked in
pqi_wait_if_ctrl_blocked() during ioctl calls like
pqi_submit_raid_request_synchronous() will be unblocked.

Since there is no subsequent synchronization to wait for these threads to
finish, won't they resume execution and dereference ctrl_info after
pqi_free_ctrl_info() frees it at the end of pqi_free_ctrl_resources()?

> +	disable_work_sync(&ctrl_info->ctrl_offline_work);
>  	pqi_free_interrupts(ctrl_info);
> +	cancel_work_sync(&ctrl_info->event_work);
> +	pqi_cancel_rescan_worker(ctrl_info);
> +	cancel_work_sync(&ctrl_info->ofa_memory_alloc_work);
> +	cancel_work_sync(&ctrl_info->ofa_quiesce_work);

[Severity: High]
This isn't a bug introduced by this patch, but can the heartbeat timer be
re-armed by the OFA quiesce worker and cause a use-after-free?

When cancel_work_sync() runs, if the OFA quiesce worker is currently
executing, it can call pqi_process_soft_reset(). Under error conditions,
this can lead to pqi_ofa_ctrl_unquiesce(), which re-arms the timer via
pqi_start_heartbeat_timer().

Since the teardown path does not stop the timer again after draining the
workers, won't the timer remain armed when the controller is freed, leading
to a use-after-free when the timer fires and dereferences ctrl_info in
pqi_heartbeat_timer_handler()?

>  	if (ctrl_info->queue_memory_base)
>  		dma_free_coherent(&ctrl_info->pci_dev->dev,

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