Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571246 > unrolled thread

[PATCH v2 3/4] driver property: constify property arrays values

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/4] driver property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-01 03:20 +0100
    Re: [PATCH v2 3/4] driver property: constify property arrays values Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-02-01 15:50 +0100
      Re: [PATCH v2 3/4] driver property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-01 18:10 +0100

#1571246 — [PATCH v2 3/4] driver property: constify property arrays values

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().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/property.c  | 65 +++++++++++++++++++++++++++++++++---------------
 include/linux/property.h | 12 ++++-----
 2 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index edc09854520b..fd91e0891665 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -682,44 +682,65 @@ 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]);
+			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) {
+				error = -ENOMEM;
+				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 +750,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;
-- 
2.11.0.483.g087da7b7c-goog

[toc] | [next] | [standalone]

#1571619

On Tue, Jan 31, 2017 at 06:11:29PM -0800, Dmitry Torokhov wrote:
> 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().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/property.c  | 65 +++++++++++++++++++++++++++++++++---------------
>  include/linux/property.h | 12 ++++-----
>  2 files changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index edc09854520b..fd91e0891665 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -682,44 +682,65 @@ 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]);

Should we free d as well here?

> +			return -ENOMEM;
> +		}
> +	}
> +
> +	dst->pointer.str = (void *)d;
> +	return 0;
> +}

[toc] | [prev] | [next] | [standalone]

#1571770

On Wed, Feb 01, 2017 at 04:42:33PM +0200, Mika Westerberg wrote:
> On Tue, Jan 31, 2017 at 06:11:29PM -0800, Dmitry Torokhov wrote:
> > 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().
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/base/property.c  | 65 +++++++++++++++++++++++++++++++++---------------
> >  include/linux/property.h | 12 ++++-----
> >  2 files changed, 51 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index edc09854520b..fd91e0891665 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -682,44 +682,65 @@ 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]);
> 
> Should we free d as well here?

Yep.

> 
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	dst->pointer.str = (void *)d;
> > +	return 0;
> > +}

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web