[PATCH v2 1/4] platform/x86: hp-bioscfg: Remove unused obj in hp_add_other_attributes()

[PATCH v2 1/4] platform/x86: hp-bioscfg: Remove unused obj in hp_add_other_attributes()

[Harshit Mogalapalli]

acpi_object *obj is unused in this function, so delete it, also
delete a unnecessary kfree(obj);

Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 5798b49ddaba..0b563582d90d 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -575,82 +575,80 @@ static void release_attributes_data(void)
 /**
  * hp_add_other_attributes() - Initialize HP custom attributes not
  * reported by BIOS and required to support Secure Platform and Sure
  * Start.
  *
  * @attr_type: Custom HP attribute not reported by BIOS
  *
  * Initialize all 2 types of attributes: Platform and Sure Start
  * object.  Populates each attribute types respective properties
  * under sysfs files.
  *
  * Returns zero(0) if successful. Otherwise, a negative value.
  */
 static int hp_add_other_attributes(int attr_type)
 {
 	struct kobject *attr_name_kobj;
-	union acpi_object *obj = NULL;
 	int ret;
 	char *attr_name;
 
 	mutex_lock(&bioscfg_drv.mutex);
 
 	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
 	if (!attr_name_kobj) {
 		ret = -ENOMEM;
 		goto err_other_attr_init;
 	}
 
 	/* Check if attribute type is supported */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
 		attr_name = SPM_STR;
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
 		attr_name = SURE_START_STR;
 		break;
 
 	default:
 		pr_err("Error: Unknown attr_type: %d\n", attr_type);
 		ret = -EINVAL;
 		goto err_other_attr_init;
 	}
 
 	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
 				   NULL, "%s", attr_name);
 	if (ret) {
 		pr_err("Error encountered [%d]\n", ret);
 		kobject_put(attr_name_kobj);
 		goto err_other_attr_init;
 	}
 
 	/* Populate attribute data */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		ret = hp_populate_secure_platform_data(attr_name_kobj);
 		if (ret)
 			goto err_other_attr_init;
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		ret = hp_populate_sure_start_data(attr_name_kobj);
 		if (ret)
 			goto err_other_attr_init;
 		break;
 
 	default:
 		ret = -EINVAL;
 		goto err_other_attr_init;
 	}
 
 	mutex_unlock(&bioscfg_drv.mutex);
 	return 0;
 
 err_other_attr_init:
 	mutex_unlock(&bioscfg_drv.mutex);
-	kfree(obj);
 	return ret;
 }
 
-- 
2.39.3

[PATCH v2 2/4] platform/x86: hp-bioscfg: Simplify return check in hp_add_other_attributes()

[Harshit Mogalapalli]

All cases in switch-case have a same goto on error, move the return
check out of the switch. This is a cleanup.

Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 0b563582d90d..3b735b071a01 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -575,79 +575,77 @@ static void release_attributes_data(void)
 /**
  * hp_add_other_attributes() - Initialize HP custom attributes not
  * reported by BIOS and required to support Secure Platform and Sure
  * Start.
  *
  * @attr_type: Custom HP attribute not reported by BIOS
  *
  * Initialize all 2 types of attributes: Platform and Sure Start
  * object.  Populates each attribute types respective properties
  * under sysfs files.
  *
  * Returns zero(0) if successful. Otherwise, a negative value.
  */
 static int hp_add_other_attributes(int attr_type)
 {
 	struct kobject *attr_name_kobj;
 	int ret;
 	char *attr_name;
 
 	mutex_lock(&bioscfg_drv.mutex);
 
 	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
 	if (!attr_name_kobj) {
 		ret = -ENOMEM;
 		goto err_other_attr_init;
 	}
 
 	/* Check if attribute type is supported */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
 		attr_name = SPM_STR;
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
 		attr_name = SURE_START_STR;
 		break;
 
 	default:
 		pr_err("Error: Unknown attr_type: %d\n", attr_type);
 		ret = -EINVAL;
 		goto err_other_attr_init;
 	}
 
 	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
 				   NULL, "%s", attr_name);
 	if (ret) {
 		pr_err("Error encountered [%d]\n", ret);
 		kobject_put(attr_name_kobj);
 		goto err_other_attr_init;
 	}
 
 	/* Populate attribute data */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		ret = hp_populate_secure_platform_data(attr_name_kobj);
-		if (ret)
-			goto err_other_attr_init;
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		ret = hp_populate_sure_start_data(attr_name_kobj);
-		if (ret)
-			goto err_other_attr_init;
 		break;
 
 	default:
 		ret = -EINVAL;
