Re: [PATCH v5 07/12] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked return value of ioremap() can lead to a NULL pointer dereference.
- [High] Interrupt handler is registered before the DMA channels list is initialized, risking a NULL pointer dereference.
- [High] DMA device is unregistered after channels and IRQs are torn down.
- [High] Holding a spinlock while busy-waiting for up to 100 milliseconds disables preemption for an unacceptable duration, potentially causing RCU stalls or soft lockups.
- [Medium] The return value of dma_set_mask_and_coherent() is ignored.
- [Medium] Missing put_device() causes reference count leak on the device structure in the error path of switchtec_dma_create().
--

commit 789f2f9ebf1e9dd9f52e2557e403d93f922154be
Author: Logan Gunthorpe <logang@deltatee.com>
Subject: dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

This commit ensures that chan_status_irq is properly cleaned up on error by
using pci_free_irq() instead of free_irq(). It also initializes the IRQ vector
index to -1 so that a valid index of 0 is no longer skipped during cleanup.

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
> index c30530ba78caf..3068d9de4e6d3 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1262,6 +1262,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 shouldn't we check the return value of 
ioremap() here?

If ioremap() fails, swdma_dev->bar will be NULL. The code immediately proceeds
to dereference it via readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC). Can 
this lead to a NULL pointer dereference and kernel panic during device probe?

[ ... ]

>  err_exit:
> -	if (swdma_dev->chan_status_irq)
> -		free_irq(swdma_dev->chan_status_irq, swdma_dev);
> +	if (swdma_dev->chan_status_irq >= 0)
> +		pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
>  
>  	iounmap(swdma_dev->bar);
>  	kfree(swdma_dev);
>  	return rc;

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the device 
reference?

Earlier in switchtec_dma_create(), a reference is acquired:

    dma->dev = get_device(&pdev->dev);

If dma_async_device_register() fails, the code jumps to 
err_chans_release_exit and eventually falls through to err_exit, but 
put_device(dma->dev) is never called. Since the device wasn't successfully 
registered, switchtec_dma_release() won't be called to drop this reference.

In drivers/dma/switchtec_dma.c:switchtec_dma_probe():

    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
    rc = pci_request_mem_regions(pdev, KBUILD_MODNAME);

[Severity: Medium]
This is a pre-existing issue, but is the return value of 
dma_set_mask_and_coherent() intentionally ignored here?

If the platform or IOMMU rejects the 64-bit mask, this function can fail. 
Should the driver check the return value and gracefully fallback to 32-bit
DMA or fail the probe to prevent silent data corruption or IOMMU faults?

In drivers/dma/switchtec_dma.c:switchtec_dma_create():

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

[Severity: High]
This is a pre-existing issue, but does registering the interrupt handler here 
risk a NULL pointer dereference?

Later in switchtec_dma_create(), switchtec_dma_chans_enumerate() is called, 
which initializes the dma->channels list:

    INIT_LIST_HEAD(&dma->channels);

At the time of IRQ registration, swdma_dev was allocated with kzalloc_obj(), 
meaning dma_dev->channels.next is NULL. If an interrupt fires immediately 
after registration, switchtec_dma_chan_status_isr() will execute:

    list_for_each_entry(chan, &dma_dev->channels, device_node) {

Since the list head is zeroed, this loop could underflow and dereference an 
invalid pointer, leading to a kernel oops.

In drivers/dma/switchtec_dma.c:switchtec_dma_remove():

    switchtec_dma_chans_release(pdev, swdma_dev);
    
    rcu_assign_pointer(swdma_dev->pdev, NULL);
    synchronize_rcu();
    
    pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
    pci_free_irq_vectors(pdev);
    
    dma_async_device_unregister(&swdma_dev->dma_dev);

[Severity: High]
This is a pre-existing issue, but is the DMA device unregistered too late in
the teardown process?

The driver initiates hardware teardown by stopping channels and freeing IRQs
before calling dma_async_device_unregister(). Tearing down hardware and IRQs
while the device is still exposed to the DMA engine subsystem can lead to 
use-after-free, hanging clients, or hardware access races if a client 
actively uses the DMA device during removal. Should the subsystem be 
unregistered first?

In drivers/dma/switchtec_dma.c:wait_for_chan_status():

    return readl_poll_timeout_atomic(&chan_hw->status, status,
                                     (set && (status & mask)) ||
                                     (!set && !(status & mask)),
                                     10, 100 * USEC_PER_MSEC);

[Severity: High]
This is a pre-existing issue, but could this atomic poll loop cause soft
lockups or RCU stalls?

This function is called from halt_channel() while holding a spinlock:

    spin_lock(&swdma_chan->hw_ctrl_lock);
    writeb(SWITCHTEC_CHAN_CTRL_HALT, &chan_hw->ctrl);
    ret = wait_for_chan_status(chan_hw, SWITCHTEC_CHAN_STS_HALTED, true);
    spin_unlock(&swdma_chan->hw_ctrl_lock);

Since holding a spinlock disables preemption, blocking the CPU for up to 100
milliseconds without preemption could severely degrade system latency.

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