[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

[PATCH v3 01/34] dmaengine: add tasklet-backed channel BH helpers

[Allen Pais]

DMAengine drivers commonly use a per-channel tasklet to invoke client
callbacks. Add helpers that initialize, schedule, and kill a channel
bottom half, with an initial tasklet-backed implementation that preserves
the existing execution context.

Convert virt-dma to the new API and remove its private tasklet. Update all
drivers that directly kill or override that tasklet in the same change so
no stale users remain. While touching the completion handler, avoid forming
a result pointer from a NULL cyclic descriptor.

This establishes a backend-independent API before changing how channel
bottom halves are dispatched.

Signed-off-by: Allen Pais <allen.lkml@gmail.com>
---
 drivers/dma/bcm2835-dma.c                     |  2 +-
 drivers/dma/dma-axi-dmac.c                    | 10 +++--
 drivers/dma/dma-jz4780.c                      |  2 +-
 drivers/dma/dmaengine.c                       | 42 +++++++++++++++++++
 .../dma/dw-axi-dmac/dw-axi-dmac-platform.c    |  2 +-
 drivers/dma/dw-edma/dw-edma-core.c            |  2 +-
 drivers/dma/fsl-edma-common.c                 |  2 +-
 drivers/dma/fsl-qdma.c                        |  2 +-
 drivers/dma/hisi_dma.c                        |  2 +-
 drivers/dma/hsu/hsu.c                         |  2 +-
 drivers/dma/idma64.c                          |  4 +-
 drivers/dma/img-mdc-dma.c                     |  2 +-
 drivers/dma/imx-sdma.c                        |  4 +-
 drivers/dma/k3dma.c                           |  2 +-
 drivers/dma/loongson/loongson1-apb-dma.c      |  2 +-
 drivers/dma/mediatek/mtk-cqdma.c              |  2 +-
 drivers/dma/mediatek/mtk-hsdma.c              |  2 +-
 drivers/dma/mediatek/mtk-uart-apdma.c         |  4 +-
 drivers/dma/owl-dma.c                         |  2 +-
 drivers/dma/pxa_dma.c                         |  2 +-
 drivers/dma/qcom/bam_dma.c                    |  4 +-
 drivers/dma/qcom/qcom_adm.c                   |  4 +-
 drivers/dma/sa11x0-dma.c                      |  2 +-
 drivers/dma/sf-pdma/sf-pdma.c                 |  2 +-
 drivers/dma/sprd-dma.c                        |  2 +-
 drivers/dma/st_fdma.c                         |  2 +-
 drivers/dma/sun6i-dma.c                       |  2 +-
 drivers/dma/tegra186-gpc-dma.c                |  2 +-
 drivers/dma/tegra210-adma.c                   |  2 +-
 drivers/dma/ti/edma.c                         |  2 +-
 drivers/dma/ti/k3-udma.c                      | 12 +++---
 drivers/dma/ti/omap-dma.c                     |  2 +-
 drivers/dma/virt-dma.c                        | 12 +++---
 drivers/dma/virt-dma.h                        |  7 ++--
 include/linux/dmaengine.h                     | 28 +++++++++++++
 35 files changed, 125 insertions(+), 54 deletions(-)

diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 06d830d36882..c8add249dbfb 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -829,7 +829,7 @@ static void bcm2835_dma_free(struct bcm2835_dmadev *od)
 	list_for_each_entry_safe(c, next, &od->ddev.channels,
 				 vc.chan.device_node) {
 		list_del(&c->vc.chan.device_node);
-		tasklet_kill(&c->vc.task);
+		dmaengine_kill_bh(&c->vc.chan);
 	}
 
 	dma_unmap_page_attrs(od->ddev.dev, od->zero_page, PAGE_SIZE,
diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index d47ff27e1408..d245d04c3a27 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -1195,9 +1195,11 @@ static int axi_dmac_detect_caps(struct axi_dmac *dmac, unsigned int version)
 	return 0;
 }
 
-static void axi_dmac_tasklet_kill(void *task)
+static void axi_dmac_kill_bh(void *data)
 {
-	tasklet_kill(task);
+	struct dma_chan *chan = data;
+
+	dmaengine_kill_bh(chan);
 }
 
 static void axi_dmac_free_dma_controller(void *of_node)
@@ -1302,8 +1304,8 @@ static int axi_dmac_probe(struct platform_device *pdev)
 	 * Put the action in here so it get's done before unregistering the DMA
 	 * device.
 	 */
-	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_tasklet_kill,
-				       &dmac->chan.vchan.task);
+	ret = devm_add_action_or_reset(&pdev->dev, axi_dmac_kill_bh,
+				       &dmac->chan.vchan.chan);
 	if (ret)
 		return ret;
 
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 6070dfdb7114..738801501e29 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -1019,7 +1019,7 @@ static void jz4780_dma_remove(struct platform_device *pdev)
 	free_irq(jzdma->irq, jzdma);
 
 	for (i = 0; i < jzdma->soc_data->nb_channels; i++)
