Re: [PATCH v8 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] Lockless check and free of `uc->terminated_desc` in `udma_reset_rings` races with the ring IRQ handler, leading to a double free and memory corruption.
--

--- Patch [6]: [PATCH v8 06/19] dmaengine: ti: k3-udma: move ring management functions to k3-udma-common.c ---
commit 0d8096a9286d7cd074f83a1328af2729690e353f
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. No functional changes intended.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> index 3130805a8ec40..3ec5cc932280c 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;
> +
> +	switch (uc->config.dir) {
> +	case DMA_DEV_TO_MEM:
> +		if (uc->rchan) {
> +			ring1 = uc->rflow->fd_ring;
> +			ring2 = uc->rflow->r_ring;
> +		}
> +		break;
> +	case DMA_MEM_TO_DEV:
> +	case DMA_MEM_TO_MEM:
> +		if (uc->tchan) {
> +			ring1 = uc->tchan->t_ring;
> +			ring2 = uc->tchan->tc_ring;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	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;
> +	}
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless check and
free of uc->terminated_desc race with the ring irq handler?

When tearing down a channel, udma_free_chan_resources() calls
udma_terminate_all(), which initiates hardware teardown and drops uc->vc.lock.

When teardown finishes, a hardware interrupt can fire on another cpu:

drivers/dma/ti/k3-udma.c:udma_ring_irq_handler() {
    ...
    complete_all(&uc->teardown_completed);

    if (uc->terminated_desc) {
        udma_desc_free(&uc->terminated_desc->vd);
        uc->terminated_desc = NULL;
    }
}

Meanwhile, the first cpu can concurrently proceed to call udma_reset_rings()
locklessly:

drivers/dma/ti/k3-udma.c:udma_free_chan_resources() {
    udma_terminate_all(chan);
    if (uc->terminated_desc) {
        udma_reset_chan(uc, false);
        udma_reset_rings(uc);
    }
}

Since udma_reset_rings() also evaluates and frees uc->terminated_desc without
holding a lock, can both threads evaluate it as true and trigger a double
free before either sets the pointer to NULL?

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