| Message ID | 20170202163918.GA21924@dtor-ws (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Data that is fed into property arrays should not be modified, so let's > mark > relevant pointers as const. This will allow us making source arrays as > const/__initconst. > > Also fix memory leaks on errors in property_entry_copy(). While the code looks okay, I'm not sure what memory leaks you are referring to. The idea as far as I remember was to run *free() function if *copy() fails. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > Not sending the rest of the series as to not clutter mailing lists too > much... > > drivers/base/property.c | 66 +++++++++++++++++++++++++++++++---- > ----------- > include/linux/property.h | 12 ++++---- > 2 files changed, 51 insertions(+), 27 deletions(-) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index edc09854520b..09fb9757e086 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -682,44 +682,64 @@ int fwnode_property_match_string(struct > fwnode_handle *fwnode, > } > EXPORT_SYMBOL_GPL(fwnode_property_match_string); > > +static int property_copy_string_array(struct property_entry *dst, > + const struct property_entry > *src) > +{ > + char **d; > + size_t nval = src->length / sizeof(*d); > + size_t i; > + > + d = kcalloc(nval, sizeof(*d), GFP_KERNEL); > + if (!d) > + return -ENOMEM; > + > + for (i = 0; i < nval; i++) { > + d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL); > + if (!d[i] && src->pointer.str[i]) { > + while (--i >= 0) > + kfree(d[i]); > + kfree(d); > + return -ENOMEM; > + } > + } > + > + dst->pointer.str = (void *)d; > + return 0; > +} > + > static int property_entry_copy(struct property_entry *dst, > const struct property_entry *src) > { > - const char **d, **s; > - size_t i, nval; > + int error; > > dst->name = kstrdup(src->name, GFP_KERNEL); > if (!dst->name) > return -ENOMEM; > > if (src->is_array) { > - if (!src->length) > - return -ENODATA; > + if (!src->length) { > + error = -ENODATA; > + goto out_free_name; > + } > > if (src->is_string) { > - nval = src->length / sizeof(const char *); > - dst->pointer.str = kcalloc(nval, sizeof(const > char *), > - GFP_KERNEL); > - if (!dst->pointer.str) > - return -ENOMEM; > - > - d = dst->pointer.str; > - s = src->pointer.str; > - for (i = 0; i < nval; i++) { > - d[i] = kstrdup(s[i], GFP_KERNEL); > - if (!d[i] && s[i]) > - return -ENOMEM; > - } > + error = property_copy_string_array(dst, src); > + if (error) > + goto out_free_name; > } else { > dst->pointer.raw_data = kmemdup(src- > >pointer.raw_data, > src->length, > GFP_KERNEL); > - if (!dst->pointer.raw_data) > - return -ENOMEM; > + if (!dst->pointer.raw_data) { > + error = -ENOMEM; > + goto out_free_name; > + } > } > } else if (src->is_string) { > dst->value.str = kstrdup(src->value.str, GFP_KERNEL); > - if (!dst->value.str && src->value.str) > - return -ENOMEM; > + if (!dst->value.str && src->value.str) { > + error = -ENOMEM; > + goto out_free_name; > + } > } else { > dst->value.raw_data = src->value.raw_data; > } > @@ -729,6 +749,10 @@ static int property_entry_copy(struct > property_entry *dst, > dst->is_string = src->is_string; > > return 0; > + > +out_free_name: > + kfree(dst->name); > + return error; > } > > /** > diff --git a/include/linux/property.h b/include/linux/property.h > index 5746e9927016..64e3a9c6d95f 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -160,12 +160,12 @@ struct property_entry { > bool is_string; > union { > union { > - void *raw_data; > - u8 *u8_data; > - u16 *u16_data; > - u32 *u32_data; > - u64 *u64_data; > - const char **str; > + const void *raw_data; > + const u8 *u8_data; > + const u16 *u16_data; > + const u32 *u32_data; > + const u64 *u64_data; > + const char * const *str; > } pointer; > union { > unsigned long long raw_data;
On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: >> From: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> >> Data that is fed into property arrays should not be modified, so >let's >> mark >> relevant pointers as const. This will allow us making source arrays >as >> const/__initconst. >> > >> Also fix memory leaks on errors in property_entry_copy(). > >While the code looks okay, I'm not sure what memory leaks you are >referring to. The idea as far as I remember was to run *free() function >if *copy() fails. That could have been OK for internal function, but will not work for public API, as it goes against normal pattern. You will be old and grey and still correcting patches that would be getting it wrong :) Thanks.
On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote: > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko > @linux.intel.com> wrote: > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > > Data that is fed into property arrays should not be modified, so > > > > let's > > > mark > > > relevant pointers as const. This will allow us making source > > > arrays > > > > as > > > const/__initconst. > > > > > > Also fix memory leaks on errors in property_entry_copy(). > > > > While the code looks okay, I'm not sure what memory leaks you are > > referring to. The idea as far as I remember was to run *free() > > function > > if *copy() fails. > > That could have been OK for internal function, but will not work for > public API, as it goes against normal pattern. > > You will be old and grey and still correcting patches that would be > getting it wrong :) Yes, which sounds not exactly as "we have memory leaks and here we are fixing them". So, my comment regarding to phrasing of the commit message. Someone might mistakenly think that it needs to be ported as earlier as this had been introduced.
On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote: > On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote: > > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko > > @linux.intel.com> wrote: > > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > > > > Data that is fed into property arrays should not be modified, so > > > > > > let's > > > > mark > > > > relevant pointers as const. This will allow us making source > > > > arrays > > > > > > as > > > > const/__initconst. > > > > > > > > Also fix memory leaks on errors in property_entry_copy(). > > > > > > While the code looks okay, I'm not sure what memory leaks you are > > > referring to. The idea as far as I remember was to run *free() > > > function > > > if *copy() fails. > > > > That could have been OK for internal function, but will not work for > > public API, as it goes against normal pattern. > > > > You will be old and grey and still correcting patches that would be > > getting it wrong :) > > Yes, which sounds not exactly as "we have memory leaks and here we are > fixing them". So, my comment regarding to phrasing of the commit > message. Someone might mistakenly think that it needs to be ported as > earlier as this had been introduced. OK, I'll leave it up to Rafael to massage the commit message as he sees fit. Thanks.
On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote: >> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote: >> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko >> > @linux.intel.com> wrote: >> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: >> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> > > > >> > > > Data that is fed into property arrays should not be modified, so >> > > >> > > let's >> > > > mark >> > > > relevant pointers as const. This will allow us making source >> > > > arrays >> > > >> > > as >> > > > const/__initconst. >> > > > >> > > > Also fix memory leaks on errors in property_entry_copy(). >> > > >> > > While the code looks okay, I'm not sure what memory leaks you are >> > > referring to. The idea as far as I remember was to run *free() >> > > function >> > > if *copy() fails. >> > >> > That could have been OK for internal function, but will not work for >> > public API, as it goes against normal pattern. But it is an internal function, isn't it? Also its only caller does the right thing AFAICS. >> > You will be old and grey and still correcting patches that would be >> > getting it wrong :) >> >> Yes, which sounds not exactly as "we have memory leaks and here we are >> fixing them". So, my comment regarding to phrasing of the commit >> message. Someone might mistakenly think that it needs to be ported as >> earlier as this had been introduced. > > OK, I'll leave it up to Rafael to massage the commit message as he sees > fit. To be precise, there are no memory leaks and this is just adding an unnecessary label along with some code around it, equally unnecessary. Are you planning on making property_entry_copy() non-static? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 03, 2017 at 12:16:29AM +0100, Rafael J. Wysocki wrote: > On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote: > >> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote: > >> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko > >> > @linux.intel.com> wrote: > >> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: > >> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> > > > > >> > > > Data that is fed into property arrays should not be modified, so > >> > > > >> > > let's > >> > > > mark > >> > > > relevant pointers as const. This will allow us making source > >> > > > arrays > >> > > > >> > > as > >> > > > const/__initconst. > >> > > > > >> > > > Also fix memory leaks on errors in property_entry_copy(). > >> > > > >> > > While the code looks okay, I'm not sure what memory leaks you are > >> > > referring to. The idea as far as I remember was to run *free() > >> > > function > >> > > if *copy() fails. > >> > > >> > That could have been OK for internal function, but will not work for > >> > public API, as it goes against normal pattern. > > But it is an internal function, isn't it? > > Also its only caller does the right thing AFAICS. No, actually property_entries_dup() does not do the right thing anymore :(. > > >> > You will be old and grey and still correcting patches that would be > >> > getting it wrong :) > >> > >> Yes, which sounds not exactly as "we have memory leaks and here we are > >> fixing them". So, my comment regarding to phrasing of the commit > >> message. Someone might mistakenly think that it needs to be ported as > >> earlier as this had been introduced. > > > > OK, I'll leave it up to Rafael to massage the commit message as he sees > > fit. > > To be precise, there are no memory leaks and this is just adding an > unnecessary label along with some code around it, equally unnecessary. > > Are you planning on making property_entry_copy() non-static? Maybe, but not yet. Still, I am uncomfortable with functions not cleaning up but rather requiring leaving failed property structure in such state that cleanup function will not crash on it. I think it is fragile and I'd rather rework it so we clean up on the spot. Thanks.
On Fri, Feb 3, 2017 at 1:16 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Fri, Feb 03, 2017 at 12:16:29AM +0100, Rafael J. Wysocki wrote: >> On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote: >> >> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote: >> >> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko >> >> > @linux.intel.com> wrote: >> >> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: >> >> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> >> > > > >> >> > > > Data that is fed into property arrays should not be modified, so >> >> > > >> >> > > let's >> >> > > > mark >> >> > > > relevant pointers as const. This will allow us making source >> >> > > > arrays >> >> > > >> >> > > as >> >> > > > const/__initconst. >> >> > > > >> >> > > > Also fix memory leaks on errors in property_entry_copy(). >> >> > > >> >> > > While the code looks okay, I'm not sure what memory leaks you are >> >> > > referring to. The idea as far as I remember was to run *free() >> >> > > function >> >> > > if *copy() fails. >> >> > >> >> > That could have been OK for internal function, but will not work for >> >> > public API, as it goes against normal pattern. >> >> But it is an internal function, isn't it? >> >> Also its only caller does the right thing AFAICS. > > No, actually property_entries_dup() does not do the right thing anymore > :(. Well, it looks like this is because of patch [1/4], so IMO the changes to clean up on errors in property_entry_copy() should be made in that patch as well. Right now we seem to have potential memory leaks introduced in patch [1/4] and then fixed up in patch [3/4] in the same series which doesn't feel quite right to be honest. >> >> >> > You will be old and grey and still correcting patches that would be >> >> > getting it wrong :) >> >> >> >> Yes, which sounds not exactly as "we have memory leaks and here we are >> >> fixing them". So, my comment regarding to phrasing of the commit >> >> message. Someone might mistakenly think that it needs to be ported as >> >> earlier as this had been introduced. >> > >> > OK, I'll leave it up to Rafael to massage the commit message as he sees >> > fit. >> >> To be precise, there are no memory leaks and this is just adding an >> unnecessary label along with some code around it, equally unnecessary. >> >> Are you planning on making property_entry_copy() non-static? > > Maybe, but not yet. Still, I am uncomfortable with functions not > cleaning up but rather requiring leaving failed property structure in > such state that cleanup function will not crash on it. I think it is > fragile and I'd rather rework it so we clean up on the spot. Fair enough, but that should happen in patch [1/4] already IMO. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 03, 2017 at 01:37:25AM +0100, Rafael J. Wysocki wrote: > On Fri, Feb 3, 2017 at 1:16 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Fri, Feb 03, 2017 at 12:16:29AM +0100, Rafael J. Wysocki wrote: > >> On Thu, Feb 2, 2017 at 7:38 PM, Dmitry Torokhov > >> <dmitry.torokhov@gmail.com> wrote: > >> > On Thu, Feb 02, 2017 at 07:52:58PM +0200, Andy Shevchenko wrote: > >> >> On Thu, 2017-02-02 at 09:07 -0800, Dmitry Torokhov wrote: > >> >> > On February 2, 2017 8:48:30 AM PST, Andy Shevchenko <andriy.shevchenko > >> >> > @linux.intel.com> wrote: > >> >> > > On Thu, 2017-02-02 at 08:39 -0800, Dmitry Torokhov wrote: > >> >> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> >> > > > > >> >> > > > Data that is fed into property arrays should not be modified, so > >> >> > > > >> >> > > let's > >> >> > > > mark > >> >> > > > relevant pointers as const. This will allow us making source > >> >> > > > arrays > >> >> > > > >> >> > > as > >> >> > > > const/__initconst. > >> >> > > > > >> >> > > > Also fix memory leaks on errors in property_entry_copy(). > >> >> > > > >> >> > > While the code looks okay, I'm not sure what memory leaks you are > >> >> > > referring to. The idea as far as I remember was to run *free() > >> >> > > function > >> >> > > if *copy() fails. > >> >> > > >> >> > That could have been OK for internal function, but will not work for > >> >> > public API, as it goes against normal pattern. > >> > >> But it is an internal function, isn't it? > >> > >> Also its only caller does the right thing AFAICS. > > > > No, actually property_entries_dup() does not do the right thing anymore > > :(. > > Well, it looks like this is because of patch [1/4], so IMO the changes > to clean up on errors in property_entry_copy() should be made in that > patch as well. > > Right now we seem to have potential memory leaks introduced in patch > [1/4] and then fixed up in patch [3/4] in the same series which > doesn't feel quite right to be honest. Totally agree, I'm reshuffling and will repost the series in a few.
diff --git a/drivers/base/property.c b/drivers/base/property.c index edc09854520b..09fb9757e086 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -682,44 +682,64 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode, } EXPORT_SYMBOL_GPL(fwnode_property_match_string); +static int property_copy_string_array(struct property_entry *dst, + const struct property_entry *src) +{ + char **d; + size_t nval = src->length / sizeof(*d); + size_t i; + + d = kcalloc(nval, sizeof(*d), GFP_KERNEL); + if (!d) + return -ENOMEM; + + for (i = 0; i < nval; i++) { + d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL); + if (!d[i] && src->pointer.str[i]) { + while (--i >= 0) + kfree(d[i]); + kfree(d); + return -ENOMEM; + } + } + + dst->pointer.str = (void *)d; + return 0; +} + static int property_entry_copy(struct property_entry *dst, const struct property_entry *src) { - const char **d, **s; - size_t i, nval; + int error; dst->name = kstrdup(src->name, GFP_KERNEL); if (!dst->name) return -ENOMEM; if (src->is_array) { - if (!src->length) - return -ENODATA; + if (!src->length) { + error = -ENODATA; + goto out_free_name; + } if (src->is_string) { - nval = src->length / sizeof(const char *); - dst->pointer.str = kcalloc(nval, sizeof(const char *), - GFP_KERNEL); - if (!dst->pointer.str) - return -ENOMEM; - - d = dst->pointer.str; - s = src->pointer.str; - for (i = 0; i < nval; i++) { - d[i] = kstrdup(s[i], GFP_KERNEL); - if (!d[i] && s[i]) - return -ENOMEM; - } + error = property_copy_string_array(dst, src); + if (error) + goto out_free_name; } else { dst->pointer.raw_data = kmemdup(src->pointer.raw_data, src->length, GFP_KERNEL); - if (!dst->pointer.raw_data) - return -ENOMEM; + if (!dst->pointer.raw_data) { + error = -ENOMEM; + goto out_free_name; + } } } else if (src->is_string) { dst->value.str = kstrdup(src->value.str, GFP_KERNEL); - if (!dst->value.str && src->value.str) - return -ENOMEM; + if (!dst->value.str && src->value.str) { + error = -ENOMEM; + goto out_free_name; + } } else { dst->value.raw_data = src->value.raw_data; } @@ -729,6 +749,10 @@ static int property_entry_copy(struct property_entry *dst, dst->is_string = src->is_string; return 0; + +out_free_name: + kfree(dst->name); + return error; } /** diff --git a/include/linux/property.h b/include/linux/property.h index 5746e9927016..64e3a9c6d95f 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -160,12 +160,12 @@ struct property_entry { bool is_string; union { union { - void *raw_data; - u8 *u8_data; - u16 *u16_data; - u32 *u32_data; - u64 *u64_data; - const char **str; + const void *raw_data; + const u8 *u8_data; + const u16 *u16_data; + const u32 *u32_data; + const u64 *u64_data; + const char * const *str; } pointer; union { unsigned long long raw_data;