Skip to contentSkip to conversation

gpio: regression fixes

PATCH In review Johan Hovold 5 participants 9 messages Mar 1, 2021 – Mar 2, 2021

Subsystems (2)
Stable branch GPIO
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v2 cover gpio: regression fixes

Here's a fix for a regression in 5.12 due to the new stub-driver hack, and a fix for potential list corruption due to missing locking which has been there since the introduction of the character-device interface in 4.6.

Johan

Changes in v2
- drop the corresponding drv_set_drvdata() which is no longer needed after patch 1/2
- add Saravanas's reviewed-by tag to patch 2/2

Johan Hovold (2):
gpio: fix NULL-deref-on-deregistration regression
gpio: fix gpio-device list corruption

 drivers/gpio/gpiolib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Signature · 1 line
2.26.2
author
#1 lore raw

PATCH v2 1/2 gpio: fix NULL-deref-on-deregistration regression

Fix a NULL-pointer deference when deregistering the gpio character device that was introduced by the recent stub-driver hack. When the new "driver" is unbound as part of deregistration, driver core clears the driver-data pointer which is used to retrieve the struct gpio_device in its release callback.

Fix this by using container_of() in the release callback as should have been done all along.

Fixes 4731210c09f5 ("gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default")
Cc Saravana Kannan <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reported-by [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpio/gpiolib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/gpio/gpiolib.c +1 -2
index adf55db080d8..6e0572515d02 100644
@@ -474,7 +474,7 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 
 static void gpiodevice_release(struct device *dev)
 {
-	struct gpio_device *gdev = dev_get_drvdata(dev);
+	struct gpio_device *gdev = container_of(dev, struct gpio_device, dev);
 
 	list_del(&gdev->list);
 	ida_free(&gpio_ida, gdev->id);

@@ -605,7 +605,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		goto err_free_ida;
 
 	device_initialize(&gdev->dev);
-	dev_set_drvdata(&gdev->dev, gdev);
 	if (gc->parent && gc->parent->driver)
 		gdev->owner = gc->parent->driver->owner;
 	else if (gc->owner)
Signature · 1 line
2.26.2
Reviewed-by on v2 1/2
#2 lore raw

Re: [PATCH v2 1/2] gpio: fix NULL-deref-on-deregistration regression

Quoted 6 lines · Johan Hovold

Fix a NULL-pointer deference when deregistering the gpio character device that was introduced by the recent stub-driver hack. When the new "driver" is unbound as part of deregistration, driver core clears the driver-data pointer which is used to retrieve the struct gpio_device in its release callback.

Quoted 8 lines

Fix this by using container_of() in the release callback as should have been done all along.

Fixes 4731210c09f5 ("gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default")
Cc Saravana Kannan <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Reported-by [email protected]
Signed-off-by Johan Hovold <[email protected]>

Oh nice catch!

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

author
#3 lore raw

PATCH v2 2/2 gpio: fix gpio-device list corruption

In reply to Johan Hovold

Make sure to hold the gpio_lock when removing the gpio device from the gpio_devices list (when dropping the last reference) to avoid corrupting the list when there are concurrent accesses.

Fixes ff2b13592299 ("gpio: make the gpiochip a real device")
Cc [email protected] # 4.6
Reviewed-by Saravana Kannan <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpio/gpiolib.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/gpio/gpiolib.c +4 -0
index 6e0572515d02..4253837f870b 100644
@@ -475,8 +475,12 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 static void gpiodevice_release(struct device *dev)
 {
 	struct gpio_device *gdev = container_of(dev, struct gpio_device, dev);
+	unsigned long flags;
 
+	spin_lock_irqsave(&gpio_lock, flags);
 	list_del(&gdev->list);
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
 	ida_free(&gpio_ida, gdev->id);
 	kfree_const(gdev->label);
 	kfree(gdev->descs);
Signature · 1 line
2.26.2
Reviewed-by on v2 2/2
#4 lore raw

Re: [PATCH v2 2/2] gpio: fix gpio-device list corruption

Quoted 8 lines · Johan Hovold

Make sure to hold the gpio_lock when removing the gpio device from the gpio_devices list (when dropping the last reference) to avoid corrupting the list when there are concurrent accesses.

Fixes ff2b13592299 ("gpio: make the gpiochip a real device")
Cc [email protected] # 4.6
Reviewed-by Saravana Kannan <[email protected]>
Signed-off-by Johan Hovold <[email protected]>

Excellent fix as well,

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

#5 lore raw
In reply to Johan Hovold
Quoted 18 lines · Johan Hovold

Here's a fix for a regression in 5.12 due to the new stub-driver hack, and a fix for potential list corruption due to missing locking which has been there since the introduction of the character-device interface in 4.6.

Johan

Changes in v2
- drop the corresponding drv_set_drvdata() which is no longer needed after patch 1/2
- add Saravanas's reviewed-by tag to patch 2/2

Johan Hovold (2):
gpio: fix NULL-deref-on-deregistration regression
gpio: fix gpio-device list corruption

Quoted 6 lines
 drivers/gpio/gpiolib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--
2.26.2

Patches applied, thanks!

Bartosz

Tested-by
#6 lore raw
Quoted 15 lines · Bartosz Golaszewski
Quoted 14 lines · Johan Hovold

Here's a fix for a regression in 5.12 due to the new stub-driver hack, and a fix for potential list corruption due to missing locking which has been there since the introduction of the character-device interface in 4.6.

Johan

Changes in v2
- drop the corresponding drv_set_drvdata() which is no longer needed after patch 1/2
- add Saravanas's reviewed-by tag to patch 2/2

Quoted 8 lines
Quoted 6 lines

Johan Hovold (2):
gpio: fix NULL-deref-on-deregistration regression
gpio: fix gpio-device list corruption

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

Patches applied, thanks!

Ooops, you are fast!
Anyway, I have tested with gpio-aggregator (w/o this series it sparkles with all possible bugs).

Tested-by Andy Shevchenko <[email protected]>
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#7 lore raw
Quoted 22 lines · Andy Shevchenko
Quoted 19 lines · Bartosz Golaszewski
Quoted 18 lines · Johan Hovold

Here's a fix for a regression in 5.12 due to the new stub-driver hack, and a fix for potential list corruption due to missing locking which has been there since the introduction of the character-device interface in 4.6.

Johan

Changes in v2
- drop the corresponding drv_set_drvdata() which is no longer needed after patch 1/2
- add Saravanas's reviewed-by tag to patch 2/2

Johan Hovold (2):
gpio: fix NULL-deref-on-deregistration regression
gpio: fix gpio-device list corruption

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
 drivers/gpio/gpiolib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Patches applied, thanks!

Ooops, you are fast!

The regression broke my gpip-sim I'm working on right now so I tested it too.

Bartosz

#8 lore raw
In reply to Bartosz Golaszewski
Quoted 20 lines · Bartosz Golaszewski
Quoted 18 lines · Johan Hovold

Here's a fix for a regression in 5.12 due to the new stub-driver hack, and a fix for potential list corruption due to missing locking which has been there since the introduction of the character-device interface in 4.6.

Johan

Changes in v2
- drop the corresponding drv_set_drvdata() which is no longer needed after patch 1/2
- add Saravanas's reviewed-by tag to patch 2/2

Johan Hovold (2):
gpio: fix NULL-deref-on-deregistration regression
gpio: fix gpio-device list corruption

Quoted 8 lines
Quoted 6 lines
 drivers/gpio/gpiolib.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--
2.26.2

Patches applied, thanks!

Thanks Johan and Bartosz!

-Saravana