Skip to content

Commit 16fdabe

Browse files
Tzung-Bi ShihBartosz Golaszewski
authored andcommitted
gpio: Fix resource leaks on errors in gpiochip_add_data_with_key()
Since commit aab5c6f ("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: aab5c6f ("gpio: set device type for GPIO chips") Reviewed-by: Linus Walleij <linusw@kernel.org> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org> Link: https://patch.msgid.link/20260205092840.2574840-1-tzungbi@kernel.org Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
1 parent 6de23f8 commit 16fdabe

1 file changed

Lines changed: 48 additions & 53 deletions

File tree

drivers/gpio/gpiolib.c

Lines changed: 48 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -892,13 +892,15 @@ static const struct device_type gpio_dev_type = {
892892
#define gcdev_unregister(gdev) device_del(&(gdev)->dev)
893893
#endif
894894

895+
/*
896+
* An initial reference count has been held in gpiochip_add_data_with_key().
897+
* The caller should drop the reference via gpio_device_put() on errors.
898+
*/
895899
static int gpiochip_setup_dev(struct gpio_device *gdev)
896900
{
897901
struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
898902
int ret;
899903

900-
device_initialize(&gdev->dev);
901-
902904
/*
903905
* If fwnode doesn't belong to another device, it's safe to clear its
904906
* initialized flag.
@@ -964,9 +966,11 @@ static void gpiochip_setup_devs(void)
964966
list_for_each_entry_srcu(gdev, &gpio_devices, list,
965967
srcu_read_lock_held(&gpio_devices_srcu)) {
966968
ret = gpiochip_setup_dev(gdev);
967-
if (ret)
969+
if (ret) {
970+
gpio_device_put(gdev);
968971
dev_err(&gdev->dev,
969972
"Failed to initialize gpio device (%d)\n", ret);
973+
}
970974
}
971975
}
972976

@@ -1047,71 +1051,72 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
10471051
int base = 0;
10481052
int ret;
10491053

1050-
/*
1051-
* First: allocate and populate the internal stat container, and
1052-
* set up the struct device.
1053-
*/
10541054
gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
10551055
if (!gdev)
10561056
return -ENOMEM;
1057-
1058-
gdev->dev.type = &gpio_dev_type;
1059-
gdev->dev.bus = &gpio_bus_type;
1060-
gdev->dev.parent = gc->parent;
1061-
rcu_assign_pointer(gdev->chip, gc);
1062-
10631057
gc->gpiodev = gdev;
10641058
gpiochip_set_data(gc, data);
10651059

1066-
device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
1067-
10681060
ret = ida_alloc(&gpio_ida, GFP_KERNEL);
10691061
if (ret < 0)
10701062
goto err_free_gdev;
10711063
gdev->id = ret;
10721064

1073-
ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
1065+
ret = init_srcu_struct(&gdev->srcu);
10741066
if (ret)
10751067
goto err_free_ida;
1068+
rcu_assign_pointer(gdev->chip, gc);
10761069

1077-
if (gc->parent && gc->parent->driver)
1078-
gdev->owner = gc->parent->driver->owner;
1079-
else if (gc->owner)
1080-
/* TODO: remove chip->owner */
1081-
gdev->owner = gc->owner;
1082-
else
1083-
gdev->owner = THIS_MODULE;
1070+
ret = init_srcu_struct(&gdev->desc_srcu);
1071+
if (ret)
1072+
goto err_cleanup_gdev_srcu;
1073+
1074+
ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
1075+
if (ret)
1076+
goto err_cleanup_desc_srcu;
1077+
1078+
device_initialize(&gdev->dev);
1079+
/*
1080+
* After this point any allocated resources to `gdev` will be
1081+
* free():ed by gpiodev_release(). If you add new resources
1082+
* then make sure they get free():ed there.
1083+
*/
1084+
gdev->dev.type = &gpio_dev_type;
1085+
gdev->dev.bus = &gpio_bus_type;
1086+
gdev->dev.parent = gc->parent;
1087+
device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
10841088

10851089
ret = gpiochip_get_ngpios(gc, &gdev->dev);
10861090
if (ret)
1087-
goto err_free_dev_name;
1091+
goto err_put_device;
1092+
gdev->ngpio = gc->ngpio;
10881093

10891094
gdev->descs = kcalloc(gc->ngpio, sizeof(*gdev->descs), GFP_KERNEL);
10901095
if (!gdev->descs) {
10911096
ret = -ENOMEM;
1092-
goto err_free_dev_name;
1097+
goto err_put_device;
10931098
}
10941099

10951100
gdev->label = kstrdup_const(gc->label ?: "unknown", GFP_KERNEL);
10961101
if (!gdev->label) {
10971102
ret = -ENOMEM;
1098-
goto err_free_descs;
1103+
goto err_put_device;
10991104
}
11001105

1101-
gdev->ngpio = gc->ngpio;
11021106
gdev->can_sleep = gc->can_sleep;
1103-
11041107
rwlock_init(&gdev->line_state_lock);
11051108
RAW_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
11061109
BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
1107-
1108-
ret = init_srcu_struct(&gdev->srcu);
1109-
if (ret)
1110-
goto err_free_label;
1111-
1112-
ret = init_srcu_struct(&gdev->desc_srcu);
1113-
if (ret)
1114-
goto err_cleanup_gdev_srcu;
1110+
#ifdef CONFIG_PINCTRL
1111+
INIT_LIST_HEAD(&gdev->pin_ranges);
1112+
#endif
1113+
if (gc->parent && gc->parent->driver)
1114+
gdev->owner = gc->parent->driver->owner;
1115+
else if (gc->owner)
1116+
/* TODO: remove chip->owner */
1117+
gdev->owner = gc->owner;
1118+
else
1119+
gdev->owner = THIS_MODULE;
11151120

11161121
scoped_guard(mutex, &gpio_devices_lock) {
11171122
/*
@@ -1127,7 +1132,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
11271132
if (base < 0) {
11281133
ret = base;
11291134
base = 0;
1130-
goto err_cleanup_desc_srcu;
1135+
goto err_put_device;
11311136
}
11321137

11331138
/*
@@ -1147,14 +1152,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
11471152
ret = gpiodev_add_to_list_unlocked(gdev);
11481153
if (ret) {
11491154
gpiochip_err(gc, "GPIO integer space overlap, cannot add chip\n");
1150-
goto err_cleanup_desc_srcu;
1155+
goto err_put_device;
11511156
}
11521157
}
11531158

1154-
#ifdef CONFIG_PINCTRL
1155-
INIT_LIST_HEAD(&gdev->pin_ranges);
1156-
#endif
1157-
11581159
if (gc->names)
11591160
gpiochip_set_desc_names(gc);
11601161

@@ -1248,25 +1249,19 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
12481249
scoped_guard(mutex, &gpio_devices_lock)
12491250
list_del_rcu(&gdev->list);
12501251
synchronize_srcu(&gpio_devices_srcu);
1251-
if (gdev->dev.release) {
1252-
/* release() has been registered by gpiochip_setup_dev() */
1253-
gpio_device_put(gdev);
1254-
goto err_print_message;
1255-
}
1252+
err_put_device:
1253+
gpio_device_put(gdev);
1254+
goto err_print_message;
1255+
12561256
err_cleanup_desc_srcu:
12571257
cleanup_srcu_struct(&gdev->desc_srcu);
12581258
err_cleanup_gdev_srcu:
12591259
cleanup_srcu_struct(&gdev->srcu);
1260-
err_free_label:
1261-
kfree_const(gdev->label);
1262-
err_free_descs:
1263-
kfree(gdev->descs);
1264-
err_free_dev_name:
1265-
kfree(dev_name(&gdev->dev));
12661260
err_free_ida:
12671261
ida_free(&gpio_ida, gdev->id);
12681262
err_free_gdev:
12691263
kfree(gdev);
1264+
12701265
err_print_message:
12711266
/* failures here can mean systems won't boot... */
12721267
if (ret != -EPROBE_DEFER) {

0 commit comments

Comments
 (0)