-		tasklet_kill(&jzdma->chan[i].vchan.task);
+		dmaengine_kill_bh(&jzdma->chan[i].vchan.chan);
 }
 
 static const struct jz4780_dma_soc_data jz4740_dma_soc_data = {
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 9049171df857..d8fc7eb71b48 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -1428,6 +1428,48 @@ static void dmaengine_destroy_unmap_pool(void)
 	}
 }
 
+static void dma_chan_bh_entry(struct tasklet_struct *tasklet)
+{
+	struct dma_chan *chan = from_tasklet(chan, tasklet, bh_tasklet);
+	dmaengine_bh_work_fn fn = READ_ONCE(chan->bh_work_fn);
+
+	if (fn)
+		fn(chan);
+}
+
+void dmaengine_init_bh(struct dma_chan *chan, dmaengine_bh_work_fn fn)
+{
+	if (WARN_ON(!fn))
+		return;
+
+	if (WARN_ON(chan->bh_work_initialized))
+		return;
+
+	chan->bh_work_fn = fn;
+	tasklet_setup(&chan->bh_tasklet, dma_chan_bh_entry);
+	chan->bh_work_initialized = true;
+}
+EXPORT_SYMBOL_GPL(dmaengine_init_bh);
+
+bool dmaengine_schedule_bh(struct dma_chan *chan)
+{
+	if (WARN_ON(!chan->bh_work_initialized))
+		return false;
+
+	tasklet_schedule(&chan->bh_tasklet);
+	return true;
+}
+EXPORT_SYMBOL_GPL(dmaengine_schedule_bh);
+
+void dmaengine_kill_bh(struct dma_chan *chan)
+{
+	if (!chan->bh_work_initialized)
+		return;
+
+	tasklet_kill(&chan->bh_tasklet);
+}
+EXPORT_SYMBOL_GPL(dmaengine_kill_bh);
+
 static int __init dmaengine_init_unmap_pool(void)
 {
 	int i;
diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
index bcefaff03b5c..a2b688e7f47e 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -1663,7 +1663,7 @@ static void dw_remove(struct platform_device *pdev)
 	list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
 				 vc.chan.device_node) {
 		list_del(&chan->vc.chan.device_node);
-		tasklet_kill(&chan->vc.task);
+		dmaengine_kill_bh(&chan->vc.chan);
 	}
 }
 
diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 89a4c498a17b..ade866fba2ad 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -1170,7 +1170,7 @@ int dw_edma_remove(struct dw_edma_chip *chip)
 	dma_async_device_unregister(&dw->dma);
 	list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
 				 vc.chan.device_node) {
-		tasklet_kill(&chan->vc.task);
+		dmaengine_kill_bh(&chan->vc.chan);
 		list_del(&chan->vc.chan.device_node);
 	}
 
diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
index bb7531c456df..90ae678c68d4 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);
+		dmaengine_kill_bh(&chan->vchan.chan);
 	}
 }
 
diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
index df843fad0ece..b0ace8bad498 100644
--- a/drivers/dma/fsl-qdma.c
+++ b/drivers/dma/fsl-qdma.c
@@ -1255,7 +1255,7 @@ static void fsl_qdma_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);
+		dmaengine_kill_bh(&chan->vchan.chan);
 	}
 }
 
diff --git a/drivers/dma/hisi_dma.c b/drivers/dma/hisi_dma.c
index 28bf818f9aa6..10eb98350b9f 100644
--- a/drivers/dma/hisi_dma.c
+++ b/drivers/dma/hisi_dma.c
@@ -720,7 +720,7 @@ static void hisi_dma_disable_qps(struct hisi_dma_dev *hdma_dev)
 
 	for (i = 0; i < hdma_dev->chan_num; i++) {
 		hisi_dma_disable_qp(hdma_dev, i);
-		tasklet_kill(&hdma_dev->chan[i].vc.task);
+		dmaengine_kill_bh(&hdma_dev->chan[i].vc.chan);
 	}
 }
 
diff --git a/drivers/dma/hsu/hsu.c b/drivers/dma/hsu/hsu.c
index f62d60d7bc6b..315d0ebecd57 100644
--- a/drivers/dma/hsu/hsu.c
+++ b/drivers/dma/hsu/hsu.c
@@ -500,7 +500,7 @@ int hsu_dma_remove(struct hsu_dma_chip *chip)
 	for (i = 0; i < hsu->nr_channels; i++) {
 		struct hsu_dma_chan *hsuc = &hsu->chan[i];
 
-		tasklet_kill(&hsuc->vchan.task);
+		dmaengine_kill_bh(&hsuc->vchan.chan);
 	}
 
 	return 0;
diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
index 5fcd1befc92d..1d395fb735ff 100644
--- a/drivers/dma/idma64.c
+++ b/drivers/dma/idma64.c
@@ -617,14 +617,14 @@ static void idma64_remove(struct idma64_chip *chip)
 
 	/*
 	 * Explicitly call devm_request_irq() to avoid the side effects with
-	 * the scheduled tasklets.
+	 * scheduled BH work.
 	 */
 	devm_free_irq(chip->dev, chip->irq, idma64);
 
 	for (i = 0; i < idma64->dma.chancnt; i++) {
 		struct idma64_chan *idma64c = &idma64->chan[i];
 
-		tasklet_kill(&idma64c->vchan.task);
+		dmaengine_kill_bh(&idma64c->vchan.chan);
 	}
 }
 
diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
index b3765ba15803..0c6024088444 100644
--- a/drivers/dma/img-mdc-dma.c
+++ b/drivers/dma/img-mdc-dma.c
@@ -1031,7 +1031,7 @@ static void mdc_dma_remove(struct platform_device *pdev)
 
 		devm_free_irq(&pdev->dev, mchan->irq, mchan);
 
-		tasklet_kill(&mchan->vc.task);
+		dmaengine_kill_bh(&mchan->vc.chan);
 	}
 
 	pm_runtime_disable(&pdev->dev);
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 36368835a845..4d13b9d2880d 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);
+		dmaengine_kill_bh(&sdmac->vc.chan);
 		sdma_free_chan_resources(&sdmac->vc.chan);
 	}
 
diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index e84f197fea76..3d73b391e42e 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -976,7 +976,7 @@ static void k3_dma_remove(struct platform_device *op)
 
 	list_for_each_entry_safe(c, cn, &d->slave.channels, vc.chan.device_node) {
 		list_del(&c->vc.chan.device_node);
-		tasklet_kill(&c->vc.task);
+		dmaengine_kill_bh(&c->vc.chan);
 	}
 	tasklet_kill(&d->task);
 	clk_disable_unprepare(d->clk);
diff --git a/drivers/dma/loongson/loongson1-apb-dma.c b/drivers/dma/loongson/loongson1-apb-dma.c
index 89786cbd20ab..52a360719644 100644
--- a/drivers/dma/loongson/loongson1-apb-dma.c
+++ b/drivers/dma/loongson/loongson1-apb-dma.c
@@ -552,7 +552,7 @@ static void ls1x_dma_chan_remove(struct ls1x_dma *dma)
 
 		if (chan->vc.chan.device == &dma->ddev) {
 			list_del(&chan->vc.chan.device_node);
-			tasklet_kill(&chan->vc.task);
+			dmaengine_kill_bh(&chan->vc.chan);
 		}
 	}
 }
diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
index 80791e30aec2..7b44d3358cda 100644
--- a/drivers/dma/mediatek/mtk-cqdma.c
+++ b/drivers/dma/mediatek/mtk-cqdma.c
@@ -895,7 +895,7 @@ static void mtk_cqdma_remove(struct platform_device *pdev)
 		vc = &cqdma->vc[i];
 
 		list_del(&vc->vc.chan.device_node);
-		tasklet_kill(&vc->vc.task);
+		dmaengine_kill_bh(&vc->vc.chan);
 	}
 
 	/* disable interrupt */
diff --git a/drivers/dma/mediatek/mtk-hsdma.c b/drivers/dma/mediatek/mtk-hsdma.c
index a43412ff5edd..75555fa41536 100644
--- a/drivers/dma/mediatek/mtk-hsdma.c
+++ b/drivers/dma/mediatek/mtk-hsdma.c
@@ -1020,7 +1020,7 @@ static void mtk_hsdma_remove(struct platform_device *pdev)
 		vc = &hsdma->vc[i];
 
 		list_del(&vc->vc.chan.device_node);
-		tasklet_kill(&vc->vc.task);
+		dmaengine_kill_bh(&vc->vc.chan);
 	}
 
 	/* Disable DMA interrupt */
diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
index c269d84d7bd2..8ce206a0ecc5 100644
--- a/drivers/dma/mediatek/mtk-uart-apdma.c
+++ b/drivers/dma/mediatek/mtk-uart-apdma.c
@@ -312,7 +312,7 @@ static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
 
 	free_irq(c->irq, chan);
 
-	tasklet_kill(&c->vc.task);
+	dmaengine_kill_bh(&c->vc.chan);
 
 	vchan_free_chan_resources(&c->vc);
 
@@ -463,7 +463,7 @@ static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
 			struct mtk_chan, vc.chan.device_node);
 
 		list_del(&c->vc.chan.device_node);
-		tasklet_kill(&c->vc.task);
+		dmaengine_kill_bh(&c->vc.chan);
 	}
 }
 
diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
index 7c80572fc71d..0d9f324adba0 100644
--- a/drivers/dma/owl-dma.c
+++ b/drivers/dma/owl-dma.c
@@ -1055,7 +1055,7 @@ static inline void owl_dma_free(struct owl_dma *od)
 	list_for_each_entry_safe(vchan,
 				 next, &od->dma.channels, vc.chan.device_node) {
 		list_del(&vchan->vc.chan.device_node);
-		tasklet_kill(&vchan->vc.task);
+		dmaengine_kill_bh(&vchan->vc.chan);
 	}
 }
 
diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index fa2ee0b3e09f..2cca8c31929c 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -1215,7 +1215,7 @@ static void pxad_free_channels(struct dma_device *dmadev)
 	list_for_each_entry_safe(c, cn, &dmadev->channels,
 				 vc.chan.device_node) {
 		list_del(&c->vc.chan.device_node);
-		tasklet_kill(&c->vc.task);
+		dmaengine_kill_bh(&c->vc.chan);
 	}
 }
 
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 1bb26af0405f..c23496c3ac69 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1387,7 +1387,7 @@ static int bam_dma_probe(struct platform_device *pdev)
 	dma_async_device_unregister(&bdev->common);
 err_bam_channel_exit:
 	for (i = 0; i < bdev->num_channels; i++)
-		tasklet_kill(&bdev->channels[i].vc.task);
+		dmaengine_kill_bh(&bdev->channels[i].vc.chan);
 err_tasklet_kill:
 	tasklet_kill(&bdev->task);
 err_disable_clk:
@@ -1413,7 +1413,7 @@ static void bam_dma_remove(struct platform_device *pdev)
 
 	for (i = 0; i < bdev->num_channels; i++) {
 		bam_dma_terminate_all(&bdev->channels[i].vc.chan);
-		tasklet_kill(&bdev->channels[i].vc.task);
+		dmaengine_kill_bh(&bdev->channels[i].vc.chan);
 
 		if (!bdev->channels[i].fifo_virt)
 			continue;
diff --git a/drivers/dma/qcom/qcom_adm.c b/drivers/dma/qcom/qcom_adm.c
index 07fbe32d31fa..13f5ca8ff808 100644
--- a/drivers/dma/qcom/qcom_adm.c
+++ b/drivers/dma/qcom/qcom_adm.c
@@ -918,8 +918,8 @@ static void adm_dma_remove(struct platform_device *pdev)
 		/* mask IRQs for this channel/EE pair */
 		writel(0, adev->regs + ADM_CH_RSLT_CONF(achan->id, adev->ee));
 
-		tasklet_kill(&adev->channels[i].vc.task);
-		adm_terminate_all(&adev->channels[i].vc.chan);
+		dmaengine_kill_bh(&achan->vc.chan);
+		adm_terminate_all(&achan->vc.chan);
 	}
 
 	devm_free_irq(adev->dev, adev->irq, adev);
diff --git a/drivers/dma/sa11x0-dma.c b/drivers/dma/sa11x0-dma.c
index a6fa431530e3..e14566fa2d74 100644
--- a/drivers/dma/sa11x0-dma.c
+++ b/drivers/dma/sa11x0-dma.c
@@ -891,7 +891,7 @@ static void sa11x0_dma_free_channels(struct dma_device *dmadev)
 
 	list_for_each_entry_safe(c, cn, &dmadev->channels, vc.chan.device_node) {
 		list_del(&c->vc.chan.device_node);
-		tasklet_kill(&c->vc.task);
+		dmaengine_kill_bh(&c->vc.chan);
 		kfree(c);
 	}
 }
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
index 6f79cc28703e..a08ca355dadb 100644
--- a/drivers/dma/sf-pdma/sf-pdma.c
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -602,7 +602,7 @@ 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);
-		tasklet_kill(&ch->vchan.task);
+		dmaengine_kill_bh(&ch->vchan.chan);
 		tasklet_kill(&ch->done_tasklet);
 		tasklet_kill(&ch->err_tasklet);
 	}
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 087fea3af2e4..f90f5d8d5a1e 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -1253,7 +1253,7 @@ static void sprd_dma_remove(struct platform_device *pdev)
 	list_for_each_entry_safe(c, cn, &sdev->dma_dev.channels,
 				 vc.chan.device_node) {
 		list_del(&c->vc.chan.device_node);
-		tasklet_kill(&c->vc.task);
+		dmaengine_kill_bh(&c->vc.chan);
 	}
 
 	of_dma_controller_free(pdev->dev.of_node);
diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
index d9547017f3bd..cae0a7fe6ceb 100644
--- a/drivers/dma/st_fdma.c
+++ b/drivers/dma/st_fdma.c
@@ -733,7 +733,7 @@ static void st_fdma_free(struct st_fdma_dev *fdev)
 	for (i = 0; i < fdev->nr_channels; i++) {
 		fchan = &fdev->chans[i];
 		list_del(&fchan->vchan.chan.device_node);
-		tasklet_kill(&fchan->vchan.task);
+		dmaengine_kill_bh(&fchan->vchan.chan);
 	}
 }
 
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index f47a326dd7ff..4ddea3cdcd66 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -1094,7 +1094,7 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
 		struct sun6i_vchan *vchan = &sdev->vchans[i];
 
 		list_del(&vchan->vc.chan.device_node);
-		tasklet_kill(&vchan->vc.task);
+		dmaengine_kill_bh(&vchan->vc.chan);
 	}
 }
 
diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
index 64cedef1050a..3a27a83a3288 100644
--- a/drivers/dma/tegra186-gpc-dma.c
+++ b/drivers/dma/tegra186-gpc-dma.c
@@ -1284,7 +1284,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	tegra_dma_terminate_all(dc);
 	synchronize_irq(tdc->irq);
 
