gpio: sysfs: fix attribute leaks and races
Subsystems (2)
cover gpio: sysfs: fix attribute leaks and races
Here are three more patches for 3.19 fixing some long-standing memory leaks and races (with userspace) in the gpio sysfs-interface implementation.
The memory leaks are marked for stable, but I'll need to backport them to pre-3.18 kernels once they are upstream due to commit 0eb4c6c2671c ("gpio: move sysfs support to its own file").
I chose not to mark the final patch for stable as the attribute-creation race with userspace (e.g. udev rule) is not as critical (and patch a bit more invasive).
Please note that these patches will cause a conflict with 3511ee7b3312 ("gpio: lib-sysfs: Add 'wakeup' attribute") in gpio/devel, which adds yet another device attribute without ever removing it. Unless the commit in question can be reverted, that leak could be fixed as part of the merge resolution, I guess.
Johan
Johan Hovold (3):
gpio: sysfs: fix gpio-chip device-attribute leak
gpio: sysfs: fix gpio device-attribute leak
gpio: sysfs: fix gpio attribute-creation race
drivers/gpio/gpiolib-sysfs.c | 92 +++++++++++++++++++++++++------------------- drivers/gpio/gpiolib.h | 1 + 2 files changed, 53 insertions(+), 40 deletions(-)
Signature · 1 line
2.0.5
PATCH 1/3 gpio: sysfs: fix gpio-chip device-attribute leak
The gpio-chip device attributes were never destroyed when the device was removed.
Fix by using device_create_with_groups() to create the device attributes of the chip class device.
Note that this also fixes the attribute-creation race with userspace.
--- drivers/gpio/gpiolib-sysfs.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpiolib-sysfs.c +8 -11
@@ -400,16 +400,13 @@ static ssize_t chip_ngpio_show(struct device *dev, } static DEVICE_ATTR(ngpio, 0444, chip_ngpio_show, NULL); -static const struct attribute *gpiochip_attrs[] = { +static struct attribute *gpiochip_attrs[] = { &dev_attr_base.attr, &dev_attr_label.attr, &dev_attr_ngpio.attr,NULL , }; - -static const struct attribute_group gpiochip_attr_group = { - .attrs = (struct attribute **) gpiochip_attrs, -}; +ATTRIBUTE_GROUPS (gpiochip);/* * /sys/class/gpio/export ... write-only
@@ -750,13 +747,13 @@ int gpiochip_export(struct gpio_chip *chip)/* use chip->base for the ID; it's already known to be unique */ mutex_lock (&sysfs_lock); - dev =device_create (&gpio_class, chip->dev,MKDEV (0 ,0 ), chip, -"gpiochip%d" , chip->base); -if (!IS_ERR (dev)) { - status =sysfs_create_group (&dev->kobj, - &gpiochip_attr_group); - }else + dev =device_create_with_groups (&gpio_class, chip->dev,MKDEV (0 ,0 ), + chip, gpiochip_groups, +"gpiochip%d" , chip->base); +if (IS_ERR (dev)) status =PTR_ERR (dev); +else + status =0 ; chip->exported = (status ==0 );mutex_unlock (&sysfs_lock);
Signature · 1 line
2.0.5
Re: [PATCH 1/3] gpio: sysfs: fix gpio-chip device-attribute leak
Quoted 7 lines · Johan Hovold
The gpio-chip device attributes were never destroyed when the device was removed.
Fix by using
device_create_with_groups()to create the device attributes of the chip class device.Note that this also fixes the attribute-creation race with userspace.
... and it also improves code readability.
PATCH 2/3 gpio: sysfs: fix gpio device-attribute leak
In reply to Johan HovoldThe gpio device attributes were never destroyed when the gpio was unexported (or on export failures).
Use device_create_with_groups() to create the default device attributes of the gpio class device. Note that this also fixes the attribute-creation race with userspace for these attributes.
Remove contingent attributes in export error path and on unexport.
--- drivers/gpio/gpiolib-sysfs.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)
drivers/gpio/gpiolib-sysfs.c +13 -15
@@ -128,7 +128,7 @@ static ssize_t gpio_value_store(struct device *dev,return status; } -static const DEVICE_ATTR(value, 0644, +static DEVICE_ATTR(value, 0644, gpio_value_show, gpio_value_store);static irqreturn_t gpio_sysfs_irq (int irq,void *priv)
@@ -353,18 +353,15 @@ static ssize_t gpio_active_low_store(struct device *dev,return status ? : size; } -static const DEVICE_ATTR(active_low, 0644, +static DEVICE_ATTR(active_low, 0644, gpio_active_low_show, gpio_active_low_store); -static const struct attribute *gpio_attrs[] = { +static struct attribute *gpio_attrs[] = { &dev_attr_value.attr, &dev_attr_active_low.attr,NULL , }; - -static const struct attribute_group gpio_attr_group = { - .attrs = (struct attribute **) gpio_attrs, -}; +ATTRIBUTE_GROUPS (gpio);/* * /sys/class/gpio/gpiochipN/
@@ -561,18 +558,15 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)if (desc->chip->names && desc->chip->names[offset]) ioname = desc->chip->names[offset]; - dev =device_create (&gpio_class, desc->chip->dev,MKDEV (0 ,0 ), - desc, ioname ? ioname :"gpio%u" , -desc_to_gpio (desc)); + dev =device_create_with_groups (&gpio_class, desc->chip->dev, +MKDEV (0 ,0 ), desc, gpio_groups, + ioname ? ioname :"gpio%u" , +desc_to_gpio (desc));if (IS_ERR (dev)) { status =PTR_ERR (dev); goto fail_unlock; } - status =sysfs_create_group (&dev->kobj, &gpio_attr_group); -if (status) - goto fail_unregister_device; -if (direction_may_change) { status =device_create_file (dev, &dev_attr_direction);if (status)
@@ -583,13 +577,15 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) !test_bit(FLAG_IS_OUT, &desc->flags))) { status =device_create_file (dev, &dev_attr_edge);if (status) - goto fail_unregister_device; + goto fail_remove_attr_direction; }set_bit (FLAG_EXPORT , &desc->flags);mutex_unlock (&sysfs_lock);return 0 ; +fail_remove_attr_direction: +device_remove_file (dev, &dev_attr_direction); fail_unregister_device:device_unregister (dev); fail_unlock:
@@ -723,6 +719,8 @@ void gpiod_unexport(struct gpio_desc *desc)mutex_unlock (&sysfs_lock);if (dev) { +device_remove_file (dev, &dev_attr_edge); +device_remove_file (dev, &dev_attr_direction);device_unregister (dev);put_device (dev); }
Signature · 1 line
2.0.5
Re: [PATCH 2/3] gpio: sysfs: fix gpio device-attribute leak
Quoted 8 lines · Johan Hovold
The gpio device attributes were never destroyed when the gpio was unexported (or on export failures).
Use
device_create_with_groups()to create the default device attributes of the gpio class device. Note that this also fixes the attribute-creation race with userspace for these attributes.Remove contingent attributes in export error path and on unexport.
Re: [PATCH 2/3] gpio: sysfs: fix gpio device-attribute leak
Quoted 11 lines · Alexandre Courbot
Quoted 8 lines · Johan Hovold
The gpio device attributes were never destroyed when the gpio was unexported (or on export failures).
Use
device_create_with_groups()to create the default device attributes of the gpio class device. Note that this also fixes the attribute-creation race with userspace for these attributes.Remove contingent attributes in export error path and on unexport.
Reviewed-by Alexandre Courbot <[email protected]>
Mmm actually this one does not seem to apply to Linus' devel branch. Seems like something has changed in between - could you rebase and resend?
PATCH 3/3 gpio: sysfs: fix gpio attribute-creation race
In reply to Johan HovoldFix attribute-creation race with userspace by using the default group to create also the contingent gpio device attributes.
--- drivers/gpio/gpiolib-sysfs.c | 61 ++++++++++++++++++++++++++++---------------- drivers/gpio/gpiolib.h | 1 + 2 files changed, 40 insertions(+), 22 deletions(-)
drivers/gpio/gpiolib-sysfs.c +39 -22
@@ -356,12 +356,44 @@ static ssize_t gpio_active_low_store(struct device *dev, static DEVICE_ATTR(active_low, 0644, gpio_active_low_show, gpio_active_low_store); +static umode_t gpio_is_visible (struct kobject *kobj,struct attribute *attr, +int n) +{ + struct device *dev = container_of(kobj, struct device, kobj); +struct gpio_desc *desc =dev_get_drvdata (dev); +umode_t mode = attr->mode; +bool show_direction =test_bit (FLAG_SYSFS_DIR , &desc->flags); + +if (attr == &dev_attr_direction.attr) { +if (!show_direction) + mode =0 ; + }else if (attr == &dev_attr_edge.attr) { +if (gpiod_to_irq (desc) <0 ) + mode =0 ; +if (!show_direction &&test_bit (FLAG_IS_OUT , &desc->flags)) + mode =0 ; + } + +return mode; +} +static struct attribute *gpio_attrs[] = { + &dev_attr_direction.attr, + &dev_attr_edge.attr, &dev_attr_value.attr, &dev_attr_active_low.attr,NULL , }; -ATTRIBUTE_GROUPS (gpio); + +static const struct attribute_group gpio_group = { + .attrs = gpio_attrs, + .is_visible = gpio_is_visible, +}; + +static const struct attribute_group *gpio_groups[] = { + &gpio_group, +NULL +};/* * /sys/class/gpio/gpiochipN/
@@ -550,8 +582,11 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) goto fail_unlock; } -if (!desc->chip->direction_input || !desc->chip->direction_output) - direction_may_change = false; +if (desc->chip->direction_input && desc->chip->direction_output && + direction_may_change) { +set_bit (FLAG_SYSFS_DIR , &desc->flags); + } +spin_unlock_irqrestore (&gpio_lock, flags); offset =gpio_chip_hwgpio (desc);
@@ -567,27 +602,10 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) goto fail_unlock; } -if (direction_may_change) { - status =device_create_file (dev, &dev_attr_direction); -if (status) - goto fail_unregister_device; - } - -if (gpiod_to_irq (desc) >=0 && (direction_may_change || - !test_bit (FLAG_IS_OUT , &desc->flags))) { - status =device_create_file (dev, &dev_attr_edge); -if (status) - goto fail_remove_attr_direction; - } -set_bit (FLAG_EXPORT , &desc->flags);mutex_unlock (&sysfs_lock);return 0 ; -fail_remove_attr_direction: -device_remove_file (dev, &dev_attr_direction); -fail_unregister_device: -device_unregister (dev); fail_unlock:mutex_unlock (&sysfs_lock);gpiod_dbg (desc,"%s: status %d\n" , __func__, status);
@@ -711,6 +729,7 @@ void gpiod_unexport(struct gpio_desc *desc) dev =class_find_device (&gpio_class,NULL , desc, match_export);if (dev) {gpio_setup_irq (desc, dev,0 ); +clear_bit (FLAG_SYSFS_DIR , &desc->flags);clear_bit (FLAG_EXPORT , &desc->flags); }else status = -ENODEV ;
@@ -719,8 +738,6 @@ void gpiod_unexport(struct gpio_desc *desc)mutex_unlock (&sysfs_lock);if (dev) { -device_remove_file (dev, &dev_attr_edge); -device_remove_file (dev, &dev_attr_direction);device_unregister (dev);put_device (dev); }
drivers/gpio/gpiolib.h +1 -0
@@ -77,6 +77,7 @@ struct gpio_desc {#define FLAG_OPEN_DRAIN 7/* Gpio is open drain type */ #define FLAG_OPEN_SOURCE 8/* Gpio is open source type */ #define FLAG_USED_AS_IRQ 9/* GPIO is connected to an IRQ */ +#define FLAG_SYSFS_DIR 10/* show sysfs direction attribute */ #define ID_SHIFT 16/* add new flags before this one */
Signature · 1 line
2.0.5
Re: [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race
Quoted 5 lines · Johan Hovold
Fix attribute-creation race with userspace by using the default group to create also the contingent gpio device attributes.
Fixes d8f388d8dc8d ("gpio: sysfs interface")Signed-off-by Johan Hovold <[email protected]>
Same, this one does not apply to Linus' devel branch. :( Too bad because there seems to be good stuff in here.
Re: [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race
Quoted 9 lines · Alexandre Courbot
Quoted 5 lines · Johan Hovold
Fix attribute-creation race with userspace by using the default group to create also the contingent gpio device attributes.
Fixes d8f388d8dc8d ("gpio: sysfs interface")Signed-off-by Johan Hovold <[email protected]>Same, this one does not apply to Linus' devel branch. :( Too bad because there seems to be good stuff in here.
All three patches are already in mainline.
Thanks,
Johan
Quoted 3 lines · Johan Hovold
Here are three more patches for 3.19 fixing some long-standing memory leaks and races (with userspace) in the gpio sysfs-interface implementation.
All three patches applied for fixes.
Thank you for daring to go into the sysfs mess, noone dares to go there but for you. This interface was merged when the GPIO subsystem was unmaintained IIRC and has serious design and code issues.
Quoted 3 lines
The memory leaks are marked for stable, but I'll need to backport them to pre-3.18 kernels once they are upstream due to commit
0eb4c6c2671c("gpio: move sysfs support to its own file").
OK we'll run into it. Just send them to Greg once this is upstream.
Quoted 5 lines
Please note that these patches will cause a conflict with
3511ee7b3312("gpio: lib-sysfs: Add 'wakeup' attribute") in gpio/devel, which adds yet another device attribute without ever removing it. Unless the commit in question can be reverted, that leak could be fixed as part of the merge resolution, I guess.
I took the patch out of my tree.
Sören: can you rebase the patch on top of my "fixes" branch or the next linux-next and I'll apply it once I can merge the next -rc into my devel branch.
Include Johan on CC and check the leak fixes in this patch series to make sure we don't add more of them.
Yours,
Linus Walleij