Re: [PATCH v2 55/64] dmaengine: dma-axi-dmac: use dma_chan_kill_bh

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

New issues:
- [Low] Several other DMA drivers fail to build because they still explicitly reference the removed `task` member in `struct virt_dma_chan`.

Pre-existing issues:
- [High] axi_dmac_terminate_all() frees active descriptors without clearing the vc->cyclic pointer, leading to a Use-After-Free in the completion bottom half.
- [High] The driver lacks a devm action or a remove callback to explicitly disable the DMA hardware upon device unbind, causing system hangs if the clock is disabled while a transfer is active.
--

--- Patch [55]: [PATCH v2 55/64] dmaengine: dma-axi-dmac: use dma_chan_kill_bh ---
commit d769925844316e4d839799af39da3b91a09472d0
Author: Allen Pais <[email protected]>

dmaengine: dma-axi-dmac: use dma_chan_kill_bh
    
virt-dma now dispatches completion callbacks through per-channel BH
work instead of its tasklet. Cancel that work during teardown before
channel storage is released.

> diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
> index d47ff27e1408f..d26bfa852806f 100644
> --- a/drivers/dma/dma-axi-dmac.c
> +++ b/drivers/dma/dma-axi-dmac.c
> @@ -1195,9 +1195,11 @@ static int axi_dmac_detect_caps(struct axi_dmac *dmac, unsigned int version)
>  	return 0;
>  }
>  
> -static void axi_dmac_tasklet_kill(void *task)
> +static void axi_dmac_kill_bh(void *data)
>  {
> -	tasklet_kill(task);
> +	struct dma_chan *chan = data;
> +
> +	dma_chan_kill_bh(chan);
>  }

[Severity: Low]
Does this patch series miss updates to other drivers?

Several other DMA drivers fail to build because they still explicitly
reference the removed task member in struct virt_dma_chan. For example:

drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
    ...
    tasklet_kill(&bdev->channels[i].vc.task);
    ...
}

Should these drivers be updated to use dma_chan_kill_bh() as well?

[ ... ]

> @@ -1302,8 +1304,8 @@ static int axi_dmac_probe(struct platform_device *pdev)
>  	 * Put the action in here so it get's done before unregistering the DMA
>  	 * device.
>  	 */
> -	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_tasklet_kill,
> -				       &dmac->chan.vchan.task);
> +	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_kill_bh,
> +				       &dmac->chan.vchan.chan);

[Severity: High]
This is a pre-existing issue, but can the system hang if the driver is
unbound while a transfer is active?

The driver relies entirely on devres for teardown and lacks an explicit
remove callback or devm action to halt the DMA hardware. 

In axi_dmac_probe(), the clock is enabled via devm_clk_get_enabled(). If the
driver is forcefully unbound via sysfs, devres unwinding will disable the
clock. 

Disabling the clock while the AXI DMA controller is actively performing bus
transactions could stall the AXI bus.

Should an action be added here to gracefully halt the DMA engine prior to
the clock being disabled?

[Severity: High]
This isn't a bug introduced by this patch, but does
axi_dmac_terminate_all() safely handle cyclic descriptor termination?

When a cyclic transfer period completes, it sets vc->cyclic to the active
descriptor and schedules the bottom half. 

If a client concurrently calls dmaengine_terminate_async(),
axi_dmac_terminate_all() frees the active descriptors without clearing the
vc->cyclic pointer:

drivers/dma/dma-axi-dmac.c:axi_dmac_terminate_all() {
    ...
    list_splice_tail_init(&chan->active_descs, &head);
    spin_unlock_irqrestore(&chan->vchan.lock, flags);

    vchan_dma_desc_free_list(&chan->vchan, &head);
    ...
}

When the scheduled BH runs, could it dereference the dangling vc->cyclic
pointer to fetch the callback, resulting in a use-after-free?

>  	if (ret)
>  		return ret;
>

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