Re: [PATCH] dmaengine: mv_xor: kill tasklet on channel add error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but self-test timeout error paths unmap and free DMA memory without terminating the active hardware transfer, risking memory corruption.
- [High] This problem wasn't introduced by this patch, but in `mv_xor_channel_remove()`, the teardown order is incorrect (`dma_free_wc` is called before `free_irq`) and `tasklet_kill()` is entirely missing, leading to identical tasklet leaks and use-after-free bugs as the one this patch fixed.
- [High] This problem wasn't introduced by this patch, but in `mv_xor_channel_add()`, `request_irq()` is called and unmasked before spinlocks and lists are initialized, creating a severe race window that can crash the kernel.
- [High] This problem wasn't introduced by this patch, but the missing `.remove` callback in the platform driver leaks non-devres resources and causes a use-after-free upon sysfs unbind.
--

commit dd96e71824475c1db94aac75b83a4f34179c56e7
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: mv_xor: kill tasklet on channel add error path

If request_irq() succeeds but a later step (self-test or device
registration) fails, the error path at err_free_irq frees the IRQ
but does not kill the tasklet.  A previously scheduled tasklet can
still run after the channel is freed, accessing freed memory.

Add tasklet_kill() after free_irq() on the error path.

Fixes: ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..24f22db5dfa11 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1162,6 +1162,7 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  
>  err_free_irq:
>  	free_irq(mv_chan->irq, mv_chan);
> +	tasklet_kill(&mv_chan->irq_tasklet);
>  err_free_dma:
>  	dma_free_wc(&pdev->dev, MV_XOR_POOL_SIZE,
>  			  mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
...
	dma_free_wc(dev, MV_XOR_POOL_SIZE,
			  mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
...
	free_irq(mv_chan->irq, mv_chan);

	return 0;
}

[Severity: High]
This problem wasn't introduced by this patch, but in mv_xor_channel_remove(),
the teardown order is incorrect (dma_free_wc is called before free_irq) and
tasklet_kill() is entirely missing.

Since this patch fixes a similar issue in mv_xor_channel_add(), does
mv_xor_channel_remove() also need tasklet_kill() and reordered teardown to
avoid tasklet leaks and use-after-free bugs? If a tasklet is scheduled before
or during free_irq(), could it remain alive after mv_chan is freed by devres,
leading to a use-after-free? Alternatively, if an interrupt fires after
dma_free_wc() but before free_irq(), will it access freed DMA descriptor
memory?

drivers/dma/mv_xor.c:mv_xor_channel_add() {
...
	ret = request_irq(mv_chan->irq, mv_xor_interrupt_handler,
			  0, dev_name(&pdev->dev), mv_chan);
	if (ret)
		goto err_free_dma;

	mv_chan_unmask_interrupts(mv_chan);
...
	spin_lock_init(&mv_chan->lock);
	INIT_LIST_HEAD(&mv_chan->chain);
	INIT_LIST_HEAD(&mv_chan->completed_slots);
...
}

[Severity: High]
This problem wasn't introduced by this patch, but in mv_xor_channel_add(),
request_irq() is called and unmasked before spinlocks and lists are
initialized.

Does this create a race window that can crash the kernel? If an interrupt
fires immediately upon being unmasked, could the interrupt handler schedule
the IRQ tasklet, which then attempts to acquire an uninitialized lock and
traverse uninitialized lists, resulting in a null pointer dereference or
kernel panic?

drivers/dma/mv_xor.c:
...
static struct platform_driver mv_xor_driver = {
	.probe		= mv_xor_probe,
	.suspend        = mv_xor_suspend,
	.resume         = mv_xor_resume,
	.driver		= {
...
}

[Severity: High]
This problem wasn't introduced by this patch, but does the missing .remove
callback in the platform driver leak non-devres resources and cause a
use-after-free upon sysfs unbind?

Unbinding the driver via sysfs causes devres_release_all() to free
devm-managed resources like mv_chan. However, explicitly allocated non-devres
resources like request_irq(), dma_alloc_wc(), and dma_async_device_register()
are never released. If an interrupt fires later, will it dereference the
devres-freed mv_chan?

drivers/dma/mv_xor.c:mv_chan_memcpy_self_test() {
...
	if (mv_xor_status(dma_chan, cookie, NULL) !=
	    DMA_COMPLETE) {
		dev_err(dma_chan->device->dev,
			"Self-test copy timed out, disabling\n");
		err = -ENODEV;
		goto free_resources;
	}
...
free_resources:
	dmaengine_unmap_put(unmap);
	mv_xor_free_chan_resources(dma_chan);
out:
	kfree(src);
	kfree(dest);
...
}

[Severity: Critical]
This problem wasn't introduced by this patch, but do the self-test timeout
error paths unmap and free DMA memory without terminating the active hardware
transfer?

If the hardware was merely delayed, could it eventually complete the transfer
and perform DMA writes into the kfree'd memory? If this memory has been
reallocated to another subsystem, would this cause silent memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909234601.101883-1-rosenp@gmail.com?part=1