My previous commit introduced a memory leak where the item allocated
from tlmi_setting was not freed.
This commit also renames it to avoid confusion with the similarly name
variable in the same function.
Fixes: 8a02d70679fc ("platform/x86: think-lmi: Add possible_values for ThinkStation")
Reported-by: Mirsad Todorovac <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/T/
Signed-off-by: Mark Pearson <[email protected]>
---
This patch series builds on top of the proposed patch from Armin Wolf
"platform/x86: think-lmi: Fix memory leak when showing current settings"
Changes in v2: None, but rebased series because of issues with second
patch.
drivers/platform/x86/think-lmi.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 6034df6d577d..87f832142d8d 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1459,10 +1459,10 @@ static int tlmi_analyze(void)
* name string.
* Try and pull that out if it's available.
*/
- char *item, *optstart, *optend;
+ char *optitem, *optstart, *optend;
- if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
- optstart = strstr(item, "[Optional:");
+ if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
+ optstart = strstr(optitem, "[Optional:");
if (optstart) {
optstart += strlen("[Optional:");
optend = strstr(optstart, "]");
@@ -1471,6 +1471,7 @@ static int tlmi_analyze(void)
kstrndup(optstart, optend - optstart,
GFP_KERNEL);
}
+ kfree(optitem);
}
}
/*
--
2.39.2
On ThinkStations on retrieving the attribute value the BIOS appends the
possible values to the string.
Clean up the display in the current_value_show function so the options
part is not displayed.
Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
Reported by Mario Limoncello <[email protected]>
Link: https://github.com/fwupd/fwupd/issues/5077#issuecomment-1488730526
Signed-off-by: Mark Pearson <[email protected]>
---
Changes in v2: For some reason v2 doesn't apply cleanly so rebased and
started again. Hopefully this one works
drivers/platform/x86/think-lmi.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 87f832142d8d..78dc82bda4dd 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -920,7 +920,7 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
- char *item, *value;
+ char *item, *value, *p;
int ret;
ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
@@ -931,9 +931,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
value = strpbrk(item, ",");
if (!value || value == item || !strlen(value + 1))
ret = -EINVAL;
- else
+ else {
+ /* On Workstations remove the Options part after the value */
+ p = strchrnul(value, ';');
+ *p = '\0';
ret = sysfs_emit(buf, "%s\n", value + 1);
-
+ }
kfree(item);
return ret;
--
2.39.2
On 4/2/23 20:31, Mark Pearson wrote:
> On ThinkStations on retrieving the attribute value the BIOS appends the
> possible values to the string.
> Clean up the display in the current_value_show function so the options
> part is not displayed.
>
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Reported by Mario Limoncello <[email protected]>
> Link: https://github.com/fwupd/fwupd/issues/5077#issuecomment-1488730526
> Signed-off-by: Mark Pearson <[email protected]>
> ---
> Changes in v2: For some reason v2 doesn't apply cleanly so rebased and
> started again. Hopefully this one works
Tested-by: Mario Limonciello <[email protected]>
tested on 6.3rc5 + prerequisite Armin's patch.
>
> drivers/platform/x86/think-lmi.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 87f832142d8d..78dc82bda4dd 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -920,7 +920,7 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
> static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> - char *item, *value;
> + char *item, *value, *p;
> int ret;
>
> ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
> @@ -931,9 +931,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
> value = strpbrk(item, ",");
> if (!value || value == item || !strlen(value + 1))
> ret = -EINVAL;
> - else
> + else {
> + /* On Workstations remove the Options part after the value */
> + p = strchrnul(value, ';');
> + *p = '\0';
> ret = sysfs_emit(buf, "%s\n", value + 1);
> -
> + }
> kfree(item);
>
> return ret;
On 3.4.2023. 3:31, Mark Pearson wrote:
> On ThinkStations on retrieving the attribute value the BIOS appends the
> possible values to the string.
> Clean up the display in the current_value_show function so the options
> part is not displayed.
>
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Reported by Mario Limoncello <[email protected]>
> Link: https://github.com/fwupd/fwupd/issues/5077#issuecomment-1488730526
> Signed-off-by: Mark Pearson <[email protected]>
> ---
> Changes in v2: For some reason v2 doesn't apply cleanly so rebased and
> started again. Hopefully this one works
>
> drivers/platform/x86/think-lmi.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 87f832142d8d..78dc82bda4dd 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -920,7 +920,7 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
> static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> - char *item, *value;
> + char *item, *value, *p;
> int ret;
>
> ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
> @@ -931,9 +931,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
> value = strpbrk(item, ",");
> if (!value || value == item || !strlen(value + 1))
> ret = -EINVAL;
> - else
> + else {
> + /* On Workstations remove the Options part after the value */
> + p = strchrnul(value, ';');
> + *p = '\0';
> ret = sysfs_emit(buf, "%s\n", value + 1);
> -
> + }
> kfree(item);
>
> return ret;
I can confirm the build against the Torvalds 6.3-rc5 tree and Armin's patch
applied w these.
[root@pc-mtodorov kernel]# uname -rms
Linux 6.3.0-rc5-mt-20230401-00005-g10de4cefccf7 x86_64
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# echo scan > !$
echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# echo scan > /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]# cat /sys/kernel/debug/kmemleak
[root@pc-mtodorov kernel]#
The leak is apparently gone in the original setup that reproduced the leak.
At your convenience, you can please add
Tested-by: Mirsad Goran Todorovac <[email protected]>
Have a nice day.
Best regards,
Mirsad
--
Mirsad Todorovac
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb
Republic of Croatia, the European Union
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
Hi,
On 4/3/23 03:31, Mark Pearson wrote:
> My previous commit introduced a memory leak where the item allocated
> from tlmi_setting was not freed.
> This commit also renames it to avoid confusion with the similarly name
> variable in the same function.
>
> Fixes: 8a02d70679fc ("platform/x86: think-lmi: Add possible_values for ThinkStation")
> Reported-by: Mirsad Todorovac <[email protected]>
> Link: https://lore.kernel.org/lkml/[email protected]/T/
> Signed-off-by: Mark Pearson <[email protected]>
> ---
> This patch series builds on top of the proposed patch from Armin Wolf
> "platform/x86: think-lmi: Fix memory leak when showing current settings"
> Changes in v2: None, but rebased series because of issues with second
> patch.
Thank you for your patch series, I've applied this series
to my fixes branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes
Note it will show up in my fixes branch once I've pushed my
local branch there, which might take a while.
I will include this series in my next fixes pull-req to Linus
for the current kernel development cycle.
Regards,
Hans
>
> drivers/platform/x86/think-lmi.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 6034df6d577d..87f832142d8d 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1459,10 +1459,10 @@ static int tlmi_analyze(void)
> * name string.
> * Try and pull that out if it's available.
> */
> - char *item, *optstart, *optend;
> + char *optitem, *optstart, *optend;
>
> - if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
> - optstart = strstr(item, "[Optional:");
> + if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
> + optstart = strstr(optitem, "[Optional:");
> if (optstart) {
> optstart += strlen("[Optional:");
> optend = strstr(optstart, "]");
> @@ -1471,6 +1471,7 @@ static int tlmi_analyze(void)
> kstrndup(optstart, optend - optstart,
> GFP_KERNEL);
> }
> + kfree(optitem);
> }
> }
> /*