-	tasklet_kill(&tdc->vc.task);
+	dmaengine_kill_bh(&tdc->vc.chan);
 	tdc->config_init = false;
 	tdc->slave_id = -1;
 	tdc->sid_dir = DMA_TRANS_NONE;
diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
index ceaee1e33e68..780863199224 100644
--- a/drivers/dma/tegra210-adma.c
+++ b/drivers/dma/tegra210-adma.c
@@ -811,7 +811,7 @@ static void tegra_adma_free_chan_resources(struct dma_chan *dc)
 
 	tegra_adma_terminate_all(dc);
 	vchan_free_chan_resources(&tdc->vc);
-	tasklet_kill(&tdc->vc.task);
+	dmaengine_kill_bh(&tdc->vc.chan);
 	free_irq(tdc->irq, tdc);
 	pm_runtime_put(tdc2dev(tdc));
 
diff --git a/drivers/dma/ti/edma.c b/drivers/dma/ti/edma.c
index d97db5af3555..c0ad0cdabb87 100644
--- a/drivers/dma/ti/edma.c
+++ b/drivers/dma/ti/edma.c
@@ -2560,7 +2560,7 @@ static void edma_cleanupp_vchan(struct dma_device *dmadev)
 	list_for_each_entry_safe(echan, _echan,
 			&dmadev->channels, vchan.chan.device_node) {
 		list_del(&echan->vchan.chan.device_node);
-		tasklet_kill(&echan->vchan.task);
+		dmaengine_kill_bh(&echan->vchan.chan);
 	}
 }
 
diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index 1cf158eb7bdb..fd428c26799a 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -4042,12 +4042,12 @@ static void udma_desc_pre_callback(struct virt_dma_chan *vc,
 }
 
 /*
- * This tasklet handles the completion of a DMA descriptor by
+ * This BH handles the completion of a DMA descriptor by
  * calling its callback and freeing it.
  */
-static void udma_vchan_complete(struct tasklet_struct *t)
+static void udma_vchan_complete(struct dma_chan *chan)
 {
-	struct virt_dma_chan *vc = from_tasklet(vc, t, task);
+	struct virt_dma_chan *vc = to_virt_chan(chan);
 	struct virt_dma_desc *vd, *_vd;
 	struct dmaengine_desc_callback cb;
 	LIST_HEAD(head);
@@ -4112,7 +4112,7 @@ static void udma_free_chan_resources(struct dma_chan *chan)
 	}
 
 	vchan_free_chan_resources(&uc->vc);
-	tasklet_kill(&uc->vc.task);
+	dmaengine_kill_bh(&uc->vc.chan);
 
 	bcdma_free_bchan_resources(uc);
 	udma_free_tx_resources(uc);
@@ -5627,8 +5627,8 @@ static int udma_probe(struct platform_device *pdev)
 		if (!uc->name)
 			return -ENOMEM;
 		vchan_init(&uc->vc, &ud->ddev);
-		/* Use custom vchan completion handling */
-		tasklet_setup(&uc->vc.task, udma_vchan_complete);
+		/* Override the default vchan completion handler */
+		uc->vc.chan.bh_work_fn = udma_vchan_complete;
 		init_completion(&uc->teardown_completed);
 		INIT_DELAYED_WORK(&uc->tx_drain.work, udma_check_tx_completion);
 	}
diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 55ece7fd0d99..899c5eeeac88 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1521,7 +1521,7 @@ static void omap_dma_free(struct omap_dmadev *od)
 			struct omap_chan, vc.chan.device_node);
 
 		list_del(&c->vc.chan.device_node);
-		tasklet_kill(&c->vc.task);
+		dmaengine_kill_bh(&c->vc.chan);
 		kfree(c);
 	}
 }
diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
index 7961172a780d..d407af6cc1da 100644
--- a/drivers/dma/virt-dma.c
+++ b/drivers/dma/virt-dma.c
@@ -77,12 +77,12 @@ struct virt_dma_desc *vchan_find_desc(struct virt_dma_chan *vc,
 EXPORT_SYMBOL_GPL(vchan_find_desc);
 
 /*
- * This tasklet handles the completion of a DMA descriptor by
- * calling its callback and freeing it.
+ * This bottom-half handler completes a DMA descriptor by invoking its
+ * callback and freeing it.
  */
-static void vchan_complete(struct tasklet_struct *t)
+static void vchan_complete(struct dma_chan *chan)
 {
-	struct virt_dma_chan *vc = from_tasklet(vc, t, task);
+	struct virt_dma_chan *vc = to_virt_chan(chan);
 	struct virt_dma_desc *vd, *_vd;
 	struct dmaengine_desc_callback cb;
 	LIST_HEAD(head);
@@ -98,7 +98,7 @@ static void vchan_complete(struct tasklet_struct *t)
 	}
 	spin_unlock_irq(&vc->lock);
 
-	dmaengine_desc_callback_invoke(&cb, &vd->tx_result);
+	dmaengine_desc_callback_invoke(&cb, vd ? &vd->tx_result : NULL);
 
 	list_for_each_entry_safe(vd, _vd, &head, node) {
 		dmaengine_desc_get_callback(&vd->tx, &cb);
@@ -131,7 +131,7 @@ void vchan_init(struct virt_dma_chan *vc, struct dma_device *dmadev)
 	INIT_LIST_HEAD(&vc->desc_completed);
 	INIT_LIST_HEAD(&vc->desc_terminated);
 
-	tasklet_setup(&vc->task, vchan_complete);
+	dmaengine_init_bh(&vc->chan, vchan_complete);
 
 	vc->chan.device = dmadev;
 	list_add_tail(&vc->chan.device_node, &dmadev->channels);
diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index 59d9eabc8b67..0ba42fded2cc 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -21,7 +21,6 @@ struct virt_dma_desc {
 
 struct virt_dma_chan {
 	struct dma_chan	chan;
-	struct tasklet_struct task;
 	void (*desc_free)(struct virt_dma_desc *);
 
 	spinlock_t lock;
@@ -106,7 +105,7 @@ static inline void vchan_cookie_complete(struct virt_dma_desc *vd)
 		 vd, cookie);
 	list_add_tail(&vd->node, &vc->desc_completed);
 
-	tasklet_schedule(&vc->task);
+	dmaengine_schedule_bh(&vc->chan);
 }
 
 /**
@@ -137,7 +136,7 @@ static inline void vchan_cyclic_callback(struct virt_dma_desc *vd)
 	struct virt_dma_chan *vc = to_virt_chan(vd->tx.chan);
 
 	vc->cyclic = vd;
-	tasklet_schedule(&vc->task);
+	dmaengine_schedule_bh(&vc->chan);
 }
 
 /**
@@ -223,7 +222,7 @@ static inline void vchan_synchronize(struct virt_dma_chan *vc)
 	LIST_HEAD(head);
 	unsigned long flags;
 
-	tasklet_kill(&vc->task);
+	dmaengine_kill_bh(&vc->chan);
 
 	spin_lock_irqsave(&vc->lock, flags);
 
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index b3d251c9734e..a1437bdbda9b 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -12,6 +12,7 @@
 #include <linux/scatterlist.h>
 #include <linux/bitmap.h>
 #include <linux/types.h>
+#include <linux/interrupt.h>
 #include <asm/page.h>
 
 /**
@@ -295,6 +296,10 @@ enum dma_desc_metadata_mode {
 	DESC_METADATA_ENGINE = BIT(1),
 };
 
+struct dma_chan;
+
+typedef void (*dmaengine_bh_work_fn)(struct dma_chan *chan);
+
 /**
  * struct dma_chan_percpu - the per-CPU part of struct dma_chan
  * @memcpy_count: transaction counter
@@ -334,6 +339,9 @@ struct dma_router {
  * @router: pointer to the DMA router structure
  * @route_data: channel specific data for the router
  * @private: private data for certain client-channel associations
+ * @bh_tasklet: bottom-half tasklet stored per-channel
+ * @bh_work_fn: callback executed when @bh_tasklet runs
+ * @bh_work_initialized: indicates whether @bh_tasklet has been initialized
  */
 struct dma_chan {
 	struct dma_device *device;
@@ -359,6 +367,9 @@ struct dma_chan {
 	void *route_data;
 
 	void *private;
+	struct tasklet_struct bh_tasklet;
+	dmaengine_bh_work_fn bh_work_fn;
+	bool bh_work_initialized;
 };
 
 /**
@@ -1529,6 +1540,9 @@ struct dma_chan *devm_dma_request_chan(struct device *dev, const char *name);
 
 void dma_release_channel(struct dma_chan *chan);
 int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps);
+void dmaengine_init_bh(struct dma_chan *chan, dmaengine_bh_work_fn fn);
+bool dmaengine_schedule_bh(struct dma_chan *chan);
+void dmaengine_kill_bh(struct dma_chan *chan);
 #else
 static inline struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type)
 {
@@ -1576,6 +1590,20 @@ static inline int dma_get_slave_caps(struct dma_chan *chan,
 {
 	return -ENXIO;
 }
+
+static inline void dmaengine_init_bh(struct dma_chan *chan,
+				     dmaengine_bh_work_fn fn)
+{
+}
+
+static inline bool dmaengine_schedule_bh(struct dma_chan *chan)
+{
+	return false;
+}
+
+static inline void dmaengine_kill_bh(struct dma_chan *chan)
+{
+}
 #endif
 
 static inline int dmaengine_desc_set_reuse(struct dma_async_tx_descriptor *tx)
-- 
2.43.0

Re: [PATCH v3 01/34] dmaengine: add tasklet-backed channel BH helpers

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free due to channel bottom-half (tasklet) being killed before hardware interrupts are disabled and synchronized during device removal.
--

--- Patch [1]: [PATCH v3 01/34] dmaengine: add tasklet-backed channel BH helpers ---
commit 552ba608049b66637e1d6db5b616db6c9c33204d
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: add tasklet-backed channel BH helpers

This commit introduces backend-independent helpers to initialize, schedule,
and kill channel bottom halves. It converts virt-dma and updates various
drivers to use the new API instead of directly overriding or killing
tasklets.

> diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
> --- a/drivers/dma/mediatek/mtk-cqdma.c
> +++ b/drivers/dma/mediatek/mtk-cqdma.c
> @@ -895,7 +895,7 @@ static void mtk_cqdma_remove(struct platform_device *pdev)
>  		vc = &cqdma->vc[i];
>  
>  		list_del(&vc->vc.chan.device_node);
> -		tasklet_kill(&vc->vc.task);
> +		dmaengine_kill_bh(&vc->vc.chan);
>  	}
>  
>  	/* disable interrupt */

[Severity: High]
This is a pre-existing issue, but could this teardown sequence result in a
use-after-free?

In mtk_cqdma_remove(), the channel bottom-halves are killed before the
physical channel interrupts are masked and synchronized. If an interrupt fires
within this window, the interrupt handler processes descriptors and can
reschedule the BH via dmaengine_schedule_bh(). 

Since the device is being removed, devres will subsequently free the channel
memory, potentially causing the recently scheduled BH to execute on freed
memory.

[ ... ]

> diff --git a/drivers/dma/mediatek/mtk-hsdma.c b/drivers/dma/mediatek/mtk-hsdma.c
> --- a/drivers/dma/mediatek/mtk-hsdma.c
> +++ b/drivers/dma/mediatek/mtk-hsdma.c
> @@ -1020,7 +1020,7 @@ static void mtk_hsdma_remove(struct platform_device *pdev)
>  		vc = &hsdma->vc[i];
>  
>  		list_del(&vc->vc.chan.device_node);
> -		tasklet_kill(&vc->vc.task);
> +		dmaengine_kill_bh(&vc->vc.chan);
>  	}
>  
>  	/* Disable DMA interrupt */

[Severity: High]
This is a pre-existing issue, but is it safe to kill the BH here before
disabling and synchronizing the DMA interrupts? 

Similar to mtk_cqdma_remove() above, this opens a window where an incoming
interrupt in mtk_hsdma_remove() can reschedule the BH just before the device
is torn down.

[ ... ]

> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -1215,7 +1215,7 @@ static void pxad_free_channels(struct dma_device *dmadev)
>  	list_for_each_entry_safe(c, cn, &dmadev->channels,
>  				 vc.chan.device_node) {
>  		list_del(&c->vc.chan.device_node);
> -		tasklet_kill(&c->vc.task);
> +		dmaengine_kill_bh(&c->vc.chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does pxad_free_channels() suffer from a
similar race? 

This function is called from pxad_remove(). The driver relies entirely on
devres to free the IRQ after the remove function returns, meaning interrupts
remain fully active during and after the BH is killed. 

A triggered interrupt could reschedule the BH right before devres tears down
the channel structures.

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

[PATCH v3 08/34] dmaengine: imx-dma: flip per-chan tasklet to dmaengine BH

[Allen Pais]

Replace the per-channel tasklet with the shared dmaengine 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..308fe407fe16 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 */
+	dmaengine_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 */
+		dmaengine_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 */
+				dmaengine_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 */
+	dmaengine_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);
+		dmaengine_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);
+		dmaengine_kill_bh(&imxdmac->chan);
 	}
 }
 
