[v2] dmaengine: sun6i-dma: Fix memory leak in sun6i_dma_terminate_all

When terminating a non-cyclic DMA transfer, the active descriptor
is not properly reclaimed. The descriptor is removed from the
desc_issued list in sun6i_dma_start_desc(), but in
sun6i_dma_terminate_all(), only cyclic transfer descriptors are
added to the desc_completed list before cleanup.

For non-cyclic transfers, pchan->desc is set to NULL without first
adding the descriptor back to a list that vchan_get_all_descriptors()
can collect. This causes the descriptor and its associated LLI chain
to be permanently leaked.

Fix by ensuring both cyclic and non-cyclic active descriptors are
added to the desc_completed list before setting pchan->desc to NULL.

Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
 Change in v2;
 -Add pchan->desc != pchan->done check to prevent race condition
  where completed descriptors could be double-added to desc_completed
  list, causing list corruption
---
 drivers/dma/sun6i-dma.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
  

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

Pre-existing issues:
- [Critical] Lockless read of `vchan->phy` in `sun6i_dma_terminate_all` leads to a TOCTOU race condition that can hijack and corrupt another channel's physical state.
- [High] Concurrent execution of `sun6i_dma_terminate_all` and `sun6i_dma_interrupt` can lead to a fatal NULL pointer dereference.
- [High] Terminating an active cyclic transfer frees the descriptor without clearing the virt-dma cyclic pointer, leading to a Use-After-Free.
- [High] Plain `spin_lock` on `sdev->lock` in process context deadlocks with softirq tasklet.
--

commit f08a1b576e146ed2d00cd844a90cf04a64b43397
Author: Hongling Zeng <zenghongling@kylinos.cn>

dmaengine: sun6i-dma: Fix memory leak in sun6i_dma_terminate_all

This commit addresses a memory leak for non-cyclic DMA transfers. It ensures
both cyclic and non-cyclic active descriptors are added to the desc_completed
list before cleanup to prevent them from being permanently leaked.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..46c837e87e6e4 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -945,16 +945,14 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  
>  	spin_lock_irqsave(&vchan->vc.lock, flags);

[Severity: High]
This is a pre-existing issue, but does acquiring sdev->lock earlier in this
function create a deadlock risk?

sun6i_dma_terminate_all() calls spin_lock(&sdev->lock) without disabling
softirqs. If a hardware interrupt fires on the same CPU while the lock is
held, the kernel processes pending softirqs upon returning from the interrupt.

This runs sun6i_dma_tasklet(), which attempts to acquire the lock using
spin_lock_irq(&sdev->lock). Will this cause a self-deadlock since the
interrupted process context already holds the lock?

