Re: [PATCH 3/3] dma: mv_xor: use devm for dma pool and irq

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling `irq_dispose_mapping()` before `devm_request_irq()` teardown leaks the `irqaction` and causes a potential use-after-free.

Pre-existing issues:
- [High] Missing `tasklet_kill()` during channel teardown leads to a potential Use-After-Free.
- [High] The self-test functions fail to stop the DMA engine on timeout, leading to memory corruption.
- [High] The interrupt handler is registered and unmasked before the channel's spinlock and lists are initialized.
--

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

dma: mv_xor: use devm for dma pool and irq
    
Replace dma_alloc_wc with dmam_alloc_attrs and request_irq
with devm_request_irq. This eliminates the need for
manual cleanup of the dma pool and irq in both the channel
remove function and the channel add error labels, removing
the err_free_irq and err_free_dma labels entirely.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 3fc39cca7cbd3..2ac7f01155fe8 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1013,8 +1013,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
>  
>  	dma_async_device_unregister(&mv_chan->dmadev);
>  
> -	dma_free_wc(dev, MV_XOR_POOL_SIZE,
> -			  mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
>  	dma_unmap_single(dev, mv_chan->dummy_src_addr,
>  			 MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
>  	dma_unmap_single(dev, mv_chan->dummy_dst_addr,
> @@ -1025,8 +1023,6 @@ static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)
>  		list_del(&chan->device_node);
>  	}
>  
> -	free_irq(mv_chan->irq, mv_chan);
> -

[Severity: High]
This is a pre-existing issue, but does this function also need a call to
tasklet_kill(&mv_chan->irq_tasklet) before returning?

The mv_chan structure is allocated via devm_kzalloc() and will be freed by
devres upon device removal or probe failure. If the tasklet is scheduled by an
interrupt just before removal, it might execute asynchronously after the
mv_chan memory has been freed, causing a use-after-free panic.

>  	return 0;
>  }
>  

[ ... ]

> @@ -1112,10 +1108,10 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  	/* clear errors before enabling interrupts */
>  	mv_chan_clear_err_status(mv_chan);
>  
> -	ret = request_irq(mv_chan->irq, mv_xor_interrupt_handler,
> +	ret = devm_request_irq(&pdev->dev, mv_chan->irq, mv_xor_interrupt_handler,
>  			  0, dev_name(&pdev->dev), mv_chan);

[Severity: High]
By switching to devm_request_irq() here, could we cause an irqaction leak and
potential use-after-free during device unbind or probe error?

There are manual irq_dispose_mapping() calls in the error paths of
mv_xor_probe() and inside mv_xor_remove() which were not converted:

drivers/dma/mv_xor.c:mv_xor_remove() {
    ...
    for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
        if (xordev->channels[i]) {
            mv_xor_channel_remove(xordev->channels[i]);
            if (pdev->dev.of_node)
                irq_dispose_mapping(xordev->channels[i]->irq);
        }
}

Calling irq_dispose_mapping() will unmap and delete the IRQ descriptor from
the sparse IRQ tree. When the devres cleanup later attempts to free the
devm-managed IRQ, irq_to_desc() will return NULL, and free_irq() will
return without actually freeing the struct irqaction memory.

[Severity: High]
This is a pre-existing issue, but is it safe to request and unmask
the interrupt before the channel's lock and lists are initialized?

The interrupt is requested here, and unmasked shortly after:

	mv_chan_unmask_interrupts(mv_chan);

However, the spinlock and lists are initialized further down:

	spin_lock_init(&mv_chan->lock);
	INIT_LIST_HEAD(&mv_chan->chain);

If an interrupt fires immediately (e.g. from a previous bind/unbind or a shared
IRQ), the handler will schedule the tasklet, which then attempts to acquire the
uninitialized spinlock and access the uninitialized lists in
mv_chan_slot_cleanup(), potentially causing a crash.

>  	if (ret)
> -		goto err_free_dma;
> +		goto err_unmap_dst;
>  
>  	mv_chan_unmask_interrupts(mv_chan);
>  
> @@ -1138,14 +1134,14 @@ mv_xor_channel_add(struct mv_xor_device *xordev,
>  		ret = mv_chan_memcpy_self_test(mv_chan);
>  		dev_dbg(&pdev->dev, "memcpy self test returned %d\n", ret);
>  		if (ret)
> -			goto err_free_irq;
> +			goto err_unmap_dst;

[Severity: High]
This is a pre-existing issue, but if the DMA self-test times out, are we missing
a hardware abort before freeing the destination buffer?

In mv_chan_memcpy_self_test() and mv_chan_xor_self_test(), if the DMA hardware
times out, the function jumps to the free_resources label:

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);
	return err;
}

The code unmaps the DMA buffers and frees the destination memory via kfree(),
but never attempts to halt or reset the underlying hardware channel. If the
delayed hardware completes the transfer later, will it write the DMA results
directly into the freed kernel memory pages?

>  	}
>  
>  	if (dma_has_cap(DMA_XOR, dma_dev->cap_mask)) {
>  		ret = mv_chan_xor_self_test(mv_chan);
>  		dev_dbg(&pdev->dev, "xor self test returned %d\n", ret);
>  		if (ret)
> -			goto err_free_irq;
> +			goto err_unmap_dst;
>  	}

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