[PATCH 0/6] gpio: fix gpio_chip add and remove

[PATCH 0/6] gpio: fix gpio_chip add and remove

[Johan Hovold]

This series fix a few issues with gpiochip_add and gpiochip_remove.

The gpio-chip removal paths have probably not been exercised much, and
there are further issues here that I'm working on fixing in order better
support hot-plugging of gpio chips. I believe these patches should go
into 3.19 meanwhile.

Johan


Johan Hovold (6):
  gpio: fix memory and reference leaks in gpiochip_add error path
  gpio: fix gpio-chip list corruption
  gpio: clean up gpiochip_add error handling
  gpio: fix memory leak and sleep-while-atomic
  gpio: fix sleep-while-atomic in gpiochip_remove
  gpio: unregister gpiochip device before removing it

 drivers/gpio/gpiolib.c | 58 ++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

-- 
2.0.5

[PATCH 1/6] gpio: fix memory and reference leaks in gpiochip_add error path

[Johan Hovold]

Memory allocated and references taken by of_gpiochip_add and
acpi_gpiochip_add were never released on errors in gpiochip_add (e.g.
failure to find free gpio range).

Fixes: 391c970c0dd1 ("of/gpio: add default of_xlate function if device
has a node pointer")
Fixes: 664e3e5ac64c ("gpio / ACPI: register to ACPI events
automatically")
Cc: stable <stable@vger.kernel.org>

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 487afe6f22fc..89c59f5f1924 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -277,6 +277,9 @@ int gpiochip_add(struct gpio_chip *chip)
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
+	if (status)
+		goto fail;
+
 #ifdef CONFIG_PINCTRL
 	INIT_LIST_HEAD(&chip->pin_ranges);
 #endif
@@ -284,12 +287,12 @@ int gpiochip_add(struct gpio_chip *chip)
 	of_gpiochip_add(chip);
 	acpi_gpiochip_add(chip);
 
-	if (status)
-		goto fail;
-
 	status = gpiochip_export(chip);
-	if (status)
+	if (status) {
+		acpi_gpiochip_remove(chip);
+		of_gpiochip_remove(chip);
 		goto fail;
+	}
 
 	pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__,
 		chip->base, chip->base + chip->ngpio - 1,
-- 
2.0.5

[PATCH 2/6] gpio: fix gpio-chip list corruption

[Johan Hovold]

Fix potential corruption of gpio-chip list due to failure to remove the
chip from the list before returning in gpiochip_add error path.

The chip could be long gone when the global list is next traversed,
something which could lead to a null-pointer dereference. In the best
case (chip not deallocated) we are just leaking the gpio range.

Fixes: 14e85c0e69d5 ("gpio: remove gpio_descs global array")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 89c59f5f1924..ac5944b4e4d8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -248,7 +248,8 @@ int gpiochip_add(struct gpio_chip *chip)
 		base = gpiochip_find_base(chip->ngpio);
 		if (base < 0) {
 			status = base;
-			goto unlock;
+			spin_unlock_irqrestore(&gpio_lock, flags);
+			goto err_free_descs;
 		}
 		chip->base = base;
 	}
@@ -288,11 +289,8 @@ int gpiochip_add(struct gpio_chip *chip)
 	acpi_gpiochip_add(chip);
 
 	status = gpiochip_export(chip);
-	if (status) {
-		acpi_gpiochip_remove(chip);
-		of_gpiochip_remove(chip);
-		goto fail;
-	}
+	if (status)
+		goto err_remove_chip;
 
 	pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__,
 		chip->base, chip->base + chip->ngpio - 1,
@@ -300,9 +298,14 @@ int gpiochip_add(struct gpio_chip *chip)
 
 	return 0;
 
-unlock:
+err_remove_chip:
+	acpi_gpiochip_remove(chip);
+	of_gpiochip_remove(chip);
+	spin_lock_irqsave(&gpio_lock, flags);
+	list_del(&chip->list);
 	spin_unlock_irqrestore(&gpio_lock, flags);
 fail:
+err_free_descs:
 	kfree(descs);
 	chip->desc = NULL;
 
-- 
2.0.5

[PATCH 3/6] gpio: clean up gpiochip_add error handling

[Johan Hovold]

Clean up gpiochip_add error handling.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ac5944b4e4d8..4efb92ca3463 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -255,32 +255,29 @@ int gpiochip_add(struct gpio_chip *chip)
 	}
 
 	status = gpiochip_add_to_list(chip);
+	if (status) {
+		spin_unlock_irqrestore(&gpio_lock, flags);
+		goto err_free_descs;
+	}
 
