gpiolib: fix reference leaks when removing GPIO chips still in use

PATCH In review Bartosz Golaszewski 3 participants 10 messages Aug 11, 2023 – Aug 16, 2023 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2023-08-11 19:30 UTC #0 lore raw

PATCH v3 1/1 gpiolib: fix reference leaks when removing GPIO chips still in use

From: Bartosz Golaszewski <[email protected]>

After we remove a GPIO chip that still has some requested descriptors, gpiod_free_commit() will fail and we will never put the references to the GPIO device and the owning module in gpiod_free().

Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally so that the release callback actually gets called when the remaining references are dropped by external GPIO users

Signed-off-by Bartosz Golaszewski <[email protected]>

---
v1 -> v2:
- add a comment about why we can't use VALIDATE_DESC_VOID()

v2 -> v3:
- we must drop the reference to the owner module before we drop the one to the gpio_device as the latter may be removed if this is the last reference and we'll end up calling module_put() on freed memory

 drivers/gpio/gpiolib.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/gpio/gpiolib.c +11 -5

index 251c875b5c34..76e0c38026c3 100644
@@ -2167,12 +2167,18 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 
 void gpiod_free(struct gpio_desc *desc)
 {
-	if (desc && desc->gdev && gpiod_free_commit(desc)) {
-		module_put(desc->gdev->owner);
-		gpio_device_put(desc->gdev);
-	} else {
+	/*
+	 * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
+	 * may already be NULL but we still want to put the references.
+	 */
+	if (!desc)
+		return;
+
+	if (!gpiod_free_commit(desc))
 		WARN_ON(extra_checks);
-	}
+
+	module_put(desc->gdev->owner);
+	gpio_device_put(desc->gdev);
 }
 
 /**

Signature · 1 line

2.39.2

Linus Walleij

Reviewed-by on v3 1/1

2023-08-15 08:26 UTC #1 lore raw

Quoted 14 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
After we remove a GPIO chip that still has some requested descriptors, gpiod_free_commit() will fail and we will never put the references to the GPIO device and the owning module in gpiod_free().
Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally so that the release callback actually gets called when the remaining references are dropped by external GPIO users
Signed-off-by Bartosz Golaszewski <[email protected]>

Quoted 8 lines

---
v1 -> v2:
- add a comment about why we can't use VALIDATE_DESC_VOID()
v2 -> v3:
- we must drop the reference to the owner module before we drop the one to the gpio_device as the latter may be removed if this is the last reference and we'll end up calling module_put() on freed memory

v3 looks good to me.

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Andy Shevchenko

2023-08-15 09:50 UTC #2 lore raw

In reply to Bartosz Golaszewski

Quoted 12 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
After we remove a GPIO chip that still has some requested descriptors, gpiod_free_commit() will fail and we will never put the references to the GPIO device and the owning module in gpiod_free().
Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally so that the release callback actually gets called when the remaining references are dropped by external GPIO users

...

drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2170

Quoted 1 line

drivers/gpio/gpiolib.c +0 -1

-	if (desc && desc->gdev && gpiod_free_commit(desc)) {

The commit message doesn't explain disappearing of gdev check.

Quoted 7 lines

drivers/gpio/gpiolib.c +4 -3

-		module_put(desc->gdev->owner);
-		gpio_device_put(desc->gdev);
-	} else {
+	/*
+	 * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
+	 * may already be NULL but we still want to put the references.
+	 */

drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2182

Quoted 10 lines

drivers/gpio/gpiolib.c +7 -1

+	if (!desc)
+		return;
+
+	if (!gpiod_free_commit(desc))
 		WARN_ON(extra_checks);
-	}
+
+	module_put(desc->gdev->owner);
+	gpio_device_put(desc->gdev);
 }

So, if gdev can be NULL, you will get an Oops with new code.

To keep a status quo this needs to be rewritten.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2023-08-15 11:41 UTC #3 lore raw

Quoted 31 lines · Andy Shevchenko

