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@xxxxxx>
> > > Sent: Thursday, November 20, 2025 6:32 AM
> > > To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; jackson.lee
> > > <jackson.lee@xxxxxxxxxxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>;
> > > linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Nicolas
> > > Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> > > Cc: Darren Etheridge <detheridge@xxxxxx>; Brandon Brnich <b-brnich@xxxxxx>
> > > 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@xxxxxx>
> > > ---
> > >
> > > 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