[PATCH v2 0/2] Fix two bugs in the DMA status path

[PATCH v2 0/2] Fix two bugs in the DMA status path

[Christian Lugnberg]

Thank you for the review. Patch 2 has been updated with a corrected
commit message — the original overclaimed an oops; the fix removes
undefined behaviour but no memory access actually occurs at that point.
Changes in v2:
  - Patch 2: correct commit message to accurately describe the undefined
    behaviour rather than claiming an imminent null pointer dereference

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 v2 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.

For ALSA cyclic buffers the over-counted residue can reach the full buffer
size, causing the computed playback position to appear to jump backward to
near zero. The ALSA PCM core treats such a backward discontinuity in hw_ptr
as evidence that the buffer has underrun and declares an xrun.

On the Barix IPAM400 (Allwinner H3, kernel 6.12) this manifests as audible
glitches accompanied by spurious xrun log entries, confirmed by two
independent observations:

First, the ALSA buffer in the affected configuration is 2 seconds deep with
a 500 ms refill period (the interval at which the player software wakes up
to top up the buffer). For a real underrun to occur the player would have
to stall for the full 2 seconds without writing any audio — effectively
impossible under normal scheduling conditions. Yet xruns are observed
regularly.

Second, the underrun duration reported by the kernel at xrun time is
~30 µs, roughly one audio sample at 44100 Hz. A genuine drain of a 2
second buffer cannot resolve in 30 µs; only a phantom position jump
caused by a register read race can produce such a number.

Observed on a 44100 Hz stereo S16_LE stream:

  $ cat /proc/asound/Codec/pcm0p/sub0/status
  state: XRUN
  delay: 0
  avail: 88200
  avail_max: 22514

The avail_max of 22514 frames (511 ms) matches exactly one ALSA period —
the amount added by starting the LLI chain walk one entry too early.

The race window itself is narrow. Each DMA descriptor covers approximately
88 samples (~2 ms at 44100 Hz), so the engine advances to a new descriptor
roughly every 2 ms. The two readl() calls must straddle that exact boundary
for the corruption to occur, which explains why the bug is intermittent.

The bug is further confirmed by the xrun_debug bit 2 toggle (jiffies
position validation). With it enabled xruns cease immediately and do not
return; clearing it causes xruns to reappear within minutes. This on/off
reproducibility isolates the fault to the hw_ptr position reporting path;
the DMA engine itself is functioning correctly, as evidenced by hw_ptr
advancing at a steady 44100 frames/sec between events:

  $ echo 4 > /proc/asound/Codec/pcm0p/xrun_debug  # xruns stop
  $ echo 0 > /proc/asound/Codec/pcm0p/xrun_debug  # xruns return

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 v2 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers

[Frank Li]

On Fri, Aug 14, 2026 at 04:21:10PM +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.

Thanks for this fix. This common problem, above already clean enough.

please cut below debug/test proccess. and keep

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.

Frank

>
> For ALSA cyclic buffers the over-counted residue can reach the full buffer
> size, causing the computed playback position to appear to jump backward to
> near zero. The ALSA PCM core treats such a backward discontinuity in hw_ptr
> as evidence that the buffer has underrun and declares an xrun.
>
> On the Barix IPAM400 (Allwinner H3, kernel 6.12) this manifests as audible
> glitches accompanied by spurious xrun log entries, confirmed by two
> independent observations:
>
> First, the ALSA buffer in the affected configuration is 2 seconds deep with
> a 500 ms refill period (the interval at which the player software wakes up
> to top up the buffer). For a real underrun to occur the player would have
> to stall for the full 2 seconds without writing any audio — effectively
> impossible under normal scheduling conditions. Yet xruns are observed
> regularly.
>
> Second, the underrun duration reported by the kernel at xrun time is
> ~30 µs, roughly one audio sample at 44100 Hz. A genuine drain of a 2
> second buffer cannot resolve in 30 µs; only a phantom position jump
> caused by a register read race can produce such a number.
>
> Observed on a 44100 Hz stereo S16_LE stream:
>
>   $ cat /proc/asound/Codec/pcm0p/sub0/status
>   state: XRUN
>   delay: 0
>   avail: 88200
>   avail_max: 22514
>
> The avail_max of 22514 frames (511 ms) matches exactly one ALSA period —
> the amount added by starting the LLI chain walk one entry too early.
>
> The race window itself is narrow. Each DMA descriptor covers approximately
> 88 samples (~2 ms at 44100 Hz), so the engine advances to a new descriptor
> roughly every 2 ms. The two readl() calls must straddle that exact boundary
> for the corruption to occur, which explains why the bug is intermittent.
>
> The bug is further confirmed by the xrun_debug bit 2 toggle (jiffies
> position validation). With it enabled xruns cease immediately and do not
> return; clearing it causes xruns to reappear within minutes. This on/off
> reproducibility isolates the fault to the hw_ptr position reporting path;
> the DMA engine itself is functioning correctly, as evidenced by hw_ptr
> advancing at a steady 44100 frames/sec between events:
>
>   $ echo 4 > /proc/asound/Codec/pcm0p/xrun_debug  # xruns stop
>   $ echo 0 > /proc/asound/Codec/pcm0p/xrun_debug  # xruns return
>


