gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
Subsystems (2)
PATCH v3 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.
---
v3:
- Remove the parentheses in the commit message.
- Add code comment before setting the device type.
- Add R-b tag.
v2: https://lore.kernel.org/all/[email protected]
- 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 | 101 ++++++++++++++++++++--------------------- 1 file changed, 48 insertions(+), 53 deletions(-)
drivers/gpio/gpiolib.c +48 -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,72 @@ 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); +/* +* After this point any allocated resources to `gdev` will be +* free():ed by gpiodev_release(). If you add new resources +* then make sure they get free():ed there. +*/ + 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 +1133,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 +1153,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 +1250,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
Bartosz Golaszewski
maintainerQuoted 21 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 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.Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Some initialization mainly on
gdev.
The rest of initialization. Go to ERR ZONE 2 on errors. Chip registration success and exit.
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
ERR ZONE 2.
gpio_device_put()and exit.
ERR ZONE 1.Fixes aab5c6f20023 ("gpio: set device type for GPIO chips")Reviewed-by Linus Walleij <[email protected]>Signed-off-by Tzung-Bi Shih <[email protected]>---
This looks good, I will queue this right after v7.0-rc1 is tagged because this is way too risky for rc8. Let's let it spend a couple weeks in next.
Bart
Quoted 10 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:[...]
Hi Tzung-Bi!
For now I queued this for v7.1. I want it to spend some time in linux-next as I think it's a high-risk change. However, I preemptively put it into an immutable branch - if all is good for three weeks or so, I'll send it upstream for v7.0.
I see you added a Fixes tag dating back to 2024 and Cc'ed stable. I'm afraid this change will not apply very far back, do you plan to backport it all the way to v6.8?
[1/1] gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
https://git.kernel.org/brgl/c/16fdabe143fce2cbf89139677728e17e21b46c28
Best regards,
Signature · 1 line
Bartosz Golaszewski <[email protected]>
Quoted 15 lines · Bartosz Golaszewski
Quoted 10 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:[...]
Hi Tzung-Bi!
Quoted 8 lines
For now I queued this for v7.1. I want it to spend some time in linux-next as I think it's a high-risk change. However, I preemptively put it into an immutable branch - if all is good for three weeks or so, I'll send it upstream for v7.0.
I see you added a Fixes tag dating back to 2024 and Cc'ed stable. I'm afraid this change will not apply very far back, do you plan to backport it all the way to v6.8?
Probably no, I think the patch won't apply very far back as well. I added the Fixes tag for providing context about the fix target.