-- 
2.43.0

Re: [PATCH v3 08/34] dmaengine: imx-dma: flip per-chan tasklet to dmaengine BH

[sashiko-bot]

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

Pre-existing issues:
- [High] The watchdog timer is not safely canceled during teardown or error paths, leading to Use-After-Free.
- [High] Completed descriptors are moved to the free list before callback invocation, allowing lockless data races.
- [High] Bottom-half work is not flushed before freeing channel descriptors, leading to Use-After-Free.
- [High] IRQ is requested before initializing channel data structures and masking interrupts, leading to a NULL pointer dereference.
--

--- Patch [8]: [PATCH v3 08/34] dmaengine: imx-dma: flip per-chan tasklet to dmaengine BH ---
commit a54288aab8aa66cb0228a7320efe5075981ac5ce
Author: Allen Pais <allen.lkml@gmail.com>

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

Replace the per-channel tasklet with the shared dmaengine 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..308fe407fe161 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 isn't a bug introduced by this patch, but could this lockless data race
cause a loss of completion callbacks?

In imxdma_tasklet(), the completed descriptor is placed on the ld_free list
before the channel lock is dropped and the callback is invoked:

imxdma_tasklet()
    list_move_tail(imxdmac->ld_active.next, &imxdmac->ld_free);
    spin_unlock_irqrestore(&imxdma->lock, flags);
    dmaengine_desc_get_callback_invoke(&desc->desc, NULL);

If another thread calls imxdma_prep_dma_memcpy() concurrently, it locklessly
inspects ld_free. If ld_free was empty, it might immediately pick up this
newly freed descriptor and overwrite its fields (desc->desc.callback = NULL)
before imxdma_tasklet() invokes the callback.

Does this need to hold the descriptor off the free list until after the
callback completes?

[ ... ]

> @@ -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);
> +		dmaengine_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 does the interrupt handler get registered
too early in imxdma_probe()?

