Re: [PATCH 07/26] dmaengine: moxart-dma: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a missing NULL pointer check for `txstate` in `moxart_tx_status()`, which will cause a kernel panic if a caller passes a NULL state.
- [High] This problem wasn't introduced by this patch, but there is a Time-Of-Check to Time-Of-Use (TOCTOU) race condition in the interrupt handler `moxart_dma_interrupt()` accessing `ch->desc` outside of its spinlock, leading to a NULL pointer dereference.
- [High] This problem wasn't introduced by this patch, but there is a lifecycle mismatch where `dma_device` is allocated with `devm_kzalloc`, leading to a Use-After-Free if the driver is unbound while clients hold channel references.
--

--- Patch [7]: [PATCH 07/26] dmaengine: moxart-dma: Remove redundant dev_err()/dev_err_probe() ---
commit 9d354e585688446b04a5e03b938d46845d466dab
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: moxart-dma: Remove redundant dev_err()/dev_err_probe()

The devm_request_irq() now automatically logs detailed error messages on
failure. This eliminates the need for driver-specific dev_err() and
dev_err_probe() calls that previously printed generic messages.

> diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
> index 442f5aa16031f..16a14a5179c17 100644
> --- a/drivers/dma/moxart-dma.c
> +++ b/drivers/dma/moxart-dma.c
> @@ -600,10 +600,8 @@ static int moxart_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_irq(dev, irq, moxart_dma_interrupt, 0,
                                         ^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but there is a Time-Of-Check to
Time-Of-Use (TOCTOU) race condition in moxart_dma_interrupt() accessing
ch->desc outside of its spinlock:

drivers/dma/moxart-dma.c:moxart_dma_interrupt() {
...
		if (ctrl & APB_DMA_FIN_INT_STS) {
			ctrl &= ~APB_DMA_FIN_INT_STS;
			if (ch->desc) {
				spin_lock(&ch->vc.lock);
				if (++ch->sgidx < ch->desc->sglen) {
...
}

If a concurrent call to moxart_terminate_all() on another CPU acquires the
lock, frees ch->desc, sets it to NULL, and releases the lock between the if
check and the spin_lock, the interrupt handler will unconditionally dereference
ch->desc via ch->desc->sglen.

Does this race condition lead to a NULL pointer dereference?

>  			       "moxart-dma-engine", mdc);
> -	if (ret) {
> -		dev_err(dev, "devm_request_irq failed\n");
> +	if (ret)
>  		return ret;
> -	}
>  	mdc->irq = irq;
>  
>  	ret = dma_async_device_register(&mdc->dma_slave);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but there is a lifecycle mismatch
where dma_device is allocated with devm_kzalloc.

Earlier in moxart_probe():
	mdc = devm_kzalloc(dev, sizeof(*mdc), GFP_KERNEL);

And during unbind:
drivers/dma/moxart-dma.c:moxart_remove() {
...
	dma_async_device_unregister(&m->dma_slave);
...
}

The DMA engine subsystem independently refcounts the dma_device via kref_init().
If the driver is unbound via sysfs while DMA clients hold references to the
channels, moxart_remove() returns and devres frees the mdc structure memory
immediately.

When the clients later drop their references, does this trigger a
Use-After-Free on the freed memory?

While reviewing the driver, I also noticed an issue in moxart_tx_status():

drivers/dma/moxart-dma.c:moxart_tx_status() {
...
	if (vd) {
		d = to_moxart_dma_desc(&vd->tx);
		txstate->residue = moxart_dma_desc_size(d, 0);
...
	} else if (ch->desc && ch->desc->vd.tx.cookie == cookie) {
		txstate->residue = moxart_dma_desc_size_in_flight(ch);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but there is a missing NULL pointer
check for txstate in moxart_tx_status().

The dmaengine API allows callers to pass a NULL state argument to
dmaengine_tx_status() if they only want to query the completion status and
do not need the residue. However, this function unconditionally dereferences
txstate.

Will this cause a kernel panic if a caller passes a NULL state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709135846.97972-1-panchuang@vivo.com?part=7