Re: [PATCHv4 04/15] dmaengine: fsldma: provide device_release callback

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Use-After-Free (Read/Write) and Double-Free of `fdev` during device removal.
- [High] Teardown reordering causes a race condition leading to hung transfers.

Pre-existing issues:
- [Critical] Invoking client callbacks with the descriptor spinlock held causes a recursive deadlock.
- [High] DMA channels are unconditionally destroyed leading to potential Use-After-Free by active clients.
- [High] Missing tasklet cancellation leads to a Use-After-Free of the channel structure.
- [High] The error rollback path in fsldma_suspend_late() double-unlocks spinlocks.
--

commit dc56cbc463b043a9535d2906ead690c87f19ad18
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: provide device_release callback

The DMA core requires drivers to set dma_device.device_release so that
the container structure is only freed after all references to it have
been dropped (see the comment above dma_async_device_register()).

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..06a6996a53229 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1211,6 +1211,8 @@ static void fsl_dma_chan_remove(struct fsldma_chan *chan)
>  	kfree(chan);
>  }

[Severity: High]
This is a pre-existing issue, but does this unconditionally destroy the
channel even if clients are still using it?

Since dma_async_device_unregister() doesn't enforce that clients drop all
channel references before it returns, clients might still hold pointers to
the freed chan structure. 

If they try to use it later, couldn't this cause a use-after-free?

[Severity: High]
This is also a pre-existing issue, but is it safe to free the channel without
killing the tasklet first?

The code frees the IRQ but does not call tasklet_kill(&chan->tasklet). 

If an interrupt fires and schedules a tasklet just before the IRQ is
disabled, the tasklet could execute concurrently or later and dereference
the freed chan structure in dma_do_tasklet().

>  
> +static void fsldma_device_release(struct dma_device *dma_dev);
> +

[ ... ]

> @@ -1308,19 +1311,33 @@ static int fsldma_of_probe(struct platform_device *op)
>  	return err;
>  }
>  
> +static void fsldma_device_release(struct dma_device *dma_dev)
> +{
> +	struct fsldma_device *fdev = container_of(dma_dev, struct fsldma_device,
> +						  common);
> +	kfree(fdev);
> +}
> +
>  static void fsldma_of_remove(struct platform_device *op)
>  {
> -	struct fsldma_device *fdev;
> +	struct fsldma_device *fdev = platform_get_drvdata(op);
> +	struct fsldma_chan *chans[FSL_DMA_MAX_CHANS_PER_DEVICE];
>  	unsigned int i;
>  
> -	fdev = platform_get_drvdata(op);
> -	dma_async_device_unregister(&fdev->common);
> +	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++)
> +		chans[i] = fdev->chan[i];
>  
>  	fsldma_free_irqs(fdev);

[Severity: High]
Does this reordering open a race condition that could hang transfers?

By calling fsldma_free_irqs(fdev) before dma_async_device_unregister(),
there is a window where the DMA device is still fully active and accepting
new transfers, but its interrupts have been disabled and freed. 

If a transaction completes during this window, will the completion interrupt
never fire?

>  
> +	/*
> +	 * fdev may be freed by fsldma_device_release inside this call;
> +	 * use saved copies of the channel pointers afterwards.
> +	 */
> +	dma_async_device_unregister(&fdev->common);
> +
>  	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
> -		if (fdev->chan[i])
> -			fsl_dma_chan_remove(fdev->chan[i]);
> +		if (chans[i])
> +			fsl_dma_chan_remove(chans[i]);
>  	}
>  	irq_dispose_mapping(fdev->irq);
>  

[Severity: Critical]
Does this sequence result in a use-after-free and double-free of fdev?

If dma_async_device_unregister() drops the final reference and triggers
fsldma_device_release(), fdev is freed. 

However, the function then continues to access fdev indirectly by calling
fsl_dma_chan_remove(chans[i]), which modifies the list. It also reads
fdev->irq here to perform unmapping, and eventually explicitly calls
kfree(fdev) at the end of the function.

[Severity: High]
This is a pre-existing issue, but does fsldma_suspend_late() double-unlock
spinlocks in the error rollback path?

In fsldma_suspend_late(), the forward iteration acquires the descriptor
spinlock, updates the state, and then unlocks it on success. If it
encounters a busy channel, it jumps to the out label to roll back. 

However, the rollback loop unconditionally calls
spin_unlock_bh(&chan->desc_lock) for all previously processed channels,
even though their locks had already been successfully released in the
forward loop.

[Severity: Critical]
This is a pre-existing issue, but does invoking client callbacks with the
descriptor spinlock held cause a recursive deadlock?

fsldma_cleanup_descriptors() is called with chan->desc_lock held. It then
calls fsldma_run_tx_complete_actions(), which synchronously invokes the
client's completion callback. 

If the client submits a new DMA transaction from within this callback:

fsldma_run_tx_complete_actions()
   dmaengine_desc_get_callback_invoke()
      client_callback()
         fsl_dma_tx_submit()

Will fsl_dma_tx_submit() attempt to acquire the exact same chan->desc_lock
and result in an immediate deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611035245.13439-5-rosenp@gmail.com?part=1