Re: [PATCHv2 1/9] dmaengine: mv_xor: initialize chan state before requesting IRQ

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `tasklet_kill()` on error paths and during channel removal can lead to a use-after-free of the channel structure.
- [High] Unprotected concurrent access to shared MBUS window hardware registers and `xordev` arrays across multiple DMA channels.
- [High] DMA descriptor slots are permanently leaked if `mv_xor_add_io_win()` fails during DMA prep.
- [High] Missing `dma_wmb()` before updating the hardware chain descriptor.
- [High] Cache-line tearing risk due to unaligned embedded DMA buffers.
--

commit ea8e635be2ff110a83818007cc43ae217683e3c9
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: mv_xor: initialize chan state before requesting IRQ

This commit fixes a potential race condition in the mv_xor driver by
initializing the channel's spinlock, lists, and tasklet before registering
the IRQ. This prevents a scenario where an immediately firing interrupt
could access uninitialized structures and cause undefined behavior.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..93a8e9f7c529a 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1054,6 +1054,18 @@ mv_xor_channel_add(struct mv_xor_device *xordev,

[ ... ]

> +	spin_lock_init(&mv_chan->lock);
> +	INIT_LIST_HEAD(&mv_chan->chain);
> +	INIT_LIST_HEAD(&mv_chan->completed_slots);
> +	INIT_LIST_HEAD(&mv_chan->free_slots);
> +	INIT_LIST_HEAD(&mv_chan->allocated_slots);
> +	mv_chan->dmachan.device = dma_dev;
> +	dma_cookie_init(&mv_chan->dmachan);
> +
> +	mv_chan->mmr_base = xordev->xor_base;
> +	mv_chan->mmr_high_base = xordev->xor_high_base;
> +	tasklet_setup(&mv_chan->irq_tasklet, mv_xor_tasklet);

[Severity: High]
This isn't a bug introduced by this patch, but since you are moving the
tasklet setup here, could this trigger a cache-line tearing issue?

Looking at struct mv_xor_chan in drivers/dma/mv_xor.h, dummy_src and
dummy_dst are embedded as 128-byte character arrays without cacheline
alignment annotations.

Shortly after this tasklet_setup(), dummy_src is mapped with
DMA_FROM_DEVICE:

mv_chan->dummy_src_addr = dma_map_single(dma_dev->dev,
        mv_chan->dummy_src, MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);

Since the array shares a cache line with adjacent struct fields like
irq_tasklet and op_in_desc which are initialized before the mapping, won't
the DMA cache invalidation destroy any recent CPU writes to these fields?

[ ... ]

>  	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
>  		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;
>  	}

[ ... ]

> err_free_irq:
> 	free_irq(mv_chan->irq, mv_chan);
> err_free_dma:

[Severity: High]
This is a pre-existing issue, but does the error path here miss a
tasklet_kill() call?

If the probe fails after tasklet_setup() and request_irq(), the error
path frees the IRQ but leaves a pending tasklet that might execute and use
the soon-to-be freed channel structure. Should tasklet_kill() be called
during channel removal and in this error path?


While reviewing this driver, I also noticed a few other pre-existing issues
in drivers/dma/mv_xor.c that might need attention:

[Severity: High]
This is a pre-existing issue, but is there a missing lock when accessing
the shared MBUS window registers?

In mv_xor_add_io_win(), the function reads WINDOW_BAR_ENABLE(0), searches
for a free window index, and updates both xordev->win_start[i] and the
hardware registers:

drivers/dma/mv_xor.c:mv_xor_add_io_win() {
        ...
        win_enable = readl(base + WINDOW_BAR_ENABLE(0));
        i = ffs(~win_enable) - 1;
        ...
        xordev->win_start[i] = addr;
        xordev->win_end[i] = addr + size;
        ...
}

Since xordev is shared across multiple XOR channels and the DMA engine
framework allows concurrent device_prep_dma_* calls, can concurrent
operations pick the same window index and overwrite each other's target
mappings?

[Severity: High]
This is a pre-existing issue, but does mv_xor_prep_dma_xor() leak DMA
descriptor slots if mv_xor_add_io_win() fails?

If mv_xor_add_io_win() fails, the function returns NULL directly:

drivers/dma/mv_xor.c:mv_xor_prep_dma_xor() {
        ...
        ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);
        if (ret)
                return NULL;
        ...
}

Since the descriptor slot was previously allocated and moved to
mv_chan->allocated_slots, returning NULL here without returning the
descriptor to mv_chan->free_slots appears to permanently leak the slot.
Could this eventually exhaust the channel's descriptor pool?

[Severity: High]
This isn't a bug introduced by this patch, but is a memory barrier required
before updating the hardware chain descriptor?

In mv_xor_tx_submit(), when appending a new descriptor to an active chain,
the code modifies the previous descriptor in write-combined memory:

drivers/dma/mv_xor.c:mv_xor_tx_submit() {
        ...
        /* fix up the hardware chain */
        mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
        ...
}

Without a dma_wmb() before this assignment, could the hardware read the
updated phy_next_desc before the new descriptor's contents are flushed
from the CPU write-combine buffer?

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