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

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

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