Re: [PATCH v7 06/19] dmaengine: ti: k3-udma: move ring management functions to k3-udma-common.c

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

Pre-existing issues:
- [High] A concurrent interrupt can lead to a double-free or use-after-free of `uc->terminated_desc` and a race on ring reset during channel teardown in `udma_free_chan_resources`.
--

--- Patch [6]: [PATCH v7 06/19] dmaengine: ti: k3-udma: move ring management functions to k3-udma-common.c ---
commit cd5ab4996b5e12e7dbc4883f2a24b4c6b573b3a1
Author: Sai Sree Kartheek Adivi <[email protected]>

dmaengine: ti: k3-udma: move ring management functions to k3-udma-common.c

Relocate the ring management functions such as push, pop, and reset
from k3-udma.c to k3-udma-common.c file. These operations are
common across multiple K3 UDMA variants and will be reused by
future implementations like K3 UDMA v2.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> index daf5f19dfb3ab..fc5e2c99ee28a 100644
> --- a/drivers/dma/ti/k3-udma-common.c
> +++ b/drivers/dma/ti/k3-udma-common.c

[ ... ]

> @@ -1239,5 +1239,108 @@ void udma_desc_pre_callback(struct virt_dma_chan *vc,

[ ... ]

> +void udma_reset_rings(struct udma_chan *uc)
> +{
> +	struct k3_ring *ring1 = NULL;
> +	struct k3_ring *ring2 = NULL;

[ ... ]

> +	if (ring1)
> +		k3_ringacc_ring_reset_dma(ring1,
> +					  k3_ringacc_ring_get_occ(ring1));
> +	if (ring2)
> +		k3_ringacc_ring_reset(ring2);
> +
> +	/* make sure we are not leaking memory by stalled descriptor */
> +	if (uc->terminated_desc) {
> +		udma_desc_free(&uc->terminated_desc->vd);
> +		uc->terminated_desc = NULL;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(udma_reset_rings);

[Severity: High]
This is a pre-existing issue, but can this lead to a double-free or
use-after-free of uc->terminated_desc during channel teardown?

If a hardware teardown times out during udma_synchronize(), it falls back to a
hard reset via udma_reset_chan(), which calls udma_free_chan_resources().

udma_free_chan_resources() accesses uc->terminated_desc and calls
udma_reset_rings() without holding uc->vc.lock, before synchronizing and
freeing the IRQ handler.

If a delayed Teardown Completion Message (TDCM) interrupt fires concurrently,
udma_ring_irq_handler() will acquire uc->vc.lock and free uc->terminated_desc:

drivers/dma/ti/k3-udma.c:udma_ring_irq_handler() {
    ...
    if (uc->terminated_desc) {
        udma_desc_free(&uc->terminated_desc->vd);
        uc->terminated_desc = NULL;
    }
    ...
}

Since udma_reset_rings() does not hold the lock, could both threads enter
udma_desc_free() for the same descriptor, or could udma_reset_rings()
dereference a NULL pointer if the handler sets it to NULL just before access?

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