For IMX1 DMA, devm_request_irq() is called before the interrupt mask register
(DMA_DIMR) is disabled and before channel list heads like ld_active are
initialized.

If an interrupt is pending or asserts immediately, dma_irq_handler() runs
and calls dma_irq_handle_channel(). This evaluates list_empty() on
uninitialized memory, incorrectly returning false, and then dereferences an
invalid pointer from list_first_entry(), causing a crash.

Should the IRQ registration be moved after the hardware masking and list
initializations?

[ ... ]

> @@ -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);
> +		dmaengine_kill_bh(&imxdmac->chan);
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free with the watchdog timer?

In imxdma_free_irq(), the dmaengine BH is killed, but the imxdmac->watchdog
timer is never safely canceled. When devm frees the imxdma structure, a
still-active timer could fire, executing imxdma_watchdog(). This would
dereference freed memory to call dmaengine_schedule_bh().

Similarly in imxdma_probe(), if devm_request_irq() fails for a later channel,
the error path jumps to disable_dma_ahb_clk and omits timer cleanup for
already initialized channels.

Does this require timer_shutdown_sync() during teardown and error paths?

[Severity: High]
This isn't a bug introduced by this patch, but could descriptors be freed
while the bottom-half work is still running?

When a channel is released, imxdma_free_chan_resources() iterates through
ld_free and frees the descriptors with kfree(). However, the driver does
not implement device_synchronize, and imxdma_free_chan_resources() does
not flush pending bottom-half work.

If imxdma_tasklet() is executing concurrently, imxdma_free_chan_resources()
could acquire the lock, splice the lists, and free the descriptor just as
the tasklet drops the spinlock to invoke the callback. The bottom-half
would then dereference the freed desc->desc to invoke the callback.

Does imxdma_free_chan_resources() need to flush pending BH work before
freeing descriptors?

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

[PATCH v3 13/34] dmaengine: mxs-dma: use dmaengine BH scheduling

[Allen Pais]

Replace the per-channel tasklet with the shared dmaengine 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..149ac402f7b4 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 */
+	dmaengine_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);
+		dmaengine_init_bh(&mxs_chan->chan, mxs_dma_tasklet);
 
 
 		/* Add the channel to mxs_chan list */
-- 
2.43.0

Re: [PATCH v3 13/34] dmaengine: mxs-dma: use dmaengine BH scheduling

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver does not kill the deferred bottom-half work during channel teardown or error paths, nor does it provide a device_synchronize callback, leading to a potential use-after-free.
- [High] Requesting the IRQ before enabling the device clock can result in a synchronous external abort and bus hang.
--

--- Patch [13]: [PATCH v3 13/34] dmaengine: mxs-dma: use dmaengine BH scheduling ---
commit 84e84c7d902dd9157e789fe8d760954306bc43d6
Author: Allen Pais <allen.lkml@gmail.com>
dmaengine: mxs-dma: use dmaengine BH scheduling

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

> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index 7acb3d29dad30..149ac402f7b4b 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
[ ... ]
> @@ -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 */
> +	dmaengine_schedule_bh(&mxs_chan->chan);
>  
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but since the driver is moving to the BH
mechanism, should we also ensure the scheduled work is properly flushed during
teardown and error paths?

Looking at mxs_dma_alloc_chan_resources(), if clk_prepare_enable() fails,
the error path frees the IRQ and the channel ccw structure:

mxs_dma_alloc_chan_resources() {
    ...
err_clk:
    free_irq(mxs_chan->chan_irq, mxs_dma);
err_irq:
    dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
            mxs_chan->ccw, mxs_chan->ccw_phys);
    ...
}

Because free_irq() waits for the hardirq handler but not the scheduled
workqueue, if an early interrupt schedules the BH work, can the workqueue
execute after the channel resources have been freed, leading to a
use-after-free?

Similarly, mxs_dma_free_chan_resources() frees channel resources without
calling dmaengine_kill_bh(). The driver also lacks a device_synchronize
callback, which makes dmaengine_terminate_sync() a no-op.

Should we add dmaengine_kill_bh() and a synchronization callback to prevent
the scheduled work from running after the client frees the channel?

[ ... ]
> @@ -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);
> +		dmaengine_init_bh(&mxs_chan->chan, mxs_dma_tasklet);
>  
>  
>  		/* Add the channel to mxs_chan list */

[Severity: High]
This isn't a bug introduced by this patch, but I wanted to ask about the
initialization ordering which can trigger the interrupt handler early.

When a channel is allocated in mxs_dma_alloc_chan_resources(), the IRQ is
requested before the clock is enabled:

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 an interrupt line is asserted early (for example, from a previously
aborted transaction where the client didn't call terminate_all),
mxs_dma_int_handler() will fire immediately. It then performs an MMIO read
on the memory-mapped controller registers (mxs_dma->base + HW_APBHX_CTRL1).

Since accessing device registers with a disabled clock results in a bus hang
on this platform, would it be safer to enable the clock before requesting
the IRQ to prevent a synchronous external abort?

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