-		goto err_other_attr_init;
 	}
 
+	if (ret)
+		goto err_other_attr_init;
+
 	mutex_unlock(&bioscfg_drv.mutex);
 	return 0;
 
 err_other_attr_init:
 	mutex_unlock(&bioscfg_drv.mutex);
 	return ret;
 }
-- 
2.39.3

[PATCH v2 3/4] platform/x86: hp-bioscfg: move mutex_lock down in hp_add_other_attributes()

[Harshit Mogalapalli]

attr_name_kobj's memory allocation is done with mutex_lock held, this
probably is not needed.

Move the mutex_lock downward so we need not unlock when allocation
fails.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 3b735b071a01..351d782f3e96 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -575,77 +575,75 @@ static void release_attributes_data(void)
 /**
  * hp_add_other_attributes() - Initialize HP custom attributes not
  * reported by BIOS and required to support Secure Platform and Sure
  * Start.
  *
  * @attr_type: Custom HP attribute not reported by BIOS
  *
  * Initialize all 2 types of attributes: Platform and Sure Start
  * object.  Populates each attribute types respective properties
  * under sysfs files.
  *
  * Returns zero(0) if successful. Otherwise, a negative value.
  */
 static int hp_add_other_attributes(int attr_type)
 {
 	struct kobject *attr_name_kobj;
 	int ret;
 	char *attr_name;
 
-	mutex_lock(&bioscfg_drv.mutex);
-
 	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
-	if (!attr_name_kobj) {
-		ret = -ENOMEM;
-		goto err_other_attr_init;
-	}
+	if (!attr_name_kobj)
+		return -ENOMEM;
+
+	mutex_lock(&bioscfg_drv.mutex);
 
 	/* Check if attribute type is supported */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
 		attr_name = SPM_STR;
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
 		attr_name = SURE_START_STR;
 		break;
 
 	default:
 		pr_err("Error: Unknown attr_type: %d\n", attr_type);
 		ret = -EINVAL;
 		goto err_other_attr_init;
 	}
 
 	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
 				   NULL, "%s", attr_name);
 	if (ret) {
 		pr_err("Error encountered [%d]\n", ret);
 		kobject_put(attr_name_kobj);
 		goto err_other_attr_init;
 	}
 
 	/* Populate attribute data */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		ret = hp_populate_secure_platform_data(attr_name_kobj);
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		ret = hp_populate_sure_start_data(attr_name_kobj);
 		break;
 
 	default:
 		ret = -EINVAL;
 	}
 
 	if (ret)
 		goto err_other_attr_init;
 
 	mutex_unlock(&bioscfg_drv.mutex);
 	return 0;
 
 err_other_attr_init:
 	mutex_unlock(&bioscfg_drv.mutex);
 	return ret;
 }
-- 
2.39.3

Re: [PATCH v2 3/4] platform/x86: hp-bioscfg: move mutex_lock down in hp_add_other_attributes()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3107 bytes --]

On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:

> attr_name_kobj's memory allocation is done with mutex_lock held, this

Please use () with function names.

> probably is not needed.

Just remove probably.

> Move the mutex_lock downward so we need not unlock when allocation
> fails.

Move allocation outside of mutex_lock() so unlock is not needed when
allocation fails.

The code change looks fine.

-- 
 i.

> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 3b735b071a01..351d782f3e96 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -575,77 +575,75 @@ static void release_attributes_data(void)
>  /**
>   * hp_add_other_attributes() - Initialize HP custom attributes not
>   * reported by BIOS and required to support Secure Platform and Sure
>   * Start.
>   *
>   * @attr_type: Custom HP attribute not reported by BIOS
>   *
>   * Initialize all 2 types of attributes: Platform and Sure Start
>   * object.  Populates each attribute types respective properties
>   * under sysfs files.
>   *
>   * Returns zero(0) if successful. Otherwise, a negative value.
>   */
>  static int hp_add_other_attributes(int attr_type)
>  {
>  	struct kobject *attr_name_kobj;
>  	int ret;
>  	char *attr_name;
>  
> -	mutex_lock(&bioscfg_drv.mutex);
> -
>  	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
> -	if (!attr_name_kobj) {
> -		ret = -ENOMEM;
> -		goto err_other_attr_init;
> -	}
> +	if (!attr_name_kobj)
> +		return -ENOMEM;
> +
> +	mutex_lock(&bioscfg_drv.mutex);
>  
>  	/* Check if attribute type is supported */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
>  		attr_name = SPM_STR;
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
>  		attr_name = SURE_START_STR;
>  		break;
>  
>  	default:
>  		pr_err("Error: Unknown attr_type: %d\n", attr_type);
>  		ret = -EINVAL;
>  		goto err_other_attr_init;
>  	}
>  
>  	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
>  				   NULL, "%s", attr_name);
>  	if (ret) {
>  		pr_err("Error encountered [%d]\n", ret);
>  		kobject_put(attr_name_kobj);
>  		goto err_other_attr_init;
>  	}
>  
>  	/* Populate attribute data */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		ret = hp_populate_secure_platform_data(attr_name_kobj);
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		ret = hp_populate_sure_start_data(attr_name_kobj);
>  		break;
>  
>  	default:
>  		ret = -EINVAL;
>  	}
>  
>  	if (ret)
>  		goto err_other_attr_init;
>  
>  	mutex_unlock(&bioscfg_drv.mutex);
>  	return 0;
>  
>  err_other_attr_init:
>  	mutex_unlock(&bioscfg_drv.mutex);
>  	return ret;
>  }
> 

[PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Harshit Mogalapalli]

We have two issues:
1. Memory leak of 'attr_name_kobj' in the error handling path.
2. When kobject_init_and_add() fails on every subsequent error path call
   kobject_put() to cleanup.

Both of these issues will be fixed when we add kobject_put() in the goto
label, as kfree() is already part of kobject_put().

Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/r/202309201412.on0VXJGo-lkp@intel.com/
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
Only compile tested, based on static analysis
v1-> v2: Split this into mutliple patches doing one thing in a patch.
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 351d782f3e96..8c9f4f3227fc 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -575,75 +575,77 @@ static void release_attributes_data(void)
 /**
  * hp_add_other_attributes() - Initialize HP custom attributes not
  * reported by BIOS and required to support Secure Platform and Sure
  * Start.
  *
  * @attr_type: Custom HP attribute not reported by BIOS
  *
  * Initialize all 2 types of attributes: Platform and Sure Start
  * object.  Populates each attribute types respective properties
  * under sysfs files.
  *
  * Returns zero(0) if successful. Otherwise, a negative value.
  */
 static int hp_add_other_attributes(int attr_type)
 {
 	struct kobject *attr_name_kobj;
 	int ret;
 	char *attr_name;
 
 	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
 	if (!attr_name_kobj)
 		return -ENOMEM;
 
 	mutex_lock(&bioscfg_drv.mutex);
 
 	/* Check if attribute type is supported */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
 		attr_name = SPM_STR;
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
 		attr_name = SURE_START_STR;
 		break;
 
 	default:
 		pr_err("Error: Unknown attr_type: %d\n", attr_type);
 		ret = -EINVAL;
-		goto err_other_attr_init;
+		kfree(attr_name_kobj);
+		goto unlock_drv_mutex;
 	}
 
 	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
 				   NULL, "%s", attr_name);
 	if (ret) {
 		pr_err("Error encountered [%d]\n", ret);
-		kobject_put(attr_name_kobj);
 		goto err_other_attr_init;
 	}
 
 	/* Populate attribute data */
 	switch (attr_type) {
 	case HPWMI_SECURE_PLATFORM_TYPE:
 		ret = hp_populate_secure_platform_data(attr_name_kobj);
 		break;
 
 	case HPWMI_SURE_START_TYPE:
 		ret = hp_populate_sure_start_data(attr_name_kobj);
 		break;
 
 	default:
 		ret = -EINVAL;
 	}
 
 	if (ret)
 		goto err_other_attr_init;
 
 	mutex_unlock(&bioscfg_drv.mutex);
 	return 0;
 
 err_other_attr_init:
+	kobject_put(attr_name_kobj);
+unlock_drv_mutex:
 	mutex_unlock(&bioscfg_drv.mutex);
 	return ret;
 }
-- 
2.39.3

Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Ilpo Järvinen]

On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:

This changelog needs to be rewritten, it contains multiple errors. I 
suppose even this patch could be split into two but I'll not be too picky 
here if you insist on fixing them in the same patch.

> We have two issues:
> 1. Memory leak of 'attr_name_kobj' in the error handling path.

True, but not specific enough to be useful.

> 2. When kobject_init_and_add() fails on every subsequent error path call
>    kobject_put() to cleanup.

This makes no sense. The only case when there old code had no issue is 
"when kobject_init_and_add() fails" but now your wording claims it to be 
source of problem. Please rephrase this.

