[PATCH v3 0/2] dmaengine: sun6i: Fix two bugs in the DMA status path

[PATCH v3 0/2] dmaengine: sun6i: Fix two bugs in the DMA status path

[Christian Lugnberg]

Thank you for the review, Frank.

Changes in v3:
  - Patch 1: trim commit message to the root-cause explanation and fix
    description, dropping the debug/reproduction part per feedback
  - Patch 2: unchanged since v2

Christian Lugnberg (2):
  dmaengine: sun6i: fix non-atomic read of DMA position registers
  dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status

 drivers/dma/sun6i-dma.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.54.0 (Apple Git-156)

[PATCH v3 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers

[Christian Lugnberg]

sun6i_get_chan_size() reads DMA_CHAN_LLI_ADDR and DMA_CHAN_CUR_CNT in two
separate readl() calls with no synchronisation between them:

    pos   = readl(pchan->base + DMA_CHAN_LLI_ADDR);
    bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);

DMA_CHAN_LLI_ADDR holds the physical address of the *next* descriptor the
engine will load once the current one completes. DMA_CHAN_CUR_CNT holds the
remaining byte count for the *current* descriptor. If the DMA engine
advances to the next LLI entry between the two reads, pos becomes stale: it
still points to what was the next descriptor at the time of the first read,
but that descriptor is now the current one and CUR_CNT reflects its initial
(full) byte count. The subsequent virtual-chain walk starts one entry too
early and accumulates an extra full period's worth of bytes into the
residue estimate.

Fix this by re-reading DMA_CHAN_LLI_ADDR after DMA_CHAN_CUR_CNT and
retrying if the value changed. This double-read pattern guarantees that
both registers were sampled during the same descriptor interval. The cost
is at most one extra readl() pair per call in the racy case, which occurs
only at descriptor boundaries (~every 2 ms) and is negligible.

Fixes: a90e173f3faf ("dmaengine: sun6i: Add cyclic capability")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
---
 drivers/dma/sun6i-dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index f47a326dd7ff..04fe1f5042e9 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -354,8 +354,10 @@ static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
 	size_t bytes;
 	dma_addr_t pos;
 
-	pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
-	bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
+	do {
+		pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
+		bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
+	} while (pos != readl(pchan->base + DMA_CHAN_LLI_ADDR));
 
 	if (pos == LLI_LAST_ITEM)
 		return bytes;
-- 
2.54.0 (Apple Git-156)

