Re: [PATCH v3 23/34] dmaengine: timb_dma: route callbacks via channel BH

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

New issues:
- [High] Data race and out-of-bounds read in `__td_ier_mask` introduced by asynchronous BH
- [High] Logic Error leading to permanent device hang for non-interrupt descriptors
- [High] Use-After-Free of `td` structure during driver teardown

Pre-existing issues:
- [High] Unsynchronized read-modify-write on global `TIMBDMA_IER` register
- [High] TOCTOU race causing premature descriptor completion
--

--- Patch [23]: [PATCH v3 23/34] dmaengine: timb_dma: route callbacks via channel BH ---
commit a7e38226e52fe6d87f6d1e2cd0a0ee22fbd778a9
Author: Allen Pais <[email protected]>

dmaengine: timb_dma: route callbacks via channel BH

This commit introduces a per-channel BH workqueue 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
> index 4fc5119854ec1..5a2374aab02a6 100644
> --- 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);
> +			dmaengine_schedule_bh(&td_chan->chan);
>  		}

[Severity: High]
This is a pre-existing issue, but does the lockless caching of the
completion event lead to a time-of-check to time-of-use race?

The td_tasklet() caches the completion status locklessly into the ipr
variable before this loop. If td_issue_pending() executes concurrently,
processes the real completion, and starts a new transfer, would the
subsequent lock acquisition by the BH unconditionally pop the active
descriptor via __td_finish() while the hardware is still actively
processing the new transfer?

>  
>  	ier = __td_ier_mask(td);
>  	iowrite32(ier, td->membase + TIMBDMA_IER);
>  }

[Severity: High]
Does this sequence result in a permanent hardware hang regression for
non-interrupt descriptors?

The tasklet schedules the BH but then immediately recalculates the hardware
IER register using the stale ongoing = true state, which leaks the interrupt
enable mask.

When the BH later completes the descriptor in __td_finish() by setting
ongoing = false, it never updates IER. If a subsequent non-interrupt
descriptor is submitted, might it trigger a spurious hardware interrupt
that the tasklet drops, preventing the BH from ever being scheduled and
hanging the channel?

[Severity: High]
Could the new asynchronous BH introduce a data race regression with
__td_ier_mask()?

By deferring __td_finish() to a workqueue, the driver can now mutate
active_list concurrently with the tasklet's execution of __td_ier_mask()
above. If a concurrent list_move() in the BH sets the next pointer to the
list head itself:

drivers/dma/timb_dma.c:__td_ier_mask() {
    ...
    if (td_chan->ongoing) {
        struct timb_dma_desc *td_desc =
            list_entry(td_chan->active_list.next,
            struct timb_dma_desc, desc_node);
        if (td_desc->interrupt)
    ...
}

Would the lockless __td_ier_mask() cast the list head to a descriptor and
read an out-of-bounds interrupt field?

[ ... ]

> @@ -697,6 +707,7 @@ static int td_probe(struct platform_device *pdev)
>  		INIT_LIST_HEAD(&td_chan->active_list);
>  		INIT_LIST_HEAD(&td_chan->queue);
>  		INIT_LIST_HEAD(&td_chan->free_list);
> +		dmaengine_init_bh(&td_chan->chan, timb_dma_chan_bh);

[Severity: High]
Does the driver properly terminate these newly introduced channel BH
workqueues during driver teardown to prevent a use-after-free regression?

While td_free_chan_resources() kills the BH, if a driver unbind occurs
immediately after a transfer finishes via td_remove(), the td object is
freed without calling dmaengine_kill_bh():

drivers/dma/timb_dma.c:td_remove() {
    ...
    tasklet_kill(&td->tasklet);
    iounmap(td->membase);
    kfree(td);
    ...
}

Could the pending system workqueue then execute timb_dma_chan_bh() on
the freed memory to acquire the spinlock?

[Severity: High]
This isn't a bug introduced by this patch, but is there an unsynchronized
read-modify-write on the global TIMBDMA_IER register?

Concurrent execution of td_tx_submit() on multiple channels appears to
modify a shared hardware register protected only by a local per-channel
lock:

drivers/dma/timb_dma.c:__td_enable_chan_irq() {
    ...
    ier = ioread32(td->membase + TIMBDMA_IER);
    ier |= 1 << id;
    iowrite32(ier, td->membase + TIMBDMA_IER);
    ...
}

Could this permanently drop interrupt masks for racing channels and stall
their operation?

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