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

[Brandon Brnich <b-brnich@ti.com>]

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