[PATCH 0/3] gpio: sysfs: fix attribute leaks and races

[PATCH 0/3] gpio: sysfs: fix attribute leaks and races

[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.

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

-- 
2.0.5

[PATCH 1/3] gpio: sysfs: fix gpio-chip device-attribute leak

[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.

Fixes: d8f388d8dc8d ("gpio: sysfs interface")
Cc: stable <stable@vger.kernel.org>	# v2.6.27+
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 2ac1800b58bb..33cf4bd0ed2d 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -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);
 
-- 
2.0.5

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

[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 <stable@vger.kernel.org>	# v2.6.27+
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 33cf4bd0ed2d..fd4d9423f6e6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -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);
 	}
-- 
2.0.5

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

[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 <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 61 ++++++++++++++++++++++++++++----------------
 drivers/gpio/gpiolib.h       |  1 +
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index fd4d9423f6e6..f62aa115d79a 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -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);
 	}
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index e3a52113a541..550a5eafbd38 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -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 */
 
-- 
2.0.5

Re: [PATCH 0/3] gpio: sysfs: fix attribute leaks and races

[Linus Walleij]

On Tue, Jan 13, 2015 at 1:00 PM, Johan Hovold <johan@kernel.org> wrote:

> 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.

> 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.

> 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

Re: [PATCH 1/3] gpio: sysfs: fix gpio-chip device-attribute leak

[Alexandre Courbot]

On Tue, Jan 13, 2015 at 9:00 PM, Johan Hovold <johan@kernel.org> wrote:
> 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.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

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

[Alexandre Courbot]

On Tue, Jan 13, 2015 at 9:00 PM, Johan Hovold <johan@kernel.org> wrote:
> 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 <acourbot@nvidia.com>

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

[Alexandre Courbot]

On Sun, Jan 25, 2015 at 6:27 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Jan 13, 2015 at 9:00 PM, Johan Hovold <johan@kernel.org> wrote:
>> 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 <acourbot@nvidia.com>

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?

Re: [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race

[Alexandre Courbot]

On Tue, Jan 13, 2015 at 9:00 PM, Johan Hovold <johan@kernel.org> wrote:
> 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 <johan@kernel.org>

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

[Johan Hovold]

On Sun, Jan 25, 2015 at 06:33:48PM +0900, Alexandre Courbot wrote:
> On Tue, Jan 13, 2015 at 9:00 PM, Johan Hovold <johan@kernel.org> wrote:
> > 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 <johan@kernel.org>
> 
> 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