[PATCH] platform/x86: dell-wmi-sysman: fix init_bios_attributes() error handling

[PATCH] platform/x86: dell-wmi-sysman: fix init_bios_attributes() error handling

[Dan Carpenter]

Calling release_attributes_data() while holding the "wmi_priv.mutex"
will lead to a dead lock.  The other problem is that if kzalloc() fails
then this should return -ENOMEM but currently it returns success.

Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems Management Driver over WMI for Dell Systems")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
The "platform/x86: Introduce support ... " commit doesn't use the patch
prefix which the driver will use going forward.  That means that whoever
fixes the first bug has to pick the patch prefix and sometimes people
are not happy with that.

 drivers/platform/x86/dell-wmi-sysman/sysman.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell-wmi-sysman/sysman.c
index 3842575a6c18..055556d5c70d 100644
--- a/drivers/platform/x86/dell-wmi-sysman/sysman.c
+++ b/drivers/platform/x86/dell-wmi-sysman/sysman.c
@@ -443,8 +443,10 @@ static int init_bios_attributes(int attr_type, const char *guid)
 
 		/* build attribute */
 		attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
-		if (!attr_name_kobj)
+		if (!attr_name_kobj) {
+			retval = -ENOMEM;
 			goto err_attr_init;
+		}
 
 		attr_name_kobj->kset = tmp_set;
 
@@ -486,13 +488,13 @@ static int init_bios_attributes(int attr_type, const char *guid)
 		elements = obj ? obj->package.elements : NULL;
 	}
 
-	goto out;
+	mutex_unlock(&wmi_priv.mutex);
+	return 0;
 
 err_attr_init:
+	mutex_unlock(&wmi_priv.mutex);
 	release_attributes_data();
 	kfree(obj);
-out:
-	mutex_unlock(&wmi_priv.mutex);
 	return retval;
 }
 
-- 
2.28.0

Re: [PATCH] platform/x86: dell-wmi-sysman: fix init_bios_attributes() error handling

[Hans de Goede]

Hi,

On 11/3/20 11:17 AM, Dan Carpenter wrote:
> Calling release_attributes_data() while holding the "wmi_priv.mutex"
> will lead to a dead lock.  The other problem is that if kzalloc() fails
> then this should return -ENOMEM but currently it returns success.
> 
> Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems Management Driver over WMI for Dell Systems")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
> The "platform/x86: Introduce support ... " commit doesn't use the patch
> prefix which the driver will use going forward.  That means that whoever
> fixes the first bug has to pick the patch prefix and sometimes people
> are not happy with that.
> 
>  drivers/platform/x86/dell-wmi-sysman/sysman.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell-wmi-sysman/sysman.c
> index 3842575a6c18..055556d5c70d 100644
> --- a/drivers/platform/x86/dell-wmi-sysman/sysman.c
> +++ b/drivers/platform/x86/dell-wmi-sysman/sysman.c
> @@ -443,8 +443,10 @@ static int init_bios_attributes(int attr_type, const char *guid)
>  
>  		/* build attribute */
>  		attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
> -		if (!attr_name_kobj)
> +		if (!attr_name_kobj) {
> +			retval = -ENOMEM;
>  			goto err_attr_init;
> +		}
>  
>  		attr_name_kobj->kset = tmp_set;
>  
> @@ -486,13 +488,13 @@ static int init_bios_attributes(int attr_type, const char *guid)
>  		elements = obj ? obj->package.elements : NULL;
>  	}
>  
> -	goto out;
> +	mutex_unlock(&wmi_priv.mutex);
> +	return 0;
>  
>  err_attr_init:
> +	mutex_unlock(&wmi_priv.mutex);
>  	release_attributes_data();
>  	kfree(obj);
> -out:
> -	mutex_unlock(&wmi_priv.mutex);
>  	return retval;
>  }
>  
>