> Both of these issues will be fixed when we add kobject_put() in the goto
> label, as kfree() is already part of kobject_put().

No, you're fixing a problem in the patch which is not covered by moving 
kobject_put()!

-- 
 i.

> Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/r/202309201412.on0VXJGo-lkp@intel.com/
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
> Only compile tested, based on static analysis
> v1-> v2: Split this into mutliple patches doing one thing in a patch.
> ---
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 351d782f3e96..8c9f4f3227fc 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -575,75 +575,77 @@ static void release_attributes_data(void)
>  /**
>   * hp_add_other_attributes() - Initialize HP custom attributes not
>   * reported by BIOS and required to support Secure Platform and Sure
>   * Start.
>   *
>   * @attr_type: Custom HP attribute not reported by BIOS
>   *
>   * Initialize all 2 types of attributes: Platform and Sure Start
>   * object.  Populates each attribute types respective properties
>   * under sysfs files.
>   *
>   * Returns zero(0) if successful. Otherwise, a negative value.
>   */
>  static int hp_add_other_attributes(int attr_type)
>  {
>  	struct kobject *attr_name_kobj;
>  	int ret;
>  	char *attr_name;
>  
>  	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
>  	if (!attr_name_kobj)
>  		return -ENOMEM;
>  
>  	mutex_lock(&bioscfg_drv.mutex);
>  
>  	/* Check if attribute type is supported */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
>  		attr_name = SPM_STR;
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
>  		attr_name = SURE_START_STR;
>  		break;
>  
>  	default:
>  		pr_err("Error: Unknown attr_type: %d\n", attr_type);
>  		ret = -EINVAL;
> -		goto err_other_attr_init;
> +		kfree(attr_name_kobj);
> +		goto unlock_drv_mutex;
>  	}
>  
>  	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
>  				   NULL, "%s", attr_name);
>  	if (ret) {
>  		pr_err("Error encountered [%d]\n", ret);
> -		kobject_put(attr_name_kobj);
>  		goto err_other_attr_init;
>  	}
>  
>  	/* Populate attribute data */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		ret = hp_populate_secure_platform_data(attr_name_kobj);
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		ret = hp_populate_sure_start_data(attr_name_kobj);
>  		break;
>  
>  	default:
>  		ret = -EINVAL;
>  	}
>  
>  	if (ret)
>  		goto err_other_attr_init;
>  
>  	mutex_unlock(&bioscfg_drv.mutex);
>  	return 0;
>  
>  err_other_attr_init:
> +	kobject_put(attr_name_kobj);
> +unlock_drv_mutex:
>  	mutex_unlock(&bioscfg_drv.mutex);
>  	return ret;
>  }
> 

Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Harshit Mogalapalli]

Hi Ilpo,

On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
> On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
> 

Thanks for the review.

> This changelog needs to be rewritten, it contains multiple errors. I
> suppose even this patch could be split into two but I'll not be too picky
> here if you insist on fixing them in the same patch.
> 

Any thoughts on how to split this into two patches ?

I thought of fixing memory leak in separate patch, but that would add 
more code which should be removed when we move kobject_put() to the end.

>> We have two issues:
>> 1. Memory leak of 'attr_name_kobj' in the error handling path.
> 
> True, but not specific enough to be useful.
> 

Should I mention something like:

'attr_name_kobj' is allocated using kzalloc, but on all the error paths 
we don't free it, hence we have a memory leak.

>> 2. When kobject_init_and_add() fails on every subsequent error path call
>>     kobject_put() to cleanup.
> 
> This makes no sense. The only case when there old code had no issue is
> "when kobject_init_and_add() fails" but now your wording claims it to be
> source of problem. Please rephrase this.
> 

Does this look better:

kobject_put() must be called to cleanup memory associated with the 
object if kobject_init_and_add() returns an error , before this patch 
only the error path which is immediately next to kobject_init_and_add() 
has a kobject_put() not any other error paths after it. Fix this by 
moving the kobject_put() into a goto label "err_other_attr_init:" and 
use that for error paths after kobject_init_and_add().


>> Both of these issues will be fixed when we add kobject_put() in the goto
>> label, as kfree() is already part of kobject_put().
> 
> No, you're fixing a problem in the patch which is not covered by moving
> kobject_put()!
Sure, I will try to rephrase it to:

