[PATCH v2 08/64] dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH

[PATCH v2 08/64] dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH

[Allen Pais]

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

Signed-off-by: Allen Pais <allen.lkml@gmail.com>
---
 drivers/dma/imx-dma.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index 81c6276436f8..3ff0bfbfa5ff 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -145,7 +145,6 @@ struct imxdma_channel {
 	struct imxdma_engine		*imxdma;
 	unsigned int			channel;
 
-	struct tasklet_struct		dma_tasklet;
 	struct list_head		ld_free;
 	struct list_head		ld_queue;
 	struct list_head		ld_active;
@@ -344,8 +343,8 @@ static void imxdma_watchdog(struct timer_list *t)
 
 	imx_dmav1_writel(imxdma, 0, DMA_CCR(channel));
 
-	/* Tasklet watchdog error handler */
-	tasklet_schedule(&imxdmac->dma_tasklet);
+	/* BH watchdog error handler */
+	dma_chan_schedule_bh(&imxdmac->chan);
 	dev_dbg(imxdma->dev, "channel %d: watchdog timeout!\n",
 		imxdmac->channel);
 }
@@ -390,8 +389,8 @@ static irqreturn_t imxdma_err_handler(int irq, void *dev_id)
 			imx_dmav1_writel(imxdma, 1 << i, DMA_DBOSR);
 			errcode |= IMX_DMA_ERR_BUFFER;
 		}
-		/* Tasklet error handler */
-		tasklet_schedule(&imxdma->channel[i].dma_tasklet);
+		/* BH error handler */
+		dma_chan_schedule_bh(&imxdma->channel[i].chan);
 
 		dev_warn(imxdma->dev,
 			 "DMA timeout on channel %d -%s%s%s%s\n", i,
@@ -448,8 +447,8 @@ static void dma_irq_handle_channel(struct imxdma_channel *imxdmac)
 			imx_dmav1_writel(imxdma, tmp, DMA_CCR(chno));
 
 			if (imxdma_chan_is_doing_cyclic(imxdmac))
-				/* Tasklet progression */
-				tasklet_schedule(&imxdmac->dma_tasklet);
+				/* BH progression */
+				dma_chan_schedule_bh(&imxdmac->chan);
 
 			return;
 		}
@@ -462,8 +461,8 @@ static void dma_irq_handle_channel(struct imxdma_channel *imxdmac)
 
 out:
 	imx_dmav1_writel(imxdma, 0, DMA_CCR(chno));
-	/* Tasklet irq */
-	tasklet_schedule(&imxdmac->dma_tasklet);
+	/* Schedule the IRQ BH */
+	dma_chan_schedule_bh(&imxdmac->chan);
 }
 
 static irqreturn_t dma_irq_handler(int irq, void *dev_id)
@@ -592,9 +591,10 @@ static int imxdma_xfer_desc(struct imxdma_desc *d)
 	return 0;
 }
 
-static void imxdma_tasklet(struct tasklet_struct *t)
+static void imxdma_tasklet(struct dma_chan *chan)
 {
-	struct imxdma_channel *imxdmac = from_tasklet(imxdmac, t, dma_tasklet);
+	struct imxdma_channel *imxdmac = container_of(chan, struct imxdma_channel,
+						      chan);
 	struct imxdma_engine *imxdma = imxdmac->imxdma;
 	struct imxdma_desc *desc, *next_desc;
 	unsigned long flags;
@@ -1142,7 +1142,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 		INIT_LIST_HEAD(&imxdmac->ld_free);
 		INIT_LIST_HEAD(&imxdmac->ld_active);
 
-		tasklet_setup(&imxdmac->dma_tasklet, imxdma_tasklet);
+		dma_chan_init_bh(&imxdmac->chan, imxdma_tasklet);
 		imxdmac->chan.device = &imxdma->dma_device;
 		dma_cookie_init(&imxdmac->chan);
 		imxdmac->channel = i;
@@ -1211,7 +1211,7 @@ static void imxdma_free_irq(struct platform_device *pdev, struct imxdma_engine *
 		if (!is_imx1_dma(imxdma))
 			disable_irq(imxdmac->irq);
 
-		tasklet_kill(&imxdmac->dma_tasklet);
+		dma_chan_kill_bh(&imxdmac->chan);
 	}
 }
 