> 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)
>

[PATCH v2 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 v2 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Lockless read of `vchan->phy` leads to Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free and state corruption.
- [Critical] Interrupt handler races with terminate_all, causing a NULL pointer dereference.
- [High] sun6i_dma_tx_status incorrectly reports the residue of the active descriptor for unissued descriptors due to missing cookie validation.
- [High] Deadlock risk due to plain spin_lock() usage on sdev->lock which is shared with a softirq context.
--

--- Patch [2]: [PATCH v2 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status ---
commit fc20cb9c5238894b0536bcd9408e43b44dedb04e
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 check inside the 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 does this lockless read of vchan->phy
introduce a time-of-check to time-of-use (TOCTOU) use-after-free risk?

If sun6i_dma_tasklet concurrently frees this physical channel and allocates
it to a different stream, by the time sun6i_dma_tx_status() acquires the lock
below and calls sun6i_get_chan_size(pchan), the descriptor might belong to
another channel and could be concurrently freed. Could this cause a
use-after-free when iterating txd->v_lli?

>  	struct sun6i_dma_lli *lli;
>  	struct virt_dma_desc *vd;
[ ... ]
> @@ -996,9 +995,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) {

[Severity: High]
This is a pre-existing issue, but does this fallback logic incorrectly report
the residue of the active descriptor for unissued descriptors?

When queried for a submitted but unissued descriptor, vchan_find_desc() returns
NULL because the descriptor is in desc_submitted instead of desc_issued. The
code then falls back to calculating residue for the currently running transfer
using pchan->desc in the else block, without verifying if
pchan->desc->vd.tx.cookie matches the requested cookie.

While reviewing the file, I noticed a couple of other pre-existing issues:

[Severity: Critical]
This is a pre-existing issue, but is there a race condition between the
interrupt handler and sun6i_dma_terminate_all() that can lead to a NULL
pointer dereference?

If sun6i_dma_interrupt() reads vchan locklessly and proceeds to handle it,
while sun6i_dma_terminate_all() concurrently runs on another CPU, acquires
vchan->vc.lock, and clears pchan->desc to NULL:

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
    ...
    vchan_cyclic_callback(&pchan->desc->vd);
    ...
    vchan_cookie_complete(&pchan->desc->vd);
    ...
}

Will the lockless call to vchan_cyclic_callback() or the subsequent call to
vchan_cookie_complete() dereference a NULL pointer since the descriptor could
have just been cleared?

[Severity: High]
This is a pre-existing issue, but is there a deadlock risk due to plain
spin_lock() usage on sdev->lock?

In sun6i_dma_terminate_all() and sun6i_dma_pause():

drivers/dma/sun6i-dma.c:sun6i_dma_pause() {
    ...
    spin_lock(&sdev->lock);
    ...
}

These functions acquire sdev->lock using a plain spin_lock() from process
context, leaving softirqs enabled. If a softirq interrupts the CPU while the
lock is held, and sun6i_dma_tasklet (running in softirq context) attempts to
acquire the same lock via spin_lock_irq(&sdev->lock), might this result in a
deadlock?

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

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

[Frank Li]

On Fri, Aug 14, 2026 at 04:21:11PM +0200, Christian Lugnberg wrote:
> [You don't often get email from christian.lugnberg@soundtrack.io. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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)
>