Re: [PATCH v2 23/64] dmaengine: timb_dma: route callbacks via channel BH

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

New issues:
- [High] Data race and out-of-bounds read in __td_ier_mask due to lockless traversal of active_list racing with asynchronous workqueue.

Pre-existing issues:
- [High] Premature completion of active transfers due to a race between td_issue_pending and timb_dma_chan_bh.
--

--- Patch [23]: [PATCH v2 23/64] dmaengine: timb_dma: route callbacks via channel BH ---
commit 2a07cb9b339578f9caa75d42037da515bda38de2
Author: Allen Pais <[email protected]>

dmaengine: timb_dma: route callbacks via channel BH

This patch schedules per-channel BH work for completions so callbacks run
through the dma_chan deferral mechanism while keeping the controller
tasklet for IRQ handling.

> diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
> --- a/drivers/dma/timb_dma.c
> +++ b/drivers/dma/timb_dma.c

[ ... ]

> @@ -580,17 +582,25 @@ static void td_tasklet(struct tasklet_struct *t)
>  	for (i = 0; i < td->dma.chancnt; i++)
>  		if (ipr & (1 << i)) {
>  			struct timb_dma_chan *td_chan = td->channels + i;
> -			spin_lock(&td_chan->lock);
> -			__td_finish(td_chan);
> -			if (!list_empty(&td_chan->queue))
> -				__td_start_next(td_chan);
> -			spin_unlock(&td_chan->lock);
> +			dma_chan_schedule_bh(&td_chan->chan);
>  		}
>  
>  	ier = __td_ier_mask(td);

[Severity: High]
Can this lead to an out-of-bounds memory read?

With completion processing moved to an asynchronous workqueue via
dma_chan_schedule_bh(), td_tasklet() and timb_dma_chan_bh() can now run
concurrently. The tasklet calls __td_ier_mask() right here, which
locklessly evaluates td_chan->ongoing and traverses td_chan->active_list.

If __td_ier_mask() reads ongoing as true right before the workqueue removes
the final descriptor from active_list in __td_finish(), wouldn't list_entry()
calculate a bogus pointer relative to the empty list head? This could cause a
read of unmapped memory or garbage when accessing td_desc->interrupt.

>  	iowrite32(ier, td->membase + TIMBDMA_IER);
>  }
>  
> +static void timb_dma_chan_bh(struct dma_chan *chan)
> +{
> +	struct timb_dma_chan *td_chan = container_of(chan, struct timb_dma_chan,
> +						     chan);
> +
> +	spin_lock(&td_chan->lock);
> +	__td_finish(td_chan);

[Severity: High]
This is a pre-existing issue, but does moving to an asynchronous workqueue
widen a race condition here?

Currently, if td_issue_pending() races with a hardware interrupt, it can
acknowledge the interrupt and complete the transfer before the completion
path runs.

If td_issue_pending() also started the next transfer, this newly scheduled
timb_dma_chan_bh() workqueue will blindly call __td_finish() and complete
whatever is currently in the active_list. By changing the completion from a
tasklet (which had a very short race window) to a scheduled workqueue, the
delay is much longer. Will this result in erroneously aborting and completing
the newly started transfer?

> +	if (!list_empty(&td_chan->queue))
> +		__td_start_next(td_chan);
> +	spin_unlock(&td_chan->lock);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=23