* [PATCH 2/3] platform/x86: think-lmi: Split kobject_init() and kobject_add() calls
2021-07-17 14:36 [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir Hans de Goede
@ 2021-07-17 14:36 ` Hans de Goede
2021-07-17 14:36 ` [PATCH 3/3] platform/x86: think-lmi: Fix possible mem-leaks on tlmi_analyze() error-exit Hans de Goede
2021-07-17 15:27 ` [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2021-07-17 14:36 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko, Mark Pearson
Cc: Hans de Goede, platform-driver-x86
tlmi_sysfs_init() calls tlmi_release_attr() on errors which calls
kobject_put() for attributes created by tlmi_analyze(), but if we
bail early because of an error, then this means that some of the
kobjects will not have been initialized yet; and we should thus not
call kobject_put() on them.
Switch from using kobject_init_and_add() inside tlmi_sysfs_init() to
initializing all the created kobjects directly in tlmi_analyze() and
only adding them from tlmi_sysfs_init(). This way all kobjects will
always be initialized when tlmi_release_attr() gets called.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/platform/x86/think-lmi.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 989a8221dcd8..c22edcf26aaa 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -723,8 +723,8 @@ static int tlmi_sysfs_init(void)
/* Build attribute */
tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset;
- ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &tlmi_attr_setting_ktype,
- NULL, "%s", tlmi_priv.setting[i]->display_name);
+ ret = kobject_add(&tlmi_priv.setting[i]->kobj, NULL,
+ "%s", tlmi_priv.setting[i]->display_name);
if (ret)
goto fail_create_attr;
@@ -745,8 +745,7 @@ static int tlmi_sysfs_init(void)
goto fail_create_attr;
}
tlmi_priv.pwd_admin->kobj.kset = tlmi_priv.authentication_kset;
- ret = kobject_init_and_add(&tlmi_priv.pwd_admin->kobj, &tlmi_pwd_setting_ktype,
- NULL, "%s", "Admin");
+ ret = kobject_add(&tlmi_priv.pwd_admin->kobj, NULL, "%s", "Admin");
if (ret)
goto fail_create_attr;
@@ -755,8 +754,7 @@ static int tlmi_sysfs_init(void)
goto fail_create_attr;
tlmi_priv.pwd_power->kobj.kset = tlmi_priv.authentication_kset;
- ret = kobject_init_and_add(&tlmi_priv.pwd_power->kobj, &tlmi_pwd_setting_ktype,
- NULL, "%s", "System");
+ ret = kobject_add(&tlmi_priv.pwd_power->kobj, NULL, "%s", "System");
if (ret)
goto fail_create_attr;
@@ -836,6 +834,7 @@ static int tlmi_analyze(void)
pr_info("Error retrieving possible values for %d : %s\n",
i, setting->display_name);
}
+ kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
tlmi_priv.setting[i] = setting;
tlmi_priv.settings_count++;
kfree(item);
@@ -862,6 +861,8 @@ static int tlmi_analyze(void)
if (pwdcfg.password_state & TLMI_PAP_PWD)
tlmi_priv.pwd_admin->valid = true;
+ kobject_init(&tlmi_priv.pwd_admin->kobj, &tlmi_pwd_setting_ktype);
+
tlmi_priv.pwd_power = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
if (!tlmi_priv.pwd_power) {
ret = -ENOMEM;
@@ -877,6 +878,8 @@ static int tlmi_analyze(void)
if (pwdcfg.password_state & TLMI_POP_PWD)
tlmi_priv.pwd_power->valid = true;
+ kobject_init(&tlmi_priv.pwd_power->kobj, &tlmi_pwd_setting_ktype);
+
return 0;
fail_clear_attr:
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] platform/x86: think-lmi: Fix possible mem-leaks on tlmi_analyze() error-exit
2021-07-17 14:36 [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir Hans de Goede
2021-07-17 14:36 ` [PATCH 2/3] platform/x86: think-lmi: Split kobject_init() and kobject_add() calls Hans de Goede
@ 2021-07-17 14:36 ` Hans de Goede
2021-07-17 15:27 ` [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2021-07-17 14:36 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko, Mark Pearson
Cc: Hans de Goede, platform-driver-x86
Fix 2 possible memleaks on error-exits from tlmi_analyze():
1. If the kzalloc of pwd_power fails, then not only free the atributes,
but also the allocated pwd_admin struct.
2. Freeing the attributes should also free the possible_values strings.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/platform/x86/think-lmi.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index c22edcf26aaa..6cfed4427fb0 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -866,7 +866,7 @@ static int tlmi_analyze(void)
tlmi_priv.pwd_power = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
if (!tlmi_priv.pwd_power) {
ret = -ENOMEM;
- goto fail_clear_attr;
+ goto fail_free_pwd_admin;
}
strscpy(tlmi_priv.pwd_power->kbdlang, "us", TLMI_LANG_MAXLEN);
tlmi_priv.pwd_power->encoding = TLMI_ENCODING_ASCII;
@@ -882,9 +882,15 @@ static int tlmi_analyze(void)
return 0;
+fail_free_pwd_admin:
+ kfree(tlmi_priv.pwd_admin);
fail_clear_attr:
- for (i = 0; i < TLMI_SETTINGS_COUNT; ++i)
- kfree(tlmi_priv.setting[i]);
+ for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
+ if (tlmi_priv.setting[i]) {
+ kfree(tlmi_priv.setting[i]->possible_values);
+ kfree(tlmi_priv.setting[i]);
+ }
+ }
return ret;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir
2021-07-17 14:36 [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir Hans de Goede
2021-07-17 14:36 ` [PATCH 2/3] platform/x86: think-lmi: Split kobject_init() and kobject_add() calls Hans de Goede
2021-07-17 14:36 ` [PATCH 3/3] platform/x86: think-lmi: Fix possible mem-leaks on tlmi_analyze() error-exit Hans de Goede
@ 2021-07-17 15:27 ` Hans de Goede
2021-07-19 12:49 ` [External]Re: " Mark Pearson
2 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2021-07-17 15:27 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko, Mark Pearson; +Cc: platform-driver-x86
Hi,
On 7/17/21 4:36 PM, Hans de Goede wrote:
> From: Mark Pearson <markpearson@lenovo.com>
>
> Move the pending_reboot node under attributes dir where it should live, as
> documented in: Documentation/ABI/testing/sysfs-class-firmware-attributes.
>
> Also move the create / remove code to be together with the other code
> populating / cleaning the attributes sysfs dir. In the removal path this
> is necessary so that the remove is done before the
> kset_unregister(tlmi_priv.attribute_kset) call.
>
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> Co-developed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
I've added this series to my review-hans and the pdx86/fixes
branches now.
Regards,
Hans
> ---
> drivers/platform/x86/think-lmi.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 64dcec53a7a0..989a8221dcd8 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -672,6 +672,7 @@ static void tlmi_release_attr(void)
> kobject_put(&tlmi_priv.setting[i]->kobj);
> }
> }
> + sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
> kset_unregister(tlmi_priv.attribute_kset);
>
> /* Authentication structures */
> @@ -680,7 +681,6 @@ static void tlmi_release_attr(void)
> sysfs_remove_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group);
> kobject_put(&tlmi_priv.pwd_power->kobj);
> kset_unregister(tlmi_priv.authentication_kset);
> - sysfs_remove_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
> }
>
> static int tlmi_sysfs_init(void)
> @@ -733,6 +733,10 @@ static int tlmi_sysfs_init(void)
> goto fail_create_attr;
> }
>
> + ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
> + if (ret)
> + goto fail_create_attr;
> +
> /* Create authentication entries */
> tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL,
> &tlmi_priv.class_dev->kobj);
> @@ -760,11 +764,6 @@ static int tlmi_sysfs_init(void)
> if (ret)
> goto fail_create_attr;
>
> - /* Create global sysfs files */
> - ret = sysfs_create_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
> - if (ret)
> - goto fail_create_attr;
> -
> return ret;
>
> fail_create_attr:
>
^ permalink raw reply [flat|nested] 5+ messages in thread