Skip to contentSkip to conversation

staging: greybus: light: fix leak of cdev->name

PATCH In review Cong Nguyen 3 participants 3 messages Aug 2 – Sep 1

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

PATCH v2 1/1 staging: greybus: light: fix leak of cdev->name

gb_lights_channel_config() builds the LED classdev name with kasprintf() and stores it in cdev->name for every channel during the configuration phase, which runs before the channel is registered.

That name was only released by __gb_lights_led_unregister(), i.e. only for a normal LED channel that was actually registered. It was therefore leaked in two cases:

- a channel that is configured but never registered, e.g. channel_attr_groups_set() fails, the flash configuration fails, or a later channel in the same light fails to configure and the whole light is torn down; the release path calls gb_lights_channel_unregister() (which returns early because the channel is not registered) and then gb_lights_channel_free();

- a flash, torch or indicator channel, whose unregister path (__gb_lights_flash_led_unregister()) never releases cdev->name at all, so the name leaks even on a successful teardown.

cdev->name is a configuration-phase allocation, like channel->color_name and channel->mode_name. Free it in gb_lights_channel_free() next to those, since that runs unconditionally on every teardown path, and drop the special case free from __gb_lights_led_unregister(). gb_lights_channel_unregister() is only ever called from gb_lights_channel_release(), immediately followed by gb_lights_channel_free(), so the name is still released on the registered path and is freed exactly once.

Commit 04820da21050 ("staging: greybus: light: Release memory obtained by kasprintf") fixed the leak for the registered normal-LED path only; this covers the remaining cases.

Fixes 2870b52bae4c ("greybus: lights: add lights implementation")
Assisted-by Claude:claude-opus-4

---
Changes in v2:
- Add Assisted-by: tag to document AI assistance, per Documentation/process/coding-assistants.rst (Greg KH).
- No functional change.

 drivers/staging/greybus/light.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/staging/greybus/light.c +3 -2
index cab02b5da867..1ecca479b5f4 100644
@@ -905,8 +905,6 @@ static void __gb_lights_led_unregister(struct gb_channel *channel)
 		return;
 
 	led_classdev_unregister(cdev);
-	kfree(cdev->name);
-	cdev->name = NULL;
 	channel->led = NULL;
 }
 
@@ -1063,6 +1061,9 @@ static int gb_lights_light_register(struct gb_light *light)
 
 static void gb_lights_channel_free(struct gb_channel *channel)
 {
+	struct led_classdev *cdev = get_channel_cdev(channel);
+
+	kfree(cdev->name);
 	kfree(channel->attrs);
 	kfree(channel->attr_group);
 	kfree(channel->attr_groups);
Signature · 1 line
2.25.1
Reviewed-by on v2 1/1
#1 lore raw

Hi Cong,
Thanks for the patch.

Quoted 28 lines · Cong Nguyen

gb_lights_channel_config() builds the LED classdev name with kasprintf() and stores it in cdev->name for every channel during the configuration phase, which runs before the channel is registered.

That name was only released by __gb_lights_led_unregister(), i.e. only for a normal LED channel that was actually registered. It was therefore leaked in two cases:

- a channel that is configured but never registered, e.g. channel_attr_groups_set() fails, the flash configuration fails, or a later channel in the same light fails to configure and the whole light is torn down; the release path calls gb_lights_channel_unregister() (which returns early because the channel is not registered) and then gb_lights_channel_free();

- a flash, torch or indicator channel, whose unregister path (__gb_lights_flash_led_unregister()) never releases cdev->name at all, so the name leaks even on a successful teardown.

cdev->name is a configuration-phase allocation, like channel->color_name and channel->mode_name. Free it in gb_lights_channel_free() next to those, since that runs unconditionally on every teardown path, and drop the special case free from __gb_lights_led_unregister(). gb_lights_channel_unregister() is only ever called from gb_lights_channel_release(), immediately followed by gb_lights_channel_free(), so the name is still released on the registered path and is freed exactly once.

Quoted 7 lines

Commit 04820da21050 ("staging: greybus: light: Release memory obtained by kasprintf") fixed the leak for the registered normal-LED path only; this covers the remaining cases.

Fixes 2870b52bae4c ("greybus: lights: add lights implementation")
Assisted-by Claude:claude-opus-4

Good Catch, LGTM.

Cheers,
RUi

Quoted 34 lines

---
Changes in v2:
- Add Assisted-by: tag to document AI assistance, per Documentation/process/coding-assistants.rst (Greg KH).
- No functional change.

 drivers/staging/greybus/light.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/staging/greybus/light.c +3 -2
index cab02b5da867..1ecca479b5f4 100644
@@ -905,8 +905,6 @@ static void __gb_lights_led_unregister(struct gb_channel *channel)
 		return;
 
 	led_classdev_unregister(cdev);
-	kfree(cdev->name);
-	cdev->name = NULL;
 	channel->led = NULL;
 }
 
@@ -1063,6 +1061,9 @@ static int gb_lights_light_register(struct gb_light *light)
 
 static void gb_lights_channel_free(struct gb_channel *channel)
 {
+	struct led_classdev *cdev = get_channel_cdev(channel);
+
+	kfree(cdev->name);
 	kfree(channel->attrs);
 	kfree(channel->attr_group);
 	kfree(channel->attr_groups);
Signature · 1 line
2.25.1
#2 lore raw
In reply to Cong Nguyen
Quoted 32 lines · Cong Nguyen

gb_lights_channel_config() builds the LED classdev name with kasprintf() and stores it in cdev->name for every channel during the configuration phase, which runs before the channel is registered.

That name was only released by __gb_lights_led_unregister(), i.e. only for a normal LED channel that was actually registered. It was therefore leaked in two cases:

- a channel that is configured but never registered, e.g. channel_attr_groups_set() fails, the flash configuration fails, or a later channel in the same light fails to configure and the whole light is torn down; the release path calls gb_lights_channel_unregister() (which returns early because the channel is not registered) and then gb_lights_channel_free();

- a flash, torch or indicator channel, whose unregister path (__gb_lights_flash_led_unregister()) never releases cdev->name at all, so the name leaks even on a successful teardown.

cdev->name is a configuration-phase allocation, like channel->color_name and channel->mode_name. Free it in gb_lights_channel_free() next to those, since that runs unconditionally on every teardown path, and drop the special case free from __gb_lights_led_unregister(). gb_lights_channel_unregister() is only ever called from gb_lights_channel_release(), immediately followed by gb_lights_channel_free(), so the name is still released on the registered path and is freed exactly once.

Commit 04820da21050 ("staging: greybus: light: Release memory obtained by kasprintf") fixed the leak for the registered normal-LED path only; this covers the remaining cases.

Quoted 8 lines
Fixes 2870b52bae4c ("greybus: lights: add lights implementation")
Assisted-by Claude:claude-opus-4

---
Changes in v2:
- Add Assisted-by: tag to document AI assistance, per Documentation/process/coding-assistants.rst (Greg KH).
- No functional change.

Please see https://lore.kernel.org/r/2026080354-skater-urgent-31b2@gregkh

sorry,

greg k-h