Skip to contentSkip to conversation

leds: fix attribute-creation races

PATCH In review Johan Hovold 5 participants 21 messages Jun 25, 2014 – Jun 30, 2014

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

cover leds: fix attribute-creation races

LED-class device attributes should be created using the attribute groups of struct device, rather than be added manually after the device has been registered in order to avoid racing with userspace.

The first patch adds an attribute groups field to struct led_classdev, which is used during registration to add device attributes in a race-free manner.

The remaining patches convert the current racy LED-device attribute creations to use the new facility. Hopefully my grep-patterns have caught most instances. Note that this also tends to simplify probe error paths.

The lp55xx-common failed to remove the attribute group it is currently creating, something which is fixed separately.

Note that this series include one driver from drivers/input/keyboard.

The individual led-driver patches have been compile tested only.

Johan

Johan Hovold (13):
leds: add led-class attribute-group support
leds: lm3550: fix attribute-creation race
leds: lm3533: fix attribute-creation race
leds: lm355x: fix attribute-creation race
leds: lm3642: fix attribute-creation race
leds: max8997: fix attribute-creation race
leds: netxbig: fix attribute-creation race
leds: ns2: fix attribute-creation race
leds: ss4200: fix attribute-creation race
leds: wm831x-status: fix attribute-creation race
input: lm8323: fix attribute-creation race
leds: lp55xx-common: fix sysfs entry leak
leds: lp55xx-common: fix attribute-creation race

 drivers/input/keyboard/lm8323.c   | 22 +++++++++-------------
 drivers/leds/led-class.c          |  5 +++--
 drivers/leds/leds-lm3530.c        | 20 +++++++-------------
 drivers/leds/leds-lm3533.c        | 20 ++++++++------------
 drivers/leds/leds-lm355x.c        | 21 +++++++++------------
 drivers/leds/leds-lm3642.c        | 30 ++++++++++++++----------------
 drivers/leds/leds-lp55xx-common.c | 20 +++-----------------
 drivers/leds/leds-max8997.c       | 16 +++++++---------
 drivers/leds/leds-netxbig.c       | 26 ++++++++++++--------------
 drivers/leds/leds-ns2.c           | 16 +++++++---------
 drivers/leds/leds-ss4200.c        | 14 +++++++++-----
 drivers/leds/leds-wm831x-status.c | 23 +++++++++--------------
 include/linux/leds.h              |  2 ++
 13 files changed, 99 insertions(+), 136 deletions(-)
Signature · 1 line
1.8.5.5
#1 lore raw

PATCH 1/13 leds: add led-class attribute-group support

Allow led-class devices to be created with optional attribute groups.

This is needed in order to allow led drivers to create custom device attributes in a race-free manner.

---
 drivers/leds/led-class.c | 5 +++--
 include/linux/leds.h     | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)
drivers/leds/led-class.c +3 -2
index f37d63cf726b..aa29198fca3e 100644
@@ -210,8 +210,9 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
  */
 int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 {
-	led_cdev->dev = device_create(leds_class, parent, 0, led_cdev,
-				      "%s", led_cdev->name);
+	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
+					led_cdev, led_cdev->groups,
+					"%s", led_cdev->name);
 	if (IS_ERR(led_cdev->dev))
 		return PTR_ERR(led_cdev->dev);
 
include/linux/leds.h +2 -0
index 0287ab296689..e43686472197 100644
@@ -63,6 +63,8 @@ struct led_classdev {
 				     unsigned long *delay_off);
 
 	struct device		*dev;
+	const struct attribute_group	**groups;
+
 	struct list_head	 node;			/* LED Device list */
 	const char		*default_trigger;	/* Trigger to use */
 
Signature · 1 line
1.8.5.5
#2 lore raw

PATCH 2/13 leds: lm3550: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the mode attribute during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-lm3530.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
drivers/leds/leds-lm3530.c +7 -13
index 652368c2ea9a..91325de3cd33 100644
@@ -400,6 +400,12 @@ static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
 }
 static DEVICE_ATTR(mode, 0644, lm3530_mode_get, lm3530_mode_set);
 
