Re: [PATCH v6 06/18] dmaengine: sh: rz-dmac: Save the start LM descriptor

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly introduced `start_lmdesc` field can be dereferenced while `NULL` in `rz_dmac_calculate_residue_bytes_in_vd()`.

Pre-existing issues:
- [High] Multiple MEMCPY transfers overwrite each other's hardware descriptors causing data corruption.
- [High] Missing `dma_wmb()` memory barrier before setting the Link Valid (`HEADER_LV`) bit in DMA descriptors.
- [High] Silent ring buffer overflow when preparing slave SG descriptors if the scatterlist exceeds 64 entries.
--

commit 1282aed7089327e34eb2eeee4b69973599e70406
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

dmaengine: sh: rz-dmac: Save the start LM descriptor
    
This commit saves the start LM descriptor in the channel's descriptor 
structure to avoid unnecessary iterations from the beginning of the list 
when calculating residue bytes.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index c48858b68dee6..d3926ecd63ac1 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c

[ ... ]

> @@ -343,6 +344,8 @@ static void rz_dmac_prepare_desc_for_memcpy(struct rz_dmac_chan *channel)
>  	struct rz_dmac_desc *d = channel->desc;
>  	u32 chcfg = CHCFG_MEM_COPY;
>  
> +	d->start_lmdesc = lmdesc;
> +
>  	/* prepare descriptor */
>  	lmdesc->sa = d->src;
>  	lmdesc->da = d->dest;

[Severity: High]
This isn't a bug introduced by this patch, but does 
rz_dmac_prepare_desc_for_memcpy() overwrite hardware descriptors of an 
already active memcpy transfer?

The transfer is configured at channel->lmdesc.tail, but the tail pointer is 
never advanced (unlike in the slave_sg path). If rz_dmac_issue_pending() 
is called while a memcpy transfer is already active, the new transfer is 
prepared into the exact same channel->lmdesc.tail slot currently being read 
by the active DMA hardware.

Can this cause memory corruption and unpredictable hardware behavior?

I note this is fixed later in the series in commit d09437d0a9b5a 
("dmaengine: sh: rz-dmac: Use virt-dma APIs for channel descriptor 
processing"), which adds a !channel->desc check.

[ ... ]

> @@ -377,6 +380,7 @@ static void rz_dmac_prepare_descs_for_slave_sg(struct rz_dmac_chan *channel)
>  	}
>  
>  	lmdesc = channel->lmdesc.tail;
> +	d->start_lmdesc = lmdesc;
>  
>  	for (i = 0, sg = sgl; i < sg_len; i++, sg = sg_next(sg)) {
>  		if (d->direction == DMA_DEV_TO_MEM) {

[Severity: High]
This isn't a bug introduced by this patch, but does this loop lack a 
bounds check for sg_len?

The driver iterates over sg_len without checking if it exceeds the hardware 
ring buffer size (DMAC_NR_LMDESC, which is 64).

If a client submits a scatterlist with more than 64 entries, the loop will 
wrap around and overwrite the beginning of the very same transfer it is 
currently preparing.

Could this result in a corrupted DMA ring buffer and an infinite hardware 
loop?

[Severity: High]
This is a pre-existing issue, but does setting the Link Valid (HEADER_LV) 
bit require a dma_wmb() barrier?

The DMA descriptors (lmdesc) are allocated in coherent memory. The driver 
writes the payload (sa, da, tb, chcfg) and then sets the HEADER_LV bit to 
indicate to the hardware that the descriptor is ready:

rz_dmac_prepare_descs_for_slave_sg() {
    ...
    lmdesc->tb = sg_dma_len(sg);
    lmdesc->chitvl = 0;
    lmdesc->chext = 0;
    if (i == (sg_len - 1)) {
        lmdesc->chcfg = (channel->chcfg & ~CHCFG_DEM);
        lmdesc->header = HEADER_LV;
    ...
}

Because writes to Normal Non-Cacheable memory can be reordered by the CPU 
write buffer, is a dma_wmb() barrier strictly required before setting 
lmdesc->header = HEADER_LV?

Without it, if the hardware is executing the chain and polling for the 
next descriptor, it might see the HEADER_LV bit set before the payload 
writes have completed.

[ ... ]

> @@ -693,9 +697,10 @@ rz_dmac_get_next_lmdesc(struct rz_lmdesc *base, struct rz_lmdesc *lmdesc)
>  	return next;
>  }
>  
> -static u32 rz_dmac_calculate_residue_bytes_in_vd(struct rz_dmac_chan *channel, u32 crla)
> +static u32 rz_dmac_calculate_residue_bytes_in_vd(struct rz_dmac_chan *channel,
> +						 struct rz_dmac_desc *desc, u32 crla)
>  {
> -	struct rz_lmdesc *lmdesc = channel->lmdesc.head;
> +	struct rz_lmdesc *lmdesc = desc->start_lmdesc;

[Severity: High]
Can desc->start_lmdesc be NULL here and cause a kernel panic when 
accessing lmdesc->nxla?

Because struct rz_dmac_desc is allocated via kzalloc_obj(), start_lmdesc 
is initially NULL and only assigned when the descriptor is prepared by the 
hardware.

Due to the driver's internal queue management, if a descriptor is prepared 
but not yet submitted, an unrelated IRQ or issue_pending call can move it 
from ld_queue to ld_active without processing it.

If rz_dmac_chan_get_residue() is then called and matches this descriptor's 
cookie, it will unconditionally dereference the NULL pointer.

I note this is addressed later in the series by commit d09437d0a9b5a 
("dmaengine: sh: rz-dmac: Use virt-dma APIs for channel descriptor 
processing"), which completely removes ld_queue and ld_active.

>  	struct dma_chan *chan = &channel->vc.chan;
>  	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	u32 residue = 0, i = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526084710.3491480-1-claudiu.beznea@kernel.org?part=6