Quoted 12 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
After we remove a GPIO chip that still has some requested descriptors, gpiod_free_commit() will fail and we will never put the references to the GPIO device and the owning module in gpiod_free().
Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally so that the release callback actually gets called when the remaining references are dropped by external GPIO users
...
Quoted 1 line
- if (desc && desc->gdev && gpiod_free_commit(desc)) {
The commit message doesn't explain disappearing of gdev check.
Quoted 11 lines
drivers/gpio/gpiolib.c +8 -3
-             module_put(desc->gdev->owner);
-             gpio_device_put(desc->gdev);
-     } else {
+     /*
+      * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
+      * may already be NULL but we still want to put the references.
+      */
+     if (!desc)
+             return;
+
+     if (!gpiod_free_commit(desc))

Quoted 8 lines

drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2182
Quoted 6 lines
              WARN_ON(extra_checks);
drivers/gpio/gpiolib.c +3 -1
-     }
+
+     module_put(desc->gdev->owner);
+     gpio_device_put(desc->gdev);
 }
So, if gdev can be NULL, you will get an Oops with new code.

I read it such that gdev->chip can be NULL, but not gdev, and desc->gdev->owner is fine to reference?

Yours,
Linus Walleij

Andy Shevchenko

2023-08-15 12:58 UTC #4 lore raw

Quoted 15 lines · Linus Walleij

Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
After we remove a GPIO chip that still has some requested descriptors, gpiod_free_commit() will fail and we will never put the references to the GPIO device and the owning module in gpiod_free().
Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally so that the release callback actually gets called when the remaining references are dropped by external GPIO users

...

Quoted 24 lines

Quoted 15 lines
Quoted 1 line
- if (desc && desc->gdev && gpiod_free_commit(desc)) {
The commit message doesn't explain disappearing of gdev check.
Quoted 11 lines
drivers/gpio/gpiolib.c +8 -3
-             module_put(desc->gdev->owner);
-             gpio_device_put(desc->gdev);
-     } else {
+     /*
+      * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
+      * may already be NULL but we still want to put the references.
+      */
+     if (!desc)
+             return;
+
+     if (!gpiod_free_commit(desc))
Quoted 8 lines
drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2182
Quoted 6 lines
              WARN_ON(extra_checks);
drivers/gpio/gpiolib.c +3 -1
-     }
+
+     module_put(desc->gdev->owner);
+     gpio_device_put(desc->gdev);
 }
So, if gdev can be NULL, you will get an Oops with new code.

Quoted 2 lines

I read it such that gdev->chip can be NULL, but not gdev, and desc->gdev->owner is fine to reference?

Basically the Q is
"if desc is non-NULL, does it guarantee that gdev is non-NULL either?"

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2023-08-15 13:08 UTC #5 lore raw

Quoted 43 lines · Andy Shevchenko

Quoted 15 lines · Linus Walleij
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
After we remove a GPIO chip that still has some requested descriptors, gpiod_free_commit() will fail and we will never put the references to the GPIO device and the owning module in gpiod_free().
Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally so that the release callback actually gets called when the remaining references are dropped by external GPIO users
...
Quoted 24 lines
Quoted 23 lines
Quoted 1 line
- if (desc && desc->gdev && gpiod_free_commit(desc)) {
The commit message doesn't explain disappearing of gdev check.
Quoted 7 lines
drivers/gpio/gpiolib.c +4 -3
-             module_put(desc->gdev->owner);
-             gpio_device_put(desc->gdev);
-     } else {
+     /*
+      * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
+      * may already be NULL but we still want to put the references.
+      */
drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2182
Quoted 10 lines
drivers/gpio/gpiolib.c +7 -1
+     if (!desc)
+             return;
+
+     if (!gpiod_free_commit(desc))
              WARN_ON(extra_checks);
-     }
+
+     module_put(desc->gdev->owner);
+     gpio_device_put(desc->gdev);
 }
So, if gdev can be NULL, you will get an Oops with new code.

Quoted 5 lines

Quoted 2 lines
I read it such that gdev->chip can be NULL, but not gdev, and desc->gdev->owner is fine to reference?
Basically the Q is
"if desc is non-NULL, does it guarantee that gdev is non-NULL either?"

