Re: [PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing `chan->lock` protection when freeing descriptors in error paths leads to data corruption in `chan->free_seg_list`.
- [Critical] Race condition between dmaengine_terminate_all() and dmaengine_submit() overwrites the chan->terminating flag, leading to a Use-After-Free.
- [High] Race condition between xilinx_dma_chan_desc_cleanup and xilinx_dma_terminate_all corrupts the static descriptor ring order, defeating the purpose of this patch.
- [High] Rejecting a transaction in xilinx_dma_tx_submit() due to cyclic mode frees descriptors out of order, permanently breaking the hardware static ring.
- [High] Calling dma_run_dependencies() while holding chan->lock causes a deadlock if the dependent transaction is on the same channel.
--

--- Patch [1]: [PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order ---
commit 55ac771a816780299793bc5e59ad89b6ef3446b0
Author: Alex Bereza <alex@bereza.email>

dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order
    
This patch corrects the order in which hardware buffer descriptors are returned
to the free segment list during channel cleanup. By freeing the pending list
last, it ensures descriptors are returned in chronological order to maintain
the hardware static ring structure.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index bef2b031dba19..0817b74f74505 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -920,9 +920,9 @@ static void xilinx_dma_free_descriptors(struct xilinx_dma_chan *chan)
>  
>  	spin_lock_irqsave(&chan->lock, flags);
>  
> -	xilinx_dma_free_desc_list(chan, &chan->pending_list);
>  	xilinx_dma_free_desc_list(chan, &chan->done_list);
>  	xilinx_dma_free_desc_list(chan, &chan->active_list);
> +	xilinx_dma_free_desc_list(chan, &chan->pending_list);
>  
>  	spin_unlock_irqrestore(&chan->lock, flags);
>  }

[Severity: High]
This is a pre-existing issue, but does a race condition between
xilinx_dma_chan_desc_cleanup() and xilinx_dma_terminate_all() corrupt the
static descriptor ring order, defeating the purpose of this patch?

The tasklet xilinx_dma_chan_desc_cleanup() drops chan->lock to execute client
callbacks:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
        ...
        spin_unlock_irqrestore(&chan->lock, flags);
        dmaengine_desc_get_callback_invoke(&desc->async_tx, &result);
        spin_lock_irqsave(&chan->lock, flags);
        ...
        xilinx_dma_free_tx_descriptor(chan, desc);
}

During this window, xilinx_dma_terminate_all() can execute concurrently,
acquire the lock, and free newer descriptors into chan->free_seg_list
by calling xilinx_dma_free_descriptors(chan).

When the tasklet resumes and frees its older descriptor, it appends it after
the newer ones, which scrambles the static ring order the patch intended to
preserve. Can this lead to a fatal dma internal error when the channel
is reused?

[Severity: High]
This is a pre-existing issue, but does rejecting a transaction in
xilinx_dma_tx_submit() due to cyclic mode also break the hardware static ring?

If a normal transfer is submitted while chan->cyclic is true, the descriptors
are immediately freed:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
        ...
        if (chan->cyclic) {
                xilinx_dma_free_tx_descriptor(chan, desc);
                return -EBUSY;
        }
        ...
}

Because older cyclic segments are still active and will be freed later, this
appends newer segments before older ones into chan->free_seg_list. Could this
permanently break the chronological order required by the hardware?

[Severity: Critical]
This is a pre-existing issue, but does missing chan->lock protection when
freeing descriptors in error paths lead to linked list corruption?

In the cyclic check above and in the error path of xilinx_dma_prep_slave_sg():

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_prep_slave_sg() {
        ...
error:
        xilinx_dma_free_tx_descriptor(chan, desc);
        return NULL;
}

The function xilinx_dma_free_tx_descriptor() is called without acquiring
chan->lock. This leads to an unlocked list_add_tail():

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_free_tx_segment() {
        ...
        xilinx_dma_clean_hw_desc(&segment->hw);
        list_add_tail(&segment->node, &chan->free_seg_list);
}

Could this race against other contexts like the tasklet or normal allocations
that modify the list under the lock, causing list corruption in
chan->free_seg_list?

[Severity: High]
This is a pre-existing issue, but does calling dma_run_dependencies() while
holding chan->lock cause a deadlock if the dependent transaction is on the
same channel?

The tasklet re-acquires the lock before running dependencies:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
        ...
        spin_lock_irqsave(&chan->lock, flags);

        /* Run any dependencies, then free the descriptor */
        dma_run_dependencies(&desc->async_tx);
        ...
}

If CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH is enabled and there is a dependent
descriptor on the same channel, dma_run_dependencies() invokes the submit
callback, routing back to xilinx_dma_tx_submit():

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
        ...
        spin_lock_irqsave(&chan->lock, flags);
        ...
}

Since spinlocks are not recursive, won't this attempt to acquire chan->lock
again result in an immediate deadlock?

[Severity: Critical]
This is a pre-existing issue, but does a race condition between
dmaengine_terminate_all() and dmaengine_submit() overwrite the
chan->terminating flag, causing a use-after-free?

A concurrent xilinx_dma_terminate_all() sets the flag without the lock:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_terminate_all() {
        ...
        if (!chan->cyclic)
                chan->terminating = true;
        ...
}

Then xilinx_dma_tx_submit() unconditionally clears it under the lock:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
        ...
        chan->terminating = false;

        spin_unlock_irqrestore(&chan->lock, flags);
        ...
}

If the tasklet xilinx_dma_chan_desc_cleanup() dropped the lock to run
callbacks, and a client terminates and then submits a new transfer,
chan->terminating becomes false.

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
        ...
        /*
         * While we ran a callback the user called a terminate function,
         * which takes care of cleaning up any remaining descriptors
         */
        if (chan->terminating)
                break;
        ...
}

When the tasklet resumes, it fails to break out of the loop. Does this result
in dereferencing a freed and recycled next pointer from the safe list
iterator?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-fix-hw-buf-desc-reuse-v1-1-d79827a844c7@bereza.email?part=1