| Message ID | 20241204122152.1312051-1-joe@pf.is.s.u-tokyo.ac.jp |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev() | expand |
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,
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?
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?
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
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
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?
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
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; }
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(-)