gpiochip_setup_dev() calls gpio_device_put() on error when
gdev->dev.release is set, but this field is always empty as the cleanup
function moved to gpio_dev_type in commit aab5c6f20023 ("gpio: set
device type for GPIO chips"), resulting in a memory leak. Call
gpio_device_put() at the end of the gpiochip_setup_dev() to trigger the
resource cleanup, and remove the GPIO device from gpio_devices. Because
of this operation, gpiochip_setup_devs() may modify gpio_devices, so
acquire a mutex instead of a read lock.
This bug was found by an experimental verification tool that I am
developing. Tested the behavior with gpio-sim and confirmed basic
operations on GPIO devices worked as intended without any error logs.
Fixes: aab5c6f20023 ("gpio: set device type for GPIO chips")
Signed-off-by: Joe Hattori <joe@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/gpio/gpiolib.c | 42 ++++++++++++++++++++++++------------------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 679ed764cb14..1f612a54a475 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -784,7 +784,7 @@ static const struct device_type gpio_dev_type = {
#define gcdev_unregister(gdev) device_del(&(gdev)->dev)
#endif
-static int gpiochip_setup_dev(struct gpio_device *gdev)
+static int gpiochip_setup_dev(struct gpio_device *gdev, bool locked)
{
struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
int ret;
@@ -800,7 +800,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
ret = gcdev_register(gdev, gpio_devt);
if (ret)
- return ret;
+ goto err_put_device;
ret = gpiochip_sysfs_register(gdev);
if (ret)
@@ -813,6 +813,14 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
err_remove_device:
gcdev_unregister(gdev);
+err_put_device:
+ if (locked)
+ list_del_rcu(&gdev->list);
+ else
+ scoped_guard(mutex, &gpio_devices_lock)
+ list_del_rcu(&gdev->list);
+ synchronize_srcu(&gpio_devices_srcu);
+ gpio_device_put(gdev);
return ret;
}
@@ -850,17 +858,15 @@ static void machine_gpiochip_add(struct gpio_chip *gc)
static void gpiochip_setup_devs(void)
{
- struct gpio_device *gdev;
+ struct gpio_device *gdev, *tmp;
int ret;
- guard(srcu)(&gpio_devices_srcu);
-
- list_for_each_entry_srcu(gdev, &gpio_devices, list,
- srcu_read_lock_held(&gpio_devices_srcu)) {
- ret = gpiochip_setup_dev(gdev);
- if (ret)
- dev_err(&gdev->dev,
- "Failed to initialize gpio device (%d)\n", ret);
+ scoped_guard(mutex, &gpio_devices_lock) {
+ list_for_each_entry_safe(gdev, tmp, &gpio_devices, list) {
+ ret = gpiochip_setup_dev(gdev, true);
+ if (ret)
+ pr_err("Failed to initialize gpio device (%d)\n", ret);
+ }
}
}
@@ -921,6 +927,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
struct lock_class_key *request_key)
{
struct gpio_device *gdev;
+ bool already_put = false;
unsigned int desc_index;
int base = 0;
int ret = 0;
@@ -1098,9 +1105,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
* Otherwise, defer until later.
*/
if (gpiolib_initialized) {
- ret = gpiochip_setup_dev(gdev);
- if (ret)
+ ret = gpiochip_setup_dev(gdev, false);
+ if (ret) {
+ already_put = true;
goto err_remove_irqchip;
+ }
}
return 0;
@@ -1116,6 +1125,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
of_gpiochip_remove(gc);
err_free_valid_mask:
gpiochip_free_valid_mask(gc);
+ if (already_put)
+ goto err_print_message;
err_cleanup_desc_srcu:
cleanup_srcu_struct(&gdev->desc_srcu);
err_cleanup_gdev_srcu:
@@ -1124,11 +1135,6 @@ 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_free_label:
kfree_const(gdev->label);
err_free_descs:
