[PATCH] media: chips-media: wave5: Fix Potential Probe Resource Leak

[PATCH] media: chips-media: wave5: Fix Potential Probe Resource Leak

[Brandon Brnich]

After kthread creation during probe sequence, a handful of other
failures could occur. If this were to happen, the kthread is never
explicitly deleted which results in a resource leak. Add explicit cleanup
of this resource.

Signed-off-by: Brandon Brnich <b-brnich@ti.com>
---

I am aware that all the dev attributes would be freed since it is
allocated using the devm_* framework. But I did not believe that this
framework would recursively free the thread and stop the timer. These
would just be dangling resources unable to get killed unless
deliberately removed in the probe function. 

 drivers/media/platform/chips-media/wave5/wave5-vpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
index e1715d3f43b0..f027b4ac775a 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
@@ -339,6 +339,11 @@ static int wave5_vpu_probe(struct platform_device *pdev)
 	v4l2_device_unregister(&dev->v4l2_dev);
 err_vdi_release:
 	wave5_vdi_release(&pdev->dev);
+
+	if (dev->irq < 0) {
+		kthread_destroy_worker(dev->worker);
+		hrtimer_cancel(&dev->hrtimer);
+	}
 err_clk_dis:
 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
 err_reset_assert:
-- 
2.34.1

RE: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource Leak

[jackson.lee]

Hi Brandon


> -----Original Message-----
> From: Brandon Brnich <b-brnich@ti.com>
> Sent: Thursday, November 20, 2025 6:32 AM
> To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
> <jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Nicolas
> Dufresne <nicolas.dufresne@collabora.com>
> Cc: Darren Etheridge <detheridge@ti.com>; Brandon Brnich <b-brnich@ti.com>
> Subject: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource
> Leak
> 
> After kthread creation during probe sequence, a handful of other failures
> could occur. If this were to happen, the kthread is never explicitly
> deleted which results in a resource leak. Add explicit cleanup of this
> resource.
> 
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
> 
> I am aware that all the dev attributes would be freed since it is
> allocated using the devm_* framework. But I did not believe that this
> framework would recursively free the thread and stop the timer. These
> would just be dangling resources unable to get killed unless deliberately
> removed in the probe function.
> 
>  drivers/media/platform/chips-media/wave5/wave5-vpu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> index e1715d3f43b0..f027b4ac775a 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> @@ -339,6 +339,11 @@ static int wave5_vpu_probe(struct platform_device
> *pdev)
>  	v4l2_device_unregister(&dev->v4l2_dev);
>  err_vdi_release:
>  	wave5_vdi_release(&pdev->dev);
> +
> +	if (dev->irq < 0) {
> +		kthread_destroy_worker(dev->worker);
> +		hrtimer_cancel(&dev->hrtimer);
> +	}

I'd like to change the above to as below.
I think we have to distinguish failure between registering IRQ handler and registering v4l2_device_register.

err_irq_release:
	if (dev->irq < 0) {
		kthread_destroy_worker(dev->worker);
		hrtimer_cancel(&dev->hrtimer);
	}
err_vdi_release:

thanks
Jackson


>  err_clk_dis:
>  	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
>  err_reset_assert:
> --
> 2.34.1

Re: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource Leak

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

Hi,