gdev->desc is assigned in one single spot, which is in gpiochip_add_data_with_key():

       for (i = 0; i < gc->ngpio; i++)
                gdev->descs[i].gdev = gdev;

It is never assigned anywhere else, so I guess yes.

We may also ask if it is ever invalid (i.e. if desc->gdev can point to junk).

A gdev turns to junk when its reference count goes down to zero and gpiodev_release() is called effectively calling kfree() on the struct gpio_device *.

But that can only happen as a result of module_put() getting called, pulling the references down to zero. Which is what we are discussing. The line after module_put(), desc->gdev *could* be NULL.

But then we just call gpio_device_put(desc->gdev) which is just a call to device_put(), which is NULL-tolerant.

Yours,
Linus Walleij

Andy Shevchenko

2023-08-15 14:44 UTC #6 lore raw

Quoted 6 lines · Linus Walleij

Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Linus Walleij
Quoted 1 line · Andy Shevchenko
On Fri, Aug 11, 2023 at 09:30:34PM +0200, Bartosz Golaszewski wrote:

...

Quoted 22 lines

Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2181
Quoted 2 lines
drivers/gpio/gpiolib.c +2 -0
+     module_put(desc->gdev->owner);
+     gpio_device_put(desc->gdev);
So, if gdev can be NULL, you will get an Oops with new code.
I read it such that gdev->chip can be NULL, but not gdev, and desc->gdev->owner is fine to reference?
Basically the Q is
"if desc is non-NULL, does it guarantee that gdev is non-NULL either?"
gdev->desc is assigned in one single spot, which is in gpiochip_add_data_with_key():
       for (i = 0; i < gc->ngpio; i++)
                gdev->descs[i].gdev = gdev;
It is never assigned anywhere else, so I guess yes.
We may also ask if it is ever invalid (i.e. if desc->gdev can point to junk).

Quoted 8 lines

A gdev turns to junk when its reference count goes down to zero and gpiodev_release() is called effectively calling kfree() on the struct gpio_device *.
But that can only happen as a result of module_put() getting called, pulling the references down to zero. Which is what we are discussing. The line after module_put(), desc->gdev *could* be NULL.

Yes.

Quoted 2 lines

But then we just call gpio_device_put(desc->gdev) which is just a call to device_put(), which is NULL-tolerant.

But gpio_device_put() does not NULL tolerant.
So, oops in this line then.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2023-08-15 14:46 UTC #7 lore raw

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Linus Walleij
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Linus Walleij
Quoted 1 line · Andy Shevchenko
On Fri, Aug 11, 2023 at 09:30:34PM +0200, Bartosz Golaszewski wrote:

...

Quoted 31 lines

Quoted 22 lines
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2181
Quoted 2 lines
drivers/gpio/gpiolib.c +2 -0
+     module_put(desc->gdev->owner);
+     gpio_device_put(desc->gdev);
So, if gdev can be NULL, you will get an Oops with new code.
I read it such that gdev->chip can be NULL, but not gdev, and desc->gdev->owner is fine to reference?
Basically the Q is
"if desc is non-NULL, does it guarantee that gdev is non-NULL either?"
gdev->desc is assigned in one single spot, which is in gpiochip_add_data_with_key():
       for (i = 0; i < gc->ngpio; i++)
                gdev->descs[i].gdev = gdev;
It is never assigned anywhere else, so I guess yes.
We may also ask if it is ever invalid (i.e. if desc->gdev can point to junk).
Quoted 8 lines
A gdev turns to junk when its reference count goes down to zero and gpiodev_release() is called effectively calling kfree() on the struct gpio_device *.
But that can only happen as a result of module_put() getting called, pulling the references down to zero. Which is what we are discussing. The line after module_put(), desc->gdev *could* be NULL.

Quoted 7 lines

Yes.
Quoted 2 lines
But then we just call gpio_device_put(desc->gdev) which is just a call to device_put(), which is NULL-tolerant.
But gpio_device_put() does not NULL tolerant.
So, oops in this line then.

