gpio: regression fixes

PATCH Superseded Johan Hovold 2 participants 6 messages Feb 26, 2021 – Mar 1, 2021

Subsystems (2)

Stable branch GPIO

lore

Share & tools

lore

Johan Hovold

author

2021-02-26 14:56 UTC #0 lore raw

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

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

 drivers/gpio/gpiolib.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Signature · 1 line

2.26.2

Johan Hovold

author

2021-02-26 14:56 UTC #1 lore raw

PATCH 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpio/gpiolib.c +1 -1

index adf55db080d8..e1016bc8cf14 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);

Signature · 1 line

2.26.2

Saravana Kannan

2021-02-26 21:55 UTC #2 lore raw

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

Quoted 24 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.
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib.c +0 -0
index adf55db080d8..e1016bc8cf14 100644

drivers/gpio/gpiolib.c EXPORT_SYMBOL_GPL(gpiochip_line_is_valid); · line 477

Quoted 5 lines

drivers/gpio/gpiolib.c +1 -1

@@ -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);

Can you also delete the dev_set_drvdata() in gpiochip_add_data_with_key() if the drvdata is not used elsewhere anymore? I skimmed the code and it doesn't look like it, but I could be wrong.

-Saravana

Johan Hovold

author

2021-03-01 08:45 UTC #3 lore raw

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

Quoted 26 lines · Saravana Kannan

Quoted 25 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.
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib.c +0 -0
index adf55db080d8..e1016bc8cf14 100644
@@ -474,7 +474,7 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 

Quoted 9 lines

drivers/gpio/gpiolib.c EXPORT_SYMBOL_GPL(gpiochip_line_is_valid); · line 477
Quoted 4 lines
drivers/gpio/gpiolib.c +1 -1
 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);
Can you also delete the dev_set_drvdata() in gpiochip_add_data_with_key() if the drvdata is not used elsewhere anymore? I skimmed the code and it doesn't look like it, but I could be wrong.

Yeah, I considered it but didn't want to risk introducing any new regressions just to clean up a redundant store.

But looking at it again today, I agree that it looks like it isn't used anywhere else. I'll send a v2. Thanks.

Johan

Johan Hovold

author

2021-02-26 14:56 UTC #4 lore raw

PATCH 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

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpio/gpiolib.c +4 -0

index e1016bc8cf14..42bdc55a15f9 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

Saravana Kannan

Reviewed-by on v1 2/2

2021-02-26 21:00 UTC #5 lore raw

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

Quoted 17 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
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/gpio/gpiolib.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/gpio/gpiolib.c +0 -0
index e1016bc8cf14..42bdc55a15f9 100644

drivers/gpio/gpiolib.c EXPORT_SYMBOL_GPL(gpiochip_line_is_valid); · line 483

Quoted 9 lines

drivers/gpio/gpiolib.c +4 -0

@@ -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);
+

Reviewed-by Saravana Kannan <[email protected]>

-Saravana

Quoted 6 lines

drivers/gpio/gpiolib.c +0 -1

        ida_free(&gpio_ida, gdev->id);
        kfree_const(gdev->label);
        kfree(gdev->descs);
--

2.26.2