Re: [PATCH v5 14/25] soundwire: amd: fix work drain ordering in remove path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] soundwire: amd: race with in-flight IRQ handler in
  amd_sdw_manager_remove()
--

commit 96eb0e85d023d4d06d5fe26f4e91328e3104ebef
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix work drain ordering in remove path

This commit reorders work cancellation in amd_sdw_manager_remove() to disable
SoundWire interrupts first and then cancel amd_sdw_irq_thread and amd_sdw_work.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 767bc5510dfef..4083a3f0ec2f8 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1278,8 +1278,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>  	int ret;
>  
>  	pm_runtime_disable(&pdev->dev);
> -	cancel_work_sync(&amd_manager->amd_sdw_work);
>  	amd_disable_sdw_interrupts(amd_manager);
> +	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
> +	cancel_work_sync(&amd_manager->amd_sdw_work);
>  	sdw_bus_master_delete(&amd_manager->bus);
>  	ret = amd_disable_sdw_manager(amd_manager);
>  	if (ret)

[Severity: High]
Can an in-flight parent ACP interrupt handler still queue amd_sdw_irq_thread
after cancel_work_sync() returns?

Unlike disable_work_sync(), cancel_work_sync() only drains work that is
already queued or running and leaves the work item enabled before returning.
Without synchronize_irq() or disable_work_sync(), an interrupt asserted just
before amd_disable_sdw_interrupts() masks ACP_EXTERNAL_INTR_CNTL can already
be running acp63_irq_handler() on another CPU:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
	...
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
	...
}

This allows schedule_work() to run after both cancel_work_sync() calls finish:

CPU0                                      CPU1 (<hardirq>)
amd_sdw_manager_remove()                  acp63_irq_handler()
                                            readl(... + ACP_EXTERNAL_INTR_STAT)
                                            amd_manager = dev_get_drvdata(...)
  amd_disable_sdw_interrupts()
  cancel_work_sync(&...->amd_sdw_irq_thread)
  cancel_work_sync(&...->amd_sdw_work)
                                            schedule_work(&...->amd_sdw_irq_thread)
  sdw_bus_master_delete(&amd_manager->bus)
  ...
  devres_release_all()
                                          amd_sdw_irq_thread()
                                            readl(amd_manager->mmio + ...)
                                              <-- use-after-free

Even with commit "soundwire: amd: use interrupt_enabled flag to guard IRQ
work scheduling" later in this series, interrupt_enabled is only checked
inside amd_sdw_irq_thread() before queueing amd_sdw_work, so the hardirq
handler still schedules amd_sdw_irq_thread unconditionally.

Also, because amd_disable_sdw_interrupts() now runs before
cancel_work_sync(&amd_manager->amd_sdw_work), an already running
amd_sdw_update_slave_status_work() with status[0] == SDW_SLAVE_ATTACHED can
write AMD_SDW_IRQ_MASK_0TO7 and AMD_SDW_IRQ_MASK_8TO11 back to
ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7 and ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11,
overwriting the 0x00 masks written by amd_disable_sdw_interrupts().

Using disable_work_sync() instead of cancel_work_sync() (or synchronizing with
the parent IRQ after masking) and masking the state-change registers after the
work items are drained would prevent both races.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=14