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

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

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