Le mardi 02 décembre 2025 à 02:06 +0000, jackson.lee a écrit :
> Hi Brandon
> 
> 
> > -----Original Message-----
> > From: Brandon Brnich <b-brnich@ti.com>
> > Sent: Thursday, November 20, 2025 6:32 AM
> > To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
> > <jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
> > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Nicolas
> > Dufresne <nicolas.dufresne@collabora.com>
> > Cc: Darren Etheridge <detheridge@ti.com>; Brandon Brnich <b-brnich@ti.com>
> > Subject: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource
> > Leak
> > 
> > After kthread creation during probe sequence, a handful of other failures
> > could occur. If this were to happen, the kthread is never explicitly
> > deleted which results in a resource leak. Add explicit cleanup of this
> > resource.
> > 
> > Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> > ---
> > 
> > I am aware that all the dev attributes would be freed since it is
> > allocated using the devm_* framework. But I did not believe that this
> > framework would recursively free the thread and stop the timer. These
> > would just be dangling resources unable to get killed unless deliberately
> > removed in the probe function.
> > 
> >  drivers/media/platform/chips-media/wave5/wave5-vpu.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > index e1715d3f43b0..f027b4ac775a 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > @@ -339,6 +339,11 @@ static int wave5_vpu_probe(struct platform_device
> > *pdev)
> >  	v4l2_device_unregister(&dev->v4l2_dev);
> >  err_vdi_release:
> >  	wave5_vdi_release(&pdev->dev);
> > +
> > +	if (dev->irq < 0) {
> > +		kthread_destroy_worker(dev->worker);
> > +		hrtimer_cancel(&dev->hrtimer);
> > +	}
> 
> I'd like to change the above to as below.
> I think we have to distinguish failure between registering IRQ handler and
> registering v4l2_device_register.
> 
> err_irq_release:
> 	if (dev->irq < 0) {
> 		kthread_destroy_worker(dev->worker);
> 		hrtimer_cancel(&dev->hrtimer);
> 	}
> err_vdi_release:

That's seems more then just a suggestion, I see that err_vdi_release: is reached
on worker creation failure. Checking the kthread code, this will cause a use
after free instead of a leak.

An additional question, aren't we are supposed to also cleanup irq_thread ? We
have this code being introduced in the remove function now:


		if (dev->irq_thread) {
			kthread_stop(dev->irq_thread);
			up(&dev->irq_sem);
			dev->irq_thread = NULL;
		}



regards,
Nicolas


> 
> thanks
> Jackson
> 
> 
> >  err_clk_dis:
> >  	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> >  err_reset_assert:
> > --
> > 2.34.1
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource Leak

[Brandon Brnich]

Hi Jackson and Nicolas,

On 12/11/2025 9:04 AM, Nicolas Dufresne wrote:
> Hi,
> 
> Le mardi 02 décembre 2025 à 02:06 +0000, jackson.lee a écrit :
>> Hi Brandon
>>
>>
>>> -----Original Message-----
>>> From: Brandon Brnich <b-brnich@ti.com>
>>> Sent: Thursday, November 20, 2025 6:32 AM
>>> To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
>>> <jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
>>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Nicolas
>>> Dufresne <nicolas.dufresne@collabora.com>
>>> Cc: Darren Etheridge <detheridge@ti.com>; Brandon Brnich <b-brnich@ti.com>
>>> Subject: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource
>>> Leak
>>>
>>> After kthread creation during probe sequence, a handful of other failures
>>> could occur. If this were to happen, the kthread is never explicitly
>>> deleted which results in a resource leak. Add explicit cleanup of this
>>> resource.
>>>
>>> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
>>> ---
>>>
>>> I am aware that all the dev attributes would be freed since it is
>>> allocated using the devm_* framework. But I did not believe that this
>>> framework would recursively free the thread and stop the timer. These
>>> would just be dangling resources unable to get killed unless deliberately
>>> removed in the probe function.
>>>
>>>   drivers/media/platform/chips-media/wave5/wave5-vpu.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>>> b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>>> index e1715d3f43b0..f027b4ac775a 100644
>>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>>> @@ -339,6 +339,11 @@ static int wave5_vpu_probe(struct platform_device
>>> *pdev)
>>>   	v4l2_device_unregister(&dev->v4l2_dev);
>>>   err_vdi_release:
>>>   	wave5_vdi_release(&pdev->dev);
>>> +
>>> +	if (dev->irq < 0) {
>>> +		kthread_destroy_worker(dev->worker);
>>> +		hrtimer_cancel(&dev->hrtimer);
>>> +	}
>>
>> I'd like to change the above to as below.
>> I think we have to distinguish failure between registering IRQ handler and
>> registering v4l2_device_register.
>>
>> err_irq_release:
>> 	if (dev->irq < 0) {
>> 		kthread_destroy_worker(dev->worker);
>> 		hrtimer_cancel(&dev->hrtimer);
>> 	}
>> err_vdi_release:
> 
> That's seems more then just a suggestion, I see that err_vdi_release: is reached
> on worker creation failure. Checking the kthread code, this will cause a use
> after free instead of a leak.