1. Add a new label "unlock_drv_mutex"
2. Add a kfree() in the default statement of switch before going to 
"unlock_drv_mutex" to free up the memory allocated with kzalloc.
2. Move kobject_put() to goto "err_other_attr_init:" and use this goto 
label in every error path after kobject_init_and_add().

Please let me know if you have any comments.

Thank you very much.

Regards,
Harshit
> 

Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

On Sat, 11 Nov 2023, Harshit Mogalapalli wrote:
> On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
> > On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
> > 
> 
> Thanks for the review.
> 
> > This changelog needs to be rewritten, it contains multiple errors. I
> > suppose even this patch could be split into two but I'll not be too picky
> > here if you insist on fixing them in the same patch.
> > 
> 
> Any thoughts on how to split this into two patches ?
> 
> I thought of fixing memory leak in separate patch, but that would add more
> code which should be removed when we move kobject_put() to the end.

I meant that in the first patch you fix add the missing kfree(). Then in 
the second one, you correct kobject_put() + play with goto labels. There's 
no extra code that needs to be added and then removed AFAICT.

That way, you can make the commit messages more to the point too per 
patch.

> > > We have two issues:
> > > 1. Memory leak of 'attr_name_kobj' in the error handling path.
> > 
> > True, but not specific enough to be useful.
> > 
> 
> Should I mention something like:
> 
> 'attr_name_kobj' is allocated using kzalloc, but on all the error paths we
> don't free it, hence we have a memory leak.
>
> > > 2. When kobject_init_and_add() fails on every subsequent error path call
> > >     kobject_put() to cleanup.
> > 
> > This makes no sense. The only case when there old code had no issue is
> > "when kobject_init_and_add() fails" but now your wording claims it to be
> > source of problem. Please rephrase this.
> > 
> 
> Does this look better:
> 
> kobject_put() must be called to cleanup memory associated with the object if
> kobject_init_and_add() returns an error , before this patch only the error
> path which is immediately next to kobject_init_and_add() has a kobject_put()
> not any other error paths after it. Fix this by moving the kobject_put() into
> a goto label "err_other_attr_init:" and use that for error paths after
> kobject_init_and_add().

This is easier to understand I think:

kobject_put() must be always called after passing the object to 
kobject_init_and_add(). Only the error path which is immediately next
to kobject_init_and_add() calls kobject_put() and not any other error 
path after it.

Fix the error handling by moving the kobject_put() into the goto label 
err_other_attr_init that is already used by all the error paths after
kobject_init_and_add().

> > > Both of these issues will be fixed when we add kobject_put() in the goto
> > > label, as kfree() is already part of kobject_put().
> > 
> > No, you're fixing a problem in the patch which is not covered by moving
> > kobject_put()!
>
> Sure, I will try to rephrase it to:
> 
> 1. Add a new label "unlock_drv_mutex"
> 2. Add a kfree() in the default statement of switch before going to
> "unlock_drv_mutex" to free up the memory allocated with kzalloc.
> 2. Move kobject_put() to goto "err_other_attr_init:" and use this goto label
> in every error path after kobject_init_and_add().
> 
> Please let me know if you have any comments.

I think none of this is needed for this patch after you move the other fix 
into a separate patch. Those two paragraphs I suggest above would explain 
the problem and solution (no need to have these numbered bullets or 
anything else besides those 2 paragraphs).

-- 
 i.

Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Harshit Mogalapalli]

Hi Ilpo,

On 13/11/23 7:01 pm, Ilpo Järvinen wrote:
> On Sat, 11 Nov 2023, Harshit Mogalapalli wrote:
>> On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
>>> On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
>>>
>>
>> Thanks for the review.
>>
>>> This changelog needs to be rewritten, it contains multiple errors. I
>>> suppose even this patch could be split into two but I'll not be too picky
>>> here if you insist on fixing them in the same patch.
>>>
>>
>> Any thoughts on how to split this into two patches ?
>>
>> I thought of fixing memory leak in separate patch, but that would add more
>> code which should be removed when we move kobject_put() to the end.
> 
Thanks for the suggestions.

> I meant that in the first patch you fix add the missing kfree(). Then in
> the second one, you correct kobject_put() + play with goto labels. There's
> no extra code that needs to be added and then removed AFAICT.
> 

This is the problem I am trying to explain:

Let us say we do memory leak fixing in first patch:

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c 
b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 351d782f3e96..7f29a746210e 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -612,6 +612,7 @@ static int hp_add_other_attributes(int attr_type)
         default:
                 pr_err("Error: Unknown attr_type: %d\n", attr_type);
                 ret = -EINVAL;
