[PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Joe Hattori]

In gpiochip_setup_dev(), the refcount incremented in device_initialize()
is not decremented in the error path. Fix it by calling put_device().

Fixes: aab5c6f20023 ("gpio: set device type for GPIO chips")
Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
 drivers/gpio/gpiolib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 679ed764cb14..6b2d50370ab7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -800,7 +800,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 
 	ret = gcdev_register(gdev, gpio_devt);
 	if (ret)
-		return ret;
+		goto err_put_device;
 
 	ret = gpiochip_sysfs_register(gdev);
 	if (ret)
@@ -813,6 +813,8 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 
 err_remove_device:
 	gcdev_unregister(gdev);
+err_put_device:
+	put_device(&gdev->dev);
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Linus Walleij]

On Wed, Dec 4, 2024 at 1:21 PM Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> wrote:

> In gpiochip_setup_dev(), the refcount incremented in device_initialize()
> is not decremented in the error path. Fix it by calling put_device().
>
> Fixes: aab5c6f20023 ("gpio: set device type for GPIO chips")
> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>

Cc: stable@vger.kernel.org
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 04 Dec 2024 21:21:52 +0900, Joe Hattori wrote:
> In gpiochip_setup_dev(), the refcount incremented in device_initialize()
> is not decremented in the error path. Fix it by calling put_device().
> 
> 

Applied, thanks!

[1/1] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()
      commit: f81934e9457799e3b8381de2fc75b96fd1498a65

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Andy Shevchenko]

On Wed, Dec 04, 2024 at 09:21:52PM +0900, Joe Hattori wrote:
> In gpiochip_setup_dev(), the refcount incremented in device_initialize()
> is not decremented in the error path. Fix it by calling put_device().

First of all, we have gpio_put_device().
Second, what the problem do you have in practice? Can you show any backtrace?
Third, how had this change been tested?

Looking at the current code I noticed the following:
1) gpiochip_add_data_with_key() has already that call;
2) gpiochip_setup_devs() misses that call.

This effectively means that you inroduce a regression while fixing a bug.

The GPIO device initialisation is non-trivial, please pay more attention to the
code.

Bart, can this be removed or reverted before it poisons stable?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Andy Shevchenko]

On Thu, Dec 05, 2024 at 03:20:51PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 04, 2024 at 09:21:52PM +0900, Joe Hattori wrote:
> > In gpiochip_setup_dev(), the refcount incremented in device_initialize()
> > is not decremented in the error path. Fix it by calling put_device().
> 
> First of all, we have gpio_put_device().

Should be gpio_device_put().

> Second, what the problem do you have in practice? Can you show any backtrace?
> Third, how had this change been tested?
> 
> Looking at the current code I noticed the following:
> 1) gpiochip_add_data_with_key() has already that call;
> 2) gpiochip_setup_devs() misses that call.
> 
> This effectively means that you inroduce a regression while fixing a bug.
> 
> The GPIO device initialisation is non-trivial, please pay more attention to the
> code.
> 
> Bart, can this be removed or reverted before it poisons stable?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Bartosz Golaszewski]

On Thu, Dec 5, 2024 at 2:20 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Dec 04, 2024 at 09:21:52PM +0900, Joe Hattori wrote:
> > In gpiochip_setup_dev(), the refcount incremented in device_initialize()
> > is not decremented in the error path. Fix it by calling put_device().
>
> First of all, we have gpio_put_device().
> Second, what the problem do you have in practice? Can you show any backtrace?
> Third, how had this change been tested?
>
> Looking at the current code I noticed the following:
> 1) gpiochip_add_data_with_key() has already that call;
> 2) gpiochip_setup_devs() misses that call.
>
> This effectively means that you inroduce a regression while fixing a bug.
>
> The GPIO device initialisation is non-trivial, please pay more attention to the
> code.
>
> Bart, can this be removed or reverted before it poisons stable?
>

Yes, sorry, should have paid more attention.

Bartosz

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Joe Hattori]

