| Message ID | 20221117090247.122980-1-zengheng4@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] gpiolib: fix memory leak in gpiochip_setup_dev() | expand |
On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote: > Here is a report about memory leak detected in gpiochip_setup_dev(): > > unreferenced object 0xffff88810b406400 (size 512): > comm "python3", pid 1682, jiffies 4295346908 (age 24.090s) > backtrace: > kmalloc_trace > device_add device_private_init at drivers/base/core.c:3361 Seems like unneeded space after device_add. Also note, we refer to the functions as func(). > (inlined by) device_add at drivers/base/core.c:3411 > cdev_device_add > gpiolib_cdev_register > gpiochip_setup_dev > gpiochip_add_data_with_key > > gcdev_register() & gcdev_unregister() would call device_add() & > device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to > register/unregister device. > > However, if device_add() succeeds, some resource (like > struct device_private allocated by device_private_init()) > is not released by device_del(). > > Therefore, after device_add() succeeds by gcdev_register(), it > needs to call put_device() to release resource in the error handle > path. > > Here we move forward the register of release function, and let it > release every piece of resource by put_device() instead of kfree(). > > Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization") > Signed-off-by: Zeng Heng <zengheng4@huawei.com> > --- Where is changelog since we see this as v3? ... > err_free_gpiochip_mask: > gpiochip_remove_pin_ranges(gc); > gpiochip_free_valid_mask(gc); > + /* > + * If gdev->dev.release has been registered by > + * gpiochip_setup_dev(), print err msg and > + * call put_device() to release all. > + */ > + if (gdev->dev.release) > + goto err_free_gdev; (1) > err_remove_from_list: > spin_lock_irqsave(&gpio_lock, flags); > list_del(&gdev->list); ... > - kfree(gdev); > + if (gdev->dev.release) > + put_device(&gdev->dev); Why you can't do this above at (1)? Is there any other hidden way to get here with release set? > + else > + kfree(gdev); > return ret;
On 2022/11/17 18:49, Andy Shevchenko wrote: > On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote: >> Here is a report about memory leak detected in gpiochip_setup_dev(): >> >> unreferenced object 0xffff88810b406400 (size 512): >> comm "python3", pid 1682, jiffies 4295346908 (age 24.090s) >> backtrace: >> kmalloc_trace >> device_add device_private_init at drivers/base/core.c:3361 > Seems like unneeded space after device_add. Also note, we refer to > the functions as func(). Just emphasize the location of memory leak happened. >> (inlined by) device_add at drivers/base/core.c:3411 >> cdev_device_add >> gpiolib_cdev_register >> gpiochip_setup_dev >> gpiochip_add_data_with_key >> >> gcdev_register() & gcdev_unregister() would call device_add() & >> device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to >> register/unregister device. >> >> However, if device_add() succeeds, some resource (like >> struct device_private allocated by device_private_init()) >> is not released by device_del(). >> >> Therefore, after device_add() succeeds by gcdev_register(), it >> needs to call put_device() to release resource in the error handle >> path. >> >> Here we move forward the register of release function, and let it >> release every piece of resource by put_device() instead of kfree(). >> >> Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization") >> Signed-off-by: Zeng Heng <zengheng4@huawei.com> >> --- > Where is changelog since we see this as v3? > > ... change in v3: - use put_device() instead of kfree() >> err_free_gpiochip_mask: >> gpiochip_remove_pin_ranges(gc); >> gpiochip_free_valid_mask(gc); >> + /* >> + * If gdev->dev.release has been registered by >> + * gpiochip_setup_dev(), print err msg and >> + * call put_device() to release all. >> + */ >> + if (gdev->dev.release) >> + goto err_free_gdev; > (1) > >> err_remove_from_list: >> spin_lock_irqsave(&gpio_lock, flags); >> list_del(&gdev->list); > ... > >> - kfree(gdev); >> + if (gdev->dev.release) >> + put_device(&gdev->dev); > Why you can't do this above at (1)? > Is there any other hidden way to get here with release set? As already mentioned in the mail, keep the error print info. B.R.， Zeng Heng >> + else >> + kfree(gdev); >> return ret;
On Thu, Nov 17, 2022 at 10:12:31PM +0800, Zeng Heng wrote: > On 2022/11/17 18:49, Andy Shevchenko wrote: > > On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote: ... > > > + /* > > > + * If gdev->dev.release has been registered by > > > + * gpiochip_setup_dev(), print err msg and > > > + * call put_device() to release all. > > > + */ > > > + if (gdev->dev.release) > > > + goto err_free_gdev; > > (1) > > > > > err_remove_from_list: > > > spin_lock_irqsave(&gpio_lock, flags); > > > list_del(&gdev->list); > > ... > > > > > - kfree(gdev); > > > + if (gdev->dev.release) > > > + put_device(&gdev->dev); > > Why you can't do this above at (1)? > > Is there any other hidden way to get here with release set? > > As already mentioned in the mail, keep the error print info. Can you refactor that to avoid double condition on the ->release() presence? > > > + else > > > + kfree(gdev); > > > return ret;
On 2022/11/17 23:31, Andy Shevchenko wrote: > On Thu, Nov 17, 2022 at 10:12:31PM +0800, Zeng Heng wrote: >> On 2022/11/17 18:49, Andy Shevchenko wrote: >>> On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote: > ... > >>>> + /* >>>> + * If gdev->dev.release has been registered by >>>> + * gpiochip_setup_dev(), print err msg and >>>> + * call put_device() to release all. >>>> + */ >>>> + if (gdev->dev.release) >>>> + goto err_free_gdev; >>> (1) >>> >>>> err_remove_from_list: >>>> spin_lock_irqsave(&gpio_lock, flags); >>>> list_del(&gdev->list); >>> ... >>> >>>> - kfree(gdev); >>>> + if (gdev->dev.release) >>>> + put_device(&gdev->dev); >>> Why you can't do this above at (1)? >>> Is there any other hidden way to get here with release set? >> As already mentioned in the mail, keep the error print info. > Can you refactor that to avoid double condition on the ->release() presence? Sure. Zeng Heng > >>>> + else >>>> + kfree(gdev); >>>> return ret;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 4756ea08894f..bb4dedc154b7 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -526,12 +526,13 @@ static int gpiochip_setup_dev(struct gpio_device *gdev) if (ret) return ret; + /* From this point, the .release() function cleans up gpio_device */ + gdev->dev.release = gpiodevice_release; + ret = gpiochip_sysfs_register(gdev); if (ret) goto err_remove_device; - /* From this point, the .release() function cleans up gpio_device */ - gdev->dev.release = gpiodevice_release; dev_dbg(&gdev->dev, "registered GPIOs %d to %d on %s\n", gdev->base, gdev->base + gdev->ngpio - 1, gdev->chip->label ? : "generic"); @@ -816,6 +817,13 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, err_free_gpiochip_mask: gpiochip_remove_pin_ranges(gc); gpiochip_free_valid_mask(gc); + /* + * If gdev->dev.release has been registered by + * gpiochip_setup_dev(), print err msg and + * call put_device() to release all. + */ + if (gdev->dev.release) + goto err_free_gdev; err_remove_from_list: spin_lock_irqsave(&gpio_lock, flags); list_del(&gdev->list); @@ -835,7 +843,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, gdev->base, gdev->base + gdev->ngpio - 1, gc->label ? : "generic", ret); } - kfree(gdev); + if (gdev->dev.release) + put_device(&gdev->dev); + else + kfree(gdev); return ret; } EXPORT_SYMBOL_GPL(gpiochip_add_data_with_key);
Here is a report about memory leak detected in gpiochip_setup_dev(): unreferenced object 0xffff88810b406400 (size 512): comm "python3", pid 1682, jiffies 4295346908 (age 24.090s) backtrace: kmalloc_trace device_add device_private_init at drivers/base/core.c:3361 (inlined by) device_add at drivers/base/core.c:3411 cdev_device_add gpiolib_cdev_register gpiochip_setup_dev gpiochip_add_data_with_key gcdev_register() & gcdev_unregister() would call device_add() & device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to register/unregister device. However, if device_add() succeeds, some resource (like struct device_private allocated by device_private_init()) is not released by device_del(). Therefore, after device_add() succeeds by gcdev_register(), it needs to call put_device() to release resource in the error handle path. Here we move forward the register of release function, and let it release every piece of resource by put_device() instead of kfree(). Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization") Signed-off-by: Zeng Heng <zengheng4@huawei.com> --- drivers/gpio/gpiolib.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)