Re: [PATCH v2 35/64] dmaengine: altera-msgdma: use channel BH helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The device interrupt is requested before essential locks, lists, and bottom-half structures are initialized, risking a crash if an interrupt fires early.
- [High] Missing dma_async_device_unregister() in the probe error path causes Use-After-Free.
--

--- Patch [35]: [PATCH v2 35/64] dmaengine: altera-msgdma: use channel BH helpers ---
Note: The format of this review has been altered to use a free-form summary 
rather than inline diff quoting due to strict recitation restrictions.

commit bf40fd1cbe95a4c51ca6b86e4123ac5282a06f01
Author: Allen Pais <allen.lkml@gmail.com>
Subject: dmaengine: altera-msgdma: use channel BH helpers

Replace the device tasklet with the per-channel BH helper so completion 
handling runs via dma_chan_*_bh.

[Severity: High]
This isn't a bug introduced by this patch, but looking at msgdma_probe() in 
drivers/dma/altera-msgdma.c, the interrupt is requested via 
devm_request_irq() before mdev->lock, the pending_list, and the bottom-half 
structures are initialized.

If a pending hardware interrupt fires immediately, could msgdma_irq_handler() 
execute and attempt to acquire the uninitialized mdev->lock? Furthermore, if 
it then calls msgdma_start_transfer(), it appears list_first_entry_or_null() 
on the zero-initialized pending_list would return an invalid pointer. Would 
this cause a page fault during list_splice_tail_init() when it subsequently 
evaluates list_empty() inside the splice operations? Is it possible to move 
the devm_request_irq() call after all these structures and lists are fully 
initialized?

[Severity: High]
This is also a pre-existing issue, but I have a question about the error 
handling further down in msgdma_probe().

If of_dma_controller_register() fails with an error other than -EINVAL or 
-ENODEV, the code jumps to the fail label and calls msgdma_dev_remove(). 
However, msgdma_dev_remove() only cleans up the IRQ and bottom half, without 
calling dma_async_device_unregister().

Since dma_async_device_register() was already called earlier in the probe 
function, won't devres subsequently free the mdev structure while the DMA 
engine core still holds pointers to its embedded dma_dev and channels? Could 
this lead to a use-after-free if the DMA engine core later traverses its 
channel lists? Does dma_async_device_unregister() need to be added to the 
cleanup path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=35