diff mbox

[v3,3/4] driver property: constify property arrays values

Message ID 20170201173125.40354-4-dmitry.torokhov@gmail.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Dmitry Torokhov Feb. 1, 2017, 5:31 p.m. UTC 
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  | 66 +++++++++++++++++++++++++++++++++---------------
 include/linux/property.h | 12 ++++-----
 2 files changed, 52 insertions(+), 26 deletions(-)

Comments

Joe Perches Feb. 1, 2017, 11:30 p.m. UTC | #1 
On Wed, 2017-02-01 at 09:31 -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.

trivia:

> diff --git a/drivers/base/property.c b/drivers/base/property.c
[]
> @@ -682,44 +682,66 @@ 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) {
> +				error = -ENOMEM;

An unnecessary set as the return from the above
property_copy_string_array is either 0 or -ENOMEM

--
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
Mika Westerberg Feb. 2, 2017, 10:08 a.m. UTC | #2 
On Wed, Feb 01, 2017 at 09:31:24AM -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>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
--
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
Rafael J. Wysocki Feb. 2, 2017, 11:24 a.m. UTC | #3 
On Wednesday, February 01, 2017 03:30:03 PM Joe Perches wrote:
> On Wed, 2017-02-01 at 09:31 -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.
> 
> trivia:
> 
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> []
> > @@ -682,44 +682,66 @@ 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) {
> > +				error = -ENOMEM;
> 
> An unnecessary set as the return from the above
> property_copy_string_array is either 0 or -ENOMEM

Even if it is not -ENOMEM, it still can be propagated to the caller.

I can fix this up unless Dmitry wants to send an updated patch.

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
diff mbox

Patch

diff --git a/drivers/base/property.c b/drivers/base/property.c
index edc09854520b..8cfbc94cafb1 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -682,44 +682,66 @@  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) {
+				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 +751,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;