[PATCH 1/2] dmaengine: qcom_hidma: fix memory leak on probe failure

[PATCH 1/2] dmaengine: qcom_hidma: fix memory leak on probe failure

[Qasim Ijaz]

hidma_ll_init() is invoked to create and initialise a struct hidma_lldev
object during hidma probe. During this a FIFO buffer is allocated, but
if some failure occurs after (like hidma_ll_setup failure) we should
clean up the FIFO.

Fixes: d1615ca2e085 ("dmaengine: qcom_hidma: implement lower level hardware interface")
Cc: stable@vger.kernel.org
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
---
 drivers/dma/qcom/hidma_ll.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
index 53244e0e34a3..fee448499777 100644
--- a/drivers/dma/qcom/hidma_ll.c
+++ b/drivers/dma/qcom/hidma_ll.c
@@ -788,8 +788,10 @@ struct hidma_lldev *hidma_ll_init(struct device *dev, u32 nr_tres,
 		return NULL;
 
 	rc = hidma_ll_setup(lldev);
-	if (rc)
+	if (rc) {
+		kfifo_free(&lldev->handoff_fifo);
 		return NULL;
+	}
 
 	spin_lock_init(&lldev->lock);
 	tasklet_setup(&lldev->task, hidma_ll_tre_complete);
-- 
2.39.5

Re: [PATCH 1/2] dmaengine: qcom_hidma: fix memory leak on probe failure

[Eugen Hristev]

On 6/2/25 01:42, Qasim Ijaz wrote:
> hidma_ll_init() is invoked to create and initialise a struct hidma_lldev
> object during hidma probe. During this a FIFO buffer is allocated, but
> if some failure occurs after (like hidma_ll_setup failure) we should
> clean up the FIFO.
> 
> Fixes: d1615ca2e085 ("dmaengine: qcom_hidma: implement lower level hardware interface")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>

Reviewed-by: Eugen Hristev <eugen.hristev@linaro.org>

[PATCH 2/2] dmaengine: qcom_hidma: fix handoff FIFO memory leak on driver removal

[Qasim Ijaz]

hidma_ll_init() allocates a handoff FIFO, but the matching 
hidma_ll_uninit() function (which is invoked in remove()) 
never releases it, leaking memory.

To fix this call kfifo_free in hidma_ll_uninit().

Fixes: d1615ca2e085 ("dmaengine: qcom_hidma: implement lower level hardware interface")
Cc: stable@vger.kernel.org
Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>

---
 drivers/dma/qcom/hidma_ll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
index fee448499777..0c2bae46746c 100644
--- a/drivers/dma/qcom/hidma_ll.c
+++ b/drivers/dma/qcom/hidma_ll.c
@@ -816,6 +816,7 @@ int hidma_ll_uninit(struct hidma_lldev *lldev)
 
 	required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
 	tasklet_kill(&lldev->task);
+	kfifo_free(&lldev->handoff_fifo);
 	memset(lldev->trepool, 0, required_bytes);
 	lldev->trepool = NULL;
 	atomic_set(&lldev->pending_tre_count, 0);
-- 
2.39.5

Re: [PATCH 2/2] dmaengine: qcom_hidma: fix handoff FIFO memory leak on driver removal

[Eugen Hristev]

On 6/2/25 01:42, Qasim Ijaz wrote:
> hidma_ll_init() allocates a handoff FIFO, but the matching 
> hidma_ll_uninit() function (which is invoked in remove()) 
> never releases it, leaking memory.
> 
> To fix this call kfifo_free in hidma_ll_uninit().
> 
> Fixes: d1615ca2e085 ("dmaengine: qcom_hidma: implement lower level hardware interface")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
> 
> ---
>  drivers/dma/qcom/hidma_ll.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
> index fee448499777..0c2bae46746c 100644
> --- a/drivers/dma/qcom/hidma_ll.c
> +++ b/drivers/dma/qcom/hidma_ll.c
> @@ -816,6 +816,7 @@ int hidma_ll_uninit(struct hidma_lldev *lldev)
>  
>  	required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
>  	tasklet_kill(&lldev->task);
> +	kfifo_free(&lldev->handoff_fifo);
>  	memset(lldev->trepool, 0, required_bytes);
>  	lldev->trepool = NULL;
>  	atomic_set(&lldev->pending_tre_count, 0);

Is it possible that the handoff_fifo is freed, then we could observe
reset complete interrupts before they are being cleared in
hidma_ll_uninit later on, which would lead to the following call chain

 hidma_ll_inthandler - hidma_ll_int_handler_internal -
hidma_handle_tre_completion - hidma_post_completed -
tasklet_schedule(&lldev->task); - hidma_ll_tre_complete - kfifo_out

?

Re: [PATCH 2/2] dmaengine: qcom_hidma: fix handoff FIFO memory leak on driver removal

[Sinan Kaya]

On 6/5/2025 9:04 AM, Eugen Hristev wrote:
>> diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
>> index fee448499777..0c2bae46746c 100644
>> --- a/drivers/dma/qcom/hidma_ll.c
>> +++ b/drivers/dma/qcom/hidma_ll.c
>> @@ -816,6 +816,7 @@ int hidma_ll_uninit(struct hidma_lldev *lldev)
>>   
>>   	required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
>>   	tasklet_kill(&lldev->task);
>> +	kfifo_free(&lldev->handoff_fifo);
>>   	memset(lldev->trepool, 0, required_bytes);
>>   	lldev->trepool = NULL;
>>   	atomic_set(&lldev->pending_tre_count, 0);
> Is it possible that the handoff_fifo is freed, then we could observe
> reset complete interrupts before they are being cleared in
> hidma_ll_uninit later on, which would lead to the following call chain
>
>   hidma_ll_inthandler - hidma_ll_int_handler_internal -
> hidma_handle_tre_completion - hidma_post_completed -
> tasklet_schedule(&lldev->task); - hidma_ll_tre_complete - kfifo_out

According to the documentation, the way to guarantee this from not happening

is to call tasklet_disable() to ensure that tasklet completes execution. 
Only after that

data structures used by the tasklet can be freed.

I think proper order is:

1. tasklet_disable

2. tasklet_kill

3. kfifo_free

Re: [PATCH 2/2] dmaengine: qcom_hidma: fix handoff FIFO memory leak on driver removal

[Qasim Ijaz]

On Fri, Jun 06, 2025 at 09:35:44AM -0400, Sinan Kaya wrote:
> 
> On 6/5/2025 9:04 AM, Eugen Hristev wrote:
> > > diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
> > > index fee448499777..0c2bae46746c 100644
> > > --- a/drivers/dma/qcom/hidma_ll.c
> > > +++ b/drivers/dma/qcom/hidma_ll.c
> > > @@ -816,6 +816,7 @@ int hidma_ll_uninit(struct hidma_lldev *lldev)
> > >   	required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
> > >   	tasklet_kill(&lldev->task);
> > > +	kfifo_free(&lldev->handoff_fifo);
> > >   	memset(lldev->trepool, 0, required_bytes);
> > >   	lldev->trepool = NULL;
> > >   	atomic_set(&lldev->pending_tre_count, 0);
> > Is it possible that the handoff_fifo is freed, then we could observe
> > reset complete interrupts before they are being cleared in
> > hidma_ll_uninit later on, which would lead to the following call chain
> > 
> >   hidma_ll_inthandler - hidma_ll_int_handler_internal -
> > hidma_handle_tre_completion - hidma_post_completed -
> > tasklet_schedule(&lldev->task); - hidma_ll_tre_complete - kfifo_out
> 
> According to the documentation, the way to guarantee this from not happening
> 
> is to call tasklet_disable() to ensure that tasklet completes execution.
> Only after that
> 
> data structures used by the tasklet can be freed.
> 
> I think proper order is:
> 
> 1. tasklet_disable
> 
> 2. tasklet_kill
> 
> 3. kfifo_free

Hi Sinan, hi Eugen,

Thanks for reviewing the patch and for pointing out the correct
shutdown ordering.

If you’re both happy with it, I’ll send a v2 that calls
tasklet_disable() before tasklet_kill(), then frees the handoff_fifo.

Just let me know and I’ll resend.

Thanks
Qasim
> 
> 
> 

Re: [PATCH 2/2] dmaengine: qcom_hidma: fix handoff FIFO memory leak on driver removal

[Sinan Kaya]

On 6/15/2025 3:15 PM, Qasim Ijaz wrote:
> Thanks for reviewing the patch and for pointing out the correct
> shutdown ordering.
>
> If you’re both happy with it, I’ll send a v2 that calls
> tasklet_disable() before tasklet_kill(), then frees the handoff_fifo.
>
> Just let me know and I’ll resend.

Sure, please test it on a test kernel module before posting with a tasklet.

This should be straightforward to verify.