| Message ID | 20221118022236.871576-1-zengheng4@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] gpiolib: fix memory leak in gpiochip_setup_dev() | expand |
On Fri, Nov 18, 2022 at 10:22:36AM +0800, Zeng Heng wrote: > Here is a backtrace 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 You missed my comment here about extra space. > (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(). ... > +static void gpiochip_print_register_fail(struct gpio_chip *gc, > + struct gpio_device *gdev, > + const char *func, int ret) > +{ > + /* failures here can mean systems won't boot... */ > + if (ret != -EPROBE_DEFER) { Wouldn't the following work better? if (ret == -EPROBE_DEFER) return; /* failures here can mean systems won't boot... */ pr_err(...); > + pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", func, > + gdev->base, gdev->base + gdev->ngpio - 1, > + gc->label ? : "generic", ret); > + } > +} ... > err_free_gpiochip_mask: > gpiochip_remove_pin_ranges(gc); > gpiochip_free_valid_mask(gc); > + if (gdev->dev.release) { > + /* release() has been registered by gpiochip_setup_dev() */ This comment is most probably in a wrong line and should be one line below. > + gpiochip_print_register_fail(gc, gdev, __func__, ret); > + put_device(&gdev->dev); > + return ret; > + } ... > err_free_gdev: > - /* failures here can mean systems won't boot... */ > - if (ret != -EPROBE_DEFER) { > - pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, > - gdev->base, gdev->base + gdev->ngpio - 1, > - gc->label ? : "generic", ret); > - } > + gpiochip_print_register_fail(gc, gdev, __func__, ret); > kfree(gdev); > return ret; Now it looks cleaner, but why you can't use the same return point with the message? What you need is to neep the range on the stack (which is almost there). ... Okay, let's leave this for a while, I will think how it can be improved and then I come up with particular suggestions.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 4756ea08894f..c7a55f4f7ef6 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"); @@ -590,6 +591,18 @@ static void gpiochip_setup_devs(void) } } +static void gpiochip_print_register_fail(struct gpio_chip *gc, + struct gpio_device *gdev, + const char *func, int ret) +{ + /* failures here can mean systems won't boot... */ + if (ret != -EPROBE_DEFER) { + pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", func, + gdev->base, gdev->base + gdev->ngpio - 1, + gc->label ? : "generic", ret); + } +} + int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, struct lock_class_key *lock_key, struct lock_class_key *request_key) @@ -816,6 +829,12 @@ 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) { + /* release() has been registered by gpiochip_setup_dev() */ + gpiochip_print_register_fail(gc, gdev, __func__, ret); + put_device(&gdev->dev); + return ret; + } err_remove_from_list: spin_lock_irqsave(&gpio_lock, flags); list_del(&gdev->list); @@ -829,12 +848,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, err_free_ida: ida_free(&gpio_ida, gdev->id); err_free_gdev: - /* failures here can mean systems won't boot... */ - if (ret != -EPROBE_DEFER) { - pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, - gdev->base, gdev->base + gdev->ngpio - 1, - gc->label ? : "generic", ret); - } + gpiochip_print_register_fail(gc, gdev, __func__, ret); kfree(gdev); return ret; }
Here is a backtrace 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> --- changes in v4: - add gpiochip_print_register_fail() changes in v3: - use put_device() instead of kfree() explicitly changes in v2: - correct fixes tag --- drivers/gpio/gpiolib.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)