-- 
2.43.0

Re: [PATCH v2 08/64] dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH

[sashiko-bot]

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

Pre-existing issues:
- [High] The per-channel watchdog timer is not synchronously canceled during driver teardown, leading to a Use-After-Free.
- [High] Pending BH works are not canceled on the probe error path, causing a Use-After-Free.
- [High] Early interrupts cause a NULL pointer dereference because IRQs are requested before channel lists are initialized.
- [High] Descriptor state is accessed after being placed on the free list and unlocking the spinlock, leading to a TOCTOU race with prep functions.
- [High] Channel state and lists are read and modified without proper lock protection in descriptor preparation functions.
- [High] The driver frees DMA descriptors in `imxdma_free_chan_resources` without synchronizing with the completion BH, causing a Use-After-Free.
--

--- Patch [8]: [PATCH v2 08/64] dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH ---
commit 6c1578dd7a98a9ceebdc129c266b46d0c2bdf6e3
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
> index 81c6276436f81..3ff0bfbfa5ffa 100644
> --- a/drivers/dma/imx-dma.c
> +++ b/drivers/dma/imx-dma.c

[ ... ]

> @@ -592,9 +591,10 @@ static int imxdma_xfer_desc(struct imxdma_desc *d)
>  	return 0;
>  }
>  
> -static void imxdma_tasklet(struct tasklet_struct *t)
> +static void imxdma_tasklet(struct dma_chan *chan)
>  {
> -	struct imxdma_channel *imxdmac = from_tasklet(imxdmac, t, dma_tasklet);
> +	struct imxdma_channel *imxdmac = container_of(chan, struct imxdma_channel,
> +						      chan);
>  	struct imxdma_engine *imxdma = imxdmac->imxdma;
>  	struct imxdma_desc *desc, *next_desc;
>  	unsigned long flags;

[Severity: High]
This is a pre-existing issue, but is it safe to invoke the callback after
releasing the spinlock and moving the descriptor to the free list?

At the end of imxdma_tasklet(), the lock is released and the callback is
subsequently invoked directly from the descriptor:

out:
        spin_unlock_irqrestore(&imxdma->lock, flags);

        dmaengine_desc_get_callback_invoke(&desc->desc, NULL);
}

Since the descriptor is already on the free list, couldn't a concurrent
call to imxdma_prep_dma_memcpy() pick this exact descriptor from ld_free
and clear desc->desc.callback = NULL, leading to execution of a NULL or
corrupted callback?

[Severity: High]
This isn't a bug introduced by this patch, but how are the channel lists
synchronized against concurrent preparation?

The completion BH actively splices and modifies ld_free and ld_active while
holding imxdma->lock. However, prep functions like imxdma_prep_dma_memcpy()
read ld_free and call list_first_entry() without acquiring the channel lock:

drivers/dma/imx-dma.c:imxdma_prep_dma_memcpy() {
        ...
        if (list_empty(&imxdmac->ld_free) ||
            imxdma_chan_is_doing_cyclic(imxdmac))
                return NULL;

        desc = list_first_entry(&imxdmac->ld_free, struct imxdma_desc, node);
        ...
}

Could this lead to linked list corruption or NULL pointer dereferences if
clients actively submit transfers while completions fire?

[ ... ]

> @@ -1142,7 +1142,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  		INIT_LIST_HEAD(&imxdmac->ld_free);
>  		INIT_LIST_HEAD(&imxdmac->ld_active);
>  
> -		tasklet_setup(&imxdmac->dma_tasklet, imxdma_tasklet);
> +		dma_chan_init_bh(&imxdmac->chan, imxdma_tasklet);
>  		imxdmac->chan.device = &imxdma->dma_device;
>  		dma_cookie_init(&imxdmac->chan);
>  		imxdmac->channel = i;

[Severity: High]
This is a pre-existing issue, but what happens if the probe fails after
this point?

Looking at the error path later in imxdma_probe():

err_of_dma_controller:
        dma_async_device_unregister(&imxdma->dma_device);
