Skip to contentSkip to conversation

gpio: sysfs: fix attribute leaks and races

PATCH In review Johan Hovold 3 participants 10 messages Jan 13, 2015 – Jan 25, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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.

Fixes d8f388d8dc8d ("gpio: sysfs interface")
Cc stable <[email protected]> # v2.6.27+
---
 drivers/gpio/gpiolib-sysfs.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpiolib-sysfs.c +8 -11
index 2ac1800b58bb..33cf4bd0ed2d 100644
@@ -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
Reviewed-by on v1 1/3
#2 lore raw

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.

#3 lore raw

PATCH 2/3 gpio: sysfs: fix gpio device-attribute leak

In reply to 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.

Fixes d8f388d8dc8d ("gpio: sysfs interface")
Cc stable <[email protected]> # v2.6.27+
---
 drivers/gpio/gpiolib-sysfs.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)
drivers/gpio/gpiolib-sysfs.c +13 -15
index 33cf4bd0ed2d..fd4d9423f6e6 100644
@@ -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
Reviewed-by on v1 2/3
#4 lore raw

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.

#5 lore raw

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.

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?

#6 lore raw

PATCH 3/3 gpio: sysfs: fix gpio attribute-creation race

In reply to 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")
---
 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
index fd4d9423f6e6..f62aa115d79a 100644
@@ -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
index e3a52113a541..550a5eafbd38 100644
@@ -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
#7 lore raw

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")

Same, this one does not apply to Linus' devel branch. :( Too bad because there seems to be good stuff in here.

#8 lore raw

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")

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

#9 lore raw
In reply to Johan Hovold
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