Agreed with all above statements. I will update to fix use after free 
that I introduced in v1.

> 
> An additional question, aren't we are supposed to also cleanup irq_thread ? We
> have this code being introduced in the remove function now:
> 
> 
> 		if (dev->irq_thread) {
> 			kthread_stop(dev->irq_thread);
> 			up(&dev->irq_sem);
> 			dev->irq_thread = NULL;
> 		}

This portion of code is being introduced in Jackson's performance 
series. I did not base my patch on this series since it hasn't been 
accepted yet. I assumed my patch would make it in before since this is 
easier to review than that series. Apologies if I need to base on that 
series. Can rebase this in v2 if requested.

Otherwise, I suggest Jackson to add irq_thread cleanup in next iteration 
of performance series.

Best,
Brandon

> 
> 
> 
> regards,
> Nicolas
> 
> 
>>
>> thanks
>> Jackson
>>
>>
>>>   err_clk_dis:
>>>   	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
>>>   err_reset_assert:
>>> --
>>> 2.34.1
>>

Re: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource Leak

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 4466 bytes --]

Le jeudi 11 décembre 2025 à 15:36 -0600, Brandon Brnich a écrit :
> Hi Jackson and Nicolas,
> 
> On 12/11/2025 9:04 AM, Nicolas Dufresne wrote:
> > Hi,
> > 
> > Le mardi 02 décembre 2025 à 02:06 +0000, jackson.lee a écrit :
> > > Hi Brandon
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Brandon Brnich <b-brnich@ti.com>
> > > > Sent: Thursday, November 20, 2025 6:32 AM
> > > > To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
> > > > <jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>;
> > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Nicolas
> > > > Dufresne <nicolas.dufresne@collabora.com>
> > > > Cc: Darren Etheridge <detheridge@ti.com>; Brandon Brnich <b-brnich@ti.com>
> > > > Subject: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource
> > > > Leak
> > > > 
> > > > After kthread creation during probe sequence, a handful of other failures
> > > > could occur. If this were to happen, the kthread is never explicitly
> > > > deleted which results in a resource leak. Add explicit cleanup of this
> > > > resource.
> > > > 
> > > > Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> > > > ---
> > > > 
> > > > I am aware that all the dev attributes would be freed since it is
> > > > allocated using the devm_* framework. But I did not believe that this
> > > > framework would recursively free the thread and stop the timer. These
> > > > would just be dangling resources unable to get killed unless deliberately
> > > > removed in the probe function.
> > > > 
> > > >   drivers/media/platform/chips-media/wave5/wave5-vpu.c | 5 +++++
> > > >   1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > index e1715d3f43b0..f027b4ac775a 100644
> > > > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > @@ -339,6 +339,11 @@ static int wave5_vpu_probe(struct platform_device
> > > > *pdev)
> > > >   	v4l2_device_unregister(&dev->v4l2_dev);
> > > >   err_vdi_release:
> > > >   	wave5_vdi_release(&pdev->dev);
> > > > +
> > > > +	if (dev->irq < 0) {
> > > > +		kthread_destroy_worker(dev->worker);
> > > > +		hrtimer_cancel(&dev->hrtimer);
> > > > +	}
> > > 
> > > I'd like to change the above to as below.
> > > I think we have to distinguish failure between registering IRQ handler and
> > > registering v4l2_device_register.
> > > 
> > > err_irq_release:
> > > 	if (dev->irq < 0) {
> > > 		kthread_destroy_worker(dev->worker);
> > > 		hrtimer_cancel(&dev->hrtimer);
> > > 	}
> > > err_vdi_release:
> > 
> > That's seems more then just a suggestion, I see that err_vdi_release: is reached
> > on worker creation failure. Checking the kthread code, this will cause a use
> > after free instead of a leak.
> 
> Agreed with all above statements. I will update to fix use after free 
> that I introduced in v1.
> 
> > 
> > An additional question, aren't we are supposed to also cleanup irq_thread ? We
> > have this code being introduced in the remove function now:
> > 
> > 
> > 		if (dev->irq_thread) {
> > 			kthread_stop(dev->irq_thread);
> > 			up(&dev->irq_sem);
> > 			dev->irq_thread = NULL;
> > 		}
> 
> This portion of code is being introduced in Jackson's performance 
> series. I did not base my patch on this series since it hasn't been 
> accepted yet. I assumed my patch would make it in before since this is 
> easier to review than that series. Apologies if I need to base on that 
> series. Can rebase this in v2 if requested.
> 
> Otherwise, I suggest Jackson to add irq_thread cleanup in next iteration 
> of performance series.