Re: [PATCH v3 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers

[sashiko-bot]

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

Pre-existing issues:
- [High] The `sun6i_dma_tx_status` function reads and caches `vchan->phy` into a local `pchan` variable before acquiring the `vchan->vc.lock`. If the DMA transfer completes concurrently and the tasklet reassigns this physical channel to another virtual channel, `tx_status` uses the stale `pchan` pointer to calculate the DMA residue. This results in accessing another channel's hardware registers and descriptor lists.
--

--- Patch [1]: [PATCH v3 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers ---
commit bea51abd69d8527a2183678d78b9d1b2e28b24a1
Author: Christian Lugnberg <christian.lugnberg@soundtrack.io>

dmaengine: sun6i: fix non-atomic read of DMA position registers

sun6i_get_chan_size() reads DMA_CHAN_LLI_ADDR and DMA_CHAN_CUR_CNT in two
separate readl() calls with no synchronisation between them. The subsequent
virtual-chain walk starts one entry too early and accumulates an extra full
period's worth of bytes into the residue estimate. Fix this by re-reading
DMA_CHAN_LLI_ADDR after DMA_CHAN_CUR_CNT and retrying if the value changed.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..eb060ba2d4738 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -354,8 +354,10 @@ static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
>  	size_t bytes;
>  	dma_addr_t pos;
>  
> -	pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> -	bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	do {
> +		pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> +		bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	} while (pos != readl(pchan->base + DMA_CHAN_LLI_ADDR));

[Severity: High]
This is a pre-existing issue, but since sun6i_get_chan_size() uses the
pchan pointer to read hardware registers, can it be called with a stale
pchan pointer from sun6i_dma_tx_status()?

In sun6i_dma_tx_status(), pchan is cached outside the lock:

drivers/dma/sun6i-dma.c:sun6i_dma_tx_status() {
	struct sun6i_pchan *pchan = vchan->phy;
	...
	spin_lock_irqsave(&vchan->vc.lock, flags);
	...
	} else {
		bytes = sun6i_get_chan_size(pchan);
	}
	...
}

If the transfer finishes and the tasklet reassigns this physical channel
to another virtual channel concurrently, sun6i_dma_tx_status() could use
the stale pchan. This would lead to calculating the residue using another
channel's hardware state and descriptor lists.

Should vchan->phy be re-read inside the lock?

>  
>  	if (pos == LLI_LAST_ITEM)
>  		return bytes;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817135723.12807-1-christian.lugnberg@soundtrack.io?part=1

Re: [PATCH v3 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers

[Frank Li]

On Mon, Aug 17, 2026 at 03:51:22PM +0200, Christian Lugnberg wrote:
> sun6i_get_chan_size() reads DMA_CHAN_LLI_ADDR and DMA_CHAN_CUR_CNT in two
> separate readl() calls with no synchronisation between them:
>
>     pos   = readl(pchan->base + DMA_CHAN_LLI_ADDR);
>     bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
>
> DMA_CHAN_LLI_ADDR holds the physical address of the *next* descriptor the
> engine will load once the current one completes. DMA_CHAN_CUR_CNT holds the
> remaining byte count for the *current* descriptor. If the DMA engine
> advances to the next LLI entry between the two reads, pos becomes stale: it
> still points to what was the next descriptor at the time of the first read,
> but that descriptor is now the current one and CUR_CNT reflects its initial
> (full) byte count. The subsequent virtual-chain walk starts one entry too
> early and accumulates an extra full period's worth of bytes into the
> residue estimate.
>
> Fix this by re-reading DMA_CHAN_LLI_ADDR after DMA_CHAN_CUR_CNT and
> retrying if the value changed. This double-read pattern guarantees that
> both registers were sampled during the same descriptor interval. The cost
> is at most one extra readl() pair per call in the racy case, which occurs
> only at descriptor boundaries (~every 2 ms) and is negligible.
>
> Fixes: a90e173f3faf ("dmaengine: sun6i: Add cyclic capability")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/sun6i-dma.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index f47a326dd7ff..04fe1f5042e9 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -354,8 +354,10 @@ static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
>  	size_t bytes;
>  	dma_addr_t pos;
>
> -	pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> -	bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	do {
> +		pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> +		bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	} while (pos != readl(pchan->base + DMA_CHAN_LLI_ADDR));
>
>  	if (pos == LLI_LAST_ITEM)
>  		return bytes;
> --
> 2.54.0 (Apple Git-156)
>

[PATCH v3 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status

[Christian Lugnberg]

sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
descriptor for a given cookie, before checking whether the pointer
vd is NULL:

    vd = vchan_find_desc(&vchan->vc, cookie);
    txd = to_sun6i_desc(&vd->tx);   /* vd may be NULL here */

    if (vd) {
        for (lli = txd->v_lli; ...)

vchan_find_desc() returns NULL when the descriptor has already been
completed or is in-flight on a physical channel and no longer present
in the virtual channel's descriptor list. When vd is NULL,
to_sun6i_desc() is called unconditionally on &vd->tx before the NULL
check, which is undefined behaviour. Move the call inside the if (vd)
guard to ensure it is only reached with a valid pointer.

    vd = vchan_find_desc(&vchan->vc, cookie);
    if (vd) {
        struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
        for (lli = txd->v_lli; ...)

Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
---
 drivers/dma/sun6i-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 04fe1f5042e9..7704b016aed8 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -981,7 +981,6 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
 	struct sun6i_pchan *pchan = vchan->phy;
 	struct sun6i_dma_lli *lli;
 	struct virt_dma_desc *vd;
-	struct sun6i_desc *txd;
 	enum dma_status ret;
 	unsigned long flags;
 	size_t bytes = 0;
@@ -993,9 +992,9 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
 	spin_lock_irqsave(&vchan->vc.lock, flags);
 
 	vd = vchan_find_desc(&vchan->vc, cookie);
-	txd = to_sun6i_desc(&vd->tx);
 
 	if (vd) {
+		struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
 		for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
 			bytes += lli->len;
 	} else if (!pchan || !pchan->desc) {
-- 
2.54.0 (Apple Git-156)

Re: [PATCH v3 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Cross-channel hardware state corruption due to unprotected read of `vchan->phy`.
- [Critical] Use-After-Free of `pchan->desc` due to tasklet scheduling order during transfer completion.
- [High] Incorrect hardware residue reported for unmatched cookies due to missing validation in the fallback path.
--

--- Patch [2]: [PATCH v3 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status ---
commit 6902e79f0976789cfe066fe9007a8898713c0622
Author: Christian Lugnberg <christian.lugnberg@soundtrack.io>

dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status
    
sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
descriptor for a given cookie, before checking whether the pointer vd is
NULL. This patch moves the to_sun6i_desc() call inside the if (vd) guard
to ensure it is only reached with a valid pointer.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index eb060ba2d4738..070101c109a3f 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -984,7 +984,6 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  	struct sun6i_pchan *pchan = vchan->phy;

[Severity: Critical]
This is a pre-existing issue, but caching vchan->phy outside the lock
creates a race window. If the thread is preempted right after this read,
an interrupt could fire, causing sun6i_dma_tasklet() in sun6i-dma.c to clear
vchan->phy and dynamically reassign the physical channel to a different
virtual channel.

When the thread resumes and acquires the lock, it might use a stale pchan
pointer, potentially reading the state of an unrelated transfer locklessly
in sun6i_dma_tx_status() or improperly terminating one in
sun6i_dma_terminate_all().

Could this unprotected read lead to cross-channel hardware state corruption?

[ ... ]

>  	vd = vchan_find_desc(&vchan->vc, cookie);
> -	txd = to_sun6i_desc(&vd->tx);
>  
>  	if (vd) {
> +		struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
>  		for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
>  			bytes += lli->len;
>  	} else if (!pchan || !pchan->desc) {
>  		bytes = 0;
>  	} else {

[Severity: High]
This isn't a bug introduced by this patch, but if a requested cookie is
newly submitted or recently completed, vchan_find_desc() will return NULL.
The driver then falls back to querying the active hardware channel (pchan)
and returns its residue.

Since vchan_find_desc() only searches the desc_issued list, does this
mean the driver assumes pchan is executing the requested cookie without
verifying if pchan->desc->vd.tx.cookie matches? Could this cause the
progress of an unrelated transfer to be reported as residue?

>  		bytes = sun6i_get_chan_size(pchan);

[Severity: Critical]
This is a pre-existing issue, but when a non-cyclic transfer completes,
sun6i_dma_interrupt() in drivers/dma/sun6i-dma.c calls
vchan_cookie_complete() and then schedules the driver's tasklet.

vchan_cookie_complete() schedules the virt-dma tasklet, which executes
first and frees the descriptor memory. If another processor concurrently
queries sun6i_dma_tx_status() for an unrelated cookie, it will fall back
to calling sun6i_get_chan_size(pchan):

drivers/dma/sun6i-dma.c:sun6i_get_chan_size() {
	...
	struct sun6i_desc *txd = pchan->desc;
	...
	for (lli = txd->v_lli; lli; lli = lli->v_lli_next) {
	...
}

Since the driver's tasklet hasn't run yet to clear pchan->desc, won't
this dereference a descriptor that has already been freed by virt-dma?

>  	}
>  
>  	spin_unlock_irqrestore(&vchan->vc.lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817135723.12807-1-christian.lugnberg@soundtrack.io?part=2

Re: [PATCH v3 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status

[Frank Li]

On Mon, Aug 17, 2026 at 03:51:23PM +0200, Christian Lugnberg wrote:
> sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
> descriptor for a given cookie, before checking whether the pointer
> vd is NULL:
>
>     vd = vchan_find_desc(&vchan->vc, cookie);
>     txd = to_sun6i_desc(&vd->tx);   /* vd may be NULL here */
>
>     if (vd) {
>         for (lli = txd->v_lli; ...)
>
> vchan_find_desc() returns NULL when the descriptor has already been
> completed or is in-flight on a physical channel and no longer present
> in the virtual channel's descriptor list. When vd is NULL,
> to_sun6i_desc() is called unconditionally on &vd->tx before the NULL
> check, which is undefined behaviour. Move the call inside the if (vd)
> guard to ensure it is only reached with a valid pointer.
>
>     vd = vchan_find_desc(&vchan->vc, cookie);
>     if (vd) {
>         struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
>         for (lli = txd->v_lli; ...)
>
> Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/sun6i-dma.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 04fe1f5042e9..7704b016aed8 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -981,7 +981,6 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  	struct sun6i_pchan *pchan = vchan->phy;
>  	struct sun6i_dma_lli *lli;
>  	struct virt_dma_desc *vd;
> -	struct sun6i_desc *txd;
>  	enum dma_status ret;
>  	unsigned long flags;
>  	size_t bytes = 0;
> @@ -993,9 +992,9 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>
>  	vd = vchan_find_desc(&vchan->vc, cookie);
> -	txd = to_sun6i_desc(&vd->tx);
>
>  	if (vd) {
> +		struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
>  		for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
>  			bytes += lli->len;
>  	} else if (!pchan || !pchan->desc) {
> --
> 2.54.0 (Apple Git-156)
>