+               kfree(attr_name_kobj);
                 goto err_other_attr_init;
         }

@@ -637,8 +638,10 @@ static int hp_add_other_attributes(int attr_type)
                 ret = -EINVAL;
         }

-       if (ret)
+       if (ret) {
+               kfree(attr_name_kobj); ///// [1]
                 goto err_other_attr_init;
+       }

         mutex_unlock(&bioscfg_drv.mutex);
         return 0;

Now when we want to move kobject_put() to goto label in next patch,
we should remove the kfree [1], as kobject_put() already does a kfree().

If that sounds okay, I will split this into two patches, (first one, 
only fixing memory leak; and second one fixing missing kobject_put() 
calls on error paths)

Thanks,
Harshit

> That way, you can make the commit messages more to the point too per
> patch.
> 
>>>> We have two issues:
>>>> 1. Memory leak of 'attr_name_kobj' in the error handling path.
>>>
>>> True, but not specific enough to be useful.
>>>
>>
>> Should I mention something like:
>>
>> 'attr_name_kobj' is allocated using kzalloc, but on all the error paths we
>> don't free it, hence we have a memory leak.
>>
>>>> 2. When kobject_init_and_add() fails on every subsequent error path call
>>>>      kobject_put() to cleanup.
>>>
>>> This makes no sense. The only case when there old code had no issue is
>>> "when kobject_init_and_add() fails" but now your wording claims it to be
>>> source of problem. Please rephrase this.
>>>
>>
>> Does this look better:
>>
>> kobject_put() must be called to cleanup memory associated with the object if
>> kobject_init_and_add() returns an error , before this patch only the error
>> path which is immediately next to kobject_init_and_add() has a kobject_put()
>> not any other error paths after it. Fix this by moving the kobject_put() into
>> a goto label "err_other_attr_init:" and use that for error paths after
>> kobject_init_and_add().
> 
> This is easier to understand I think:
> 
> kobject_put() must be always called after passing the object to
> kobject_init_and_add(). Only the error path which is immediately next
> to kobject_init_and_add() calls kobject_put() and not any other error
> path after it.
> 
> Fix the error handling by moving the kobject_put() into the goto label
> err_other_attr_init that is already used by all the error paths after
> kobject_init_and_add().
> 
>>>> Both of these issues will be fixed when we add kobject_put() in the goto
>>>> label, as kfree() is already part of kobject_put().
>>>
>>> No, you're fixing a problem in the patch which is not covered by moving
>>> kobject_put()!
>>
>> Sure, I will try to rephrase it to:
>>
>> 1. Add a new label "unlock_drv_mutex"
>> 2. Add a kfree() in the default statement of switch before going to
>> "unlock_drv_mutex" to free up the memory allocated with kzalloc.
>> 2. Move kobject_put() to goto "err_other_attr_init:" and use this goto label
>> in every error path after kobject_init_and_add().
>>
>> Please let me know if you have any comments.
> 
> I think none of this is needed for this patch after you move the other fix
> into a separate patch. Those two paragraphs I suggest above would explain
> the problem and solution (no need to have these numbered bullets or
> anything else besides those 2 paragraphs).
> 

Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]

On Mon, 13 Nov 2023, Harshit Mogalapalli wrote:
> On 13/11/23 7:01 pm, Ilpo Järvinen wrote:
> > On Sat, 11 Nov 2023, Harshit Mogalapalli wrote:
> > > On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
> > > > On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
> > > > 
> > > 
> > > Thanks for the review.
> > > 
> > > > This changelog needs to be rewritten, it contains multiple errors. I
> > > > suppose even this patch could be split into two but I'll not be too
> > > > picky
> > > > here if you insist on fixing them in the same patch.
> > > > 
> > > 
> > > Any thoughts on how to split this into two patches ?
> > > 
> > > I thought of fixing memory leak in separate patch, but that would add more
> > > code which should be removed when we move kobject_put() to the end.
> > 
> Thanks for the suggestions.
> 
> > I meant that in the first patch you fix add the missing kfree(). Then in
> > the second one, you correct kobject_put() + play with goto labels. There's
> > no extra code that needs to be added and then removed AFAICT.
> > 
> 
> This is the problem I am trying to explain:
> 
> Let us say we do memory leak fixing in first patch:
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 351d782f3e96..7f29a746210e 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -612,6 +612,7 @@ static int hp_add_other_attributes(int attr_type)
>         default:
>                 pr_err("Error: Unknown attr_type: %d\n", attr_type);
>                 ret = -EINVAL;
> +               kfree(attr_name_kobj);
>                 goto err_other_attr_init;
>         }
> 
> @@ -637,8 +638,10 @@ static int hp_add_other_attributes(int attr_type)
>                 ret = -EINVAL;
>         }
> 
> -       if (ret)
> +       if (ret) {
> +               kfree(attr_name_kobj); ///// [1]

This relates to the 2nd problem (missing kobject_put()) and will be 
covered by the other patch. Don't try to solve this in the first patch
at all!

There are two indepedent problems:
- Before kobject_init_and_add(), kfree() is missing
- After kobject_init_and_add(), kobject_put() is missing

Solve each in own patch and don't mix the solutions.

I know both patches are needed to solve _both_ problems so it's fine to 
have "still broken" intermediate state as long as you didn't make things 
worse in the first patch which you didn't.

>                 goto err_other_attr_init;
> +       }
> 
>         mutex_unlock(&bioscfg_drv.mutex);
>         return 0;
> 
> Now when we want to move kobject_put() to goto label in next patch,
> we should remove the kfree [1], as kobject_put() already does a kfree().
> 
> If that sounds okay, I will split this into two patches, (first one, only
> fixing memory leak; and second one fixing missing kobject_put() calls on error
> paths)