disable_dma_ahb_clk:
        clk_disable_unprepare(imxdma->dma_ahb);
disable_dma_ipg_clk:
        clk_disable_unprepare(imxdma->dma_ipg);
        return ret;

It exits without calling dma_chan_kill_bh() or imxdma_free_irq(). If a
spurious or lingering hardware interrupt triggers before
dma_async_device_register() completes, dma_irq_handler() schedules the BH.
If a subsequent initialization step fails, devres frees the device memory
but the scheduled BH is never canceled, which might lead to a use-after-free.

[Severity: High]
This isn't a bug introduced by this patch, but does the early interrupt
setup risk a NULL pointer dereference?

The IRQ is requested earlier in imxdma_probe():

        ret = devm_request_irq(&pdev->dev, irq,
                               dma_irq_handler, 0, "DMA", imxdma);

This happens well before the list heads like ld_active are initialized in
the loop shown in the diff above. If an interrupt fires early,
dma_irq_handle_channel() checks the list:

        if (list_empty(&imxdmac->ld_active)) {
                spin_unlock_irqrestore(&imxdma->lock, flags);
                goto out;
        }

Since memory is 0-initialized, ld_active.next is NULL. This makes
list_empty() evaluate to false, leading directly to a crash via
list_first_entry() right below it.

[ ... ]

> @@ -1211,7 +1211,7 @@ static void imxdma_free_irq(struct platform_device *pdev, struct imxdma_engine *
>  		if (!is_imx1_dma(imxdma))
>  			disable_irq(imxdmac->irq);
>  
> -		tasklet_kill(&imxdmac->dma_tasklet);
> +		dma_chan_kill_bh(&imxdmac->chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but is it safe to kill the BH here without
also stopping the watchdog timer?

This function stops the BH but omits timer_shutdown_sync(&imxdmac->watchdog),
leaving it active. If the driver is unbound via sysfs while a transfer is
active, imxdma_remove() invokes imxdma_free_irq() and then unregisters
and frees the engine. Couldn't the still-armed timer fire on freed memory
and schedule the destroyed BH workqueue?

[Severity: High]
This isn't a bug introduced by this patch, but does
imxdma_free_chan_resources() safely synchronize with this BH?

When a channel is released, descriptors are indiscriminately freed without
stopping or waiting for the BH to finish:

drivers/dma/imx-dma.c:imxdma_free_chan_resources() {
        ...
        list_for_each_entry_safe(desc, _desc, &imxdmac->ld_free, node) {
                kfree(desc);
                imxdmac->descs_allocated--;
        }
        ...
}

Since the driver lacks a device_synchronize callback and does not call
dma_chan_kill_bh() during channel release, could the completion BH be
actively running on another CPU trying to invoke the callback for a
descriptor that is being freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=8

[PATCH v2 13/64] dmaengine: mxs-dma: use dma_chan BH scheduling

[Allen Pais]

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

Signed-off-by: Allen Pais <allen.lkml@gmail.com>
---
 drivers/dma/mxs-dma.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 7acb3d29dad3..8cecc0add82d 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -109,7 +109,6 @@ struct mxs_dma_chan {
 	struct mxs_dma_engine		*mxs_dma;
 	struct dma_chan			chan;
 	struct dma_async_tx_descriptor	desc;
-	struct tasklet_struct		tasklet;
 	unsigned int			chan_irq;
 	struct mxs_dma_ccw		*ccw;
 	dma_addr_t			ccw_phys;
@@ -300,9 +299,9 @@ static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
 	return dma_cookie_assign(tx);
 }
 
-static void mxs_dma_tasklet(struct tasklet_struct *t)
+static void mxs_dma_tasklet(struct dma_chan *chan)
 {
-	struct mxs_dma_chan *mxs_chan = from_tasklet(mxs_chan, t, tasklet);
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
 
 	dmaengine_desc_get_callback_invoke(&mxs_chan->desc, NULL);
 }
@@ -386,8 +385,8 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
 		dma_cookie_complete(&mxs_chan->desc);
 	}
 
-	/* schedule tasklet on this channel */
-	tasklet_schedule(&mxs_chan->tasklet);
+	/* schedule BH on this channel */
+	dma_chan_schedule_bh(&mxs_chan->chan);
 
 	return IRQ_HANDLED;
 }
