[PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[Dan Carpenter]

We're freeing "value_name" which is NULL, so that's a no-op, instead of
"location_name" and then we don't free the first zero-th elements of
token_location_attrs[] and token_value_attrs[].

Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 6a60db515bda..d8a21c7ba594 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -512,7 +512,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 		continue;
 
 loop_fail_create_value:
-		kfree(value_name);
+		kfree(location_name);
 		goto out_unwind_strings;
 	}
 	smbios_attribute_group.attrs = token_attrs;
@@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 	return 0;
 
 out_unwind_strings:
-	for (i = i-1; i > 0; i--) {
+	for (i = i-1; i >= 0; i--) {
 		kfree(token_location_attrs[i].attr.name);
 		kfree(token_value_attrs[i].attr.name);
 	}

Re: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[walter harms]

Am 18.01.2018 11:45, schrieb Dan Carpenter:
> We're freeing "value_name" which is NULL, so that's a no-op, instead of
> "location_name" and then we don't free the first zero-th elements of
> token_location_attrs[] and token_value_attrs[].
> 
> Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index 6a60db515bda..d8a21c7ba594 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -512,7 +512,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
>  		continue;
>  
>  loop_fail_create_value:
> -		kfree(value_name);
> +		kfree(location_name);
>  		goto out_unwind_strings;
>  	}
>  	smbios_attribute_group.attrs = token_attrs;
> @@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
>  	return 0;
>  
>  out_unwind_strings:
> -	for (i = i-1; i > 0; i--) {
> +	for (i = i-1; i >= 0; i--) {
>  		kfree(token_location_attrs[i].attr.name);
>  		kfree(token_value_attrs[i].attr.name);
>  	}

would you mind to reverse order here ?
you know programmers are terrible at couting backwards.


re,
 wh

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[Dan Carpenter]

On Thu, Jan 18, 2018 at 11:50:30AM +0100, walter harms wrote:
> > @@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
> >  	return 0;
> >  
> >  out_unwind_strings:
> > -	for (i = i-1; i > 0; i--) {
> > +	for (i = i-1; i >= 0; i--) {
> >  		kfree(token_location_attrs[i].attr.name);
> >  		kfree(token_value_attrs[i].attr.name);
> >  	}
> 
> would you mind to reverse order here ?
> you know programmers are terrible at couting backwards.

I prefer to always unwind in reverse order so I'd prefer to leave it
as-is.

regards,
dan carpenter

Re: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[walter harms]

Am 18.01.2018 12:03, schrieb Dan Carpenter:
> On Thu, Jan 18, 2018 at 11:50:30AM +0100, walter harms wrote:
>>> @@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
>>>  	return 0;
>>>  
>>>  out_unwind_strings:
>>> -	for (i = i-1; i > 0; i--) {
>>> +	for (i = i-1; i >= 0; i--) {
>>>  		kfree(token_location_attrs[i].attr.name);
>>>  		kfree(token_value_attrs[i].attr.name);
>>>  	}
>>
>> would you mind to reverse order here ?
>> you know programmers are terrible at couting backwards.
> 
> I prefer to always unwind in reverse order so I'd prefer to leave it
> as-is.


It is just a comment from my side. we can leave the actual decision to the
current maintainer.

re,
 wh

> 
> regards,
> dan carpenter
> 
> 

Re: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[Andy Shevchenko]

On Thu, Jan 18, 2018 at 1:20 PM, walter harms <wharms@bfs.de> wrote:
> Am 18.01.2018 12:03, schrieb Dan Carpenter:
>> On Thu, Jan 18, 2018 at 11:50:30AM +0100, walter harms wrote:

>>>>  out_unwind_strings:
>>>> -   for (i = i-1; i > 0; i--) {
>>>> +   for (i = i-1; i >= 0; i--) {
>>>>             kfree(token_location_attrs[i].attr.name);
>>>>             kfree(token_value_attrs[i].attr.name);
>>>>     }

>>> would you mind to reverse order here ?
>>> you know programmers are terrible at couting backwards.
>>
>> I prefer to always unwind in reverse order so I'd prefer to leave it
>> as-is.

> It is just a comment from my side. we can leave the actual decision to the
> current maintainer.

Right.
And maintainer would like to see simple:

while (i--) {
    kfree(token_location_attrs[i].attr.name);
    kfree(token_value_attrs[i].attr.name);
}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[Andy Shevchenko]

Walter, please, do not shrink Cc list until you sure person / people
in question is/are not interested in the topic.


On Thu, Jan 18, 2018 at 9:21 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 18, 2018 at 1:20 PM, walter harms <wharms@bfs.de> wrote:
>> Am 18.01.2018 12:03, schrieb Dan Carpenter:
>>> On Thu, Jan 18, 2018 at 11:50:30AM +0100, walter harms wrote:
>
>>>>>  out_unwind_strings:
>>>>> -   for (i = i-1; i > 0; i--) {
>>>>> +   for (i = i-1; i >= 0; i--) {
>>>>>             kfree(token_location_attrs[i].attr.name);
>>>>>             kfree(token_value_attrs[i].attr.name);
>>>>>     }
>
>>>> would you mind to reverse order here ?
>>>> you know programmers are terrible at couting backwards.
>>>
>>> I prefer to always unwind in reverse order so I'd prefer to leave it
>>> as-is.
>
>> It is just a comment from my side. we can leave the actual decision to the
>> current maintainer.
>
> Right.
> And maintainer would like to see simple:
>
> while (i--) {
>     kfree(token_location_attrs[i].attr.name);
>     kfree(token_value_attrs[i].attr.name);
> }
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[Mario.Limonciello]

Thanks, yeah this is obviously the correct way to do it.

Acked-by: Mario Limonciello <mario.limonciello@dell.com>

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, January 18, 2018 4:45 AM
> To: Pali Rohár <pali.rohar@gmail.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Cc: Darren Hart <dvhart@infradead.org>; Andy Shevchenko
> <andy@infradead.org>; platform-driver-x86@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: [PATCH] platform/x86: dell-smbios: Fix error handling in
> build_tokens_sysfs()
> 
> We're freeing "value_name" which is NULL, so that's a no-op, instead of
> "location_name" and then we don't free the first zero-th elements of
> token_location_attrs[] and token_value_attrs[].
> 
> Fixes: 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS
> tokens")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-
> smbios.c
> index 6a60db515bda..d8a21c7ba594 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -512,7 +512,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
>  		continue;
> 
>  loop_fail_create_value:
> -		kfree(value_name);
> +		kfree(location_name);
>  		goto out_unwind_strings;
>  	}
>  	smbios_attribute_group.attrs = token_attrs;
> @@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
>  	return 0;
> 
>  out_unwind_strings:
> -	for (i = i-1; i > 0; i--) {
> +	for (i = i-1; i >= 0; i--) {
>  		kfree(token_location_attrs[i].attr.name);
>  		kfree(token_value_attrs[i].attr.name);
>  	}
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

[Pali Rohár]

On Thursday 18 January 2018 21:24:44 Andy Shevchenko wrote:
> Walter, please, do not shrink Cc list until you sure person / people
> in question is/are not interested in the topic.
> 
> 
> On Thu, Jan 18, 2018 at 9:21 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jan 18, 2018 at 1:20 PM, walter harms <wharms@bfs.de> wrote:
> >> Am 18.01.2018 12:03, schrieb Dan Carpenter:
> >>> On Thu, Jan 18, 2018 at 11:50:30AM +0100, walter harms wrote:
> >
> >>>>>  out_unwind_strings:
> >>>>> -   for (i = i-1; i > 0; i--) {
> >>>>> +   for (i = i-1; i >= 0; i--) {
> >>>>>             kfree(token_location_attrs[i].attr.name);
> >>>>>             kfree(token_value_attrs[i].attr.name);
> >>>>>     }
> >
> >>>> would you mind to reverse order here ?
> >>>> you know programmers are terrible at couting backwards.
> >>>
> >>> I prefer to always unwind in reverse order so I'd prefer to leave it
> >>> as-is.
> >
> >> It is just a comment from my side. we can leave the actual decision to the
> >> current maintainer.
> >
> > Right.
> > And maintainer would like to see simple:
> >
> > while (i--) {
> >     kfree(token_location_attrs[i].attr.name);
> >     kfree(token_value_attrs[i].attr.name);
> > }

For me this looks better.

> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> 
> 
> 

-- 
Pali Rohár
pali.rohar@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html