-- 
 i.

Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Dan Carpenter]

On Mon, Nov 13, 2023 at 04:15:50PM +0200, Ilpo Järvinen wrote:
> This relates to the 2nd problem (missing kobject_put()) and will be 
> covered by the other patch. Don't try to solve this in the first patch
> at all!
> 
> There are two indepedent problems:
> - Before kobject_init_and_add(), kfree() is missing
> - After kobject_init_and_add(), kobject_put() is missing

It's the same problem, though.  The attr_name_kobj is leaked on all the
error paths.  It's just that it needs to be freed different ways
depending on where you are.  To me splitting it up makes it harder to
review and I would not allow it in Staging.  You can't fix half the
problem.

regards,
dan carpenter

Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Mon, 13 Nov 2023, Dan Carpenter wrote:

> On Mon, Nov 13, 2023 at 04:15:50PM +0200, Ilpo Järvinen wrote:
> > This relates to the 2nd problem (missing kobject_put()) and will be 
> > covered by the other patch. Don't try to solve this in the first patch
> > at all!
> > 
> > There are two indepedent problems:
> > - Before kobject_init_and_add(), kfree() is missing
> > - After kobject_init_and_add(), kobject_put() is missing
> 
> It's the same problem, though. The attr_name_kobj is leaked on all the
> error paths. 

I'll have politely disagree beyond that the symptoms are indeed about the 
same, the problem is clearly different like you immediately admit even 
yourself by stating this: ;-)

> It's just that it needs to be freed different ways depending on where 
> you are.

...And that's because "it" actually changed in between so the problem 
became a different one.

> To me splitting it up makes it harder to review

This has already been proven incorrect in the context of this patch so 
your argument is rather weak... While reviewing it I clearly noted that 
the different way of handling things was not properly covered, and that 
was because what needs to be "freed" was changed by 
kobject_init_and_add(). If one would have done them separately, each 
commit message would have been more to the point and it would have been 
simpler to review which is exactly the opposite to your claim. But I guess 
we'll end up disagreing on this too :-).

> and I would not allow it in Staging. You can't fix half the problem.

I don't have that strong opinion on this so Harshit please follow what 
Dan is suggesting, just fix the changelog to clearly cover both cases.


-- 
 i.

Re: [PATCH v2 1/4] platform/x86: hp-bioscfg: Remove unused obj in hp_add_other_attributes()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:

