gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
Subsystems (2)
PATCH v2 1/1 gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
Since commit aab5c6f20023 ("gpio: set device type for GPIO chips"), gdev->dev.release is unset. As a result, the reference count to gdev->dev isn't dropped on the error handling paths.
Drop the reference on errors.
Also reorder the instructions to make the error handling simpler. Now gpiochip_add_data_with_key() roughly looks like:
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Some memory allocation. Go to ERR ZONE 1 on errors.
device_initialize().
(gpiodev_release() takes over the responsibility for freeing the
resources of gdev->dev. The subsequent error handling paths shouldn't go through ERR ZONE 1 again which leads to double free.)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Some initialization mainly on
gdev.
The rest of initialization. Go to ERR ZONE 2 on errors. Chip registration success and exit.ERR ZONE 2.
gpio_device_put()and exit.
ERR ZONE 1.
---
v2:
- Reorder the instructions again to make the error handling simpler which fixes https://lore.kernel.org/all/[email protected]
too.
- Modify the commit message slightly.
v1: https://lore.kernel.org/all/[email protected]
drivers/gpio/gpiolib.c | 96 +++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 53 deletions(-)
drivers/gpio/gpiolib.c +43 -53
@@ -893,13 +893,15 @@ static const struct device_type gpio_dev_type = {#define gcdev_unregister (gdev) device_del(&(gdev)->dev)#endif +/* +* An initial reference count has been held in gpiochip_add_data_with_key(). +* The caller should drop the reference via gpio_device_put() on errors. +*/ static int gpiochip_setup_dev (struct gpio_device *gdev) {struct fwnode_handle *fwnode =dev_fwnode (&gdev->dev);int ret; -device_initialize (&gdev->dev); -/* * If fwnode doesn't belong to another device, it's safe to clear its * initialized flag.
@@ -965,9 +967,11 @@ static void gpiochip_setup_devs(void)list_for_each_entry_srcu (gdev, &gpio_devices, list,srcu_read_lock_held (&gpio_devices_srcu)) { ret =gpiochip_setup_dev (gdev); -if (ret) +if (ret) { +gpio_device_put (gdev);dev_err (&gdev->dev,"Failed to initialize gpio device (%d)\n" , ret); + } } }
@@ -1048,71 +1052,67 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,int base =0 ;int ret; -/* -* First: allocate and populate the internal stat container, and -* set up the struct device. -*/ gdev =kzalloc (sizeof (*gdev),GFP_KERNEL );if (!gdev)return -ENOMEM ; - - gdev->dev.type = &gpio_dev_type; - gdev->dev.bus = &gpio_bus_type; - gdev->dev.parent = gc->parent; -rcu_assign_pointer (gdev->chip, gc); - gc->gpiodev = gdev;gpiochip_set_data (gc, data); -device_set_node (&gdev->dev,gpiochip_choose_fwnode (gc)); - ret =ida_alloc (&gpio_ida,GFP_KERNEL );if (ret <0 ) goto err_free_gdev; gdev->id = ret; - ret =dev_set_name (&gdev->dev,GPIOCHIP_NAME "%d" , gdev->id); + ret =init_srcu_struct (&gdev->srcu);if (ret) goto err_free_ida; +rcu_assign_pointer (gdev->chip, gc); -if (gc->parent && gc->parent->driver) - gdev->owner = gc->parent->driver->owner; -else if (gc->owner) -/* TODO: remove chip->owner */ - gdev->owner = gc->owner; -else - gdev->owner =THIS_MODULE ; + ret =init_srcu_struct (&gdev->desc_srcu); +if (ret) + goto err_cleanup_gdev_srcu; + + ret =dev_set_name (&gdev->dev,GPIOCHIP_NAME "%d" , gdev->id); +if (ret) + goto err_cleanup_desc_srcu; + +device_initialize (&gdev->dev); + gdev->dev.type = &gpio_dev_type; + gdev->dev.bus = &gpio_bus_type; + gdev->dev.parent = gc->parent; +device_set_node (&gdev->dev,gpiochip_choose_fwnode (gc)); ret =gpiochip_get_ngpios (gc, &gdev->dev);if (ret) - goto err_free_dev_name; + goto err_put_device; + gdev->ngpio = gc->ngpio; gdev->descs =kcalloc (gc->ngpio,sizeof (*gdev->descs),GFP_KERNEL );if (!gdev->descs) { ret = -ENOMEM ; - goto err_free_dev_name; + goto err_put_device; } gdev->label =kstrdup_const (gc->label ?:"unknown" ,GFP_KERNEL );if (!gdev->label) { ret = -ENOMEM ; - goto err_free_descs; + goto err_put_device; } - gdev->ngpio = gc->ngpio; gdev->can_sleep = gc->can_sleep; -rwlock_init (&gdev->line_state_lock);RAW_INIT_NOTIFIER_HEAD (&gdev->line_state_notifier);BLOCKING_INIT_NOTIFIER_HEAD (&gdev->device_notifier); - - ret =init_srcu_struct (&gdev->srcu); -if (ret) - goto err_free_label; - - ret =init_srcu_struct (&gdev->desc_srcu); -if (ret) - goto err_cleanup_gdev_srcu; +#ifdef CONFIG_PINCTRL +INIT_LIST_HEAD (&gdev->pin_ranges); +#endif +if (gc->parent && gc->parent->driver) + gdev->owner = gc->parent->driver->owner; +else if (gc->owner) +/* TODO: remove chip->owner */ + gdev->owner = gc->owner; +else + gdev->owner =THIS_MODULE ; scoped_guard(mutex, &gpio_devices_lock) {/*
@@ -1128,7 +1128,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,if (base <0 ) { ret = base; base =0 ; - goto err_cleanup_desc_srcu; + goto err_put_device; }/*
@@ -1148,14 +1148,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, ret =gpiodev_add_to_list_unlocked (gdev);if (ret) {gpiochip_err (gc,"GPIO integer space overlap, cannot add chip\n" ); - goto err_cleanup_desc_srcu; + goto err_put_device; } } -#ifdef CONFIG_PINCTRL -INIT_LIST_HEAD (&gdev->pin_ranges); -#endif -if (gc->names)gpiochip_set_desc_names (gc);
@@ -1249,25 +1245,19 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, scoped_guard(mutex, &gpio_devices_lock)list_del_rcu (&gdev->list);synchronize_srcu (&gpio_devices_srcu); -if (gdev->dev.release) { -/* release() has been registered by gpiochip_setup_dev() */ -gpio_device_put (gdev); - goto err_print_message; - } +err_put_device: +gpio_device_put (gdev); + goto err_print_message; + err_cleanup_desc_srcu:cleanup_srcu_struct (&gdev->desc_srcu); err_cleanup_gdev_srcu:cleanup_srcu_struct (&gdev->srcu); -err_free_label: -kfree_const (gdev->label); -err_free_descs: -kfree (gdev->descs); -err_free_dev_name: -kfree (dev_name (&gdev->dev)); err_free_ida:ida_free (&gpio_ida, gdev->id); err_free_gdev:kfree (gdev); + err_print_message:/* failures here can mean systems won't boot... */ if (ret != -EPROBE_DEFER ) {
Signature · 1 line
2.53.0.rc2.204.g2597b5adb4-goog
Quoted 9 lines · Tzung-Bi Shih
Since commit
aab5c6f20023("gpio: set device type for GPIO chips"),gdev->dev.releaseis unset. As a result, the reference count togdev->devisn't dropped on the error handling paths.Drop the reference on errors.
Also reorder the instructions to make the error handling simpler. Now
gpiochip_add_data_with_key()roughly looks like:
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Some memory allocation. Go to ERR ZONE 1 on errors.
device_initialize().(
gpiodev_release()takes over the responsibility for freeing the
resources ofgdev->dev. The subsequent error handling paths shouldn't go through ERR ZONE 1 again which leads to double free.)
This doesn't need to be parenthesized, it should be pointed out so people can follow it, also:
Quoted 40 lines
drivers/gpio/gpiolib.c +2
-20@@ -1048,71 +1052,67 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,int base =0 ;int ret; -/* -* First: allocate and populate the internal stat container, and -* set up the struct device. -*/ gdev =kzalloc (sizeof (*gdev),GFP_KERNEL );if (!gdev)return -ENOMEM ; - - gdev->dev.type = &gpio_dev_type; - gdev->dev.bus = &gpio_bus_type; - gdev->dev.parent = gc->parent; -rcu_assign_pointer (gdev->chip, gc); - gc->gpiodev = gdev;gpiochip_set_data (gc, data); -device_set_node (&gdev->dev,gpiochip_choose_fwnode (gc)); - ret =ida_alloc (&gpio_ida,GFP_KERNEL );if (ret <0 ) goto err_free_gdev; gdev->id = ret; - ret =dev_set_name (&gdev->dev,GPIOCHIP_NAME "%d" , gdev->id); + ret =init_srcu_struct (&gdev->srcu);if (ret) goto err_free_ida; +rcu_assign_pointer (gdev->chip, gc); -if (gc->parent && gc->parent->driver) - gdev->owner = gc->parent->driver->owner; -else if (gc->owner) -/* TODO: remove chip->owner */ - gdev->owner = gc->owner; -else - gdev->owner =THIS_MODULE ;
drivers/gpio/gpiolib.c int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, · line 1080
Quoted 10 lines
drivers/gpio/gpiolib.c +10
-0+ ret =init_srcu_struct (&gdev->desc_srcu); +if (ret) + goto err_cleanup_gdev_srcu; + + ret =dev_set_name (&gdev->dev,GPIOCHIP_NAME "%d" , gdev->id); +if (ret) + goto err_cleanup_desc_srcu; + +device_initialize (&gdev->dev); + gdev->dev.type = &gpio_dev_type;
This is the point where, from this point on, gpiodev_release() will get called from the type.
This is worth a comment in the code don't you think?
Something like:
/*
* After this point any allocated resources will be free():d by
* gpiodev_release(), if you add new resources then make sure
* they get free():ed there.
*/gdev->dev.type = &gpio_dev_type;
This helps developers to do the right thing.
With that added comment:
Yours,
Linus Walleij
Quoted 14 lines · Linus Walleij
Quoted 9 lines · Tzung-Bi Shih
Also reorder the instructions to make the error handling simpler. Now
gpiochip_add_data_with_key()roughly looks like:Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Some memory allocation. Go to ERR ZONE 1 on errors.
device_initialize().(
gpiodev_release()takes over the responsibility for freeing the
resources ofgdev->dev. The subsequent error handling paths shouldn't go through ERR ZONE 1 again which leads to double free.)This doesn't need to be parenthesized, it should be pointed out so people can follow it, also:
[...]
Quoted 18 lines
drivers/gpio/gpiolib.c int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, · line 1080
Quoted 2 lines
drivers/gpio/gpiolib.c +2
-0+device_initialize (&gdev->dev); + gdev->dev.type = &gpio_dev_type;This is the point where, from this point on,
gpiodev_release()will get called from the type.This is worth a comment in the code don't you think?
Something like:
/*
* After this point any allocated resources will be free():d by
*gpiodev_release(), if you add new resources then make sure
* they get free():ed there.
*/gdev->dev.type=&gpio_dev_type;
Quoted 4 lines
This helps developers to do the right thing.
With that added comment:
Reviewed-by Linus Walleij <[email protected]>
Thanks for your review. Fix both of them in v3
https://lore.kernel.org/all/[email protected]/.