Re: [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Partial failure in dma_async_device_register() leaks global dma_ida and per-device chan_ida.
- [High] Use-after-free in ioat_dma_do_interrupt() due to mixing devres IRQ registration with manual kfree on the probe error path.
--

--- Patch [4]: [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device ---
(Note: The format of this review is altered from the usual inline commenting style due to recitation restrictions. No patch diffs are quoted below.)

commit 29a5621a4420e7d413fe2125067821903edd8980
Author: Logan Gunthorpe <logang@deltatee.com>
dmaengine: ioat: disable relaxed ordering before registering the device

This commit fixes a dangling device registration issue by moving the 
PCIe relaxed ordering disablement before the dma_async_device_register() 
call. This ensures that the device does not remain in the core's list 
if an error occurs later in the probe function.

Link: https://lore.kernel.org/dmaengine/20260707165906.249F41F000E9@smtp.kernel.org/T/#u

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential 
resource leak in drivers/dma/dmaengine.c:dma_async_device_register().

When a failure occurs and device->chancnt is greater than 0, the code 
jumps to the err_out label. However, this path seems to bypass freeing 
the global dma_ida:

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

Additionally, the loop that unregisters the channel devices omits calling 
ida_free(&device->chan_ida, chan->chan_id) and device->chancnt--. 

If a device probe fails due to memory pressure or hotplug errors, will this 
leak the global dma_ida and per-device chan_ida? If the global dma_ida 
space is exhausted, it might prevent any further DMA devices from 
registering system-wide.

[Severity: High]
This is also a pre-existing issue, but is there a risk of a use-after-free 
on the probe error path?

In drivers/dma/ioat/init.c:ioat_dma_setup_interrupts(), a shared interrupt 
is registered via devm_request_irq(), tying its lifetime to the device core. 
However, if ioat_pci_probe() later fails, it manually calls kfree(device):

ioat_pci_probe() {
    ...
    if (err) {
        for (i = 0; i < IOAT_MAX_CHANS; i++)
            kfree(device->idx[i]);
        kfree(device);
    ...
}

Because the IRQ was allocated with devres, it remains active until 
ioat_pci_probe() returns and the driver core runs the devres cleanup. 
If another device sharing this interrupt line triggers an IRQ during this 
window, wouldn't drivers/dma/ioat/dma.c:ioat_dma_do_interrupt() 
dereference the freed instance pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717221001.361421-1-logang@deltatee.com?part=4