[PATCH] watchdog: ixp4xx: fix reference leak on platform_device_register() failure

Guangshuo Li posted 1 patch 2 months ago
drivers/clocksource/timer-ixp4xx.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH] watchdog: ixp4xx: fix reference leak on platform_device_register() failure
Posted by Guangshuo Li 2 months ago
ixp4xx_timer_probe() directly returns the result of
platform_device_register(&ixp4xx_watchdog_device). When registration
fails, the embedded struct device in ixp4xx_watchdog_device has already
been initialized by device_initialize(), but the failure path does not
drop the device reference, leading to a reference leak.

The issue was identified by a static analysis tool I developed and
confirmed by manual review. Fix this by calling platform_device_put()
when platform_device_register() fails.

Fixes: 21a0a29d16c67 ("watchdog: ixp4xx: Rewrite driver to use core")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/clocksource/timer-ixp4xx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-ixp4xx.c b/drivers/clocksource/timer-ixp4xx.c
index 720ed70a2964..924dbd58c4da 100644
--- a/drivers/clocksource/timer-ixp4xx.c
+++ b/drivers/clocksource/timer-ixp4xx.c
@@ -239,11 +239,16 @@ static struct platform_device ixp4xx_watchdog_device = {
 static int ixp4xx_timer_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	int ret;
 
 	/* Pass the base address as platform data and nothing else */
 	ixp4xx_watchdog_device.dev.platform_data = local_ixp4xx_timer->base;
 	ixp4xx_watchdog_device.dev.parent = dev;
-	return platform_device_register(&ixp4xx_watchdog_device);
+	ret = platform_device_register(&ixp4xx_watchdog_device);
+	if (ret)
+		platform_device_put(&ixp4xx_watchdog_device);
+
+	return ret;
 }
 
 static const struct of_device_id ixp4xx_timer_dt_id[] = {
-- 
2.43.0
Re: [PATCH] watchdog: ixp4xx: fix reference leak on platform_device_register() failure
Posted by Linus Walleij 1 month, 3 weeks ago
Hi Guangshuo,

thanks for your patch!

On Mon, Apr 13, 2026 at 5:47 PM Guangshuo Li <lgs201920130244@gmail.com> wrote:

> ixp4xx_timer_probe() directly returns the result of
> platform_device_register(&ixp4xx_watchdog_device). When registration
> fails, the embedded struct device in ixp4xx_watchdog_device has already
> been initialized by device_initialize(), but the failure path does not
> drop the device reference, leading to a reference leak.
(...)

> -       return platform_device_register(&ixp4xx_watchdog_device);
> +       ret = platform_device_register(&ixp4xx_watchdog_device);
> +       if (ret)
> +               platform_device_put(&ixp4xx_watchdog_device);

If the problem in the description is indeed there, it seems the bug
is inside platform_device_register(), surely a function returning an
error code is supposed to clean up any resources it takes before
returning an error. It seems wrong to try to fix this in all the
consumers.

Yours,
Linus Walleij
Re: [PATCH] watchdog: ixp4xx: fix reference leak on platform_device_register() failure
Posted by Guenter Roeck 1 month, 3 weeks ago
On 4/19/26 13:22, Linus Walleij wrote:
> Hi Guangshuo,
> 
> thanks for your patch!
> 
> On Mon, Apr 13, 2026 at 5:47 PM Guangshuo Li <lgs201920130244@gmail.com> wrote:
> 
>> ixp4xx_timer_probe() directly returns the result of
>> platform_device_register(&ixp4xx_watchdog_device). When registration
>> fails, the embedded struct device in ixp4xx_watchdog_device has already
>> been initialized by device_initialize(), but the failure path does not
>> drop the device reference, leading to a reference leak.
> (...)
> 
>> -       return platform_device_register(&ixp4xx_watchdog_device);
>> +       ret = platform_device_register(&ixp4xx_watchdog_device);
>> +       if (ret)
>> +               platform_device_put(&ixp4xx_watchdog_device);
> 
> If the problem in the description is indeed there, it seems the bug
> is inside platform_device_register(), surely a function returning an
> error code is supposed to clean up any resources it takes before
> returning an error. It seems wrong to try to fix this in all the
> consumers.
> 

 From platform_device_register():

/**
  * platform_device_register - add a platform-level device
  * @pdev: platform device we're adding
  *
  * NOTE: _Never_ directly free @pdev after calling this function, even if it
  * returned an error! Always use platform_device_put() to give up the
  * reference initialised in this function instead.
  */

Not that any code actually does that as far as I can see, but isn't
the above doing exactly what the comment suggests ?

Thanks,
Guenter

Re: [PATCH] watchdog: ixp4xx: fix reference leak on platform_device_register() failure
Posted by Linus Walleij 1 month, 3 weeks ago
On Sun, Apr 19, 2026 at 11:08 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 4/19/26 13:22, Linus Walleij wrote:

> > Hi Guangshuo,
> >
> > thanks for your patch!
> >
> > On Mon, Apr 13, 2026 at 5:47 PM Guangshuo Li <lgs201920130244@gmail.com> wrote:
> >
> >> ixp4xx_timer_probe() directly returns the result of
> >> platform_device_register(&ixp4xx_watchdog_device). When registration
> >> fails, the embedded struct device in ixp4xx_watchdog_device has already
> >> been initialized by device_initialize(), but the failure path does not
> >> drop the device reference, leading to a reference leak.
> > (...)
> >
> >> -       return platform_device_register(&ixp4xx_watchdog_device);
> >> +       ret = platform_device_register(&ixp4xx_watchdog_device);
> >> +       if (ret)
> >> +               platform_device_put(&ixp4xx_watchdog_device);
> >
> > If the problem in the description is indeed there, it seems the bug
> > is inside platform_device_register(), surely a function returning an
> > error code is supposed to clean up any resources it takes before
> > returning an error. It seems wrong to try to fix this in all the
> > consumers.
> >
>
>  From platform_device_register():
>
> /**
>   * platform_device_register - add a platform-level device
>   * @pdev: platform device we're adding
>   *
>   * NOTE: _Never_ directly free @pdev after calling this function, even if it
>   * returned an error! Always use platform_device_put() to give up the
>   * reference initialised in this function instead.
>   */
>
> Not that any code actually does that as far as I can see, but isn't
> the above doing exactly what the comment suggests ?

Yeah and Johan Hovold wrote that comment and he usually knows
what he's doing so let's go with this then, I'm convinced!

Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij
Re: [PATCH] watchdog: ixp4xx: fix reference leak on platform_device_register() failure
Posted by Guangshuo Li 1 month, 3 weeks ago
Hi Linus, Guenter,

Thanks for reviewing and discussing this.

On Mon, 20 Apr 2026 at 05:34, Linus Walleij <linusw@kernel.org> wrote:
>
> On Sun, Apr 19, 2026 at 11:08 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > On 4/19/26 13:22, Linus Walleij wrote:
>
> > > Hi Guangshuo,
> > >
> > > thanks for your patch!
> > >
> > > On Mon, Apr 13, 2026 at 5:47 PM Guangshuo Li <lgs201920130244@gmail.com> wrote:
> > >
> > >> ixp4xx_timer_probe() directly returns the result of
> > >> platform_device_register(&ixp4xx_watchdog_device). When registration
> > >> fails, the embedded struct device in ixp4xx_watchdog_device has already
> > >> been initialized by device_initialize(), but the failure path does not
> > >> drop the device reference, leading to a reference leak.
> > > (...)
> > >
> > >> -       return platform_device_register(&ixp4xx_watchdog_device);
> > >> +       ret = platform_device_register(&ixp4xx_watchdog_device);
> > >> +       if (ret)
> > >> +               platform_device_put(&ixp4xx_watchdog_device);
> > >
> > > If the problem in the description is indeed there, it seems the bug
> > > is inside platform_device_register(), surely a function returning an
> > > error code is supposed to clean up any resources it takes before
> > > returning an error. It seems wrong to try to fix this in all the
> > > consumers.
> > >
> >
> >  From platform_device_register():
> >
> > /**
> >   * platform_device_register - add a platform-level device
> >   * @pdev: platform device we're adding
> >   *
> >   * NOTE: _Never_ directly free @pdev after calling this function, even if it
> >   * returned an error! Always use platform_device_put() to give up the
> >   * reference initialised in this function instead.
> >   */
> >
> > Not that any code actually does that as far as I can see, but isn't
> > the above doing exactly what the comment suggests ?
>
> Yeah and Johan Hovold wrote that comment and he usually knows
> what he's doing so let's go with this then, I'm convinced!
>
> Reviewed-by: Linus Walleij <linusw@kernel.org>
>
> Yours,
> Linus Walleij

After further checking, this patch is not appropriate for this driver.
ixp4xx_watchdog_device is a static platform_device, and it does not have
a dev.release callback. Calling platform_device_put() on the
platform_device_register() failure path can therefore trigger the missing
release callback warning.

So please disregard this patch. I will drop it and will also go back and
check the other patches I sent for the same pattern, and send follow-ups
where they should be ignored or reverted.

Sorry for the confusion, and thanks again for the review.

Best regards,
Guangshuo Li