Re: [PATCH v2 01/11] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory leak of DMA channel resources on error path in `switchtec_dma_alloc_chan_resources()`.
- [High] Memory leak of `switchtec_dma_chan` structures and channel arrays on module probe error paths.
- [High] Deadlock due to client callback invocation while holding `complete_lock`.
- [High] `switchtec_dma_prep_desc()` returns with `submit_lock` held, preventing clients from preparing multiple descriptors.
- [High] Unsafe atomic delays of up to 100ms with a spinlock held and a 60ms mdelay in atomic context.
- [Medium] Large contiguous 1MB DMA memory allocations use GFP_NOWAIT in a sleepable context, risking allocation failure.
--

--- Patch [1]: [PATCH v2 01/11] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() ---
commit 4a5b609a9359892c57ed414c821b1570151f6b0b
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

This commit adds NULL assignments to the hw_sq, hw_cq, and desc_ring pointers
after they are freed in switchtec_dma_free_desc(). This fixes a double-free
bug that occurs if switchtec_dma_alloc_chan_resources() fails during unwind
and a subsequent retry also fails, calling the free function a second time.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 3ef9286406159..a4a7d66d042de 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -886,14 +886,18 @@ static void switchtec_dma_free_desc(struct switchtec_dma_chan *swdma_chan)
>  	if (swdma_chan->hw_sq)
>  		dma_free_coherent(swdma_dev->dma_dev.dev, size,
>  				  swdma_chan->hw_sq, swdma_chan->dma_addr_sq);
> +	swdma_chan->hw_sq = NULL;
>  
>  	size = SWITCHTEC_DMA_CQ_SIZE * sizeof(*swdma_chan->hw_cq);
>  	if (swdma_chan->hw_cq)
>  		dma_free_coherent(swdma_dev->dma_dev.dev, size,
>  				  swdma_chan->hw_cq, swdma_chan->dma_addr_cq);
> +	swdma_chan->hw_cq = NULL;
>  
> -	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++)
> +	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
>  		kfree(swdma_chan->desc_ring[i]);
> +		swdma_chan->desc_ring[i] = NULL;
> +	}
>  }
>  
>  static int switchtec_dma_alloc_desc(struct switchtec_dma_chan *swdma_chan)

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
switchtec_dma_alloc_chan_resources() leak the descriptors?

If enable_channel() fails after switchtec_dma_alloc_desc() has already allocated
resources, the code returns immediately:

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_chan_resources() {
    ...
	rc = enable_channel(swdma_chan);
	if (rc)
		return rc;
    ...
}

Will this permanently leak the DMA descriptors and coherent memory since
switchtec_dma_free_desc() is not called?

[Severity: High]
This is a pre-existing issue, but does switchtec_dma_chan_free() skip freeing
the channel structure itself?

If probe fails and calls switchtec_dma_chans_release() -> switchtec_dma_chan_free(),
it stops the channel but never calls kfree(swdma_chan):

drivers/dma/switchtec_dma.c:switchtec_dma_chan_free() {
    ...
	switchtec_dma_chan_stop(swdma_chan);

	return 0;
}

Also, on module probe error paths, switchtec_dma_create() calls
switchtec_dma_chans_release() but doesn't free the swdma_dev->swdma_chans array:

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
err_chans_release_exit:
	switchtec_dma_chans_release(pdev, swdma_dev);

err_exit:
	if (swdma_dev->chan_status_irq)
		free_irq(swdma_dev->chan_status_irq, swdma_dev);

	iounmap(swdma_dev->bar);
	kfree(swdma_dev);
	return rc;
}

Could this lead to memory leaks of the switchtec_dma_chan structures and the
swdma_chans array during early probe failures?

[Severity: High]
This isn't a bug introduced by this patch, but can we self-deadlock when invoking
the client callback?

In switchtec_dma_cleanup_completed(), the callback is invoked synchronously while
holding complete_lock:

drivers/dma/switchtec_dma.c:switchtec_dma_cleanup_completed() {
    ...
		spin_lock_bh(&swdma_chan->complete_lock);
    ...
			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
    ...
}

If the client callback checks TX status via dma_async_is_tx_complete(), it routes
to switchtec_dma_tx_status(), which calls switchtec_dma_cleanup_completed() again.

Will this cause a deadlock when trying to reacquire complete_lock?

[Severity: High]
This is a pre-existing issue, but does switchtec_dma_prep_desc() return with a
spinlock held?

drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
    ...
	spin_lock_bh(&swdma_chan->submit_lock);
    ...
	/* return with the lock held, it will be released in tx_submit */

	return &desc->txd;
}

Does this violate the DMA engine API by preventing clients from preparing
multiple descriptors before submitting them?

If a client batches requests and calls prep twice, won't it self-deadlock on
submit_lock?

[Severity: High]
This isn't a bug introduced by this patch, but do we have unsafe delays in
atomic contexts?

In pause_reset_channel(), which is reachable from the device_terminate_all
callback (allowed in atomic context), there is a 60ms busy-wait:

drivers/dma/switchtec_dma.c:pause_reset_channel() {
    ...
	/* wait 60ms to ensure no pending CEs */
	mdelay(60);
    ...
}

Additionally, wait_for_chan_status() spins for up to 100ms while holding
hw_ctrl_lock:

drivers/dma/switchtec_dma.c:wait_for_chan_status() {
    ...
	return readl_poll_timeout_atomic(&chan_hw->status, status,
					 (set && (status & mask)) ||
					 (!set && !(status & mask)),
					 10, 100 * USEC_PER_MSEC);
}

Will these long atomic delays cause severe system latency spikes or soft lockups?

[Severity: Medium]
This is a pre-existing issue, but is it safe to use GFP_NOWAIT for large
allocations in switchtec_dma_alloc_desc()?

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
    ...
	size = SWITCHTEC_DMA_SQ_SIZE * sizeof(*swdma_chan->hw_sq);
	swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev->dma_dev.dev, size,
					       &swdma_chan->dma_addr_sq,
					       GFP_NOWAIT);
    ...
}

Since device_alloc_chan_resources() is called under a mutex and is allowed to sleep,
could using GFP_NOWAIT here unnecessarily cause allocation failures under memory
pressure since it prevents direct reclaim and compaction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721155739.62120-1-logang@deltatee.com?part=1