Re: [PATCH v4 26/33] dmaengine: mpc512x: route callbacks via channel BH

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] dmaengine: mpc512x: race with mdma->tasklet in
  mpc_dma_free_chan_resources()
- [High] dmaengine: mpc512x: unsynchronized completion processing in
  prep paths
- [Medium] dmaengine: mpc512x: missing tasklet and BH cleanup in
  mpc_dma_probe()
--

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

dmaengine: mpc512x: route callbacks via channel BH

This commit moves completed descriptor processing and callback execution
from the controller tasklet into per-channel BH work items, and drains
those BH work items during channel resource teardown and driver removal.

> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index f67421f2d6d3e..b6a1b3c94da41 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> @@ -372,45 +372,47 @@ static irqreturn_t mpc_dma_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -/* process completed descriptors */
> -static void mpc_dma_process_completed(struct mpc_dma *mdma)
> +static void mpc_dma_process_chan_completed(struct mpc_dma_chan *mchan)
>  {
>  	dma_cookie_t last_cookie = 0;
> -	struct mpc_dma_chan *mchan;
>  	struct mpc_dma_desc *mdesc;
>  	struct dma_async_tx_descriptor *desc;
>  	unsigned long flags;
>  	LIST_HEAD(list);
> -	int i;
>  
> -	for (i = 0; i < mdma->dma.chancnt; i++) {
> -		mchan = &mdma->channels[i];
> +	/* Get all completed descriptors */
> +	spin_lock_irqsave(&mchan->lock, flags);
> +	if (!list_empty(&mchan->completed))
> +		list_splice_tail_init(&mchan->completed, &list);
> +	spin_unlock_irqrestore(&mchan->lock, flags);
>  
> -		/* Get all completed descriptors */
> -		spin_lock_irqsave(&mchan->lock, flags);
> -		if (!list_empty(&mchan->completed))
> -			list_splice_tail_init(&mchan->completed, &list);
> -		spin_unlock_irqrestore(&mchan->lock, flags);
> +	if (list_empty(&list))
> +		return;
>  
> -		if (list_empty(&list))
> -			continue;
> +	/* Execute callbacks and run dependencies */
> +	list_for_each_entry(mdesc, &list, node) {
> +		desc = &mdesc->desc;
>  
> -		/* Execute callbacks and run dependencies */
> -		list_for_each_entry(mdesc, &list, node) {
> -			desc = &mdesc->desc;
> +		dmaengine_desc_get_callback_invoke(desc, NULL);
>  
> -			dmaengine_desc_get_callback_invoke(desc, NULL);
> +		last_cookie = desc->cookie;
> +		dma_run_dependencies(desc);
> +	}
>  
> -			last_cookie = desc->cookie;
> -			dma_run_dependencies(desc);
> -		}
> +	/* Free descriptors */
> +	spin_lock_irqsave(&mchan->lock, flags);
> +	list_splice_tail_init(&list, &mchan->free);
> +	mchan->chan.completed_cookie = last_cookie;
> +	spin_unlock_irqrestore(&mchan->lock, flags);
> +}
>  
> -		/* Free descriptors */
> -		spin_lock_irqsave(&mchan->lock, flags);
> -		list_splice_tail_init(&list, &mchan->free);
> -		mchan->chan.completed_cookie = last_cookie;
> -		spin_unlock_irqrestore(&mchan->lock, flags);
> -	}
> +/* process completed descriptors */
> +static void mpc_dma_process_completed(struct mpc_dma *mdma)
> +{
> +	int i;
> +
> +	for (i = 0; i < mdma->dma.chancnt; i++)
> +		mpc_dma_process_chan_completed(&mdma->channels[i]);
>  }

[Severity: High]
Can calling mpc_dma_process_completed() from mpc_dma_prep_memcpy() and
mpc_dma_prep_slave_sg() race with mpc_dma_chan_bh()?

When mchan->free is empty, both prep functions call
mpc_dma_process_completed(mdma) synchronously across all channels:

drivers/dma/mpc512x_dma.c:mpc_dma_prep_memcpy() {
    ...
	if (!mdesc) {
		/* try to free completed descriptors */
		mpc_dma_process_completed(mdma);
		return NULL;
	}
    ...
}

Because mpc_dma_process_chan_completed() drops mchan->lock while running
callbacks and only returns descriptors to mchan->free afterwards, a
concurrent prep call (or a callback itself) can enter
mpc_dma_process_chan_completed() for a second batch of completed
descriptors:

