1. Patchew
  2. linux
  3. View series

[PATCH v2] gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()

Tzung-Bi Shih posted 1 patch 4 days, 9 hours ago
There is a newer version of this series
drivers/gpio/gpiolib.c | 96 +++++++++++++++++++-----------------------
1 file changed, 43 insertions(+), 53 deletions(-)
[PATCH v2] gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
Posted by Tzung-Bi Shih 4 days, 9 hours ago 
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:

   >>> 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.)

   >>> 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.

Cc: stable@vger.kernel.org
Fixes: aab5c6f20023 ("gpio: set device type for GPIO chips")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v2:
- Reorder the instructions again to make the error handling simpler which
  fixes https://lore.kernel.org/all/20260116081036.352286-2-tzungbi@kernel.org
  too.
- Modify the commit message slightly.

v1: https://lore.kernel.org/all/20260116081036.352286-4-tzungbi@kernel.org

 drivers/gpio/gpiolib.c | 96 +++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 53 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c52200eaaaff..039cd3e56baf 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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) {
-- 
2.53.0.rc2.204.g2597b5adb4-goog
Re: [PATCH v2] gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
Posted by Linus Walleij 4 days, 6 hours ago 
On Tue, Feb 3, 2026 at 7:02 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:

> 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:
>
>    >>> 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.)

This doesn't need to be parenthesized, it should be pointed out
so people can follow it, also:

> @@ -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;


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:
Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij
Re: [PATCH v2] gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
Posted by Tzung-Bi Shih 2 days, 6 hours ago 
On Tue, Feb 03, 2026 at 10:35:14AM +0100, Linus Walleij wrote:
> On Tue, Feb 3, 2026 at 7:02 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > Also reorder the instructions to make the error handling simpler.
> > Now gpiochip_add_data_with_key() roughly looks like:
> >
> >    >>> 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.)
> 
> This doesn't need to be parenthesized, it should be pointed out
> so people can follow it, also:
> 
[...]
> > +       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:
> Reviewed-by: Linus Walleij <linusw@kernel.org>

Thanks for your review.  Fix both of them in v3
https://lore.kernel.org/all/20260205092840.2574840-1-tzungbi@kernel.org/.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.