Re: [PATCH v3 16/34] dmaengine: ppc4xx: replace irq tasklet with dmaengine BH

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

Pre-existing issues:
- [High] Use-After-Free due to missing dmaengine_kill_bh() on the probe error path.
- [High] Self-deadlock in the bottom-half callback when processing dependent DMA transactions on the same channel.
--

--- Patch [16]: [PATCH v3 16/34] dmaengine: ppc4xx: replace irq tasklet with dmaengine BH ---
commit 4ccd13b8e7ec3566e05878b1ca91b9e88ad9043a
Author: Allen Pais <[email protected]>

dmaengine: ppc4xx: replace irq tasklet with dmaengine BH

Replace the per-channel tasklet with the shared dmaengine BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
> index 279a431ccae38..bc10c2b5dd12e 100644
> --- a/drivers/dma/ppc4xx/adma.c
> +++ b/drivers/dma/ppc4xx/adma.c
> @@ -1658,9 +1658,9 @@ static void __ppc440spe_adma_slot_cleanup(struct ppc440spe_adma_chan *chan)
>  /**
>   * ppc440spe_adma_tasklet - clean up watch-dog initiator
>   */
> -static void ppc440spe_adma_tasklet(struct tasklet_struct *t)
> +static void ppc440spe_adma_tasklet(struct dma_chan *c)
>  {
> -	struct ppc440spe_adma_chan *chan = from_tasklet(chan, t, irq_tasklet);
> +	struct ppc440spe_adma_chan *chan = to_ppc440spe_adma_chan(c);
>  
>  	spin_lock_nested(&chan->lock, SINGLE_DEPTH_NESTING);
>  	__ppc440spe_adma_slot_cleanup(chan);

[Severity: High]
This is a pre-existing issue, but does this locking sequence cause a
self-deadlock when processing dependent transactions?

If the callback processes a dependent transaction targeting the same channel,
dma_run_dependencies() will call the submit callback:

ppc440spe_adma_tasklet()
  spin_lock_nested(&chan->lock, ...)
  __ppc440spe_adma_slot_cleanup()
    ppc440spe_adma_run_tx_complete_actions()
      dma_run_dependencies()
        ppc440spe_adma_tx_submit()
          spin_lock_bh(&chan->lock)

Since the non-recursive spinlock is already held by this CPU, will this
immediately deadlock?

Should the lock be dropped prior to evaluating completion callbacks or
dependencies?

[ ... ]

> @@ -4137,7 +4137,7 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  	chan->common.device = &adev->common;
>  	dma_cookie_init(&chan->common);
>  	list_add_tail(&chan->common.device_node, &adev->common.channels);
> -	tasklet_setup(&chan->irq_tasklet, ppc440spe_adma_tasklet);
> +	dmaengine_init_bh(&chan->common, ppc440spe_adma_tasklet);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free during probe failure?

If an early hardware interrupt fires during probe, ppc440spe_adma_eot_handler()
will schedule this bottom half.

If a subsequent initialization step fails, the error path releases the
interrupt via free_irq(), but does not cancel the scheduled bottom half:

err_dev_reg:
	ppc440spe_adma_release_irqs(adev, chan);
err_irq:
...
err_page_alloc:
	kfree(chan);

If the bottom half executes after kfree(chan), will it access freed memory?

Should dmaengine_kill_bh() be added to the probe error path?

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