Re: [PATCH v3 07/11] dmaengine: switchtec-dma: disable channels before freeing on registration failure

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

Pre-existing issues:
- [High] Accessing `swdma_dev->bar` in `iounmap()` after the structure has been kfreed.
- [High] The `chan_status_irq` interrupt is requested before channel initialization is complete, leading to a NULL pointer dereference in the ISR.
- [High] The driver invokes the client's DMA completion callback while holding the internal `complete_lock` spinlock, creating a self-deadlock risk.
- [High] Missing check on the return value of `ioremap()` leads to a NULL pointer dereference.
--

--- Patch [7]: [PATCH v3 07/11] dmaengine: switchtec-dma: disable channels before freeing on registration failure ---
commit eb5cc523afcf50094dfae39f87e70f0557894b90
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: disable channels before freeing on registration failure

When switchtec_dma_create() fails after channels have been added to
dma_dev->channels (either from switchtec_dma_chans_enumerate()'s own
error path, or from dma_async_device_register() failing), the channels
are released and freed but never removed from dma_dev->channels.

Add switchtec_dma_chans_disable() which disables interrupts and
removes the channels from the list.. Call it before releasing and
freeing channels in both error paths.

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 31feb2816e79b..800d8ecd07171 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -1217,6 +1231,7 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
>  	return chan_cnt;
>  
>  err_exit:
> +	switchtec_dma_chans_disable(pdev, swdma_dev);
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  	switchtec_dma_chans_free(swdma_dev);
>  

[Severity: High]
This is a pre-existing issue, but could an early interrupt cause a NULL
pointer dereference here?

In switchtec_dma_create(), pci_request_irq() is called to enable
switchtec_dma_chan_status_isr() before switchtec_dma_chans_enumerate()
is called.

The enumerate function is what initializes the list head:

    INIT_LIST_HEAD(&dma->channels);

If an interrupt fires before the list is initialized, the ISR will execute
list_for_each_entry() over a NULL channels.next pointer. 

Should the interrupt request be deferred until after the channel enumeration
is complete?

[ ... ]

> @@ -1320,6 +1335,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	return 0;
>  
>  err_chans_release_exit:
> +	switchtec_dma_chans_disable(pdev, swdma_dev);
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  	switchtec_dma_chans_free(swdma_dev);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the ioremap call in
switchtec_dma_create() need a return value check?

In switchtec_dma_create(), the BAR is mapped and then immediately used:

switchtec_dma_create() {
    ...
    swdma_dev->bar = ioremap(pci_resource_start(pdev, 0), ...);
    ...
    irq = readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC);
}

If ioremap() fails and returns NULL, readw() will dereference it. 

Should this include an error check before accessing swdma_dev->bar?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
driver unbind?

When switchtec_dma_remove() unregisters the device, it can drop the final
reference:

switchtec_dma_remove()
    dma_async_device_unregister()
        switchtec_dma_release()
            kfree(swdma_dev);

Immediately after dma_async_device_unregister() returns, the remove function
calls iounmap(swdma_dev->bar), dereferencing the freed pointer. 

Can this cause a kernel panic when the module is removed?

[Severity: High]
This is also a pre-existing issue, but does this locking pattern introduce a
CPU deadlock risk?

In switchtec_dma_cleanup_completed(), a spinlock is held while invoking an
opaque client callback:

switchtec_dma_cleanup_completed()
    spin_lock_bh(&swdma_chan->complete_lock);
    ...
    dmaengine_desc_get_callback_invoke(&desc->txd, &res);

If the client's callback invokes dmaengine_terminate_all(), the core will
call switchtec_dma_terminate_all(), which unconditionally attempts to acquire
the exact same complete_lock. 

Could this cause the CPU to deadlock?

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