CPU0 (kworker)                        CPU1
mpc_dma_chan_bh()
  mpc_dma_process_chan_completed()
    spin_lock_irqsave(&mchan->lock)
    list_splice_tail_init(...)
    /* batch 1, cookie N */
    spin_unlock_irqrestore(...)
    dmaengine_desc_get_callback_invoke()
                                      mpc_dma_prep_memcpy()
                                        /* mchan->free is empty */
                                        mpc_dma_process_completed()
                                          mpc_dma_process_chan_completed()
                                            /* splices batch 2, cookie N+1 */
                                            dmaengine_desc_get_callback_invoke()
                                            /* callbacks run out of order */
                                            mchan->chan.completed_cookie = N+1
    spin_lock_irqsave(&mchan->lock)
    mchan->chan.completed_cookie = N  <-- completed_cookie moves backward
    spin_unlock_irqrestore(...)

When completed_cookie regresses to N, dma_cookie_status() reports cookie
N+1 as DMA_IN_PROGRESS indefinitely.  In addition, because
mpc_dma_process_completed() iterates over all channels outside the BH
workqueue, a prep call on one channel can run
mpc_dma_process_chan_completed() on another channel concurrently with
mpc_dma_free_chan_resources() after dmaengine_kill_bh() has returned.

[ ... ]

> @@ -551,6 +568,8 @@ static void mpc_dma_free_chan_resources(struct dma_chan *chan)
>  	unsigned long flags;
>  	LIST_HEAD(descs);
>  
> +	dmaengine_kill_bh(&mchan->chan);
> +
>  	spin_lock_irqsave(&mchan->lock, flags);
>  
>  	/* Channel must be idle */

[Severity: High]
Can mpc_dma_free_chan_resources() race with mdma->tasklet because
mpc_dma_irq() schedules mdma->tasklet first rather than scheduling
mchan->chan.bh_work directly?

When a transfer completes, mpc_dma_irq_process() moves descriptors onto
mchan->completed and mpc_dma_irq() calls tasklet_schedule(&mdma->tasklet);
mchan->chan.bh_work is only queued later when mpc_dma_tasklet() runs.  Note
that mpc_dma_device_terminate_all() only splices prepared, queued, and
active into mchan->free, leaving mchan->completed untouched.

CPU0                                  CPU1
<hardirq>
  mpc_dma_irq()
    mpc_dma_irq_process()
      /* moves desc to mchan->completed */
    tasklet_schedule(&mdma->tasklet)
                                      mpc_dma_free_chan_resources()
                                        dmaengine_kill_bh(&mchan->chan)
                                        /* bh_work not scheduled yet */
                                        spin_lock_irqsave(&mchan->lock)
                                        BUG_ON(!list_empty(&mchan->completed))
                                          <-- kernel BUG

Alternatively, if mpc_dma_tasklet() schedules mchan->chan.bh_work right
after dmaengine_kill_bh() returns and mpc_dma_process_chan_completed()
splices mchan->completed into its local list before
mpc_dma_free_chan_resources() takes mchan->lock,
mpc_dma_free_chan_resources() frees mchan->tcd and the descriptors while
mpc_dma_process_chan_completed() runs callbacks and splices the freed
descriptors back onto mchan->free.

> @@ -1007,6 +1026,7 @@ static int mpc_dma_probe(struct platform_device *op)
>  		INIT_LIST_HEAD(&mchan->completed);
>  
>  		spin_lock_init(&mchan->lock);
> +		dmaengine_init_bh(&mchan->chan, mpc_dma_chan_bh);
>  		list_add_tail(&mchan->chan.device_node, &dma->channels);
>  	}
>  

[Severity: Medium]
If dma_async_device_register() fails in mpc_dma_probe(), can a pending
tasklet or channel BH work item run after mdma is freed?

mpc_dma_probe() requests mdma->irq and mdma->irq2 before initializing the
channels and mdma->tasklet, and dma_async_device_register() can invoke
mpc_dma_alloc_chan_resources() (enabling channel error interrupts) before
failing on a later step:

drivers/dma/mpc512x_dma.c:mpc_dma_probe() {
    ...
	retval = dma_async_device_register(dma);
	if (retval)
		goto err_free2;
    ...
err_free2:
	if (mdma->is_mpc8308)
		free_irq(mdma->irq2, mdma);
err_free1:
	free_irq(mdma->irq, mdma);
err_dispose2:
	if (mdma->is_mpc8308)
		irq_dispose_mapping(mdma->irq2);
err_dispose1:
	irq_dispose_mapping(mdma->irq);
err:
	return retval;
}

Unlike mpc_dma_remove(), the err_free2 error path does not call
tasklet_kill() or dmaengine_kill_bh() before devres unmaps mdma->regs and
frees mdma, which can cause a use-after-free if an interrupt scheduled the
tasklet or BH work before free_irq() completed.

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