| 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?
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(-)