Re: [PATCH v3 15/34] dmaengine: pch_dma: convert tasklet to dmaengine BH

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

Pre-existing issues:
- [High] Missing cancellation of scheduled BH work in the probe error path leads to a Use-After-Free.
- [High] The driver invokes the DMA client's callback with the channel spinlock held, causing a deadlock if the client queues a new transfer.
- [High] Missing softirq protection on process-context locks causes deadlocks when preempted by the softirq-context BH.
- [High] Using `spin_lock_irq` unconditionally re-enables interrupts, which causes IRQ state corruption if called from atomic context.
- [High] The driver requests a shared IRQ before the channel count is initialized and without masking hardware interrupts, causing a potential IRQ storm.
--

--- Patch [15]: [PATCH v3 15/34] dmaengine: pch_dma: convert tasklet to dmaengine BH ---
commit 7e489ac0179032bce9c722d41def47b8d30c50c9
Author: Allen Pais <[email protected]>

dmaengine: pch_dma: convert tasklet to dmaengine BH
    
Replace the per-channel tasklet with the shared dmaengine 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..eb372aa328a62 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;
>  }

[Severity: High]
This is a pre-existing issue, but does pd_device_terminate_all() corrupt the
caller's interrupt state?

If a DMA client calls dmaengine_terminate_all() from an atomic or
interrupt context, pd_device_terminate_all() executes:

pd_device_terminate_all() {
        spin_lock_irq(&pd_chan->lock);
        ...
        spin_unlock_irq(&pd_chan->lock);
}

Since spin_unlock_irq() unconditionally re-enables interrupts, could this
cause unexpected nested interrupts if the caller had already disabled them?

> -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;

[Severity: High]
This isn't a bug introduced by this patch, but can the BH handler deadlock
against new transfer submissions?

When the softirq executes pdc_tasklet(), it acquires pd_chan->lock using
spin_lock_irqsave(). It can then call pdc_chain_complete(), which invokes
the client's completion callback synchronously while still holding the lock:

pdc_chain_complete() {
        ...
        dmaengine_desc_callback_invoke(&cb, NULL);
}

If the client attempts to schedule a new DMA transaction during this callback,
it calls pd_tx_submit(), which tries to acquire the exact same lock:

pd_tx_submit() {
        spin_lock(&pd_chan->lock);
}

Could this sequence result in an immediate self-deadlock?

[Severity: High]
This is also a pre-existing issue, but is there a missing softirq protection
on process-context locks?

If a DMA client calls pd_tx_submit() or pd_issue_pending() from process
context, they acquire the lock without disabling softirqs:

pd_tx_submit() {
        spin_lock(&pd_chan->lock);
}

If a softirq interrupts the CPU before the lock is released, and pdc_tasklet()
executes, it will try to acquire the same lock using spin_lock_irqsave().

Will this cause the softirq to spin forever waiting for the preempted process
to release the 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);
> +		dmaengine_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 does the probe sequence
risk an unhandled IRQ storm?

In pch_dma_probe(), the shared IRQ is requested before the channel count is
initialized by dma_async_device_register():

pch_dma_probe() {
        ...
        err = request_irq(pdev->irq, pd_irq, IRQF_SHARED, DRV_NAME, pd);
        ...
        err = dma_async_device_register(&pd->dma);
}

If the hardware has a pending interrupt and fires immediately, pd_irq()
will execute while pd->dma.chancnt is still 0.

pd_irq() {
        for (i = 0; i < pd->dma.chancnt; i++) { ... }
        ...
        if (ret0)
                dma_writel(pd, STS0, sts0);
}

Because chancnt is 0, the loop is skipped, hardware status is not cleared,
and IRQ_NONE is returned. Could this cause the kernel's spurious interrupt
detector to permanently disable the shared IRQ line?

[Severity: High]
This is a pre-existing issue, but does the probe error path leave un-cancelled
BH work?

If dma_async_device_register() fails after the IRQ has been requested and
the BH has been initialized, the probe error path executes:

pch_dma_probe() {
...
err_free_pool:
        dma_pool_destroy(pd->pool);
err_free_irq:
        free_irq(pdev->irq, pd);
err_iounmap:
        pci_iounmap(pdev, pd->membase);
err_free_res:
        pci_release_regions(pdev);
err_disable_pdev:
        pci_disable_device(pdev);
err_free_mem:
        kfree(pd);
}

If a spurious interrupt fired and called dmaengine_schedule_bh() before
free_irq() is called, this error path frees the pd memory without calling
dmaengine_kill_bh(). Could the scheduled BH execute later and access the
freed pd_chan structure, resulting in a use-after-free?

[ ... ]

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