>  
> -	if (vchan->cyclic) {
> -		vchan->cyclic = false;
> -		if (pchan && pchan->desc) {
> -			struct virt_dma_desc *vd = &pchan->desc->vd;
> -			struct virt_dma_chan *vc = &vchan->vc;
> +	if (pchan && pchan->desc && pchan->desc != pchan->done) {

[Severity: Critical]
This isn't a bug introduced by this patch, but does this code have a race
condition that can corrupt another channel?

At the top of sun6i_dma_terminate_all(), the pchan pointer is cached
locklessly from vchan->phy before taking any locks.

Concurrently, sun6i_dma_tasklet() can complete a transfer, set
vchan->phy = NULL, and reassign this exact pchan to a different virtual
channel.

When this code subsequently acquires vchan->vc.lock, wouldn't it use the
stale pchan pointer to inappropriately access descriptors and halt hardware
belonging to the newly assigned channel?

> +		struct virt_dma_desc *vd = &pchan->desc->vd;
> +		struct virt_dma_chan *vc = &vchan->vc;
>  
> -			list_add_tail(&vd->node, &vc->desc_completed);
> -		}
> +		list_add_tail(&vd->node, &vc->desc_completed);

[Severity: High]
This is a pre-existing issue, but does terminating an active cyclic transfer
leave a dangling pointer here?

The active descriptor is added to desc_completed so it can be freed by
vchan_dma_desc_free_list(), but the virt-dma pointer vc->cyclic is never
cleared.

If vchan_complete() runs asynchronously from the tasklet, couldn't it see a
non-NULL vc->cyclic and dereference the freed descriptor? Standard virt-dma
drivers typically use vchan_terminate_vdesc() to clear this.

>  	}
>  
> +	vchan->cyclic = false;
>  	vchan_get_all_descriptors(&vchan->vc, &head);
>  
>  	if (pchan) {

[ ... ]

>  		vchan->phy = NULL;
>  		pchan->vchan = NULL;
>  		pchan->desc = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but can clearing pchan->desc here
race with the interrupt handler and cause a NULL pointer dereference?

In sun6i_dma_interrupt(), vchan->cyclic is read locklessly. If it evaluates
to false, the handler blocks on spin_lock(&vchan->vc.lock):

sun6i_dma_interrupt() {
    ...
    if (!vchan->cyclic) {
        spin_lock(&vchan->vc.lock);
        vchan_cookie_complete(&pchan->desc->vd);
        ...
}

If sun6i_dma_terminate_all() concurrently acquires the lock and sets
pchan->desc = NULL, when the interrupt handler unblocks, wouldn't
evaluating &pchan->desc->vd crash the kernel?

>  		pchan->done = NULL;
>  	}
  

Dne torek, 16. junij 2026 ob 08:04:49 Srednjeevropski poletni čas je Hongling Zeng napisal(a):
> When terminating a non-cyclic DMA transfer, the active descriptor
> is not properly reclaimed. The descriptor is removed from the
> desc_issued list in sun6i_dma_start_desc(), but in
> sun6i_dma_terminate_all(), only cyclic transfer descriptors are
> added to the desc_completed list before cleanup.
> 
> For non-cyclic transfers, pchan->desc is set to NULL without first
> adding the descriptor back to a list that vchan_get_all_descriptors()
> can collect. This causes the descriptor and its associated LLI chain
> to be permanently leaked.
> 
> Fix by ensuring both cyclic and non-cyclic active descriptors are
> added to the desc_completed list before setting pchan->desc to NULL.
> 
> Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> 
> ---
>  Change in v2;
>  -Add pchan->desc != pchan->done check to prevent race condition
>   where completed descriptors could be double-added to desc_completed
>   list, causing list corruption
> ---
>  drivers/dma/sun6i-dma.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 7a79f346250a..12d038ef5f2e 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -946,16 +946,14 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>  
> -	if (vchan->cyclic) {
> -		vchan->cyclic = false;
> -		if (pchan && pchan->desc) {
> -			struct virt_dma_desc *vd = &pchan->desc->vd;
> -			struct virt_dma_chan *vc = &vchan->vc;
> +	if (pchan && pchan->desc && pchan->desc != pchan->done) {
> +		struct virt_dma_desc *vd = &pchan->desc->vd;
> +		struct virt_dma_chan *vc = &vchan->vc;
>  
> -			list_add_tail(&vd->node, &vc->desc_completed);
> -		}
> +		list_add_tail(&vd->node, &vc->desc_completed);
>  	}
>  
> +	vchan->cyclic = false;
>  	vchan_get_all_descriptors(&vchan->vc, &head);
>  
>  	if (pchan) {
>
  

On Tue, Jun 16, 2026 at 02:04:49PM +0800, Hongling Zeng wrote:
> When terminating a non-cyclic DMA transfer, the active descriptor
> is not properly reclaimed. The descriptor is removed from the
> desc_issued list in sun6i_dma_start_desc(), but in
> sun6i_dma_terminate_all(), only cyclic transfer descriptors are
> added to the desc_completed list before cleanup.
>
> For non-cyclic transfers, pchan->desc is set to NULL without first
> adding the descriptor back to a list that vchan_get_all_descriptors()
> can collect. This causes the descriptor and its associated LLI chain
> to be permanently leaked.
>
> Fix by ensuring both cyclic and non-cyclic active descriptors are
> added to the desc_completed list before setting pchan->desc to NULL.
>
> Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>
> ---
>  Change in v2;
>  -Add pchan->desc != pchan->done check to prevent race condition
>   where completed descriptors could be double-added to desc_completed
>   list, causing list corruption
> ---
>  drivers/dma/sun6i-dma.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 7a79f346250a..12d038ef5f2e 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -946,16 +946,14 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>
> -	if (vchan->cyclic) {
> -		vchan->cyclic = false;
> -		if (pchan && pchan->desc) {
> -			struct virt_dma_desc *vd = &pchan->desc->vd;
> -			struct virt_dma_chan *vc = &vchan->vc;
> +	if (pchan && pchan->desc && pchan->desc != pchan->done) {
> +		struct virt_dma_desc *vd = &pchan->desc->vd;
> +		struct virt_dma_chan *vc = &vchan->vc;
>
> -			list_add_tail(&vd->node, &vc->desc_completed);
> -		}
> +		list_add_tail(&vd->node, &vc->desc_completed);

should It be in desc_terminated queue?

ref: https://lore.kernel.org/dmaengine/ajETw7uwVx_U9o5F@ryzen/T/#m541c24b45fb425c6a8a81d800db225b58411447e

Frank
>  	}
>
> +	vchan->cyclic = false;
>  	vchan_get_all_descriptors(&vchan->vc, &head);
>
>  	if (pchan) {
> --
> 2.25.1
>
  

Hi Frank and Jernej Skrabec:

      Thanks for all the review!

   You're absolutely right.After checking virt-dma.h, I see that 
desc_terminated is the correct queue for actively terminated transfers. 
I'll update to use:

   - vchan_terminate_vdesc(vd);

   I'll send v3 ,Please help to review.

   Best regards,
   Hongling Zeng

在 2026年06月17日 04:47, Frank Li 写道:
> On Tue, Jun 16, 2026 at 02:04:49PM +0800, Hongling Zeng wrote:
>> When terminating a non-cyclic DMA transfer, the active descriptor
>> is not properly reclaimed. The descriptor is removed from the
>> desc_issued list in sun6i_dma_start_desc(), but in
>> sun6i_dma_terminate_all(), only cyclic transfer descriptors are
>> added to the desc_completed list before cleanup.
>>
>> For non-cyclic transfers, pchan->desc is set to NULL without first
>> adding the descriptor back to a list that vchan_get_all_descriptors()
>> can collect. This causes the descriptor and its associated LLI chain
>> to be permanently leaked.
>>
>> Fix by ensuring both cyclic and non-cyclic active descriptors are
>> added to the desc_completed list before setting pchan->desc to NULL.
>>
>> Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>>
>> ---
>>   Change in v2;
>>   -Add pchan->desc != pchan->done check to prevent race condition
>>    where completed descriptors could be double-added to desc_completed
>>    list, causing list corruption
>> ---
>>   drivers/dma/sun6i-dma.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>> index 7a79f346250a..12d038ef5f2e 100644
>> --- a/drivers/dma/sun6i-dma.c
>> +++ b/drivers/dma/sun6i-dma.c
>> @@ -946,16 +946,14 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>>
>>   	spin_lock_irqsave(&vchan->vc.lock, flags);
>>
>> -	if (vchan->cyclic) {
>> -		vchan->cyclic = false;
>> -		if (pchan && pchan->desc) {
>> -			struct virt_dma_desc *vd = &pchan->desc->vd;
>> -			struct virt_dma_chan *vc = &vchan->vc;
>> +	if (pchan && pchan->desc && pchan->desc != pchan->done) {
>> +		struct virt_dma_desc *vd = &pchan->desc->vd;
>> +		struct virt_dma_chan *vc = &vchan->vc;
>>
>> -			list_add_tail(&vd->node, &vc->desc_completed);
>> -		}
>> +		list_add_tail(&vd->node, &vc->desc_completed);
> should It be in desc_terminated queue?
>
> ref: https://lore.kernel.org/dmaengine/ajETw7uwVx_U9o5F@ryzen/T/#m541c24b45fb425c6a8a81d800db225b58411447e
>
> Frank
>>   	}
>>
>> +	vchan->cyclic = false;
>>   	vchan_get_all_descriptors(&vchan->vc, &head);
>>
>>   	if (pchan) {
>> --
>> 2.25.1
>>