Re: [PATCH v2 08/11] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of ioremap is not checked, leading to a guaranteed NULL pointer dereference if the mapping fails.
- [High] The IRQ handler is registered and enabled before the DMA channels list head is initialized, creating a race condition that can crash the kernel.
--

--- Patch [8]: [PATCH v2 08/11] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error ---
commit ceb60e37fe28d72b900ae9ddab80b66c36311831
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

This commit fixes an issue where chan_status_irq was freed with plain
free_irq() instead of pci_free_irq(). It also initializes the irq to -1
to properly track its allocation state.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1257,6 +1257,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	if (!swdma_dev)
>  		return -ENOMEM;
>  
> +	swdma_dev->chan_status_irq = -1;
> +
>  	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
>  				 pci_resource_len(pdev, 0));

[Severity: High]
This is a pre-existing issue, but the return value of ioremap() is not
checked here.

If this allocation fails (e.g., due to exhausted vmalloc space) and returns
NULL, won't this cause a guaranteed NULL pointer dereference when the driver
later calls readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC)?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a race
condition regarding the IRQ registration in this same function.

Further down in switchtec_dma_create(), the driver registers the IRQ
handler:

	rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr,
			     swdma_dev, KBUILD_MODNAME);

At this point, the dma_dev->channels list is uninitialized (zero-filled by
kzalloc_obj). The required INIT_LIST_HEAD() is not called until later inside
switchtec_dma_chans_enumerate().

If an interrupt fires immediately after registration, won't the ISR crash
when it iterates over the uninitialized list head?

drivers/dma/switchtec_dma.c:switchtec_dma_chan_status_isr() {
    ...
	list_for_each_entry(chan, &dma_dev->channels, device_node) {
		swdma_chan = container_of(chan, struct switchtec_dma_chan,
					  dma_chan);
		chan_dev = &swdma_chan->dma_chan.dev->device;
    ...
}

Could the IRQ registration be moved after switchtec_dma_chans_enumerate(),
or the list initialized earlier?

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