> acpi_object *obj is unused in this function, so delete it, also
> delete a unnecessary kfree(obj);
> 
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 5798b49ddaba..0b563582d90d 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -575,82 +575,80 @@ static void release_attributes_data(void)
>  /**
>   * hp_add_other_attributes() - Initialize HP custom attributes not
>   * reported by BIOS and required to support Secure Platform and Sure
>   * Start.
>   *
>   * @attr_type: Custom HP attribute not reported by BIOS
>   *
>   * Initialize all 2 types of attributes: Platform and Sure Start
>   * object.  Populates each attribute types respective properties
>   * under sysfs files.
>   *
>   * Returns zero(0) if successful. Otherwise, a negative value.
>   */
>  static int hp_add_other_attributes(int attr_type)
>  {
>  	struct kobject *attr_name_kobj;
> -	union acpi_object *obj = NULL;
>  	int ret;
>  	char *attr_name;
>  
>  	mutex_lock(&bioscfg_drv.mutex);
>  
>  	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
>  	if (!attr_name_kobj) {
>  		ret = -ENOMEM;
>  		goto err_other_attr_init;
>  	}
>  
>  	/* Check if attribute type is supported */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
>  		attr_name = SPM_STR;
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
>  		attr_name = SURE_START_STR;
>  		break;
>  
>  	default:
>  		pr_err("Error: Unknown attr_type: %d\n", attr_type);
>  		ret = -EINVAL;
>  		goto err_other_attr_init;
>  	}
>  
>  	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
>  				   NULL, "%s", attr_name);
>  	if (ret) {
>  		pr_err("Error encountered [%d]\n", ret);
>  		kobject_put(attr_name_kobj);
>  		goto err_other_attr_init;
>  	}
>  
>  	/* Populate attribute data */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		ret = hp_populate_secure_platform_data(attr_name_kobj);
>  		if (ret)
>  			goto err_other_attr_init;
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		ret = hp_populate_sure_start_data(attr_name_kobj);
>  		if (ret)
>  			goto err_other_attr_init;
>  		break;
>  
>  	default:
>  		ret = -EINVAL;
>  		goto err_other_attr_init;
>  	}
>  
>  	mutex_unlock(&bioscfg_drv.mutex);
>  	return 0;
>  
>  err_other_attr_init:
>  	mutex_unlock(&bioscfg_drv.mutex);
> -	kfree(obj);
>  	return ret;
>  }

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v2 1/4] platform/x86: hp-bioscfg: Remove unused obj in hp_add_other_attributes()

[Ilpo Järvinen]

On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:

> acpi_object *obj is unused in this function, so delete it, also
> delete a unnecessary kfree(obj);
> 
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---

Please make this the last patch of the series since the others are true 
fixes and this is just a (useful!) cleanup.

-- 
 i.

>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 5798b49ddaba..0b563582d90d 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -575,82 +575,80 @@ static void release_attributes_data(void)
>  /**
>   * hp_add_other_attributes() - Initialize HP custom attributes not
>   * reported by BIOS and required to support Secure Platform and Sure
>   * Start.
>   *
>   * @attr_type: Custom HP attribute not reported by BIOS
>   *
>   * Initialize all 2 types of attributes: Platform and Sure Start
>   * object.  Populates each attribute types respective properties
>   * under sysfs files.
>   *
>   * Returns zero(0) if successful. Otherwise, a negative value.
>   */
>  static int hp_add_other_attributes(int attr_type)
>  {
>  	struct kobject *attr_name_kobj;
> -	union acpi_object *obj = NULL;
>  	int ret;
>  	char *attr_name;
>  
>  	mutex_lock(&bioscfg_drv.mutex);
>  
>  	attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
>  	if (!attr_name_kobj) {
>  		ret = -ENOMEM;
>  		goto err_other_attr_init;
>  	}
>  
>  	/* Check if attribute type is supported */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset;
>  		attr_name = SPM_STR;
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		attr_name_kobj->kset = bioscfg_drv.main_dir_kset;
>  		attr_name = SURE_START_STR;
>  		break;
>  
>  	default:
>  		pr_err("Error: Unknown attr_type: %d\n", attr_type);
>  		ret = -EINVAL;
>  		goto err_other_attr_init;
>  	}
>  
>  	ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
>  				   NULL, "%s", attr_name);
>  	if (ret) {
>  		pr_err("Error encountered [%d]\n", ret);
>  		kobject_put(attr_name_kobj);
>  		goto err_other_attr_init;
>  	}
>  
>  	/* Populate attribute data */
>  	switch (attr_type) {
>  	case HPWMI_SECURE_PLATFORM_TYPE:
>  		ret = hp_populate_secure_platform_data(attr_name_kobj);
>  		if (ret)
>  			goto err_other_attr_init;
>  		break;
>  
>  	case HPWMI_SURE_START_TYPE:
>  		ret = hp_populate_sure_start_data(attr_name_kobj);
>  		if (ret)
>  			goto err_other_attr_init;
>  		break;
>  
>  	default:
>  		ret = -EINVAL;
>  		goto err_other_attr_init;
>  	}
>  
>  	mutex_unlock(&bioscfg_drv.mutex);
>  	return 0;
>  
>  err_other_attr_init:
>  	mutex_unlock(&bioscfg_drv.mutex);
> -	kfree(obj);
>  	return ret;
>  }
>  
>