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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

 > commit 3d17698f275248947e3d2df42293a89e89cafaa6
 > Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
 > 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.