Re: [PATCH 11/17] dmaengine: idxd: disable the device before its work queues are unregistered

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: idxd: wqs freed before misc IRQ teardown in idxd_remove()
- [Medium] dmaengine: idxd: PASID detached before disabling bus mastering in idxd_remove()
--

commit 702a28ae3b6aeca10dbb038b18e88c5081667455
Author: Daniel Byrne <daniel1.byrne@intel.com>

dmaengine: idxd: disable the device before its work queues are unregistered

This commit moves idxd_shutdown() in idxd_remove() to run before
idxd_unregister_devices() so the device is disabled while the work queues
still exist, and sets idxd->state to IDXD_DEV_DISABLED after
pci_disable_device() before dropping the final reference to idxd_confdev.

> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index c4319ac27bfb1..a037a6e763622 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -1296,6 +1296,13 @@ static void idxd_remove(struct pci_dev *pdev)
>  	 * that driver while the queues still exist.
>  	 */
>  	device_release_driver(idxd_confdev(idxd));
> +	/*
> +	 * Disable the device while the queues still exist too. The
> +	 * sub-driver's remove leaves the device enabled when a command has
> +	 * timed out, a late completion can clear that state at any time,
> +	 * and the disable below then clears the queues' software state.
> +	 */
> +	idxd_shutdown(pdev);
>  	idxd_unregister_devices(idxd);

[Severity: High]
If a command has timed out and does not complete before idxd_shutdown()
runs, idxd_device_disable() in idxd_shutdown() will also fail, leaving the
device enabled in hardware while idxd_unregister_devices() frees all
idxd->wqs[i] before idxd_cleanup_interrupts() frees MSI-X vector 0. Can a
misc interrupt fire in this window and dereference freed idxd->wqs[i]?

Specifically, when IDXD_FLAG_CMD_TIMEDOUT is set, idxd_device_drv_remove()
skips idxd_device_evl_free(), leaving Event Log interrupts enabled in
hardware (gencfg.evl_en = 1, genctrl.evl_int_en = 1). In idxd_shutdown():

drivers/dma/idxd/init.c:idxd_shutdown() {
	rc = idxd_device_disable(idxd);
	if (rc)
		dev_err(&pdev->dev, "Disabling device failed\n");

	irq_entry = &idxd->ie;
	synchronize_irq(irq_entry->vector);
	idxd_mask_error_interrupts(idxd);
	flush_workqueue(idxd->wq);
}

synchronize_irq() is called before idxd_mask_error_interrupts(), and
idxd_mask_error_interrupts() only clears softerr_int_en and halt_int_en,
leaving EVL and IDXD_INTC_INT_HANDLE_REVOKED interrupts enabled while
MSI-X vector 0 (idxd_misc_thread) stays active until
idxd_cleanup_interrupts().

Once idxd_unregister_devices() unregisters and frees idxd->wqs[i], an
IDXD_INTC_EVL interrupt running idxd_misc_thread() -> process_evl_entries()
-> process_evl_entry() will index into the freed wq:

drivers/dma/idxd/irq.c:process_evl_entry() {
	...
	struct idxd_wq *wq = idxd->wqs[entry_head->wq_idx];

	if (!is_idxd_wq_user(wq) || !wq->wq) {
	...
}

Similarly, an IDXD_INTC_INT_HANDLE_REVOKED interrupt (or a halt/error
interrupt racing between synchronize_irq() and idxd_mask_error_interrupts())
can access freed idxd->wqs[i] or queue work onto idxd->wq after
flush_workqueue(idxd->wq) has already completed, which could then run
after pci_iounmap(pdev, idxd->reg_base).

Should idxd_cleanup_interrupts() (or disabling bus mastering / freeing IRQ
vector 0 before flushing idxd->wq) happen before idxd_unregister_devices()
frees the work queues?

>  	/*
>  	 * When ->release() is called for the idxd->conf_dev, it frees all
> @@ -1305,15 +1312,20 @@ static void idxd_remove(struct pci_dev *pdev)
>  	 */
>  	get_device(idxd_confdev(idxd));
>  	device_unregister(idxd_confdev(idxd));
> -	idxd_shutdown(pdev);
>  	idxd_device_remove_debugfs(idxd);
>  	perfmon_pmu_remove(idxd);
>  	idxd_cleanup_interrupts(idxd);
>  	if (device_pasid_enabled(idxd))
>  		idxd_disable_system_pasid(idxd);
>  	pci_iounmap(pdev, idxd->reg_base);
> -	put_device(idxd_confdev(idxd));
>  	pci_disable_device(pdev);
> +	/*
> +	 * Bus mastering is off, so nothing the device holds can complete.
> +	 * The store comes before the reference taken above is dropped:
> +	 * that reference is the last one, and its release frees the device.
> +	 */
> +	idxd->state = IDXD_DEV_DISABLED;
> +	put_device(idxd_confdev(idxd));
>  }

[Severity: Medium]
In the same command timeout case where the device remains enabled in
hardware until pci_disable_device(pdev) turns off bus mastering, is it
safe to call idxd_disable_system_pasid(idxd) before pci_disable_device(pdev)?

drivers/dma/idxd/init.c:idxd_disable_system_pasid() {
	...
	iommu_detach_device_pasid(domain, dev, idxd->pasid);
	iommu_free_global_pasid(idxd->pasid);

	idxd_set_user_intr(idxd, 0);
	...
}

Because bus mastering is still active when idxd_disable_system_pasid()
runs, could in-flight descriptors on the still-enabled device issue DMA
with idxd->pasid after it has been detached and freed, or could
idxd_set_user_intr() write GENCFG while the device is still enabled?

듯

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019150630.4026341-1-daniel1.byrne@intel.com?part=11