I see, this is good point. I discourage writing code against my upcoming PR
branch, its not a proper tree, but for this one you may just base you patch
against it, since it will all be sent together ideally.

https://gitlab.freedesktop.org/linux-media/users/ndufresne/-/tree/for-6.20

regards,
Nicolas

> 
> Best,
> Brandon
> 
> > 
> > 
> > 
> > regards,
> > Nicolas
> > 
> > 
> > > 
> > > thanks
> > > Jackson
> > > 
> > > 
> > > >   err_clk_dis:
> > > >   	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> > > >   err_reset_assert:
> > > > --
> > > > 2.34.1
> > > 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] media: chips-media: wave5: Fix Potential Probe Resource Leak

[Brandon Brnich]

Hi,

On 12/11/2025 3:51 PM, Nicolas Dufresne wrote:
> Le jeudi 11 décembre 2025 à 15:36 -0600, Brandon Brnich a écrit :
>>>>
>>>> err_irq_release:
>>>> 	if (dev->irq < 0) {
>>>> 		kthread_destroy_worker(dev->worker);
>>>> 		hrtimer_cancel(&dev->hrtimer);
>>>> 	}
>>>> err_vdi_release:
>>>
>>> That's seems more then just a suggestion, I see that err_vdi_release: is reached
>>> on worker creation failure. Checking the kthread code, this will cause a use
>>> after free instead of a leak.
>>
>> Agreed with all above statements. I will update to fix use after free
>> that I introduced in v1.
>>
>>>
>>> An additional question, aren't we are supposed to also cleanup irq_thread ? We
>>> have this code being introduced in the remove function now:
>>>
>>>
>>> 		if (dev->irq_thread) {
>>> 			kthread_stop(dev->irq_thread);
>>> 			up(&dev->irq_sem);
>>> 			dev->irq_thread = NULL;
>>> 		}
>>
>> This portion of code is being introduced in Jackson's performance
>> series. I did not base my patch on this series since it hasn't been
>> accepted yet. I assumed my patch would make it in before since this is
>> easier to review than that series. Apologies if I need to base on that
>> series. Can rebase this in v2 if requested.
>>
>> Otherwise, I suggest Jackson to add irq_thread cleanup in next iteration
>> of performance series.
> 
> I see, this is good point. I discourage writing code against my upcoming PR
> branch, its not a proper tree, but for this one you may just base you patch
> against it, since it will all be sent together ideally.
> 
> https://gitlab.freedesktop.org/linux-media/users/ndufresne/-/tree/for-6.20

Will do. Since I am rebasing on this tree I will also just handle the 
irq thread cleanup in my v2.

Best,
Brandon

> 
> regards,
> Nicolas
>