Re: [PATCH v2 15/64] dmaengine: pch_dma: convert tasklet to dma_chan BH

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

Pre-existing issues:
- [High] Missing cleanup of scheduled BH work on the probe error path can lead to a use-after-free.
- [High] Early registration of a shared IRQ before channel count setup leads to an unhandled IRQ storm.
- [High] Client completion callbacks are invoked with the channel spinlock held, leading to a potential self-deadlock.
- [High] Process-context functions acquire the channel spinlock without disabling softirqs, risking a deadlock if preempted by the BH work.
- [High] Checking hardware idle state outside the lock causes a TOCTOU race leading to premature completion and use-after-free.
--

--- Patch [15]: [PATCH v2 15/64] dmaengine: pch_dma: convert tasklet to dma_chan BH ---
commit a2ed59e859fb44b7074fd4693b53c278ebc94f3d
Author: Allen Pais <[email protected]>

dmaengine: pch_dma: convert 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/pch_dma.c b/drivers/dma/pch_dma.c
> index bf805f1024f64..b0044bea8fe4c 100644
> --- a/drivers/dma/pch_dma.c
> +++ b/drivers/dma/pch_dma.c

[ ... ]

> @@ -665,9 +664,10 @@ static int pd_device_terminate_all(struct dma_chan *chan)
>  	return 0;
>  }
>  
> -static void pdc_tasklet(struct tasklet_struct *t)
> +static void pdc_tasklet(struct dma_chan *c)
>  {
> -	struct pch_dma_chan *pd_chan = from_tasklet(pd_chan, t, tasklet);
> +	struct pch_dma_chan *pd_chan = container_of(c, struct pch_dma_chan,
> +						    chan);
>  	unsigned long flags;
>  
>  	if (!pdc_is_idle(pd_chan)) {

[Severity: High]
This isn't a bug introduced by this patch, but might checking the hardware
idle state outside the lock cause a race condition?

If a concurrent pd_issue_pending() acquires the lock first and starts the
next request, could the preempted thread later acquire the lock, see a stale
idle state, and improperly complete the newly started descriptor?

This same check appears unprotected in pd_issue_pending():

drivers/dma/pch_dma.c:pd_issue_pending() {
    ...
    if (pdc_is_idle(pd_chan)) {
    ...
}

[Severity: High]
This is a pre-existing issue, but would acquiring the channel lock with a
plain spin_lock() in process-context functions risk a deadlock?

If a hardware interrupt preempts process context here in pd_tx_submit() or
pd_issue_pending():

drivers/dma/pch_dma.c:pd_tx_submit() {
    ...
    spin_lock(&pd_chan->lock);
    ...
}

and schedules this pdc_tasklet() work which attempts to acquire the exact
same lock using spin_lock_irqsave(), might this deadlock the CPU?

[Severity: High]
This is a pre-existing issue, but is there a risk of a self-deadlock if the
client callback submits a new DMA transaction?

If pdc_tasklet() acquires the lock, and then pdc_chain_complete() invokes
the callback:

drivers/dma/pch_dma.c:pdc_chain_complete() {
    ...
    if (callback)
        callback(param);
    ...
}

and that callback attempts to submit a new transaction via pd_tx_submit(),
it will attempt to acquire the same pd_chan->lock.

[ ... ]

> @@ -877,7 +877,7 @@ static int pch_dma_probe(struct pci_dev *pdev,
>  		INIT_LIST_HEAD(&pd_chan->queue);
>  		INIT_LIST_HEAD(&pd_chan->free_list);
>  
> -		tasklet_setup(&pd_chan->tasklet, pdc_tasklet);
> +		dma_chan_init_bh(&pd_chan->chan, pdc_tasklet);
>  		list_add_tail(&pd_chan->chan.device_node, &pd->dma.channels);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but can requesting the shared
IRQ before dma_async_device_register() initializes the channel count lead
to an unhandled IRQ storm?

In pch_dma_probe(), request_irq() is called with IRQF_SHARED before the
channel count is initialized.

The interrupt handler relies entirely on pd->dma.chancnt:

drivers/dma/pch_dma.c:pd_irq() {
    ...
    for (i = 0; i < pd->dma.chancnt; i++) {
    ...
}

If pd->dma.chancnt is initially 0, could it loop 0 times and return
IRQ_NONE without clearing the hardware interrupt status?

[Severity: High]
This is a pre-existing issue, but does the probe error path fail to kill
the scheduled bottom-half work before freeing the device structure?

If dma_async_device_register() fails, the error path frees the memory but
appears to skip cancelling pending BH works:

drivers/dma/pch_dma.c:pch_dma_probe() {
    ...
err_free_pool:
    dma_pool_destroy(pd->pool);
err_free_irq:
    free_irq(pdev->irq, pd);
    ...
}

If an interrupt fires and schedules the BH work during this window, could
it execute on freed memory?

[ ... ]

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