Re: [PATCH v4 4/4] dmaengine: xilinx_dma: Extend metadata handling for AXI DMA and MCDMA

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] xilinx_dma_get_metadata_ptr() incorrectly returns the End-Of-Frame (EOF) descriptor's metadata for TX (MM2S) operations, breaking TX metadata offload and corrupting completion status.

Pre-existing issues:
- [High] Providing a scatterlist with zero `sg_len` or zero `sg_dma_len` entries results in an empty `desc->segments` list, causing out-of-bounds memory corruption in prep functions.
- [High] Missing channel lock when freeing DMA segments on descriptor preparation error paths leads to `free_seg_list` corruption.
--

--- Patch [4]: [PATCH v4 4/4] dmaengine: xilinx_dma: Extend metadata handling for AXI DMA and MCDMA ---
commit 416ce331f02cd7e51a8987299d3d59436877b288
Author: Suraj Gupta <suraj.gupta2@amd.com>

dmaengine: xilinx_dma: Extend metadata handling for AXI DMA and MCDMA

This patch extends metadata handling to AXI MCDMA and exposes descriptor 
status words in addition to the APP fields. It modifies the metadata API 
to fetch fields from the End-Of-Frame descriptor, and attaches metadata 
operations for MCDMA when an AXI Stream interface is present.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 9b8b6f1acd4ea..0b7bc6ab27cbe 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -649,18 +649,49 @@ static inline void xilinx_aximcdma_buf(struct xilinx_dma_chan *chan,

[ ... ]

>  static void *xilinx_dma_get_metadata_ptr(struct dma_async_tx_descriptor *tx,
>  					 size_t *payload_len, size_t *max_len)
>  {
>  	struct xilinx_dma_tx_descriptor *desc = to_dma_tx_descriptor(tx);
> -	struct xilinx_axidma_tx_segment *seg;
> +	struct xilinx_dma_chan *chan = to_xilinx_chan(tx->chan);
> +
> +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIMCDMA) {
> +		struct xilinx_aximcdma_tx_segment *seg =
> +			list_last_entry(&desc->segments,
> +					struct xilinx_aximcdma_tx_segment, node);

[Severity: Medium]
Does using list_last_entry() unconditionally for both directions break TX 
(MM2S) metadata offload?

Hardware strictly reads transmit control, sideband, and APP data from the 
Start-Of-Frame (SOP) descriptor, not the EOF descriptor.

Additionally, for MCDMA, since the returned pointer overlays the mm2s_status 
field, could a client writing to this array before submission accidentally 
set the completion bit and cause premature descriptor completion?

> -
> -	*max_len = *payload_len = sizeof(u32) * XILINX_DMA_NUM_APP_WORDS;
> -	seg = list_first_entry(&desc->segments,
> -			       struct xilinx_axidma_tx_segment, node);
> -	return seg->hw.app;
> +
> +		/*
> +		 * The union members overlay the same words, so one pointer and
> +		 * length cover both directions (see the layout above).
> +		 */
> +		*max_len = *payload_len = sizeof(seg->hw.s2mm_status) +
> +					  sizeof(seg->hw.s2mm_sideband_status) +
> +					  sizeof(seg->hw.app);
> +		return &seg->hw.s2mm_status;
> +	}

[ ... ]

> @@ -2623,6 +2654,9 @@ xilinx_mcdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>  		segment->hw.control |= XILINX_MCDMA_BD_EOP;
>  	}

[Severity: High]
This is a pre-existing issue, but if a client provides a scatterlist with a
zero sg_len or where all entries have zero length, desc->segments will be 
empty after the loop.

Looking slightly above this context in xilinx_mcdma_prep_slave_sg(), the code 
calls list_first_entry() and list_last_entry() on desc->segments without 
checking if it is empty:

xilinx_mcdma_prep_slave_sg() {
    ...
    segment = list_first_entry(&desc->segments, ...);
    desc->async_tx.phys = segment->phys;
    ...
    if (chan->direction == DMA_MEM_TO_DEV) {
        segment->hw.control |= XILINX_MCDMA_BD_SOP;
        ...
}

Doesn't this evaluate to an out-of-bounds pointer within desc->async_tx, 
potentially corrupting desc->async_tx.callback when segment->hw.control is 
overwritten? The same pattern appears to exist in xilinx_dma_prep_slave_sg().

>  
> +	if (chan->xdev->has_axistream_connected)
> +		desc->async_tx.metadata_ops = &xilinx_dma_metadata_ops;
> +
>  	return &desc->async_tx;
>  
>  error:

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in 
xilinx_mcdma_prep_slave_sg() corrupt chan->free_seg_list? 

When jumping to the error label, xilinx_dma_free_tx_descriptor() is called, 
which subsequently calls xilinx_mcdma_free_tx_segment(). This adds the 
segment to chan->free_seg_list via list_add_tail() without acquiring 
chan->lock.

Can this race with concurrent successful allocations (which acquire the lock) 
and tasklet cleanups?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713072146.45269-1-srinivas.neeli@amd.com?part=4