Re: [PATCH v3 28/34] dmaengine: sf-pdma: route error callbacks through channel BH

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

New issues:
- [Medium] Starting the next descriptor in the error path fails to reset the channel's retry counter and status.

Pre-existing issues:
- [Critical] Unconditional double-free of the active descriptor (`chan->desc`) during channel termination or resource cleanup.
- [Critical] Driver removal does not stop active DMA hardware, potentially leading to memory corruption.
- [High] Lack of synchronization between `sf_pdma_terminate_all` and channel tasklets leads to Use-After-Free and NULL pointer dereferences.
- [High] Manual removal of the channel from the `dma_dev.channels` list permanently leaks dmaengine core resources on removal.
- [High] `sf_pdma_desc_residue` returns incorrect hardware residue for active transfers.
--

--- Patch [28]: [PATCH v3 28/34] dmaengine: sf-pdma: route error callbacks through channel BH ---
commit 5c7ab16a62d08deb1ca0b90a739ae089b50989a2
Author: Allen Pais <[email protected]>

dmaengine: sf-pdma: route error callbacks through channel BH

Complete failed descriptors through virt-dma with an aborted result and the
remaining transfer size, then start the next pending transfer. During
removal, kill the producer tasklets before the channel BH so they cannot
enqueue callback work after it has been canceled.

> diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
[ ... ]
> @@ -329,7 +329,17 @@ static void sf_pdma_errbh_tasklet(struct tasklet_struct *t)
>  	if (chan->retries <= 0) {
>  		/* fail to recover */
>  		spin_unlock_irqrestore(&chan->lock, flags);
> -		dmaengine_desc_get_callback_invoke(desc->async_tx, NULL);
> +
> +		spin_lock_irqsave(&chan->vchan.lock, flags);
> +		list_del(&desc->vdesc.node);

[Severity: High]
This is a pre-existing issue, but is there a lack of synchronization
between sf_pdma_terminate_all() and channel tasklets?

sf_pdma_terminate_all() frees chan->desc and sets it to NULL without
synchronizing with scheduled tasklets:

drivers/dma/sf-pdma/sf-pdma.c:sf_pdma_terminate_all() {
        ...
        kfree(chan->desc);
        chan->desc = NULL;
        ...
}

If it races with this tasklet, the pointer is cached before the lock
and the list node is modified here after it may have been freed.

Additionally, in sf_pdma_donebh_tasklet(), the pointer is dereferenced
unconditionally:

drivers/dma/sf-pdma/sf-pdma.c:sf_pdma_donebh_tasklet() {
        ...
        list_del(&chan->desc->vdesc.node);
        ...
}

Could this lead to use-after-free and NULL pointer dereferences?

> +		desc->vdesc.tx_result.result = DMA_TRANS_ABORTED;
> +		desc->vdesc.tx_result.residue = desc->xfer_size;
> +		vchan_cookie_complete(&desc->vdesc);
> +
> +		chan->desc = sf_pdma_get_first_pending_desc(chan);
> +		if (chan->desc)
> +			sf_pdma_xfer_desc(chan);

[Severity: Medium]
When starting the next descriptor in the error path, should the channel's
retry counter and status be reset?

If chan->retries remains <= 0, will the new transfer inherit the exhausted
retry state and bypass the retry logic completely if it encounters an error?

> +		spin_unlock_irqrestore(&chan->vchan.lock, flags);
>  	} else {
>  		/* retry */
>  		chan->retries--;
[ ... ]
> @@ -603,9 +613,9 @@ static void sf_pdma_remove(struct platform_device *pdev)
>  		devm_free_irq(&pdev->dev, ch->txirq, ch);
>  		devm_free_irq(&pdev->dev, ch->errirq, ch);
>  		list_del(&ch->vchan.chan.device_node);

[Severity: High]
This is a pre-existing issue, but does manually removing the channel from
the list prevent the dmaengine core from cleaning up resources?

The dmaengine core iterates over the channels list to unregister sysfs
devices, free per-CPU memory, and release the IDA. Because the list is
emptied here, the core skips cleanup, which might leave dangling sysfs
nodes and IDAs that prevent driver reloads.

> -		dmaengine_kill_bh(&ch->vchan.chan);
>  		tasklet_kill(&ch->done_tasklet);
>  		tasklet_kill(&ch->err_tasklet);
> +		dmaengine_kill_bh(&ch->vchan.chan);
>  	}
>  
>  	if (pdev->dev.of_node)

[Severity: Critical]
This is a pre-existing issue, but shouldn't driver removal stop active
DMA hardware to prevent memory corruption?

If a transfer is running when the driver is forcefully unbound, the
hardware DMA engine continues writing to memory. Since the software
stack is torn down and the client buffers may be freed, does this lack
of a call to disable the hardware requests or terminate channels allow
ongoing hardware DMA to corrupt arbitrary physical memory?


[Severity: Critical]
This is a pre-existing issue, but does this code cause a double-free
during channel termination?

drivers/dma/sf-pdma/sf-pdma.c:sf_pdma_terminate_all() {
        ...
        kfree(chan->desc);
        chan->desc = NULL;
        chan->xfer_err = false;
        vchan_get_all_descriptors(&chan->vchan, &head);
        ...
}

Because the descriptor is never removed from the desc_issued list, the
subsequent call to vchan_get_all_descriptors() collects this
already-freed descriptor. Then, vchan_dma_desc_free_list() invokes
sf_pdma_free_desc(), executing a second kfree() on the same memory.

The exact same double-free sequence also exists in
sf_pdma_free_chan_resources().


[Severity: High]
This is a pre-existing issue, but does sf_pdma_desc_residue() return
incorrect hardware residue for active transfers?

drivers/dma/sf-pdma/sf-pdma.c:sf_pdma_desc_residue() {
        ...
        list_for_each_entry(vd, &chan->vchan.desc_submitted, node)
        ...
            residue = readq(regs->residue);
        ...
}

This iterates only over chan->vchan.desc_submitted. Actively running
descriptors reside in desc_issued and are missed, resulting in a
return value of 0. Conversely, if a transfer is merely pending in
desc_submitted, it reads the live hardware residue register,
improperly assigning the actively running transfer's residue to a
pending transfer.

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