| Message ID | 20180118104503.GA30055@mwanda (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 >
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
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 > >
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); }
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
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); > }
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 > > >
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); }
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>