* [PATCH 1/3] gpio: sysfs: fix gpio-chip device-attribute leak
2015-01-13 12:00 [PATCH 0/3] gpio: sysfs: fix attribute leaks and races Johan Hovold
@ 2015-01-13 12:00 ` Johan Hovold
2015-01-25 9:25 ` Alexandre Courbot
2015-01-13 12:00 ` [PATCH 2/3] gpio: sysfs: fix gpio " Johan Hovold
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Johan Hovold @ 2015-01-13 12:00 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, linux-gpio, linux-kernel, Johan Hovold, stable
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/3] gpio: sysfs: fix gpio device-attribute leak
2015-01-13 12:00 [PATCH 0/3] gpio: sysfs: fix attribute leaks and races Johan Hovold
2015-01-13 12:00 ` [PATCH 1/3] gpio: sysfs: fix gpio-chip device-attribute leak Johan Hovold
@ 2015-01-13 12:00 ` Johan Hovold
2015-01-25 9:27 ` Alexandre Courbot
2015-01-13 12:00 ` [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race Johan Hovold
2015-01-15 16:33 ` [PATCH 0/3] gpio: sysfs: fix attribute leaks and races Linus Walleij
3 siblings, 1 reply; 10+ messages in thread
From: Johan Hovold @ 2015-01-13 12:00 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, linux-gpio, linux-kernel, Johan Hovold, stable
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] gpio: sysfs: fix gpio device-attribute leak
2015-01-13 12:00 ` [PATCH 2/3] gpio: sysfs: fix gpio " Johan Hovold
@ 2015-01-25 9:27 ` Alexandre Courbot
2015-01-25 9:30 ` Alexandre Courbot
0 siblings, 1 reply; 10+ messages in thread
From: Alexandre Courbot @ 2015-01-25 9:27 UTC (permalink / raw)
To: Johan Hovold
Cc: Linus Walleij, linux-gpio@vger.kernel.org,
Linux Kernel Mailing List, stable
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] gpio: sysfs: fix gpio device-attribute leak
2015-01-25 9:27 ` Alexandre Courbot
@ 2015-01-25 9:30 ` Alexandre Courbot
0 siblings, 0 replies; 10+ messages in thread
From: Alexandre Courbot @ 2015-01-25 9:30 UTC (permalink / raw)
To: Johan Hovold
Cc: Linus Walleij, linux-gpio@vger.kernel.org,
Linux Kernel Mailing List, stable
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race
2015-01-13 12:00 [PATCH 0/3] gpio: sysfs: fix attribute leaks and races Johan Hovold
2015-01-13 12:00 ` [PATCH 1/3] gpio: sysfs: fix gpio-chip device-attribute leak Johan Hovold
2015-01-13 12:00 ` [PATCH 2/3] gpio: sysfs: fix gpio " Johan Hovold
@ 2015-01-13 12:00 ` Johan Hovold
2015-01-25 9:33 ` Alexandre Courbot
2015-01-15 16:33 ` [PATCH 0/3] gpio: sysfs: fix attribute leaks and races Linus Walleij
3 siblings, 1 reply; 10+ messages in thread
From: Johan Hovold @ 2015-01-13 12:00 UTC (permalink / raw)
To: Linus Walleij; +Cc: Alexandre Courbot, linux-gpio, linux-kernel, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race
2015-01-13 12:00 ` [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race Johan Hovold
@ 2015-01-25 9:33 ` Alexandre Courbot
2015-01-25 10:16 ` Johan Hovold
0 siblings, 1 reply; 10+ messages in thread
From: Alexandre Courbot @ 2015-01-25 9:33 UTC (permalink / raw)
To: Johan Hovold
Cc: Linus Walleij, linux-gpio@vger.kernel.org,
Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race
2015-01-25 9:33 ` Alexandre Courbot
@ 2015-01-25 10:16 ` Johan Hovold
0 siblings, 0 replies; 10+ messages in thread
From: Johan Hovold @ 2015-01-25 10:16 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Johan Hovold, Linus Walleij, linux-gpio@vger.kernel.org,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] gpio: sysfs: fix attribute leaks and races
2015-01-13 12:00 [PATCH 0/3] gpio: sysfs: fix attribute leaks and races Johan Hovold
` (2 preceding siblings ...)
2015-01-13 12:00 ` [PATCH 3/3] gpio: sysfs: fix gpio attribute-creation race Johan Hovold
@ 2015-01-15 16:33 ` Linus Walleij
3 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2015-01-15 16:33 UTC (permalink / raw)
To: Johan Hovold
Cc: Alexandre Courbot, linux-gpio@vger.kernel.org,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 10+ messages in thread