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

[sashiko-bot@kernel.org]

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