Re: [PATCH 1/4] dmaengine: loongson: loongson2-apb-cmc-dma: Fix cyclic residue wrap NULL dereference

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dmaengine: loongson: wrong next_sg check in loongson2_cmc_dma_desc_residue()
--

commit d1cdc3886ec73f6a09aef30478b0b2741d57690c
Author: Binbin Zhou <zhoubinbin@loongson.cn>

dmaengine: loongson: loongson2-apb-cmc-dma: Fix cyclic residue wrap NULL dereference

This commit updates loongson2_cmc_dma_desc_residue() to check desc->cyclic
instead of lchan->desc->cyclic when next_sg == 0, and adds a comment
explaining the cyclic wrap behavior.

> diff --git a/drivers/dma/loongson/loongson2-apb-cmc-dma.c b/drivers/dma/loongson/loongson2-apb-cmc-dma.c
> index 2037120b23caa..f090fbe0b8a1e 100644
> --- a/drivers/dma/loongson/loongson2-apb-cmc-dma.c
> +++ b/drivers/dma/loongson/loongson2-apb-cmc-dma.c
> @@ -484,7 +484,19 @@ static size_t loongson2_cmc_dma_desc_residue(struct loongson2_cmc_dma_chan *lcha
>  	ndtr = loongson2_cmc_dma_read(lddev, LOONGSON2_CMCDMA_CNDTR, lchan->id);
>  	residue = ndtr << width;
>  
> -	if (lchan->desc->cyclic && next_sg == 0)
> +	/*
> +	 * For a cyclic descriptor that has wrapped (next_sg == 0), the
> +	 * current period only has the in-flight entry left; the entries
> +	 * from sg[0] onwards belong to the next period. Adding them would
> +	 * make residue exceed one period.
> +	 *
> +	 * Use desc->cyclic, not lchan->desc->cyclic: this helper is also
> +	 * reachable with a descriptor that is not lchan->desc, and only
> +	 * the active-descriptor branch passes lchan->desc today. Keying
> +	 * the wrap decision on the descriptor being queried keeps the
> +	 * result correct and avoids depending on lchan->desc being set.
> +	 */
> +	if (desc->cyclic && next_sg == 0)
>  		return residue;

[Severity: Medium]
Can an active cyclic descriptor ever reach loongson2_cmc_dma_desc_residue()
in drivers/dma/loongson/loongson2-apb-cmc-dma.c with next_sg == 0?

When starting a transfer in loongson2_cmc_dma_start_transfer(), lchan->next_sg
is set to 0 before configuring sg_req[0] and immediately incremented to 1
under lchan->vchan.lock:

    if (lchan->next_sg == lchan->desc->num_sgs)
        lchan->next_sg = 0;

    sg_req = &lchan->desc->sg_req[lchan->next_sg];
    ...
    lchan->next_sg++;

Similarly, on cyclic period completion in loongson2_cmc_dma_handle_chan_done(),
loongson2_cmc_dma_configure_next_sg() wraps lchan->next_sg from
lchan->desc->num_sgs to 0 to program sg_req[0], and
loongson2_cmc_dma_handle_chan_done() immediately increments lchan->next_sg to
1 under lchan->vchan.lock:

    if (lchan->desc->cyclic) {
        vchan_cyclic_callback(&lchan->desc->vdesc);
        /* LOONGSON2_CMCDMA_CCR_CIRC mode don't need update register */
        if (lchan->desc->num_sgs == 1)
            return;
        loongson2_cmc_dma_configure_next_sg(lchan);
        lchan->next_sg++;
    }

While the final period sg_req[num_sgs - 1] is in flight, lchan->next_sg equals
desc->num_sgs, so the loop

    for (i = next_sg; i < desc->num_sgs; i++)
        residue += desc->sg_req[i].len;

naturally executes zero iterations. Thus for an active descriptor,
lchan->next_sg is always in [1, desc->num_sgs] when
loongson2_cmc_dma_tx_status() queries it under lchan->vchan.lock.

In this patch, the only caller that passes next_sg == 0 to
loongson2_cmc_dma_desc_residue() is the else if (vdesc) branch in
loongson2_cmc_dma_tx_status() for an unstarted queued descriptor on
vchan.desc_issued:

    if (lchan->desc && cookie == lchan->desc->vdesc.tx.cookie)
        state->residue = loongson2_cmc_dma_desc_residue(lchan, lchan->desc,
                                                        lchan->next_sg);
    else if (vdesc)
        state->residue = loongson2_cmc_dma_desc_residue(lchan,
                                                        to_lmdma_desc(vdesc), 0);

With desc->cyclic && next_sg == 0, querying a queued cyclic descriptor (while
another descriptor is active) returns early before summing desc->sg_req[i].len,
reporting only the active transfer's ndtr << width.

Even after the later commit in the series ("dmaengine: loongson:
loongson2-apb-cmc-dma: Fix residue for queued descriptors") switches the
else if (vdesc) branch to loongson2_cmc_dma_desc_total_len(),
loongson2_cmc_dma_desc_residue() is only called for the active descriptor
lchan->desc with lchan->next_sg in [1, desc->num_sgs]. Doesn't that leave the
next_sg == 0 check as dead code and the comment describing cyclic wrap and
non-active descriptors inaccurate?

Also, regarding the commit message description of the NULL dereference when
lchan->desc == NULL: since vchan_find_desc() searches vchan.desc_issued, and
both loongson2_cmc_dma_issue_pending() and loongson2_cmc_dma_handle_chan_done()
immediately start the first descriptor from vchan.desc_issued under
lchan->vchan.lock, can vchan.desc_issued ever be non-empty while lchan->desc
is NULL? Should this check and its comment be removed altogether once queued
descriptors use loongson2_cmc_dma_desc_total_len()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792988985.git.zhoubinbin@loongson.cn?part=1