Re: [PATCH v4 31/33] dmaengine: hidma: defer callbacks via channel BH

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] dmaengine: hidma: missing lldev tasklet kill before
  dmaengine_kill_bh()
- [High] dmaengine: hidma: race between hidma_chan_bh() and
  hidma_terminate_channel()
- [Medium] dmaengine: hidma: unconditional BH schedule in
  hidma_callback()
--

commit 2833f6ace610118cc0f61079464819e2cd18fc0f
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: hidma: defer callbacks via channel BH

This commit defers HIDMA descriptor completion processing from
hidma_callback() to per-channel BH work and drains the BH during channel
synchronization, resource freeing, and device removal.

> diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
> index 85844965f0d8c..2cde0cd08b1f5 100644
> --- a/drivers/dma/qcom/hidma.c
> +++ b/drivers/dma/qcom/hidma.c

[ ... ]

> @@ -181,7 +188,7 @@ static void hidma_callback(void *data)
>  	}
>  	spin_unlock_irqrestore(&mchan->lock, irqflags);
>  
> -	hidma_process_completed(mchan);
> +	dmaengine_schedule_bh(&mchan->chan);
>  
>  	if (queued) {
>  		pm_runtime_mark_last_busy(dmadev->ddev.dev);

[Severity: Medium]
Should dmaengine_schedule_bh(&mchan->chan) only be called when queued is
true?

During hidma_alloc_chan_resources(), hidma_ll_request() synchronously
invokes hidma_callback(mdesc) before mdesc->node is added to any list, so
mdesc->node.next is NULL and queued is false.  Scheduling the BH
unconditionally queues mchan->chan.bh_work during descriptor allocation; if
a later iteration in hidma_alloc_chan_resources() fails, the function
returns an error without calling dmaengine_kill_bh(), leaving
mchan->chan.bh_work pending after dma_chan_get() drops its device and
module references.

[ ... ]

> @@ -529,6 +537,11 @@ static int hidma_terminate_all(struct dma_chan *chan)
>  	return rc;
>  }
>  
> +static void hidma_synchronize(struct dma_chan *dmach)
> +{
> +	dmaengine_kill_bh(dmach);
> +}
> +
>  static void hidma_free_chan_resources(struct dma_chan *dmach)
>  {
>  	struct hidma_chan *mchan = to_hidma_chan(dmach);

[Severity: High]
Can dmadev->lldev->task schedule the channel BH after dmaengine_kill_bh()
returns in hidma_synchronize() and hidma_free_chan_resources()?

In hidma_remove(), tasklet_kill(&dmadev->lldev->task) is called before
dmaengine_kill_bh() because the low-level completion tasklet
(hidma_ll_tre_complete()) is what invokes hidma_callback() and schedules
the BH.  However, neither hidma_synchronize() nor
hidma_free_chan_resources() kills lldev->task first.  If an interrupt fires
just before hidma_ll_disable() masks hardware interrupts in
hidma_terminate_channel(), lldev->task can still be scheduled:

CPU0                                  CPU1 (<softirq>)
hidma_free_chan_resources()
  hidma_terminate_channel()
    hidma_ll_disable()
  dmaengine_kill_bh(dmach)
  spin_lock_irqsave(&mchan->lock)
  list_for_each_entry_safe(...) {
    hidma_ll_free(..., mdesc->tre_ch)
    list_del(&mdesc->node)
    kfree(mdesc)
  }
  spin_unlock_irqrestore(&mchan->lock)
                                      hidma_ll_tre_complete()
                                        tre->callback(tre->data)
                                          hidma_callback(mdesc)
                                            if (mdesc->node.next)
                                              list_move_tail(&mdesc->node,
                                                &mchan->completed)
                                                  <-- use-after-free
                                            dmaengine_schedule_bh()

Because list_del() sets mdesc->node.next to LIST_POISON1 (non-NULL),
hidma_callback() dereferences the freed mdesc, corrupts mchan->completed,
and schedules hidma_chan_bh() on the freed descriptor.  In the
dmaengine_terminate_sync() path, hidma_callback() moves mdesc from
mchan->free to mchan->completed and runs completion callbacks after
hidma_synchronize() has returned.

> @@ -540,6 +553,9 @@ static void hidma_free_chan_resources(struct dma_chan *dmach)
>  	/* terminate running transactions and free descriptors */
>  	hidma_terminate_channel(dmach);
>  
> +	/* drain completion work before the descriptors go away */
> +	dmaengine_kill_bh(dmach);
> +
>  	spin_lock_irqsave(&mchan->lock, irqflags);
>  
>  	/* Move data */

[Severity: High]
Can hidma_chan_bh() race with hidma_terminate_channel() and corrupt
mchan->free when dmaengine_kill_bh() is only called after
hidma_terminate_channel()?

Both dmaengine_terminate_sync() (which calls hidma_terminate_all() before
hidma_synchronize()) and hidma_free_chan_resources() enter
hidma_terminate_channel() while hidma_chan_bh() can still be running
hidma_process_completed():

CPU0 (kworker)                        CPU1
hidma_chan_bh()
  hidma_process_completed()
    spin_lock_irqsave(&mchan->lock)
    list_splice_tail_init(
      &mchan->completed, &list)
    spin_unlock_irqrestore(&mchan->lock)
                                      hidma_terminate_channel()
                                        ...
                                        list_for_each_entry_safe(...) {
                                          ...
                                          list_move(&mdesc->node,
                                                    &mchan->free)
    spin_lock_irqsave(&mchan->lock)
    list_move(&mdesc->node,
              &mchan->free)               <-- corrupts mchan->free
    spin_unlock_irqrestore(&mchan->lock)

hidma_terminate_channel() moves descriptors onto mchan->free without
holding mchan->lock:

drivers/dma/qcom/hidma.c:hidma_terminate_channel() {
    ...
	/* return all user requests */
	list_for_each_entry_safe(mdesc, tmp, &list, node) {
		struct dma_async_tx_descriptor *txd = &mdesc->desc;

		dma_descriptor_unmap(txd);
		dmaengine_desc_get_callback_invoke(txd, NULL);
		dma_run_dependencies(txd);

		/* move myself to free_list */
		list_move(&mdesc->node, &mchan->free);
	}
    ...
}

Previously hidma_process_completed() ran synchronously inside the tasklet,
Whereas now hidma_chan_bh() runs concurrently on the BH workqueue and can
also call hidma_process_completed() in parallel with the direct
hidma_process_completed() call at the top of hidma_terminate_channel() and
overwrite mchan->last_success after hidma_terminate_channel() resets it to
0.  Killing lldev->task and draining the channel BH inside
hidma_terminate_channel() after hidma_ll_disable() masks interrupts (and
before draining lists and moving descriptors to mchan->free) would avoid
these races.

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