@@ -781,7 +780,7 @@ static int mxs_dma_probe(struct platform_device *pdev)
 		mxs_chan->chan.device = &mxs_dma->dma_device;
 		dma_cookie_init(&mxs_chan->chan);
 
-		tasklet_setup(&mxs_chan->tasklet, mxs_dma_tasklet);
+		dma_chan_init_bh(&mxs_chan->chan, mxs_dma_tasklet);
 
 
 		/* Add the channel to mxs_chan list */
-- 
2.43.0

Re: [PATCH v2 13/64] dmaengine: mxs-dma: use dma_chan BH scheduling

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing synchronization/flushing of bottom-half work during channel teardown leads to a use-after-free of client resources.
- [High] Requesting IRQ before clock enable and descriptor initialization causes a kernel panic on pending interrupt.
- [High] The driver allocates the DMA engine state with devm_kzalloc but lacks a device_release callback, risking UAF on unbind.
--

--- Patch [13]: [PATCH v2 13/64] dmaengine: mxs-dma: use dma_chan BH scheduling ---
commit cc33b3c421743dc24481c221e9955791a7689087
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: mxs-dma: use dma_chan BH scheduling

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> The handler continues to run in softirq context while dmaengine owns
> the common scheduling and teardown mechanism.

[Severity: High]
This is a pre-existing issue, but since the commit message notes that the
teardown mechanism is being updated, does mxs_dma_free_chan_resources()
need to call dma_chan_kill_bh() to prevent use-after-free conditions?

If a client releases the channel concurrently with an interrupt,
mxs_dma_free_chan_resources() currently returns without flushing pending
work:

drivers/dma/mxs-dma.c:mxs_dma_free_chan_resources() {
	...
	mxs_dma_disable_chan(chan);

	free_irq(mxs_chan->chan_irq, mxs_dma);
}

Could this allow the asynchronous work to outlive the channel allocation
and dereference a freed client callback when mxs_dma_tasklet() runs?

> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index 7acb3d29dad30..8cecc0add82dc 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c

[ ... ]

> @@ -385,8 +385,8 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
>  		dma_cookie_complete(&mxs_chan->desc);
>  	}
>  
> -	/* schedule tasklet on this channel */
> -	tasklet_schedule(&mxs_chan->tasklet);
> +	/* schedule BH on this channel */
> +	dma_chan_schedule_bh(&mxs_chan->chan);
>  
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but can this handler be triggered before the
hardware clock is enabled and the descriptor is initialized?

Looking at mxs_dma_alloc_chan_resources(), the IRQ is requested before the
clock is enabled:

drivers/dma/mxs-dma.c:mxs_dma_alloc_chan_resources() {
	...
	ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
			  0, "mxs-dma", mxs_dma);
	if (ret)
		goto err_irq;

	ret = clk_prepare_enable(mxs_dma->clk);
	...
}

If a spurious or bootloader-pending interrupt fires immediately, could
mxs_dma_int_handler() process a DMA_COMPLETE status and hit a BUG_ON when
it calls dma_cookie_complete(&mxs_chan->desc) because the cookie is
still 0?

[ ... ]

> @@ -780,7 +780,7 @@ static int mxs_dma_probe(struct platform_device *pdev)
>  		mxs_chan->chan.device = &mxs_dma->dma_device;
>  		dma_cookie_init(&mxs_chan->chan);
>  
> -		tasklet_setup(&mxs_chan->tasklet, mxs_dma_tasklet);
> +		dma_chan_init_bh(&mxs_chan->chan, mxs_dma_tasklet);
>  
>  
>  		/* Add the channel to mxs_chan list */

[Severity: High]
This is a pre-existing issue, but is the driver state vulnerable to a
use-after-free if the device is unbound?

The driver allocates mxs_dma using devm_kzalloc() but appears to register
the async device without providing a device_release callback:

drivers/dma/mxs-dma.c:mxs_dma_probe() {
	...
	mxs_dma = devm_kzalloc(dev, sizeof(*mxs_dma), GFP_KERNEL);
	if (!mxs_dma)
		return -ENOMEM;
	...
	ret = dmaenginem_async_device_register(&mxs_dma->dma_device);
	...
}

