Skip to contentSkip to conversation

gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()

PATCH In review Tzung-Bi Shih 3 participants 4 messages Feb 5 – Feb 28

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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.

Fixes aab5c6f20023 ("gpio: set device type for GPIO chips")

---
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
index c52200eaaaff..5757c0475990 100644
@@ -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
#1 lore raw
Quoted 21 lines · Tzung-Bi Shih

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

---

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

#2 lore raw
In reply to Tzung-Bi Shih
Quoted 10 lines · Tzung-Bi Shih

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:

[...]

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]>
#3 lore raw
Quoted 15 lines · Bartosz Golaszewski
Quoted 10 lines · Tzung-Bi Shih

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:

[...]

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.