-	if (status == 0) {
-		for (id = 0; id < chip->ngpio; id++) {
-			struct gpio_desc *desc = &descs[id];
-			desc->chip = chip;
-
-			/* REVISIT:  most hardware initializes GPIOs as
-			 * inputs (often with pullups enabled) so power
-			 * usage is minimized.  Linux code should set the
-			 * gpio direction first thing; but until it does,
-			 * and in case chip->get_direction is not set,
-			 * we may expose the wrong direction in sysfs.
-			 */
-			desc->flags = !chip->direction_input
-				? (1 << FLAG_IS_OUT)
-				: 0;
-		}
+	for (id = 0; id < chip->ngpio; id++) {
+		struct gpio_desc *desc = &descs[id];
+
+		desc->chip = chip;
+
+		/* REVISIT: most hardware initializes GPIOs as inputs (often
+		 * with pullups enabled) so power usage is minimized. Linux
+		 * code should set the gpio direction first thing; but until
+		 * it does, and in case chip->get_direction is not set, we may
+		 * expose the wrong direction in sysfs.
+		 */
+		desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0;
 	}
 
 	chip->desc = descs;
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
-	if (status)
-		goto fail;
-
 #ifdef CONFIG_PINCTRL
 	INIT_LIST_HEAD(&chip->pin_ranges);
 #endif
@@ -304,10 +301,9 @@ err_remove_chip:
 	spin_lock_irqsave(&gpio_lock, flags);
 	list_del(&chip->list);
 	spin_unlock_irqrestore(&gpio_lock, flags);
-fail:
+	chip->desc = NULL;
 err_free_descs:
 	kfree(descs);
-	chip->desc = NULL;
 
 	/* failures here can mean systems won't boot... */
 	pr_err("%s: GPIOs %d..%d (%s) failed to register\n", __func__,
-- 
2.0.5

[PATCH 4/6] gpio: fix memory leak and sleep-while-atomic

[Johan Hovold]

Fix memory leak and sleep-while-atomic in gpiochip_remove.

The memory leak was introduced by afa82fab5e13 ("gpio / ACPI: Move event
handling registration to gpiolib irqchip helpers") that moved the
release of acpi interrupt resources to gpiochip_irqchip_remove, but by
then the resources are no longer accessible as the acpi_gpio_chip has
already been freed by acpi_gpiochip_remove.

Note that this also fixes a few potential sleep-while-atomics, which has
been around since 1425052097b5 ("gpio: add IRQ chip helpers in gpiolib")
when the call to gpiochip_irqchip_remove while holding a spinlock was
added (a couple of irq-domain paths can end up grabbing mutexes).

Fixes: afa82fab5e13 ("gpio / ACPI: Move event handling registration to
gpiolib irqchip helpers")
Fixes: 1425052097b5 ("gpio: add IRQ chip helpers in gpiolib")
Cc: stable <stable@vger.kernel.org>

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4efb92ca3463..0f8173051edc 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -327,11 +327,12 @@ void gpiochip_remove(struct gpio_chip *chip)
 	unsigned long	flags;
 	unsigned	id;
 
+	gpiochip_irqchip_remove(chip);
+
 	acpi_gpiochip_remove(chip);
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	gpiochip_irqchip_remove(chip);
 	gpiochip_remove_pin_ranges(chip);
 	of_gpiochip_remove(chip);
 
-- 
2.0.5

[PATCH 5/6] gpio: fix sleep-while-atomic in gpiochip_remove

[Johan Hovold]

Move direct and indirect calls to gpiochip_remove_pin_ranges outside of
spin lock as they can end up taking a mutex in pinctrl_remove_gpio_range.

Note that the pin ranges are already added outside of the lock.

Fixes: 9ef0d6f7628b ("gpiolib: call pin removal in chip removal function")
Fixes: f23f1516b675 ("gpiolib: provide provision to register pin ranges")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0f8173051edc..37f919dc2cb4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -330,12 +330,10 @@ void gpiochip_remove(struct gpio_chip *chip)
 	gpiochip_irqchip_remove(chip);
 
 	acpi_gpiochip_remove(chip);
-
-	spin_lock_irqsave(&gpio_lock, flags);
-
 	gpiochip_remove_pin_ranges(chip);
 	of_gpiochip_remove(chip);
 
+	spin_lock_irqsave(&gpio_lock, flags);
 	for (id = 0; id < chip->ngpio; id++) {
 		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
 			dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
-- 
2.0.5

[PATCH 6/6] gpio: unregister gpiochip device before removing it

[Johan Hovold]

Unregister gpiochip device (used to export information through sysfs)
before removing it internally. This way removal will reverse addition.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 37f919dc2cb4..568aa2b6bdb0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -327,6 +327,8 @@ void gpiochip_remove(struct gpio_chip *chip)
 	unsigned long	flags;
 	unsigned	id;
 
+	gpiochip_unexport(chip);
+
 	gpiochip_irqchip_remove(chip);
 
 	acpi_gpiochip_remove(chip);
@@ -343,7 +345,6 @@ void gpiochip_remove(struct gpio_chip *chip)
 
 	list_del(&chip->list);
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	gpiochip_unexport(chip);
 
 	kfree(chip->desc);
 	chip->desc = NULL;
-- 
2.0.5

Re: [PATCH 0/6] gpio: fix gpio_chip add and remove

[Linus Walleij]

On Mon, Jan 12, 2015 at 5:12 PM, Johan Hovold <johan@kernel.org> wrote:

> This series fix a few issues with gpiochip_add and gpiochip_remove.
>
> The gpio-chip removal paths have probably not been exercised much, and
> there are further issues here that I'm working on fixing in order better
> support hot-plugging of gpio chips. I believe these patches should go
> into 3.19 meanwhile.

GOOD STUFF!
All six applied for fixes.

Yours,
Linus Walleij