If the driver is unbound via sysfs while a client still holds a reference
to a DMA channel, will devres immediately free mxs_dma, causing a panic
when the client later accesses the channel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=13

[PATCH v2 39/64] dmaengine: fsl-edma-common: kill vchan BH on teardown

[Allen Pais]

Use dma_chan_kill_bh() for virt-dma cleanup.

Signed-off-by: Allen Pais <allen.lkml@gmail.com>
---
 drivers/dma/fsl-edma-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
index bb7531c456df..70b40e60c39a 100644
--- a/drivers/dma/fsl-edma-common.c
+++ b/drivers/dma/fsl-edma-common.c
@@ -915,7 +915,7 @@ void fsl_edma_cleanup_vchan(struct dma_device *dmadev)
 	list_for_each_entry_safe(chan, _chan,
 				&dmadev->channels, vchan.chan.device_node) {
 		list_del(&chan->vchan.chan.device_node);
-		tasklet_kill(&chan->vchan.task);
+		dma_chan_kill_bh(&chan->vchan.chan);
 	}
 }
 
-- 
2.43.0

Re: [PATCH v2 39/64] dmaengine: fsl-edma-common: kill vchan BH on teardown

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Bypassed DMA channel teardown leading to resource leaks and Use-After-Free via dangling sysfs nodes.
--

--- Patch [39]: [PATCH v2 39/64] dmaengine: fsl-edma-common: kill vchan BH on teardown ---
commit 72f422b1de100bcd764808c0c9305bd937347a17
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: fsl-edma-common: kill vchan BH on teardown

This commit updates virt-dma cleanup to use dma_chan_kill_bh() instead of
tasklet_kill() when tearing down channels in fsl_edma_cleanup_vchan().

> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index bb7531c456dfa..70b40e60c39a0 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -915,7 +915,7 @@ void fsl_edma_cleanup_vchan(struct dma_device *dmadev)
>  	list_for_each_entry_safe(chan, _chan,
>  				&dmadev->channels, vchan.chan.device_node) {
>  		list_del(&chan->vchan.chan.device_node);

[Severity: Critical]
This is a pre-existing issue, but does unlinking the channel here bypass
the core DMA engine channel teardown?

During driver removal (for example, in fsl_edma_remove() or
mcf_edma_remove()), calling list_del() before dma_async_device_unregister()
means the core unregister function will see an empty dmadev->channels list.

If the channels are unlinked prematurely, dma_async_device_unregister()
skips __dma_async_device_channel_unregister() for all channels. Can this
leave dangling sysfs nodes active and leak resources like IDAs or percpu
memory, potentially leading to a use-after-free if devres subsequently
frees the underlying channel structures while the sysfs files remain
accessible?

(Note: This same unlinking pattern before unregister also appears to exist
in mtk_cqdma_remove() and sprd_dma_remove()).

> -		tasklet_kill(&chan->vchan.task);
> +		dma_chan_kill_bh(&chan->vchan.chan);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=39

[PATCH v2 46/64] dmaengine: imx-sdma: kill vchan BH on teardown

[Allen Pais]

Use dma_chan_kill_bh() for virt-dma cleanup.

Signed-off-by: Allen Pais <allen.lkml@gmail.com>
---
 drivers/dma/imx-sdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 36368835a845..46f66e5f9f30 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2399,11 +2399,11 @@ static void sdma_remove(struct platform_device *pdev)
 	int i;
 
 	devm_free_irq(&pdev->dev, sdma->irq, sdma);
-	/* Kill the tasklet */
+	/* Kill the channel BH */
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];
 
-		tasklet_kill(&sdmac->vc.task);
+		dma_chan_kill_bh(&sdmac->vc.chan);
 		sdma_free_chan_resources(&sdmac->vc.chan);
 	}
 
-- 
2.43.0

Re: [PATCH v2 46/64] dmaengine: imx-sdma: kill vchan BH on teardown

[sashiko-bot]

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