+static struct attribute *lm3530_attrs[] = {
+	&dev_attr_mode.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(lm3530);
+
 static int lm3530_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
@@ -436,6 +442,7 @@ static int lm3530_probe(struct i2c_client *client,
 	drvdata->led_dev.name = LM3530_LED_DEV;
 	drvdata->led_dev.brightness_set = lm3530_brightness_set;
 	drvdata->led_dev.max_brightness = MAX_BRIGHTNESS;
+	drvdata->led_dev.groups = lm3530_groups;
 
 	i2c_set_clientdata(client, drvdata);
 
@@ -461,26 +468,13 @@ static int lm3530_probe(struct i2c_client *client,
 		return err;
 	}
 
-	err = device_create_file(drvdata->led_dev.dev, &dev_attr_mode);
-	if (err < 0) {
-		dev_err(&client->dev, "File device creation failed: %d\n", err);
-		err = -ENODEV;
-		goto err_create_file;
-	}
-
 	return 0;
-
-err_create_file:
-	led_classdev_unregister(&drvdata->led_dev);
-	return err;
 }
 
 static int lm3530_remove(struct i2c_client *client)
 {
 	struct lm3530_data *drvdata = i2c_get_clientdata(client);
 
-	device_remove_file(drvdata->led_dev.dev, &dev_attr_mode);
-
 	lm3530_led_disable(drvdata);
 	led_classdev_unregister(&drvdata->led_dev);
 	return 0;
Signature · 1 line
1.8.5.5
#3 lore raw

PATCH 3/13 leds: lm3533: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the attributes during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-lm3533.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/leds/leds-lm3533.c +8 -12
index e2c642c1169b..cbf61a40137d 100644
@@ -645,6 +645,11 @@ static struct attribute_group lm3533_led_attribute_group = {
 	.attrs		= lm3533_led_attributes
 };
 
+static const struct attribute_group *lm3533_led_attribute_groups[] = {
+	&lm3533_led_attribute_group,
+	NULL
+};
+
 static int lm3533_led_setup(struct lm3533_led *led,
 					struct lm3533_led_platform_data *pdata)
 {
@@ -692,6 +697,7 @@ static int lm3533_led_probe(struct platform_device *pdev)
 	led->cdev.brightness_get = lm3533_led_get;
 	led->cdev.blink_set = lm3533_led_blink_set;
 	led->cdev.brightness = LED_OFF;
+	led->cdev.groups = lm3533_led_attribute_groups,
 	led->id = pdev->id;
 
 	mutex_init(&led->mutex);
@@ -715,25 +721,16 @@ static int lm3533_led_probe(struct platform_device *pdev)
 
 	led->cb.dev = led->cdev.dev;
 
-	ret = sysfs_create_group(&led->cdev.dev->kobj,
-						&lm3533_led_attribute_group);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to create sysfs attributes\n");
-		goto err_unregister;
-	}
-
 	ret = lm3533_led_setup(led, pdata);
 	if (ret)
-		goto err_sysfs_remove;
+		goto err_unregister;
 
 	ret = lm3533_ctrlbank_enable(&led->cb);
 	if (ret)
-		goto err_sysfs_remove;
+		goto err_unregister;
 
 	return 0;
 
-err_sysfs_remove:
-	sysfs_remove_group(&led->cdev.dev->kobj, &lm3533_led_attribute_group);
 err_unregister:
 	led_classdev_unregister(&led->cdev);
 	flush_work(&led->work);
@@ -748,7 +745,6 @@ static int lm3533_led_remove(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
 	lm3533_ctrlbank_disable(&led->cb);
-	sysfs_remove_group(&led->cdev.dev->kobj, &lm3533_led_attribute_group);
 	led_classdev_unregister(&led->cdev);
 	flush_work(&led->work);
 
Signature · 1 line
1.8.5.5
#4 lore raw

PATCH 4/13 leds: lm355x: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the indicator-LED attributes during probe in order to avoid racing with userspace.

Note that the comment about the pattern attribute only being for LM3554 was incorrect and did not match the code (the original leds-lm3556 driver had the attribute before LM3554 support was added).

---
 drivers/leds/leds-lm355x.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)
drivers/leds/leds-lm355x.c +9 -12
index 591eb5e58ae3..f5112cb2d991 100644
@@ -413,6 +413,12 @@ out:
 
 static DEVICE_ATTR(pattern, S_IWUSR, NULL, lm3556_indicator_pattern_store);
 
+static struct attribute *lm355x_indicator_attrs[] = {
+	&dev_attr_pattern.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(lm355x_indicator);
+
 static const struct regmap_config lm355x_regmap = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -501,25 +507,18 @@ static int lm355x_probe(struct i2c_client *client,
 	else
 		chip->cdev_indicator.max_brightness = 8;
 	chip->cdev_indicator.brightness_set = lm355x_indicator_brightness_set;
+	/* indicator pattern control only for LM3556 */
+	if (id->driver_data == CHIP_LM3556)
+		chip->cdev_indicator.groups = lm355x_indicator_groups;
 	err = led_classdev_register((struct device *)
 				    &client->dev, &chip->cdev_indicator);
 	if (err < 0)
 		goto err_create_indicator_file;
-	/* indicator pattern control only for LM3554 */
-	if (id->driver_data == CHIP_LM3556) {
-		err =
-		    device_create_file(chip->cdev_indicator.dev,
-				       &dev_attr_pattern);
-		if (err < 0)
-			goto err_create_pattern_file;
-	}
 
 	dev_info(&client->dev, "%s is initialized\n",
 		 lm355x_name[id->driver_data]);
 	return 0;
 
-err_create_pattern_file:
-	led_classdev_unregister(&chip->cdev_indicator);
 err_create_indicator_file:
 	led_classdev_unregister(&chip->cdev_torch);
 err_create_torch_file:
@@ -534,8 +533,6 @@ static int lm355x_remove(struct i2c_client *client)
 	struct lm355x_reg_data *preg = chip->regs;
 
 	regmap_write(chip->regmap, preg[REG_OPMODE].regno, 0);
-	if (chip->type == CHIP_LM3556)
-		device_remove_file(chip->cdev_indicator.dev, &dev_attr_pattern);
 	led_classdev_unregister(&chip->cdev_indicator);
 	flush_work(&chip->work_indicator);
 	led_classdev_unregister(&chip->cdev_torch);
Signature · 1 line
1.8.5.5
#5 lore raw

PATCH 5/13 leds: lm3642: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the flash and torch-LED attributes during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-lm3642.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)
drivers/leds/leds-lm3642.c +14 -16
index ceb6b3cde6fe..d3dec0132769 100644
@@ -313,6 +313,18 @@ static const struct regmap_config lm3642_regmap = {
 	.max_register = REG_MAX,
 };
 
+static struct attribute *lm3642_flash_attrs[] = {
+	&dev_attr_strobe_pin.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(lm3642_flash);
+
+static struct attribute *lm3642_torch_attrs[] = {
+	&dev_attr_torch_pin.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(lm3642_torch);
+
 static int lm3642_probe(struct i2c_client *client,
 				  const struct i2c_device_id *id)
 {
@@ -364,17 +376,13 @@ static int lm3642_probe(struct i2c_client *client,
 	chip->cdev_flash.max_brightness = 16;
 	chip->cdev_flash.brightness_set = lm3642_strobe_brightness_set;
 	chip->cdev_flash.default_trigger = "flash";
+	chip->cdev_flash.groups = lm3642_flash_groups,
 	err = led_classdev_register((struct device *)
 				    &client->dev, &chip->cdev_flash);
 	if (err < 0) {
 		dev_err(chip->dev, "failed to register flash\n");
 		goto err_out;
 	}
-	err = device_create_file(chip->cdev_flash.dev, &dev_attr_strobe_pin);
-	if (err < 0) {
-		dev_err(chip->dev, "failed to create strobe-pin file\n");
-		goto err_create_flash_pin_file;
-	}
 
 	/* torch */
 	INIT_WORK(&chip->work_torch, lm3642_deferred_torch_brightness_set);
@@ -382,17 +390,13 @@ static int lm3642_probe(struct i2c_client *client,
 	chip->cdev_torch.max_brightness = 8;
 	chip->cdev_torch.brightness_set = lm3642_torch_brightness_set;
 	chip->cdev_torch.default_trigger = "torch";
+	chip->cdev_torch.groups = lm3642_torch_groups,
 	err = led_classdev_register((struct device *)
 				    &client->dev, &chip->cdev_torch);
 	if (err < 0) {
 		dev_err(chip->dev, "failed to register torch\n");
 		goto err_create_torch_file;
 	}
-	err = device_create_file(chip->cdev_torch.dev, &dev_attr_torch_pin);
-	if (err < 0) {
-		dev_err(chip->dev, "failed to create torch-pin file\n");
-		goto err_create_torch_pin_file;
-	}
 
 	/* indicator */
 	INIT_WORK(&chip->work_indicator,
@@ -411,12 +415,8 @@ static int lm3642_probe(struct i2c_client *client,
 	return 0;
 
 err_create_indicator_file:
-	device_remove_file(chip->cdev_torch.dev, &dev_attr_torch_pin);
-err_create_torch_pin_file:
 	led_classdev_unregister(&chip->cdev_torch);
 err_create_torch_file:
-	device_remove_file(chip->cdev_flash.dev, &dev_attr_strobe_pin);
-err_create_flash_pin_file:
 	led_classdev_unregister(&chip->cdev_flash);
 err_out:
 	return err;
@@ -428,10 +428,8 @@ static int lm3642_remove(struct i2c_client *client)
 
 	led_classdev_unregister(&chip->cdev_indicator);
 	flush_work(&chip->work_indicator);
-	device_remove_file(chip->cdev_torch.dev, &dev_attr_torch_pin);
 	led_classdev_unregister(&chip->cdev_torch);
 	flush_work(&chip->work_torch);
-	device_remove_file(chip->cdev_flash.dev, &dev_attr_strobe_pin);
 	led_classdev_unregister(&chip->cdev_flash);
 	flush_work(&chip->work_flash);
 	regmap_write(chip->regmap, REG_ENABLE, 0);
Signature · 1 line
1.8.5.5
#6 lore raw

PATCH 6/13 leds: max8997: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the mode attribute during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-max8997.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/leds/leds-max8997.c +7 -9
index f449a8bdddc7..607bc2755aba 100644
@@ -229,6 +229,12 @@ static ssize_t max8997_led_store_mode(struct device *dev,
 
 static DEVICE_ATTR(mode, 0644, max8997_led_show_mode, max8997_led_store_mode);
 
+static struct attribute *max8997_attrs[] = {
+	&dev_attr_mode.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(max8997);
+
 static int max8997_led_probe(struct platform_device *pdev)
 {
 	struct max8997_dev *iodev = dev_get_drvdata(pdev->dev.parent);
@@ -253,6 +259,7 @@ static int max8997_led_probe(struct platform_device *pdev)
 	led->cdev.brightness_set = max8997_led_brightness_set;
 	led->cdev.flags |= LED_CORE_SUSPENDRESUME;
 	led->cdev.brightness = 0;
+	led->cdev.groups = max8997_groups;
 	led->iodev = iodev;
 
 	/* initialize mode and brightness according to platform_data */
@@ -281,14 +288,6 @@ static int max8997_led_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	ret = device_create_file(led->cdev.dev, &dev_attr_mode);
-	if (ret != 0) {
-		dev_err(&pdev->dev,
-			"failed to create file: %d\n", ret);
-		led_classdev_unregister(&led->cdev);
-		return ret;
-	}
-
 	return 0;
 }
 
@@ -296,7 +295,6 @@ static int max8997_led_remove(struct platform_device *pdev)
 {
 	struct max8997_led *led = platform_get_drvdata(pdev);
 
-	device_remove_file(led->cdev.dev, &dev_attr_mode);
 	led_classdev_unregister(&led->cdev);
 
 	return 0;
Signature · 1 line
1.8.5.5
#7 lore raw

PATCH 7/13 leds: netxbig: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the sata attribute during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-netxbig.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
drivers/leds/leds-netxbig.c +12 -14
index e97f443a6e07..8ab931b8fcd3 100644
@@ -293,10 +293,14 @@ static ssize_t netxbig_led_sata_show(struct device *dev,
 
 static DEVICE_ATTR(sata, 0644, netxbig_led_sata_show, netxbig_led_sata_store);
 
+static struct attribute *netxbig_led_attrs[] = {
+	&dev_attr_sata.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(netxbig_led);
+
 static void delete_netxbig_led(struct netxbig_led_data *led_dat)
 {
-	if (led_dat->mode_val[NETXBIG_LED_SATA] != NETXBIG_LED_INVALID_MODE)
-		device_remove_file(led_dat->cdev.dev, &dev_attr_sata);
 	led_classdev_unregister(&led_dat->cdev);
 }
 
@@ -327,6 +331,12 @@ create_netxbig_led(struct platform_device *pdev,
 	led_dat->sata = 0;
 	led_dat->cdev.brightness = LED_OFF;
 	led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
+	/*
+	 * If available, expose the SATA activity blink capability through
+	 * a "sata" sysfs attribute.
+	 */
+	if (led_dat->mode_val[NETXBIG_LED_SATA] != NETXBIG_LED_INVALID_MODE)
+		led_dat->cdev.groups = netxbig_led_groups;
 	led_dat->mode_addr = template->mode_addr;
 	led_dat->mode_val = template->mode_val;
 	led_dat->bright_addr = template->bright_addr;
@@ -335,18 +345,6 @@ create_netxbig_led(struct platform_device *pdev,
 	led_dat->num_timer = pdata->num_timer;
 
 	ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * If available, expose the SATA activity blink capability through
-	 * a "sata" sysfs attribute.
-	 */
-	if (led_dat->mode_val[NETXBIG_LED_SATA] != NETXBIG_LED_INVALID_MODE) {
-		ret = device_create_file(led_dat->cdev.dev, &dev_attr_sata);
-		if (ret)
-			led_classdev_unregister(&led_dat->cdev);
-	}
 
 	return ret;
 }
Signature · 1 line
1.8.5.5
#8 lore raw

Re: [PATCH 07/13] leds: netxbig: fix attribute-creation race

Quoted 43 lines · Johan Hovold

Use the attribute groups of the led-class to create the sata attribute during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-netxbig.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)
drivers/leds/leds-netxbig.c +12 -2
index e97f443a6e07..8ab931b8fcd3 100644
@@ -293,10 +293,14 @@ static ssize_t netxbig_led_sata_show(struct device *dev,
 
 static DEVICE_ATTR(sata, 0644, netxbig_led_sata_show, netxbig_led_sata_store);
 
+static struct attribute *netxbig_led_attrs[] = {
+       &dev_attr_sata.attr,
+       NULL
+};
+ATTRIBUTE_GROUPS(netxbig_led);
+
 static void delete_netxbig_led(struct netxbig_led_data *led_dat)
 {
-       if (led_dat->mode_val[NETXBIG_LED_SATA] != NETXBIG_LED_INVALID_MODE)
-               device_remove_file(led_dat->cdev.dev, &dev_attr_sata);
        led_classdev_unregister(&led_dat->cdev);
 }
 
@@ -327,6 +331,12 @@ create_netxbig_led(struct platform_device *pdev,
        led_dat->sata = 0;
        led_dat->cdev.brightness = LED_OFF;
        led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
+       /*
+        * If available, expose the SATA activity blink capability through
+        * a "sata" sysfs attribute.
+        */
+       if (led_dat->mode_val[NETXBIG_LED_SATA] != NETXBIG_LED_INVALID_MODE)
+               led_dat->cdev.groups = netxbig_led_groups;
        led_dat->mode_addr = template->mode_addr;
        led_dat->mode_val = template->mode_val;
        led_dat->bright_addr = template->bright_addr;

drivers/leds/leds-netxbig.c create_netxbig_led(struct platform_device *pdev, · line 347

Quoted 3 lines
drivers/leds/leds-netxbig.c +0 -0
@@ -335,18 +345,6 @@ create_netxbig_led(struct platform_device *pdev,
        led_dat->num_timer = pdata->num_timer;
 
        ret = led_classdev_register(&pdev->dev, &led_dat->cdev);

Let's just "return led_classdev_register(&pdev->dev, &led_dat->cdev);" and remove "ret"

I will handle this trivial change.

-Bryan

Quoted 18 lines
drivers/leds/leds-netxbig.c +0 -13
-       if (ret < 0)
-               return ret;
-
-       /*
-        * If available, expose the SATA activity blink capability through
-        * a "sata" sysfs attribute.
-        */
-       if (led_dat->mode_val[NETXBIG_LED_SATA] != NETXBIG_LED_INVALID_MODE) {
-               ret = device_create_file(led_dat->cdev.dev, &dev_attr_sata);
-               if (ret)
-                       led_classdev_unregister(&led_dat->cdev);
-       }
 
        return ret;
 }
--

1.8.5.5

#9 lore raw

PATCH 8/13 leds: ns2: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the sata attribute during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-ns2.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
drivers/leds/leds-ns2.c +7 -9
index efa625883c83..231993d1fe21 100644
@@ -185,6 +185,12 @@ static ssize_t ns2_led_sata_show(struct device *dev,
 
 static DEVICE_ATTR(sata, 0644, ns2_led_sata_show, ns2_led_sata_store);
 
+static struct attribute *ns2_led_attrs[] = {
+	&dev_attr_sata.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ns2_led);
+
 static int
 create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	       const struct ns2_led *template)
@@ -219,6 +225,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	led_dat->cdev.blink_set = NULL;
 	led_dat->cdev.brightness_set = ns2_led_set;
 	led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
+	led_dat->cdev.groups = ns2_led_groups;
 	led_dat->cmd = template->cmd;
 	led_dat->slow = template->slow;
 
@@ -235,20 +242,11 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	if (ret < 0)
 		return ret;
 
-	ret = device_create_file(led_dat->cdev.dev, &dev_attr_sata);
-	if (ret < 0)
-		goto err_free_cdev;
-
 	return 0;
-
-err_free_cdev:
-	led_classdev_unregister(&led_dat->cdev);
-	return ret;
 }
 
 static void delete_ns2_led(struct ns2_led_data *led_dat)
 {
-	device_remove_file(led_dat->cdev.dev, &dev_attr_sata);
 	led_classdev_unregister(&led_dat->cdev);
 }
 
Signature · 1 line
1.8.5.5
#10 lore raw

PATCH 9/13 leds: ss4200: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the blink attribute during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-ss4200.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/leds/leds-ss4200.c +9 -5
index 2eb3ef62962b..046cb7008745 100644
@@ -469,6 +469,12 @@ static ssize_t nas_led_blink_store(struct device *dev,
 
 static DEVICE_ATTR(blink, 0644, nas_led_blink_show, nas_led_blink_store);
 
+static struct attribute *nasgpio_led_attrs[] = {
+	&dev_attr_blink.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(nasgpio_led);
+
 static int register_nasgpio_led(int led_nr)
 {
 	int ret;
@@ -481,20 +487,18 @@ static int register_nasgpio_led(int led_nr)
 		led->brightness = LED_FULL;
 	led->brightness_set = nasgpio_led_set_brightness;
 	led->blink_set = nasgpio_led_set_blink;
+	led->groups = nasgpio_led_groups;
 	ret = led_classdev_register(&nas_gpio_pci_dev->dev, led);
 	if (ret)
 		return ret;
-	ret = device_create_file(led->dev, &dev_attr_blink);
-	if (ret)
-		led_classdev_unregister(led);
-	return ret;
+
+	return 0;
 }
 
 static void unregister_nasgpio_led(int led_nr)
 {
 	struct led_classdev *led = get_classdev_for_led_nr(led_nr);
 	led_classdev_unregister(led);
-	device_remove_file(led->dev, &dev_attr_blink);
 }
 /*
  * module load/initialization
Signature · 1 line
1.8.5.5
#11 lore raw

PATCH 10/13 leds: wm831x-status: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the src attribute during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-wm831x-status.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)
drivers/leds/leds-wm831x-status.c +9 -14
index e72c974142d0..1b71e0701002 100644
@@ -219,6 +219,12 @@ static ssize_t wm831x_status_src_store(struct device *dev,
 
 static DEVICE_ATTR(src, 0644, wm831x_status_src_show, wm831x_status_src_store);
 
+static struct attribute *wm831x_status_attrs[] = {
+	&dev_attr_src.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(wm831x_status);
+
 static int wm831x_status_probe(struct platform_device *pdev)
 {
 	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
@@ -232,8 +238,7 @@ static int wm831x_status_probe(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
 	if (res == NULL) {
 		dev_err(&pdev->dev, "No register resource\n");
-		ret = -EINVAL;
-		goto err;
+		return -EINVAL;
 	}
 
 	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_status),
@@ -284,31 +289,21 @@ static int wm831x_status_probe(struct platform_device *pdev)
 	drvdata->cdev.default_trigger = pdata.default_trigger;
 	drvdata->cdev.brightness_set = wm831x_status_set;
 	drvdata->cdev.blink_set = wm831x_status_blink_set;
+	drvdata->cdev.groups = wm831x_status_groups;
 
 	ret = led_classdev_register(wm831x->dev, &drvdata->cdev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to register LED: %d\n", ret);
-		goto err_led;
+		return ret;
 	}
 
-	ret = device_create_file(drvdata->cdev.dev, &dev_attr_src);
-	if (ret != 0)
-		dev_err(&pdev->dev,
-			"No source control for LED: %d\n", ret);
-
 	return 0;
-
-err_led:
-	led_classdev_unregister(&drvdata->cdev);
-err:
-	return ret;
 }
 
 static int wm831x_status_remove(struct platform_device *pdev)
 {
 	struct wm831x_status *drvdata = platform_get_drvdata(pdev);
 
-	device_remove_file(drvdata->cdev.dev, &dev_attr_src);
 	led_classdev_unregister(&drvdata->cdev);
 
 	return 0;
Signature · 1 line
1.8.5.5
#12 lore raw

PATCH 11/13 input: lm8323: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the time attribute during probe in order to avoid racing with userspace.

---
 drivers/input/keyboard/lm8323.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
drivers/input/keyboard/lm8323.c +9 -13
index 0b42118cbf8f..cb32e2b506b7 100644
@@ -558,6 +558,12 @@ static ssize_t lm8323_pwm_store_time(struct device *dev,
 }
 static DEVICE_ATTR(time, 0644, lm8323_pwm_show_time, lm8323_pwm_store_time);
 
+static struct attribute *lm8323_pwm_attrs[] = {
+	&dev_attr_time.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(lm8323_pwm);
+
 static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
 		    const char *name)
 {
@@ -580,16 +586,11 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
 	if (name) {
 		pwm->cdev.name = name;
 		pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
+		pwm->cdev.groups = lm8323_pwm_groups;
 		if (led_classdev_register(dev, &pwm->cdev) < 0) {
 			dev_err(dev, "couldn't register PWM %d\n", id);
 			return -1;
 		}
-		if (device_create_file(pwm->cdev.dev,
-					&dev_attr_time) < 0) {
-			dev_err(dev, "couldn't register time attribute\n");
-			led_classdev_unregister(&pwm->cdev);
-			return -1;
-		}
 		pwm->enabled = true;
 	}
 
@@ -753,11 +754,8 @@ fail3:
 	device_remove_file(&client->dev, &dev_attr_disable_kp);
 fail2:
 	while (--pwm >= 0)
-		if (lm->pwm[pwm].enabled) {
-			device_remove_file(lm->pwm[pwm].cdev.dev,
-					   &dev_attr_time);
+		if (lm->pwm[pwm].enabled)
 			led_classdev_unregister(&lm->pwm[pwm].cdev);
-		}
 fail1:
 	input_free_device(idev);
 	kfree(lm);
@@ -777,10 +775,8 @@ static int lm8323_remove(struct i2c_client *client)
 	device_remove_file(&lm->client->dev, &dev_attr_disable_kp);
 
 	for (i = 0; i < 3; i++)
-		if (lm->pwm[i].enabled) {
-			device_remove_file(lm->pwm[i].cdev.dev, &dev_attr_time);
+		if (lm->pwm[i].enabled)
 			led_classdev_unregister(&lm->pwm[i].cdev);
-		}
 
 	kfree(lm);
 
Signature · 1 line
1.8.5.5
#13 lore raw

Re: [PATCH 11/13] input: lm8323: fix attribute-creation race

Quoted 3 lines · Johan Hovold

Use the attribute groups of the led-class to create the time attribute during probe in order to avoid racing with userspace.

I'm about to merge this, but need an Ack from input subsystem maintainers. Dmitry or Jiri, are you OK with this patch?

-Bryan

Quoted 68 lines
---
 drivers/input/keyboard/lm8323.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
drivers/input/keyboard/lm8323.c +9 -14
index 0b42118cbf8f..cb32e2b506b7 100644
@@ -558,6 +558,12 @@ static ssize_t lm8323_pwm_store_time(struct device *dev,
 }
 static DEVICE_ATTR(time, 0644, lm8323_pwm_show_time, lm8323_pwm_store_time);
 
+static struct attribute *lm8323_pwm_attrs[] = {
+       &dev_attr_time.attr,
+       NULL
+};
+ATTRIBUTE_GROUPS(lm8323_pwm);
+
 static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
                    const char *name)
 {
@@ -580,16 +586,11 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
        if (name) {
                pwm->cdev.name = name;
                pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
+               pwm->cdev.groups = lm8323_pwm_groups;
                if (led_classdev_register(dev, &pwm->cdev) < 0) {
                        dev_err(dev, "couldn't register PWM %d\n", id);
                        return -1;
                }
-               if (device_create_file(pwm->cdev.dev,
-                                       &dev_attr_time) < 0) {
-                       dev_err(dev, "couldn't register time attribute\n");
-                       led_classdev_unregister(&pwm->cdev);
-                       return -1;
-               }
                pwm->enabled = true;
        }
 
@@ -753,11 +754,8 @@ fail3:
        device_remove_file(&client->dev, &dev_attr_disable_kp);
 fail2:
        while (--pwm >= 0)
-               if (lm->pwm[pwm].enabled) {
-                       device_remove_file(lm->pwm[pwm].cdev.dev,
-                                          &dev_attr_time);
+               if (lm->pwm[pwm].enabled)
                        led_classdev_unregister(&lm->pwm[pwm].cdev);
-               }
 fail1:
        input_free_device(idev);
        kfree(lm);
@@ -777,10 +775,8 @@ static int lm8323_remove(struct i2c_client *client)
        device_remove_file(&lm->client->dev, &dev_attr_disable_kp);
 
        for (i = 0; i < 3; i++)
-               if (lm->pwm[i].enabled) {
-                       device_remove_file(lm->pwm[i].cdev.dev, &dev_attr_time);
+               if (lm->pwm[i].enabled)
                        led_classdev_unregister(&lm->pwm[i].cdev);
-               }
 
        kfree(lm);
 
--

1.8.5.5

maintainer Acked-by on v1 11/13
#14 lore raw

Re: [PATCH 11/13] input: lm8323: fix attribute-creation race

In reply to Johan Hovold
Quoted 4 lines · Johan Hovold

Use the attribute groups of the led-class to create the time attribute during probe in order to avoid racing with userspace.

Quoted 65 lines
---
 drivers/input/keyboard/lm8323.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
drivers/input/keyboard/lm8323.c +9 -14
index 0b42118cbf8f..cb32e2b506b7 100644
@@ -558,6 +558,12 @@ static ssize_t lm8323_pwm_store_time(struct device *dev,
 }
 static DEVICE_ATTR(time, 0644, lm8323_pwm_show_time, lm8323_pwm_store_time);
 
+static struct attribute *lm8323_pwm_attrs[] = {
+       &dev_attr_time.attr,
+       NULL
+};
+ATTRIBUTE_GROUPS(lm8323_pwm);
+
 static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
                    const char *name)
 {
@@ -580,16 +586,11 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
        if (name) {
                pwm->cdev.name = name;
                pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
+               pwm->cdev.groups = lm8323_pwm_groups;
                if (led_classdev_register(dev, &pwm->cdev) < 0) {
                        dev_err(dev, "couldn't register PWM %d\n", id);
                        return -1;
                }
-               if (device_create_file(pwm->cdev.dev,
-                                       &dev_attr_time) < 0) {
-                       dev_err(dev, "couldn't register time attribute\n");
-                       led_classdev_unregister(&pwm->cdev);
-                       return -1;
-               }
                pwm->enabled = true;
        }
 
@@ -753,11 +754,8 @@ fail3:
        device_remove_file(&client->dev, &dev_attr_disable_kp);
 fail2:
        while (--pwm >= 0)
-               if (lm->pwm[pwm].enabled) {
-                       device_remove_file(lm->pwm[pwm].cdev.dev,
-                                          &dev_attr_time);
+               if (lm->pwm[pwm].enabled)
                        led_classdev_unregister(&lm->pwm[pwm].cdev);
-               }
 fail1:
        input_free_device(idev);
        kfree(lm);
@@ -777,10 +775,8 @@ static int lm8323_remove(struct i2c_client *client)
        device_remove_file(&lm->client->dev, &dev_attr_disable_kp);
 
        for (i = 0; i < 3; i++)
-               if (lm->pwm[i].enabled) {
-                       device_remove_file(lm->pwm[i].cdev.dev, &dev_attr_time);
+               if (lm->pwm[i].enabled)
                        led_classdev_unregister(&lm->pwm[i].cdev);
-               }
 
        kfree(lm);
 
--
Quoted 2 lines

1.8.5.5

Thanks.

Signature · 1 line
Dmitry
#15 lore raw

Re: [PATCH 11/13] input: lm8323: fix attribute-creation race

Quoted 8 lines · Dmitry Torokhov
Quoted 4 lines · Johan Hovold

Use the attribute groups of the led-class to create the time attribute during probe in order to avoid racing with userspace.

Good, I merged this patch with others into my for-next branch.

-Bryan

Quoted 72 lines
Quoted 65 lines
---
 drivers/input/keyboard/lm8323.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
drivers/input/keyboard/lm8323.c +9 -14
index 0b42118cbf8f..cb32e2b506b7 100644
@@ -558,6 +558,12 @@ static ssize_t lm8323_pwm_store_time(struct device *dev,
 }
 static DEVICE_ATTR(time, 0644, lm8323_pwm_show_time, lm8323_pwm_store_time);
 
+static struct attribute *lm8323_pwm_attrs[] = {
+       &dev_attr_time.attr,
+       NULL
+};
+ATTRIBUTE_GROUPS(lm8323_pwm);
+
 static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
                    const char *name)
 {
@@ -580,16 +586,11 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
        if (name) {
                pwm->cdev.name = name;
                pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
+               pwm->cdev.groups = lm8323_pwm_groups;
                if (led_classdev_register(dev, &pwm->cdev) < 0) {
                        dev_err(dev, "couldn't register PWM %d\n", id);
                        return -1;
                }
-               if (device_create_file(pwm->cdev.dev,
-                                       &dev_attr_time) < 0) {
-                       dev_err(dev, "couldn't register time attribute\n");
-                       led_classdev_unregister(&pwm->cdev);
-                       return -1;
-               }
                pwm->enabled = true;
        }
 
@@ -753,11 +754,8 @@ fail3:
        device_remove_file(&client->dev, &dev_attr_disable_kp);
 fail2:
        while (--pwm >= 0)
-               if (lm->pwm[pwm].enabled) {
-                       device_remove_file(lm->pwm[pwm].cdev.dev,
-                                          &dev_attr_time);
+               if (lm->pwm[pwm].enabled)
                        led_classdev_unregister(&lm->pwm[pwm].cdev);
-               }
 fail1:
        input_free_device(idev);
        kfree(lm);
@@ -777,10 +775,8 @@ static int lm8323_remove(struct i2c_client *client)
        device_remove_file(&lm->client->dev, &dev_attr_disable_kp);
 
        for (i = 0; i < 3; i++)
-               if (lm->pwm[i].enabled) {
-                       device_remove_file(lm->pwm[i].cdev.dev, &dev_attr_time);
+               if (lm->pwm[i].enabled)
                        led_classdev_unregister(&lm->pwm[i].cdev);
-               }
 
        kfree(lm);
 
--
Quoted 2 lines

1.8.5.5

Thanks.

--
Dmitry

#16 lore raw

PATCH 12/13 leds: lp55xx-common: fix sysfs entry leak

In reply to Johan Hovold

Make sure the sysfs group is removed when the LEDs are unregistered.

---
 drivers/leds/leds-lp55xx-common.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/leds/leds-lp55xx-common.c +2 -0
index 88317b4f7bf3..3fbfb31602c7 100644
@@ -520,6 +520,8 @@ void lp55xx_unregister_leds(struct lp55xx_led *led, struct lp55xx_chip *chip)
 
 	for (i = 0; i < chip->num_leds; i++) {
 		each = led + i;
+		sysfs_remove_group(&each->cdev.dev->kobj,
+						&lp55xx_led_attr_group);
 		led_classdev_unregister(&each->cdev);
 		flush_work(&each->brightness_work);
 	}
Signature · 1 line
1.8.5.5
#17 lore raw

PATCH 13/13 leds: lp55xx-common: fix attribute-creation race

In reply to Johan Hovold

Use the attribute groups of the led-class to create the LED attributes during probe in order to avoid racing with userspace.

---
 drivers/leds/leds-lp55xx-common.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)
drivers/leds/leds-lp55xx-common.c +3 -19
index 3fbfb31602c7..77c26bc32eed 100644
@@ -127,15 +127,12 @@ static DEVICE_ATTR(led_current, S_IRUGO | S_IWUSR, lp55xx_show_current,
 		lp55xx_store_current);
 static DEVICE_ATTR(max_current, S_IRUGO , lp55xx_show_max_current, NULL);
 
-static struct attribute *lp55xx_led_attributes[] = {
+static struct attribute *lp55xx_led_attrs[] = {
 	&dev_attr_led_current.attr,
 	&dev_attr_max_current.attr,
 	NULL,
 };
-
-static struct attribute_group lp55xx_led_attr_group = {
-	.attrs = lp55xx_led_attributes
-};
+ATTRIBUTE_GROUPS(lp55xx_led);
 
 static void lp55xx_set_brightness(struct led_classdev *cdev,
 			     enum led_brightness brightness)
@@ -176,6 +173,7 @@ static int lp55xx_init_led(struct lp55xx_led *led,
 	}
 
 	led->cdev.brightness_set = lp55xx_set_brightness;
+	led->cdev.groups = lp55xx_led_groups;
 
 	if (pdata->led_config[chan].name) {
 		led->cdev.name = pdata->led_config[chan].name;
@@ -185,24 +183,12 @@ static int lp55xx_init_led(struct lp55xx_led *led,
 		led->cdev.name = name;
 	}
 
-	/*
-	 * register led class device for each channel and
-	 * add device attributes
-	 */
-
 	ret = led_classdev_register(dev, &led->cdev);
 	if (ret) {
 		dev_err(dev, "led register err: %d\n", ret);
 		return ret;
 	}
 
-	ret = sysfs_create_group(&led->cdev.dev->kobj, &lp55xx_led_attr_group);
-	if (ret) {
-		dev_err(dev, "led sysfs err: %d\n", ret);
-		led_classdev_unregister(&led->cdev);
-		return ret;
-	}
-
 	return 0;
 }
 
@@ -520,8 +506,6 @@ void lp55xx_unregister_leds(struct lp55xx_led *led, struct lp55xx_chip *chip)
 
 	for (i = 0; i < chip->num_leds; i++) {
 		each = led + i;
-		sysfs_remove_group(&each->cdev.dev->kobj,
-						&lp55xx_led_attr_group);
 		led_classdev_unregister(&each->cdev);
 		flush_work(&each->brightness_work);
 	}
Signature · 1 line
1.8.5.5
#18 lore raw
In reply to Johan Hovold
Quoted 13 lines · Johan Hovold

LED-class device attributes should be created using the attribute groups of struct device, rather than be added manually after the device has been registered in order to avoid racing with userspace.

The first patch adds an attribute groups field to struct led_classdev, which is used during registration to add device attributes in a race-free manner.

The remaining patches convert the current racy LED-device attribute creations to use the new facility. Hopefully my grep-patterns have caught most instances. Note that this also tends to simplify probe error paths.

Quoted 7 lines

The lp55xx-common failed to remove the attribute group it is currently creating, something which is fixed separately.

Note that this series include one driver from drivers/input/keyboard.

The individual led-driver patches have been compile tested only.

Thanks a lot for driving this. I will applied this patchset into my -devel branch. After I got the Ack from Input guys, I will apply it to my for-next branch then.

-Bryan

Quoted 36 lines

Johan

Johan Hovold (13):
leds: add led-class attribute-group support
leds: lm3550: fix attribute-creation race
leds: lm3533: fix attribute-creation race
leds: lm355x: fix attribute-creation race
leds: lm3642: fix attribute-creation race
leds: max8997: fix attribute-creation race
leds: netxbig: fix attribute-creation race
leds: ns2: fix attribute-creation race
leds: ss4200: fix attribute-creation race
leds: wm831x-status: fix attribute-creation race
input: lm8323: fix attribute-creation race
leds: lp55xx-common: fix sysfs entry leak
leds: lp55xx-common: fix attribute-creation race

 drivers/input/keyboard/lm8323.c   | 22 +++++++++-------------
 drivers/leds/led-class.c          |  5 +++--
 drivers/leds/leds-lm3530.c        | 20 +++++++-------------
 drivers/leds/leds-lm3533.c        | 20 ++++++++------------
 drivers/leds/leds-lm355x.c        | 21 +++++++++------------
 drivers/leds/leds-lm3642.c        | 30 ++++++++++++++----------------
 drivers/leds/leds-lp55xx-common.c | 20 +++-----------------
 drivers/leds/leds-max8997.c       | 16 +++++++---------
 drivers/leds/leds-netxbig.c       | 26 ++++++++++++--------------
 drivers/leds/leds-ns2.c           | 16 +++++++---------
 drivers/leds/leds-ss4200.c        | 14 +++++++++-----
 drivers/leds/leds-wm831x-status.c | 23 +++++++++--------------
 include/linux/leds.h              |  2 ++
 13 files changed, 99 insertions(+), 136 deletions(-)

--
1.8.5.5

#19 lore raw
Quoted 17 lines · Bryan Wu
Quoted 16 lines · Johan Hovold

LED-class device attributes should be created using the attribute groups of struct device, rather than be added manually after the device has been registered in order to avoid racing with userspace.

The first patch adds an attribute groups field to struct led_classdev, which is used during registration to add device attributes in a race-free manner.

The remaining patches convert the current racy LED-device attribute creations to use the new facility. Hopefully my grep-patterns have caught most instances. Note that this also tends to simplify probe error paths.

The lp55xx-common failed to remove the attribute group it is currently creating, something which is fixed separately.

Quoted 8 lines
Quoted 4 lines

Note that this series include one driver from drivers/input/keyboard.

The individual led-driver patches have been compile tested only.

Thanks a lot for driving this. I will applied this patchset into my -devel branch. After I got the Ack from Input guys, I will apply it to my for-next branch then.

Series looks good to me, Johan, thanks for doing this work.

greg k-h

#20 lore raw
Quoted 5 lines · Greg Kroah-Hartman
Quoted 3 lines

Thanks a lot for driving this. I will applied this patchset into my -devel branch. After I got the Ack from Input guys, I will apply it to my for-next branch then.

Series looks good to me, Johan, thanks for doing this work.

Yeah, looks good to me as well, thanks. The lm8323 change should be Acked by Dmitry though, as it's in his area.

Signature · 2 lines
Jiri Kosina
SUSE Labs