sashiko.dev review (Re: [PATCH v4 07/17] dmaengine: sh: rz-dmac: Save the start LM descriptor)

[Claudiu Beznea]

> commit 3d17698f275248947e3d2df42293a89e89cafaa6
> Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> dmaengine: sh: rz-dmac: Save the start LM descriptor
> Save the start LM descriptor to avoid looping through the entire
> channel's LM descriptor list when computing the residue. This avoids
> unnecessary iterations.
> > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> > index 6bea7c8c70537..0f871c0a28bd9 100644
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> [ ... ]
> > @@ -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) {
> This isn't a bug introduced by this patch, but is there a potential buffer
> overflow in the link descriptor ring due to a lack of bounds checking?

That can happen but this is not the scope of this series. Series is already long 
enough. I will address this in a separate series after the current one will be 
merged.

> The driver allocates a fixed-size ring buffer of 64 link descriptors
> (DMAC_NR_LMDESC). However, rz_dmac_prepare_descs_for_slave_sg() iterates over
> the provided scatterlist and increments the lmdesc pointer without ever
> verifying if the number of scatterlist elements exceeds 64.
> If a client submits a sufficiently large scatterlist, will it silently
> overflow the ring buffer, overwriting the descriptors it just wrote, and
> cause the DMA controller to read a corrupted list of operations?
> [ ... ]
> > @@ -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;
> >     struct dma_chan *chan = &channel->vc.chan;
> >     struct rz_dmac *dmac = to_rz_dmac(chan->device);
> >     u32 residue = 0, i = 0;
> Can this lead to a NULL pointer dereference?
> In rz_dmac_prep_slave_sg(), a new descriptor is immediately added to
> channel->ld_queue. If a previous transfer completes before the client calls
> dma_async_issue_pending(), rz_dmac_irq_handler_thread() attempts to start the
> next transfer from ld_queue.
> However, vchan_next_desc() returns NULL since the transfer hasn't been
> issued, causing rz_dmac_xfer_desc() to return 0 without executing the hardware
> preparation function. The IRQ handler sees the 0 return value and erroneously
> moves the unprepared descriptor to channel->ld_active.
> If dmaengine_tx_status() is subsequently called, rz_dmac_chan_get_residue()
> fetches this active descriptor. Because it was never prepared, the newly added
> desc->start_lmdesc pointer remains NULL.
> The loop while (lmdesc->nxla != crla) then immediately dereferences NULL.
> Note that this issue was fixed later in the series by commit 'dmaengine: sh:
> rz-dmac: Use virt-dma APIs for channel descriptor processing'.

All the above could happen and the scope of patch "dmaengine: sh:
rz-dmac: Use virt-dma APIs for channel descriptor processing" was to fix all 
these subtle issues. The patch here is only a preparation patch to keep things 
simple in "dmaengine: sh: rz-dmac: Use virt-dma APIs for channel descriptor 
processing"

> Additionally, this isn't a bug introduced by this patch, but does the
> mathematical logic in the residue calculation produce incorrect results?
> The loop while (lmdesc->nxla != crla) in
> rz_dmac_calculate_residue_bytes_in_vd() locates the link descriptor preceding
> the currently executing one. The subsequent loop while (lmdesc->chcfg &
> CHCFG_DEM) then iterates from this preceding descriptor, adding lmdesc->tb to
> the residue.
> If the hardware is executing the second descriptor of a transfer, it adds the
> total size of the first (already completed) descriptor, the total size of the
> second (currently executing) descriptor, and crtb (the remaining bytes of the
> second descriptor). Does this mathematically double-count bytes and include
> already transferred data?
> Conversely, if executing the first descriptor, the first loop wraps around the
> entire ring buffer, encounters the final descriptor of the previous transfer,
> and since its CHCFG_DEM bit is cleared, the second loop immediately
> terminates. Does this completely omit all subsequent descriptors from the
> residue calculation?

I think this is a valid point that I also noticed and thought about it. However, 
I would like to keep it aside from this series, as it is already too big.

> [ ... ]
> This isn't a bug introduced by this patch, but is there a pre-existing race
> condition with dma_async_issue_pending()?
> rz_dmac_issue_pending() unconditionally calls rz_dmac_xfer_desc() if
> vchan_issue_pending() returns true, without checking if a transfer is already
> actively executing in hardware (e.g., if channel->desc is already set).
> For RZ_DMAC_DESC_MEMCPY transfers, the driver does not advance
> channel->lmdesc.tail, meaning the newly issued transfer will silently
> overwrite the exact link descriptor currently being executed by the active
> hardware. Does this lead to memory corruption and undefined hardware behavior?
> Note that this is fixed later in the series by commit 'dmaengine: sh: rz-dmac:
> Use virt-dma APIs for channel descriptor processing' which properly checks
> !channel->desc.

The role of the pointed descriptors was to fix all these subtle issues.