Pre-existing issues:
- [High] Kernel OOPS on module removal due to uninitialized list head in channel 0.
- [High] Acquiring an uninitialized spinlock for channel 0 during teardown causes a kernel BUG.
- [High] Accessing hardware registers while clocks are disabled causes a synchronous external abort.
- [High] Unbalanced clock disables in `sdma_remove` trigger refcount underflow warnings.
- [High] Use-After-Free race condition in `sdma_update_channel_loop` during concurrent termination.
--

--- Patch [46]: [PATCH v2 46/64] dmaengine: imx-sdma: kill vchan BH on teardown ---
commit 3996df4e4fdc79870fd89eac795a9295e1141104
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: imx-sdma: kill vchan BH on teardown

This commit modifies the virt-dma cleanup path to use dma_chan_kill_bh()
during driver teardown.

> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2399,11 +2399,11 @@ static void sdma_remove(struct platform_device *pdev)
>  	int i;
>  
>  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
> -	/* Kill the tasklet */
> +	/* Kill the channel BH */
>  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
>  		struct sdma_channel *sdmac = &sdma->channel[i];
>  
> -		tasklet_kill(&sdmac->vc.task);
> +		dma_chan_kill_bh(&sdmac->vc.chan);
>  		sdma_free_chan_resources(&sdmac->vc.chan);

[Severity: High]
This is a pre-existing issue, but does iterating over all channels here lead
to a null pointer dereference for channel 0?

Channel 0 intentionally skips vchan_init() during probe, leaving its list
heads 0-initialized. When sdma_free_chan_resources() calls
vchan_synchronize() for channel 0, it executes:

drivers/dma/virt-dma.h:vchan_synchronize() {
    ...
    list_splice_tail_init(&vc->desc_terminated, &head);

Could this evaluate an empty list on the null next pointer and cause a panic?


[Severity: High]
This isn't a bug introduced by this patch, but does calling
sdma_free_chan_resources() on channel 0 also attempt to acquire an
uninitialized spinlock?

During teardown, sdma_terminate_all() executes:

drivers/dma/imx-sdma.c:sdma_terminate_all() {
    ...
    spin_lock_irqsave(&sdmac->vc.lock, flags);

Since vchan_init() is skipped for channel 0, the lock remains uninitialized
from devm_kzalloc. Can this result in a bad magic system crash?


[Severity: High]
This is a pre-existing issue, but does writing to the hardware registers
during this loop cause a synchronous external abort?

When iterating over all channels, unallocated channels (or when all channels
are inactive) will have the ipg and ahb clocks disabled.

sdma_free_chan_resources() calls sdma_terminate_all(), which invokes
sdma_disable_channel() doing:

drivers/dma/imx-sdma.c:sdma_disable_channel() {
    ...
    writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);

Writing to an i.MX SDMA register while the IP block clocks are gated
typically causes a hardware bus fault. Is there a way to verify the clocks
are enabled before this write?


[Severity: High]
This isn't a bug introduced by this patch, but does this cleanup loop
cause clock refcount underflow warnings?

sdma_free_chan_resources() unconditionally calls:

drivers/dma/imx-sdma.c:sdma_free_chan_resources() {
    ...
    clk_disable(sdma->clk_ipg);
    clk_disable(sdma->clk_ahb);

Since clk_enable() is only invoked dynamically when a client requests a
channel, calling disable on unallocated channels seems unbalanced. Could this
break clock tracking?


[Severity: High]
This is a pre-existing issue, but is there a use-after-free race condition
in sdma_update_channel_loop() during concurrent termination?

sdma_update_channel_loop() drops the lock to invoke the callback:

drivers/dma/imx-sdma.c:sdma_update_channel_loop() {
    ...
    spin_unlock(&sdmac->vc.lock);
    dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
    spin_lock(&sdmac->vc.lock);

    /* Assign buffer ownership to SDMA */
    bd->mode.status |= BD_DONE;

If a concurrent thread calls dmaengine_terminate_sync(), it can take the
lock, move the active descriptor to the terminated list, and schedule the
terminate_worker which frees the descriptor memory.

When the IRQ thread reacquires the lock, it writes to bd->mode.status. Since
bd points inside the freed descriptor, does this overwrite freed memory?

>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=46