Re: [PATCH] dma: at_hdmac: add device_synchronize callback

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

Pre-existing issues:
- [High] Adding `device_synchronize` is insufficient to fix descriptor synchronization races because `atc_terminate_all()` still prematurely frees active/terminated descriptors, causing a Use-After-Free.
--

--- Patch [1]: [PATCH] dma: at_hdmac: add device_synchronize callback ---
commit cf29784e04ea139aef404987d19236faf066af03
Author: Rosen Penev <[email protected]>

dma: at_hdmac: add device_synchronize callback

Implement the device_synchronize callback for the atmel DMA driver by adding
atc_synchronize() which calls vchan_synchronize() on the associated virt
channel. This ensures proper synchronization of pending descriptors when
requested by the DMA framework.

> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index e5b30a57c477a..96233eb7a3c6e 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1616,6 +1616,13 @@ static int atc_resume(struct dma_chan *chan)
>  	return 0;
>  }
>  
> +static void atc_synchronize(struct dma_chan *chan)
> +{
> +	struct at_dma_chan *atchan = to_at_dma_chan(chan);
> +
> +	vchan_synchronize(&atchan->vc);
> +}

[Severity: High]
This is a pre-existing issue, but does adding device_synchronize fully resolve
the descriptor synchronization races?

When dmaengine_terminate_sync() is called, it first invokes
device_terminate_all() (which maps to atc_terminate_all()) and then calls
device_synchronize().

Looking at atc_terminate_all(), it currently stops the hardware and moves the
active cyclic descriptor to the desc_terminated list, but then immediately
extracts and frees all lists:

atc_terminate_all() {
        ...
        if (atchan->desc) {
                vchan_terminate_vdesc(&atchan->desc->vd);
                ...
        }

        vchan_get_all_descriptors(&atchan->vc, &list);
        ...
        vchan_dma_desc_free_list(&atchan->vc, &list);
}

If the period completion tasklet (vchan_complete()) is concurrently executing
on another CPU, the cyclic descriptor will be freed out from under it. By the
time atc_synchronize() is called to wait for the tasklet via tasklet_kill(),
the memory is already freed.

Can this result in a use-after-free when the tasklet calls
dmaengine_desc_callback_invoke() and computes an address based on the freed
pointer?

Should atc_terminate_all() avoid extracting and freeing the desc_terminated
list so vchan_synchronize() can clean it up safely after the tasklet finishes?

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