Thank you for the review. And yes, I should have paid more attention to 
the code, sorry. I have reflected the reviewed points in the V2 patch, 
so please take a look at it.

On 12/5/24 22:20, Andy Shevchenko wrote:
 > On Wed, Dec 04, 2024 at 09:21:52PM +0900, Joe Hattori wrote:
 >> In gpiochip_setup_dev(), the refcount incremented in device_initialize()
 >> is not decremented in the error path. Fix it by calling put_device().
 >
 > First of all, we have gpio_put_device().

Fixed in the V2 patch.

 > Second, what the problem do you have in practice? Can you show any 
backtrace?
 > Third, how had this change been tested?

I am building a static analysis tool, and it reported this reference 
leak. I overlooked that it still reported the same error after this 
patch, and it is fixed in the V2 patch. If a backtrace is needed, I will 
try.

 >
 > Looking at the current code I noticed the following:
 > 1) gpiochip_add_data_with_key() has already that call;

Thank you for pointing out, the call has been removed in the V2 patch.
 > 2) gpiochip_setup_devs() misses that call.

I was not sure if the gpiochip_setup_devs() should have an error path to 
remove all the registered GPIO devices when a single registration fails, 
thus it is not addressed in the V2 patch. If such a cleanup path is 
needed, please let me know.

 >
 > This effectively means that you inroduce a regression while fixing a bug.
 >
 > The GPIO device initialisation is non-trivial, please pay more 
attention to the
 > code.
 >
 > Bart, can this be removed or reverted before it poisons stable?
 >
Best,
Joe

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Andy Shevchenko]

On Fri, Dec 06, 2024 at 05:53:25PM +0900, Joe Hattori wrote:
> Thank you for the review. And yes, I should have paid more attention to the
> code, sorry. I have reflected the reviewed points in the V2 patch, so please
> take a look at it.

I can't look at something I haven't received.

> On 12/5/24 22:20, Andy Shevchenko wrote:
> > On Wed, Dec 04, 2024 at 09:21:52PM +0900, Joe Hattori wrote:
> >> In gpiochip_setup_dev(), the refcount incremented in device_initialize()
> >> is not decremented in the error path. Fix it by calling put_device().
> >
> > First of all, we have gpio_put_device().
> 
> Fixed in the V2 patch.
> 
> > Second, what the problem do you have in practice? Can you show any
> backtrace?
> > Third, how had this change been tested?
> 
> I am building a static analysis tool, and it reported this reference leak. I
> overlooked that it still reported the same error after this patch, and it is
> fixed in the V2 patch. If a backtrace is needed, I will try.

You missed the guidelines / rules about static analysis tools:
Documentation/process/researcher-guidelines.rst.

> > Looking at the current code I noticed the following:
> > 1) gpiochip_add_data_with_key() has already that call;
> 
> Thank you for pointing out, the call has been removed in the V2 patch.
> > 2) gpiochip_setup_devs() misses that call.
> 
> I was not sure if the gpiochip_setup_devs() should have an error path to
> remove all the registered GPIO devices when a single registration fails,

I don't think so. But that failed device should be put. It's the only issue
I see with the current code. If I'm wrong, prove it and elaborate in the commit
message (backtraces will be very helpful).

> thus it is not addressed in the V2 patch. If such a cleanup path is needed,
> please let me know.
> 
> > This effectively means that you inroduce a regression while fixing a bug.
> >
> > The GPIO device initialisation is non-trivial, please pay more attention
> to the
> > code.
> >
> > Bart, can this be removed or reverted before it poisons stable?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Dan Carpenter]

Most static checker stuff is cleanup code.  I've written a blog entry on
clean up code.

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

You mostly get this right, but one thing which could help is rule #6
where ideally the cleanup in the function is copy and pasted to create
the release function.

So when you add cleanup to the error paths, check the release function
and see what it does.  Sometimes you've seen the clean up code is missing
in the release function.  Other times it calls wrapper like
gpio_put_device() instead of calling put_device() directly.  The function
calls should be in a specific order, (hopefully the reverse order of how
they're allocated).

regards,
dan carpenter