That said, this gpiod_free() function needs a lot of additional comments to explain all this.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2023-08-15 18:37 UTC #8 lore raw

In reply to Andy Shevchenko

Quoted 42 lines · Andy Shevchenko

Quoted 6 lines · Linus Walleij
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Linus Walleij
Quoted 1 line · Andy Shevchenko
On Fri, Aug 11, 2023 at 09:30:34PM +0200, Bartosz Golaszewski wrote:
...
Quoted 22 lines
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
drivers/gpio/gpiolib.c static bool gpiod_free_commit(struct gpio_desc *desc) · line 2181
Quoted 2 lines
drivers/gpio/gpiolib.c +2 -0
+     module_put(desc->gdev->owner);
+     gpio_device_put(desc->gdev);
So, if gdev can be NULL, you will get an Oops with new code.
I read it such that gdev->chip can be NULL, but not gdev, and desc->gdev->owner is fine to reference?
Basically the Q is
"if desc is non-NULL, does it guarantee that gdev is non-NULL either?"
gdev->desc is assigned in one single spot, which is in gpiochip_add_data_with_key():
       for (i = 0; i < gc->ngpio; i++)
                gdev->descs[i].gdev = gdev;
It is never assigned anywhere else, so I guess yes.
We may also ask if it is ever invalid (i.e. if desc->gdev can point to junk).
Quoted 8 lines
A gdev turns to junk when its reference count goes down to zero and gpiodev_release() is called effectively calling kfree() on the struct gpio_device *.
But that can only happen as a result of module_put() getting called, pulling the references down to zero. Which is what we are discussing. The line after module_put(), desc->gdev *could* be NULL.

Quoted 8 lines

Yes.
Quoted 2 lines
But then we just call gpio_device_put(desc->gdev) which is just a call to device_put(), which is NULL-tolerant.
But gpio_device_put() does not NULL tolerant.
So, oops in this line then.

No. struct gpio_device is reference counted and as long as we get the reference counting right - the descriptor is guaranteed to hold it and only put it when it itself is being destroyed. In other words:
desc->gdev cannot be NULL here and cannot be junk. If it is, then it's a programming bug on our side and we do want to crash and burn so that we can catch and fix it.

If you think more comments are needed here, feel free to add them or I'll do it at a later time. I don't want to delay this patch any longer as it's one of the issues we need to fix to make driver unbind great again. Unless you see an issue with its logic, I want to queue it tomorrow so that it gets built in next and send it upstream by the end of this week.

Bart

Bartosz Golaszewski

author

2023-08-16 11:38 UTC #9 lore raw

In reply to Bartosz Golaszewski

Quoted 56 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
After we remove a GPIO chip that still has some requested descriptors, gpiod_free_commit() will fail and we will never put the references to the GPIO device and the owning module in gpiod_free().
Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally so that the release callback actually gets called when the remaining references are dropped by external GPIO users
Signed-off-by Bartosz Golaszewski <[email protected]>
---
v1 -> v2:
- add a comment about why we can't use VALIDATE_DESC_VOID()
v2 -> v3:
- we must drop the reference to the owner module before we drop the one to the gpio_device as the latter may be removed if this is the last reference and we'll end up calling module_put() on freed memory
 drivers/gpio/gpiolib.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/gpio/gpiolib.c +11 -6
index 251c875b5c34..76e0c38026c3 100644
@@ -2167,12 +2167,18 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 
 void gpiod_free(struct gpio_desc *desc)
 {
-       if (desc && desc->gdev && gpiod_free_commit(desc)) {
-               module_put(desc->gdev->owner);
-               gpio_device_put(desc->gdev);
-       } else {
+       /*
+        * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
+        * may already be NULL but we still want to put the references.
+        */
+       if (!desc)
+               return;
+
+       if (!gpiod_free_commit(desc))
                WARN_ON(extra_checks);
-       }
+
+       module_put(desc->gdev->owner);
+       gpio_device_put(desc->gdev);
 }
 
 /**
--

Quoted 2 lines

2.39.2

Queued for fixes.

Bart