Re: [PATCH v3 09/11] dmaengine: ioat: disable relaxed ordering before registering the device

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

Pre-existing issues:
- [High] Fallback from MSI-X to MSI leaves MSI-X enabled, breaking interrupt delivery.
- [High] Possible Use-After-Free of `device` in shared interrupt handler on probe failure.
- [High] Global IDA leak in the dma_async_device_register error path.
- [Medium] Hardware prefetch limit is configured after the DMA device is exposed.
- [Medium] Sysfs kobject 'quickdata' is added after device uevent (TOCTOU race).
--

--- Patch [9]: [PATCH v3 09/11] dmaengine: ioat: disable relaxed ordering before registering the device ---
commit bae11619501364346652f2b925166039da65e184
Author: Logan Gunthorpe <[email protected]>
Subject: dmaengine: ioat: disable relaxed ordering before registering the device

Moves the capability read/write ahead of dma_async_device_register() so
failure doesn't leave dangling registered devices.

Link: https://lore.kernel.org/dmaengine/[email protected]

> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 737496391109e..a57024c4b066e 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
[ ... ]
> @@ -1194,6 +1185,15 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
>  		goto err_disable_interrupts;
>  	}
>  
> +	err = dma_async_device_register(&ioat_dma->dma_dev);
> +	if (err)
> +		goto err_disable_interrupts;
> +
> +	ioat_kobject_add(ioat_dma, &ioat_ktype);
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but doesn't this create a
TOCTOU race?  The sysfs kobject 'quickdata' and its attributes are added
after dma_async_device_register() has already emitted the KOBJ_ADD uevent.
Could userspace tools reacting to the uevent miss these sysfs attributes?

> +	if (dca)
> +		ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base);
> +
>  	if (ioat_dma->cap & IOAT_CAP_DPS)
>  		writeb(ioat_pending_level + 1,
>  		       ioat_dma->reg_base + IOAT_PREFETCH_LIMIT_OFFSET);

[Severity: Medium]
This isn't a bug introduced by this patch, but is it safe to configure the
hardware prefetch limit after the DMA device has already been exposed?
Since dma_async_device_register() makes the channels available, could
concurrent DMA clients submit and execute operations with uninitialized or
stale prefetch hardware states before this writeb() occurs?


Additionally, while reviewing this code, I noticed a few pre-existing issues
outside the modified context:

[Severity: High]
This isn't a bug introduced by this patch, but in ioat_pci_probe(), the
device struct is manually freed if ioat3_dma_probe() fails:

drivers/dma/ioat/init.c:ioat_pci_probe() {
    ...
	err = ioat3_dma_probe(device, ioat_dca_enabled);
	if (err) {
		for (i = 0; i < IOAT_MAX_CHANS; i++)
			kfree(device->idx[i]);
		kfree(device);
		dev_err(dev, "Intel(R) I/OAT DMA Engine init failed\n");
		return -ENODEV;
	}
    ...
}

However, ioat_dma_setup_interrupts() allocates IRQs using
devm_request_irq(). Because the IRQs are devm-managed, they are not actually
unregistered until ioat_pci_probe() returns. Doesn't this create a window
where a shared interrupt could trigger ioat_dma_do_interrupt(), which
then casts and dereferences the already freed device pointer?

drivers/dma/ioat/dma.c:ioat_dma_do_interrupt() {
	struct ioatdma_device *instance = data;
    ...
	intrctrl = readb(instance->reg_base + IOAT_INTRCTRL_OFFSET);
    ...
}


[Severity: High]
This isn't a bug introduced by this patch, but when falling back from MSI-X
to MSI in ioat_dma_setup_interrupts(), is MSI-X properly disabled?

drivers/dma/ioat/init.c:ioat_dma_setup_interrupts() {
    ...
	err = pci_enable_msix_exact(pdev, ioat_dma->msix_entries, msixcnt);
	if (err)
		goto msi;

	for (i = 0; i < msixcnt; i++) {
        ...
		err = devm_request_irq(dev, msix->vector, ...);
		if (err) {
			for (j = 0; j < i; j++) {
                ...
				devm_free_irq(dev, msix->vector, ioat_chan);
			}
			goto msi;
		}
	}
    ...
}

If devm_request_irq() fails, the code frees the requested IRQs and jumps
to the msi label. However, it doesn't call pci_disable_msix(). Since the
device is still programmed for MSI-X, pci_enable_msi() will fail, and
the fallback will continue to INTx. Will the device ever generate INTx
interrupts if it was left with MSI-X enabled?


[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
dma_async_device_register() leak the global device IDA?

drivers/dma/dmaengine.c:dma_async_device_register() {
    ...
err_out:
	/* if we never registered a channel just release the idr */
	if (!device->chancnt) {
		ida_free(&dma_ida, device->dev_id);
		return rc;
	}

	list_for_each_entry(chan, &device->channels, device_node) {
    ...
}

If dma_async_device_register() successfully registers at least one channel
but fails on a subsequent one (so device->chancnt is greater than 0), it jumps
to err_out. The error path explicitly skips calling ida_free() because
device->chancnt is greater than 0. Does this permanently leak the global
IDA for this device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9