[PATCH v2] device property: Get rid of union aliasing

[PATCH v2] device property: Get rid of union aliasing

[Andy Shevchenko]

The commit

  318a19718261 ("device property: refactor built-in properties support")

went way too far and brought a union aliasing. Partially revert it here
to get rid of union aliasing.

Note, Apple properties support is still utilizing this trick.

What union aliasing is?
~~~~~~~~~~~~~~~~~~~~~~~
The C99 standard in section 6.2.5 paragraph 20 defines union type as
"an overlapping nonempty set of member objects". It also states in
section 6.7.2.1 paragraph 14 that "the value of at most one of the
members can be stored in a union object at any time'.

Union aliasing is a type punning mechanism using union members to store
as one type and read back as another.

Why it's not good?
~~~~~~~~~~~~~~~~~~
Section 6.2.6.1 paragraph 6 says that a union object may not be a trap
representation, although its member objects may be.

Meanwhile annex J.1 says that "the value of a union member other than
the last one stored into" is unspecified [removed in C11].

In TC3, a footnote is added which specifies that accessing a member of a
union other than the last one stored causes "the object representation"
to be re-interpreted in the new type and specifically refers to this as
"type punning". This conflicts to some degree with Annex J.1.

While it's working in Linux with GCC, the use of union members to do
type punning is not clear area in the C standard and might lead to
unspecified behaviour.

More information is available in this [1] blog post.

[1]: https://davmac.wordpress.com/2010/02/26/c99-revisited/

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

- extend commit message to explain what union aliasing is and why it's not good

 drivers/base/property.c                 | 99 ++++++++++++++++++++-----
 drivers/firmware/efi/apple-properties.c |  8 +-
 include/linux/property.h                | 52 +++++++------
 3 files changed, 112 insertions(+), 47 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8f205f6461ed..de19d7cc073b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -56,6 +56,67 @@ pset_prop_get(const struct property_set *pset, const char *name)
 	return NULL;
 }
 
+static const void *property_get_pointer(const struct property_entry *prop)
+{
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		if (prop->is_array)
+			return prop->pointer.u8_data;
+		return &prop->value.u8_data;
+	case DEV_PROP_U16:
+		if (prop->is_array)
+			return prop->pointer.u16_data;
+		return &prop->value.u16_data;
+	case DEV_PROP_U32:
+		if (prop->is_array)
+			return prop->pointer.u32_data;
+		return &prop->value.u32_data;
+	case DEV_PROP_U64:
+		if (prop->is_array)
+			return prop->pointer.u64_data;
+		return &prop->value.u64_data;
+	default:
+		if (prop->is_array)
+			return prop->pointer.str;
+		return &prop->value.str;
+	}
+}
+
+static void property_set_pointer(struct property_entry *prop, const void *pointer)
+{
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		if (prop->is_array)
+			prop->pointer.u8_data = pointer;
+		else
+			prop->value.u8_data = *((u8 *)pointer);
+		break;
+	case DEV_PROP_U16:
+		if (prop->is_array)
+			prop->pointer.u16_data = pointer;
+		else
+			prop->value.u16_data = *((u16 *)pointer);
+		break;
+	case DEV_PROP_U32:
+		if (prop->is_array)
+			prop->pointer.u32_data = pointer;
+		else
+			prop->value.u32_data = *((u32 *)pointer);
+		break;
+	case DEV_PROP_U64:
+		if (prop->is_array)
+			prop->pointer.u64_data = pointer;
+		else
+			prop->value.u64_data = *((u64 *)pointer);
+		break;
+	default:
+		if (prop->is_array)
+			prop->pointer.str = pointer;
+		else
+			prop->value.str = pointer;
+	}
+}
+
 static const void *pset_prop_find(const struct property_set *pset,
 				  const char *propname, size_t length)
 {
@@ -65,10 +126,7 @@ static const void *pset_prop_find(const struct property_set *pset,
 	prop = pset_prop_get(pset, propname);
 	if (!prop)
 		return ERR_PTR(-EINVAL);
-	if (prop->is_array)
-		pointer = prop->pointer.raw_data;
-	else
-		pointer = &prop->value.raw_data;
+	pointer = property_get_pointer(prop);
 	if (!pointer)
 		return ERR_PTR(-ENODATA);
 	if (length > prop->length)
@@ -698,16 +756,17 @@ EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
 
 static void property_entry_free_data(const struct property_entry *p)
 {
+	const void *pointer = property_get_pointer(p);
 	size_t i, nval;
 
 	if (p->is_array) {
-		if (p->is_string && p->pointer.str) {
+		if (p->type == DEV_PROP_STRING && p->pointer.str) {
 			nval = p->length / sizeof(const char *);
 			for (i = 0; i < nval; i++)
 				kfree(p->pointer.str[i]);
 		}
-		kfree(p->pointer.raw_data);
-	} else if (p->is_string) {
+		kfree(pointer);
+	} else if (p->type == DEV_PROP_STRING) {
 		kfree(p->value.str);
 	}
 	kfree(p->name);
@@ -716,7 +775,7 @@ static void property_entry_free_data(const struct property_entry *p)
 static int property_copy_string_array(struct property_entry *dst,
 				      const struct property_entry *src)
 {
-	char **d;
+	const char **d;
 	size_t nval = src->length / sizeof(*d);
 	int i;
 
@@ -734,40 +793,44 @@ static int property_copy_string_array(struct property_entry *dst,
 		}
 	}
 
-	dst->pointer.raw_data = d;
+	dst->pointer.str = d;
 	return 0;
 }
 
 static int property_entry_copy_data(struct property_entry *dst,
 				    const struct property_entry *src)
 {
+	const void *pointer = property_get_pointer(src);
+	const void *new;
 	int error;
 
 	if (src->is_array) {
 		if (!src->length)
 			return -ENODATA;
 
-		if (src->is_string) {
+		if (src->type == DEV_PROP_STRING) {
 			error = property_copy_string_array(dst, src);
 			if (error)
 				return error;
+			new = dst->pointer.str;
 		} else {
-			dst->pointer.raw_data = kmemdup(src->pointer.raw_data,
-							src->length, GFP_KERNEL);
-			if (!dst->pointer.raw_data)
+			new = kmemdup(pointer, src->length, GFP_KERNEL);
+			if (!new)
 				return -ENOMEM;
 		}
-	} else if (src->is_string) {
-		dst->value.str = kstrdup(src->value.str, GFP_KERNEL);
-		if (!dst->value.str && src->value.str)
+	} else if (src->type == DEV_PROP_STRING) {
+		new = kstrdup(src->value.str, GFP_KERNEL);
+		if (!new && src->value.str)
 			return -ENOMEM;
 	} else {
-		dst->value.raw_data = src->value.raw_data;
+		new = pointer;
 	}
 
 	dst->length = src->length;
 	dst->is_array = src->is_array;
-	dst->is_string = src->is_string;
+	dst->type = src->type;
+
+	property_set_pointer(dst, new);
 
 	dst->name = kstrdup(src->name, GFP_KERNEL);
 	if (!dst->name)
diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
index adaa9a3714b9..b2c2d5a45f91 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -13,6 +13,9 @@
  *
  * You should have received a copy of the GNU General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * FIXME: The approach is still based on union aliasing and should be
+ * replaced by a proper resource provider.
  */
 
 #define pr_fmt(fmt) "apple-properties: " fmt
@@ -96,12 +99,13 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
 		entry[i].name = key;
 		entry[i].length = val_len - sizeof(val_len);
 		entry[i].is_array = !!entry[i].length;
-		entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
+		entry[i].type = DEV_PROP_U8;
+		entry[i].pointer.u8_data = ptr + key_len + sizeof(val_len);
 
 		if (dump_properties) {
 			dev_info(dev, "property: %s\n", entry[i].name);
 			print_hex_dump(KERN_INFO, pr_fmt(), DUMP_PREFIX_OFFSET,
-				16, 1, entry[i].pointer.raw_data,
+				16, 1, entry[i].pointer.u8_data,
 				entry[i].length, true);
 		}
 
diff --git a/include/linux/property.h b/include/linux/property.h
index 2eea4b310fc2..ac8a1ebc4c1b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -178,7 +178,7 @@ static inline int fwnode_property_read_u64(const struct fwnode_handle *fwnode,
  * @name: Name of the property.
  * @length: Length of data making up the value.
  * @is_array: True when the property is an array.
- * @is_string: True when property is a string.
+ * @type: Type of the data in unions.
  * @pointer: Pointer to the property (an array of items of the given type).
  * @value: Value of the property (when it is a single item of the given type).
  */
@@ -186,10 +186,9 @@ struct property_entry {
 	const char *name;
 	size_t length;
 	bool is_array;
-	bool is_string;
+	enum dev_prop_type type;
 	union {
 		union {
-			const void *raw_data;
 			const u8 *u8_data;
 			const u16 *u16_data;
 			const u32 *u32_data;
@@ -197,7 +196,6 @@ struct property_entry {
 			const char * const *str;
 		} pointer;
 		union {
-			unsigned long long raw_data;
 			u8 u8_data;
 			u16 u16_data;
 			u32 u32_data;
@@ -213,55 +211,55 @@ struct property_entry {
  * and structs.
  */
 
-#define PROPERTY_ENTRY_INTEGER_ARRAY(_name_, _type_, _val_)	\
-(struct property_entry) {					\
-	.name = _name_,						\
-	.length = ARRAY_SIZE(_val_) * sizeof(_type_),		\
-	.is_array = true,					\
-	.is_string = false,					\
-	{ .pointer = { ._type_##_data = _val_ } },		\
+#define PROPERTY_ENTRY_INTEGER_ARRAY(_name_, _type_, _Type_, _val_)	\
+(struct property_entry) {						\
+	.name = _name_,							\
+	.length = ARRAY_SIZE(_val_) * sizeof(_type_),			\
+	.is_array = true,						\
+	.type = DEV_PROP_##_Type_,					\
+	{ .pointer = { ._type_##_data = _val_ } },			\
 }
 
 #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u8, _val_)
+	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u8, U8, _val_)
 #define PROPERTY_ENTRY_U16_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u16, _val_)
+	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u16, U16, _val_)
 #define PROPERTY_ENTRY_U32_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, _val_)
+	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, U32, _val_)
 #define PROPERTY_ENTRY_U64_ARRAY(_name_, _val_)			\
-	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u64, _val_)
+	PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u64, U64, _val_)
 
 #define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)		\
 (struct property_entry) {					\
 	.name = _name_,						\
 	.length = ARRAY_SIZE(_val_) * sizeof(const char *),	\
 	.is_array = true,					\
-	.is_string = true,					\
+	.type = DEV_PROP_STRING,				\
 	{ .pointer = { .str = _val_ } },			\
 }
 
-#define PROPERTY_ENTRY_INTEGER(_name_, _type_, _val_)	\
-(struct property_entry) {				\
-	.name = _name_,					\
-	.length = sizeof(_type_),			\
-	.is_string = false,				\
-	{ .value = { ._type_##_data = _val_ } },	\
+#define PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_)	\
+(struct property_entry) {					\
+	.name = _name_,						\
+	.length = sizeof(_type_),				\
+	.type = DEV_PROP_##_Type_,				\
+	{ .value = { ._type_##_data = _val_ } },		\
 }
 
 #define PROPERTY_ENTRY_U8(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u8, _val_)
+	PROPERTY_ENTRY_INTEGER(_name_, u8, U8, _val_)
 #define PROPERTY_ENTRY_U16(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u16, _val_)
+	PROPERTY_ENTRY_INTEGER(_name_, u16, U16, _val_)
 #define PROPERTY_ENTRY_U32(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u32, _val_)
+	PROPERTY_ENTRY_INTEGER(_name_, u32, U32, _val_)
 #define PROPERTY_ENTRY_U64(_name_, _val_)		\
-	PROPERTY_ENTRY_INTEGER(_name_, u64, _val_)
+	PROPERTY_ENTRY_INTEGER(_name_, u64, U64, _val_)
 
 #define PROPERTY_ENTRY_STRING(_name_, _val_)		\
 (struct property_entry) {				\
 	.name = _name_,					\
 	.length = sizeof(_val_),			\
-	.is_string = true,				\
+	.type = DEV_PROP_STRING,			\
 	{ .value = { .str = _val_ } },			\
 }
 
-- 
2.17.0

Re: [PATCH v2] device property: Get rid of union aliasing

[Ard Biesheuvel]

On 8 May 2018 at 15:15, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The commit
>
>   318a19718261 ("device property: refactor built-in properties support")
>
> went way too far and brought a union aliasing. Partially revert it here
> to get rid of union aliasing.
>
> Note, Apple properties support is still utilizing this trick.
>
> What union aliasing is?
> ~~~~~~~~~~~~~~~~~~~~~~~
> The C99 standard in section 6.2.5 paragraph 20 defines union type as
> "an overlapping nonempty set of member objects". It also states in
> section 6.7.2.1 paragraph 14 that "the value of at most one of the
> members can be stored in a union object at any time'.
>
> Union aliasing is a type punning mechanism using union members to store
> as one type and read back as another.
>
> Why it's not good?
> ~~~~~~~~~~~~~~~~~~
> Section 6.2.6.1 paragraph 6 says that a union object may not be a trap
> representation, although its member objects may be.
>
> Meanwhile annex J.1 says that "the value of a union member other than
> the last one stored into" is unspecified [removed in C11].
>
> In TC3, a footnote is added which specifies that accessing a member of a
> union other than the last one stored causes "the object representation"
> to be re-interpreted in the new type and specifically refers to this as
> "type punning". This conflicts to some degree with Annex J.1.
>
> While it's working in Linux with GCC, the use of union members to do
> type punning is not clear area in the C standard and might lead to
> unspecified behaviour.
>
> More information is available in this [1] blog post.
>
> [1]: https://davmac.wordpress.com/2010/02/26/c99-revisited/
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

For the EFI change

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>
> - extend commit message to explain what union aliasing is and why it's not good
>
>  drivers/base/property.c                 | 99 ++++++++++++++++++++-----
>  drivers/firmware/efi/apple-properties.c |  8 +-
>  include/linux/property.h                | 52 +++++++------
>  3 files changed, 112 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8f205f6461ed..de19d7cc073b 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -56,6 +56,67 @@ pset_prop_get(const struct property_set *pset, const char *name)
>         return NULL;
>  }
>
> +static const void *property_get_pointer(const struct property_entry *prop)
> +{
> +       switch (prop->type) {
> +       case DEV_PROP_U8:
> +               if (prop->is_array)
> +                       return prop->pointer.u8_data;
> +               return &prop->value.u8_data;
> +       case DEV_PROP_U16:
> +               if (prop->is_array)
> +                       return prop->pointer.u16_data;
> +               return &prop->value.u16_data;
> +       case DEV_PROP_U32:
> +               if (prop->is_array)
> +                       return prop->pointer.u32_data;
> +               return &prop->value.u32_data;
> +       case DEV_PROP_U64:
> +               if (prop->is_array)
> +                       return prop->pointer.u64_data;
> +               return &prop->value.u64_data;
> +       default:
> +               if (prop->is_array)
> +                       return prop->pointer.str;
> +               return &prop->value.str;
> +       }
> +}
> +
> +static void property_set_pointer(struct property_entry *prop, const void *pointer)
> +{
> +       switch (prop->type) {
> +       case DEV_PROP_U8:
> +               if (prop->is_array)
> +                       prop->pointer.u8_data = pointer;
> +               else
> +                       prop->value.u8_data = *((u8 *)pointer);
> +               break;
> +       case DEV_PROP_U16:
> +               if (prop->is_array)
> +                       prop->pointer.u16_data = pointer;
> +               else
> +                       prop->value.u16_data = *((u16 *)pointer);
> +               break;
> +       case DEV_PROP_U32:
> +               if (prop->is_array)
> +                       prop->pointer.u32_data = pointer;
> +               else
> +                       prop->value.u32_data = *((u32 *)pointer);
> +               break;
> +       case DEV_PROP_U64:
> +               if (prop->is_array)
> +                       prop->pointer.u64_data = pointer;
> +               else
> +                       prop->value.u64_data = *((u64 *)pointer);
> +               break;
> +       default:
> +               if (prop->is_array)
> +                       prop->pointer.str = pointer;
> +               else
> +                       prop->value.str = pointer;
> +       }
> +}
> +
>  static const void *pset_prop_find(const struct property_set *pset,
>                                   const char *propname, size_t length)
>  {
> @@ -65,10 +126,7 @@ static const void *pset_prop_find(const struct property_set *pset,
>         prop = pset_prop_get(pset, propname);
>         if (!prop)
>                 return ERR_PTR(-EINVAL);
> -       if (prop->is_array)
> -               pointer = prop->pointer.raw_data;
> -       else
> -               pointer = &prop->value.raw_data;
> +       pointer = property_get_pointer(prop);
>         if (!pointer)
>                 return ERR_PTR(-ENODATA);
>         if (length > prop->length)
> @@ -698,16 +756,17 @@ EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
>
>  static void property_entry_free_data(const struct property_entry *p)
>  {
> +       const void *pointer = property_get_pointer(p);
>         size_t i, nval;
>
>         if (p->is_array) {
> -               if (p->is_string && p->pointer.str) {
> +               if (p->type == DEV_PROP_STRING && p->pointer.str) {
>                         nval = p->length / sizeof(const char *);
>                         for (i = 0; i < nval; i++)
>                                 kfree(p->pointer.str[i]);
>                 }
> -               kfree(p->pointer.raw_data);
> -       } else if (p->is_string) {
> +               kfree(pointer);
> +       } else if (p->type == DEV_PROP_STRING) {
>                 kfree(p->value.str);
>         }
>         kfree(p->name);
> @@ -716,7 +775,7 @@ static void property_entry_free_data(const struct property_entry *p)
>  static int property_copy_string_array(struct property_entry *dst,
>                                       const struct property_entry *src)
>  {
> -       char **d;
> +       const char **d;
>         size_t nval = src->length / sizeof(*d);
>         int i;
>
> @@ -734,40 +793,44 @@ static int property_copy_string_array(struct property_entry *dst,
>                 }
>         }
>
> -       dst->pointer.raw_data = d;
> +       dst->pointer.str = d;
>         return 0;
>  }
>
>  static int property_entry_copy_data(struct property_entry *dst,
>                                     const struct property_entry *src)
>  {
> +       const void *pointer = property_get_pointer(src);
> +       const void *new;
>         int error;
>
>         if (src->is_array) {
>                 if (!src->length)
>                         return -ENODATA;
>
> -               if (src->is_string) {
> +               if (src->type == DEV_PROP_STRING) {
>                         error = property_copy_string_array(dst, src);
>                         if (error)
>                                 return error;
> +                       new = dst->pointer.str;
>                 } else {
> -                       dst->pointer.raw_data = kmemdup(src->pointer.raw_data,
> -                                                       src->length, GFP_KERNEL);
> -                       if (!dst->pointer.raw_data)
> +                       new = kmemdup(pointer, src->length, GFP_KERNEL);
> +                       if (!new)
>                                 return -ENOMEM;
>                 }
> -       } else if (src->is_string) {
> -               dst->value.str = kstrdup(src->value.str, GFP_KERNEL);
> -               if (!dst->value.str && src->value.str)
> +       } else if (src->type == DEV_PROP_STRING) {
> +               new = kstrdup(src->value.str, GFP_KERNEL);
> +               if (!new && src->value.str)
>                         return -ENOMEM;
>         } else {
> -               dst->value.raw_data = src->value.raw_data;
> +               new = pointer;
>         }
>
>         dst->length = src->length;
>         dst->is_array = src->is_array;
> -       dst->is_string = src->is_string;
> +       dst->type = src->type;
> +
> +       property_set_pointer(dst, new);
>
>         dst->name = kstrdup(src->name, GFP_KERNEL);
>         if (!dst->name)
> diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
> index adaa9a3714b9..b2c2d5a45f91 100644
> --- a/drivers/firmware/efi/apple-properties.c
> +++ b/drivers/firmware/efi/apple-properties.c
> @@ -13,6 +13,9 @@
>   *
>   * You should have received a copy of the GNU General Public License
>   * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * FIXME: The approach is still based on union aliasing and should be
> + * replaced by a proper resource provider.
>   */
>
>  #define pr_fmt(fmt) "apple-properties: " fmt
> @@ -96,12 +99,13 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
>                 entry[i].name = key;
>                 entry[i].length = val_len - sizeof(val_len);
>                 entry[i].is_array = !!entry[i].length;
> -               entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
> +               entry[i].type = DEV_PROP_U8;
> +               entry[i].pointer.u8_data = ptr + key_len + sizeof(val_len);
>
>                 if (dump_properties) {
>                         dev_info(dev, "property: %s\n", entry[i].name);
>                         print_hex_dump(KERN_INFO, pr_fmt(), DUMP_PREFIX_OFFSET,
> -                               16, 1, entry[i].pointer.raw_data,
> +                               16, 1, entry[i].pointer.u8_data,
>                                 entry[i].length, true);
>                 }
>
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 2eea4b310fc2..ac8a1ebc4c1b 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -178,7 +178,7 @@ static inline int fwnode_property_read_u64(const struct fwnode_handle *fwnode,
>   * @name: Name of the property.
>   * @length: Length of data making up the value.
>   * @is_array: True when the property is an array.
> - * @is_string: True when property is a string.
> + * @type: Type of the data in unions.
>   * @pointer: Pointer to the property (an array of items of the given type).
>   * @value: Value of the property (when it is a single item of the given type).
>   */
> @@ -186,10 +186,9 @@ struct property_entry {
>         const char *name;
>         size_t length;
>         bool is_array;
> -       bool is_string;
> +       enum dev_prop_type type;
>         union {
>                 union {
> -                       const void *raw_data;
>                         const u8 *u8_data;
>                         const u16 *u16_data;
>                         const u32 *u32_data;
> @@ -197,7 +196,6 @@ struct property_entry {
>                         const char * const *str;
>                 } pointer;
>                 union {
> -                       unsigned long long raw_data;
>                         u8 u8_data;
>                         u16 u16_data;
>                         u32 u32_data;
> @@ -213,55 +211,55 @@ struct property_entry {
>   * and structs.
>   */
>
> -#define PROPERTY_ENTRY_INTEGER_ARRAY(_name_, _type_, _val_)    \
> -(struct property_entry) {                                      \
> -       .name = _name_,                                         \
> -       .length = ARRAY_SIZE(_val_) * sizeof(_type_),           \
> -       .is_array = true,                                       \
> -       .is_string = false,                                     \
> -       { .pointer = { ._type_##_data = _val_ } },              \
> +#define PROPERTY_ENTRY_INTEGER_ARRAY(_name_, _type_, _Type_, _val_)    \
> +(struct property_entry) {                                              \
> +       .name = _name_,                                                 \
> +       .length = ARRAY_SIZE(_val_) * sizeof(_type_),                   \
> +       .is_array = true,                                               \
> +       .type = DEV_PROP_##_Type_,                                      \
> +       { .pointer = { ._type_##_data = _val_ } },                      \
>  }
>
>  #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)                 \
> -       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u8, _val_)
> +       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u8, U8, _val_)
>  #define PROPERTY_ENTRY_U16_ARRAY(_name_, _val_)                        \
> -       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u16, _val_)
> +       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u16, U16, _val_)
>  #define PROPERTY_ENTRY_U32_ARRAY(_name_, _val_)                        \
> -       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, _val_)
> +       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, U32, _val_)
>  #define PROPERTY_ENTRY_U64_ARRAY(_name_, _val_)                        \
> -       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u64, _val_)
> +       PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u64, U64, _val_)
>
>  #define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)             \
>  (struct property_entry) {                                      \
>         .name = _name_,                                         \
>         .length = ARRAY_SIZE(_val_) * sizeof(const char *),     \
>         .is_array = true,                                       \
> -       .is_string = true,                                      \
> +       .type = DEV_PROP_STRING,                                \
>         { .pointer = { .str = _val_ } },                        \
>  }
>
> -#define PROPERTY_ENTRY_INTEGER(_name_, _type_, _val_)  \
> -(struct property_entry) {                              \
> -       .name = _name_,                                 \
> -       .length = sizeof(_type_),                       \
> -       .is_string = false,                             \
> -       { .value = { ._type_##_data = _val_ } },        \
> +#define PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_)  \
> +(struct property_entry) {                                      \
> +       .name = _name_,                                         \
> +       .length = sizeof(_type_),                               \
> +       .type = DEV_PROP_##_Type_,                              \
> +       { .value = { ._type_##_data = _val_ } },                \
>  }
>
>  #define PROPERTY_ENTRY_U8(_name_, _val_)               \
> -       PROPERTY_ENTRY_INTEGER(_name_, u8, _val_)
> +       PROPERTY_ENTRY_INTEGER(_name_, u8, U8, _val_)
>  #define PROPERTY_ENTRY_U16(_name_, _val_)              \
> -       PROPERTY_ENTRY_INTEGER(_name_, u16, _val_)
> +       PROPERTY_ENTRY_INTEGER(_name_, u16, U16, _val_)
>  #define PROPERTY_ENTRY_U32(_name_, _val_)              \
> -       PROPERTY_ENTRY_INTEGER(_name_, u32, _val_)
> +       PROPERTY_ENTRY_INTEGER(_name_, u32, U32, _val_)
>  #define PROPERTY_ENTRY_U64(_name_, _val_)              \
> -       PROPERTY_ENTRY_INTEGER(_name_, u64, _val_)
> +       PROPERTY_ENTRY_INTEGER(_name_, u64, U64, _val_)
>
>  #define PROPERTY_ENTRY_STRING(_name_, _val_)           \
>  (struct property_entry) {                              \
>         .name = _name_,                                 \
>         .length = sizeof(_val_),                        \
> -       .is_string = true,                              \
> +       .type = DEV_PROP_STRING,                        \
>         { .value = { .str = _val_ } },                  \
>  }
>
> --
> 2.17.0
>

Re: [PATCH v2] device property: Get rid of union aliasing

[Lukas Wunner]

On Tue, May 08, 2018 at 04:15:47PM +0300, Andy Shevchenko wrote:
> --- a/drivers/firmware/efi/apple-properties.c
> +++ b/drivers/firmware/efi/apple-properties.c
> @@ -13,6 +13,9 @@
>   *
>   * You should have received a copy of the GNU General Public License
>   * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * FIXME: The approach is still based on union aliasing and should be
> + * replaced by a proper resource provider.

Why?  All Apple EFI properties are either boolean or u8 arrays.
You've correctly changed this file to always supply u8 arrays,
so I don't see where union aliasing is happening here?

Thanks,

Lukas

>   */
>  
>  #define pr_fmt(fmt) "apple-properties: " fmt
> @@ -96,12 +99,13 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
>  		entry[i].name = key;
>  		entry[i].length = val_len - sizeof(val_len);
>  		entry[i].is_array = !!entry[i].length;
> -		entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
> +		entry[i].type = DEV_PROP_U8;
> +		entry[i].pointer.u8_data = ptr + key_len + sizeof(val_len);
>  
>  		if (dump_properties) {
>  			dev_info(dev, "property: %s\n", entry[i].name);
>  			print_hex_dump(KERN_INFO, pr_fmt(), DUMP_PREFIX_OFFSET,
> -				16, 1, entry[i].pointer.raw_data,
> +				16, 1, entry[i].pointer.u8_data,
>  				entry[i].length, true);
>  		}
>  

Re: [PATCH v2] device property: Get rid of union aliasing

[Andy Shevchenko]

On Mon, 2018-05-14 at 14:18 +0200, Lukas Wunner wrote:
> On Tue, May 08, 2018 at 04:15:47PM +0300, Andy Shevchenko wrote:
> > --- a/drivers/firmware/efi/apple-properties.c
> > +++ b/drivers/firmware/efi/apple-properties.c
> > @@ -13,6 +13,9 @@
> >   *
> >   * You should have received a copy of the GNU General Public
> > License
> >   * along with this program; if not, see <http://www.gnu.org/license
> > s/>.
> > + *
> > + * FIXME: The approach is still based on union aliasing and should
> > be
> > + * replaced by a proper resource provider.
> 
> Why?  All Apple EFI properties are either boolean or u8 arrays.
> You've correctly changed this file to always supply u8 arrays,
> so I don't see where union aliasing is happening here?

Okay, for now I can see only Thunderbolt user of these properties (is it
correct?) in upstream which uses u8 arrays indeed.

Though the implementation is quite fragile in this sense, because it
doesn't discourage people to use device_property_read_string() in case
when it's indeed a string (I saw these kind of properties in the very
dump you posted on your GH page).

So, I can agree that is "not happening _right_now_", but I can't agree
on the implementation robustness.

I can change a wording of the commit message and the FIXME, but _leave_
FIXME in place b/c of above.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2] device property: Get rid of union aliasing

[Lukas Wunner]

On Mon, May 14, 2018 at 03:48:09PM +0300, Andy Shevchenko wrote:
> On Mon, 2018-05-14 at 14:18 +0200, Lukas Wunner wrote:
> > On Tue, May 08, 2018 at 04:15:47PM +0300, Andy Shevchenko wrote:
> > > --- a/drivers/firmware/efi/apple-properties.c
> > > +++ b/drivers/firmware/efi/apple-properties.c
> > > @@ -13,6 +13,9 @@
> > > + * FIXME: The approach is still based on union aliasing and should be
> > > + * replaced by a proper resource provider.
> > 
> > Why?  All Apple EFI properties are either boolean or u8 arrays.
> > You've correctly changed this file to always supply u8 arrays,
> > so I don't see where union aliasing is happening here?
> 
> Okay, for now I can see only Thunderbolt user of these properties (is it
> correct?) in upstream which uses u8 arrays indeed.

That is correct, thunderbolt.ko is so far the only user.


> Though the implementation is quite fragile in this sense, because it
> doesn't discourage people to use device_property_read_string() in case
> when it's indeed a string (I saw these kind of properties in the very
> dump you posted on your GH page).

Well if that is your concern then you need to prevent functions which
retrieve properties to use the wrong type.

E.g. to prevent retrieval of the u8 array as string, you'd have to
amend drivers/base/property.c:pset_prop_read_string_array() to
check the type of the property found and return -EINVAL if it's not
string.

Thanks,

Lukas

Re: [PATCH v2] device property: Get rid of union aliasing

[Andy Shevchenko]

On Mon, 2018-05-14 at 17:40 +0200, Lukas Wunner wrote:
> On Mon, May 14, 2018 at 03:48:09PM +0300, Andy Shevchenko wrote:
> > On Mon, 2018-05-14 at 14:18 +0200, Lukas Wunner wrote:
> > > On Tue, May 08, 2018 at 04:15:47PM +0300, Andy Shevchenko wrote:
> > > > --- a/drivers/firmware/efi/apple-properties.c
> > > > +++ b/drivers/firmware/efi/apple-properties.c
> > > > @@ -13,6 +13,9 @@
> > > > + * FIXME: The approach is still based on union aliasing and
> > > > should be
> > > > + * replaced by a proper resource provider.
> > > 
> > > Why?  All Apple EFI properties are either boolean or u8 arrays.
> > > You've correctly changed this file to always supply u8 arrays,
> > > so I don't see where union aliasing is happening here?
> > 
> > Okay, for now I can see only Thunderbolt user of these properties
> > (is it
> > correct?) in upstream which uses u8 arrays indeed.
> 
> That is correct, thunderbolt.ko is so far the only user.
> 
> 
> > Though the implementation is quite fragile in this sense, because it
> > doesn't discourage people to use device_property_read_string() in
> > case
> > when it's indeed a string (I saw these kind of properties in the
> > very
> > dump you posted on your GH page).
> 
> Well if that is your concern then you need to prevent functions which
> retrieve properties to use the wrong type.
> 
> E.g. to prevent retrieval of the u8 array as string, you'd have to
> amend drivers/base/property.c:pset_prop_read_string_array() to
> check the type of the property found and return -EINVAL if it's not
> string.

I think it's doable. I will hack a new version later this week.

But it still not a (best) solution for Apple properties. B/c as I told
already I saw in _your_ dump the _string_ properties. Someone might have
got an idea to use them as _strings_.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2] device property: Get rid of union aliasing

[Andy Shevchenko]

On Mon, 2018-05-14 at 17:40 +0200, Lukas Wunner wrote:
> On Mon, May 14, 2018 at 03:48:09PM +0300, Andy Shevchenko wrote:
> > On Mon, 2018-05-14 at 14:18 +0200, Lukas Wunner wrote:
> > > On Tue, May 08, 2018 at 04:15:47PM +0300, Andy Shevchenko wrote:
> > > > --- a/drivers/firmware/efi/apple-properties.c
> > > > +++ b/drivers/firmware/efi/apple-properties.c
> > > > @@ -13,6 +13,9 @@
> > > > + * FIXME: The approach is still based on union aliasing and
> > > > should be
> > > > + * replaced by a proper resource provider.
> > > 
> > > Why?  All Apple EFI properties are either boolean or u8 arrays.
> > > You've correctly changed this file to always supply u8 arrays,
> > > so I don't see where union aliasing is happening here?
> > 
> > Okay, for now I can see only Thunderbolt user of these properties
> > (is it
> > correct?) in upstream which uses u8 arrays indeed.
> 
> That is correct, thunderbolt.ko is so far the only user.
> 
> 
> > Though the implementation is quite fragile in this sense, because it
> > doesn't discourage people to use device_property_read_string() in
> > case
> > when it's indeed a string (I saw these kind of properties in the
> > very
> > dump you posted on your GH page).
> 
> Well if that is your concern then you need to prevent functions which
> retrieve properties to use the wrong type.
> 
> E.g. to prevent retrieval of the u8 array as string, you'd have to
> amend drivers/base/property.c:pset_prop_read_string_array() to
> check the type of the property found and return -EINVAL if it's not
> string.

I think it's doable. I will hack a new version later this week.

But it still not a (best) solution for Apple properties. B/c as I told
already I saw in _your_ dump the _string_ properties. Someone might have
got an idea to use them as _strings_.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel F

Re: [PATCH v2] device property: Get rid of union aliasing

[Lukas Wunner]

On Mon, May 14, 2018 at 07:13:24PM +0300, Andy Shevchenko wrote:
> On Mon, 2018-05-14 at 17:40 +0200, Lukas Wunner wrote:
> > On Mon, May 14, 2018 at 03:48:09PM +0300, Andy Shevchenko wrote:
> > > On Mon, 2018-05-14 at 14:18 +0200, Lukas Wunner wrote:
> > > > On Tue, May 08, 2018 at 04:15:47PM +0300, Andy Shevchenko wrote:
> > > > > --- a/drivers/firmware/efi/apple-properties.c
> > > > > +++ b/drivers/firmware/efi/apple-properties.c
> > > > > @@ -13,6 +13,9 @@
> > > > > + * FIXME: The approach is still based on union aliasing and
> > > > > should be
> > > > > + * replaced by a proper resource provider.
> > > > 
> > > > Why?  All Apple EFI properties are either boolean or u8 arrays.
> > > > You've correctly changed this file to always supply u8 arrays,
> > > > so I don't see where union aliasing is happening here?
> > > 
> > > the implementation is quite fragile in this sense, because it
> > > doesn't discourage people to use device_property_read_string() in
> > > case when it's indeed a string (I saw these kind of properties in
> > > the very dump you posted on your GH page).
> > 
> > Well if that is your concern then you need to prevent functions which
> > retrieve properties to use the wrong type.
> > 
> > E.g. to prevent retrieval of the u8 array as string, you'd have to
> > amend drivers/base/property.c:pset_prop_read_string_array() to
> > check the type of the property found and return -EINVAL if it's not
> > string.
> 
> I think it's doable. I will hack a new version later this week.

Ok, thanks for doing this.


> But it still not a (best) solution for Apple properties. B/c as I told
> already I saw in _your_ dump the _string_ properties. Someone might have
> got an idea to use them as _strings_.

That's kind of their problem then. ;-)  The Apple EFI properties are
untyped, so making them available as u8 arrays is the best we can do.
If people need them as strings, they should retrieve as u8 array and
convert that to string themselves.

Kind regards,

Lukas

Re: [PATCH v2] device property: Get rid of union aliasing

[Rafael J. Wysocki]

On Mon, May 14, 2018 at 6:13 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2018-05-14 at 17:40 +0200, Lukas Wunner wrote:
>> On Mon, May 14, 2018 at 03:48:09PM +0300, Andy Shevchenko wrote:
>> > On Mon, 2018-05-14 at 14:18 +0200, Lukas Wunner wrote:
>> > > On Tue, May 08, 2018 at 04:15:47PM +0300, Andy Shevchenko wrote:
>> > > > --- a/drivers/firmware/efi/apple-properties.c
>> > > > +++ b/drivers/firmware/efi/apple-properties.c
>> > > > @@ -13,6 +13,9 @@
>> > > > + * FIXME: The approach is still based on union aliasing and
>> > > > should be
>> > > > + * replaced by a proper resource provider.
>> > >
>> > > Why?  All Apple EFI properties are either boolean or u8 arrays.
>> > > You've correctly changed this file to always supply u8 arrays,
>> > > so I don't see where union aliasing is happening here?
>> >
>> > Okay, for now I can see only Thunderbolt user of these properties
>> > (is it
>> > correct?) in upstream which uses u8 arrays indeed.
>>
>> That is correct, thunderbolt.ko is so far the only user.
>>
>>
>> > Though the implementation is quite fragile in this sense, because it
>> > doesn't discourage people to use device_property_read_string() in
>> > case
>> > when it's indeed a string (I saw these kind of properties in the
>> > very
>> > dump you posted on your GH page).
>>
>> Well if that is your concern then you need to prevent functions which
>> retrieve properties to use the wrong type.
>>
>> E.g. to prevent retrieval of the u8 array as string, you'd have to
>> amend drivers/base/property.c:pset_prop_read_string_array() to
>> check the type of the property found and return -EINVAL if it's not
>> string.
>
> I think it's doable. I will hack a new version later this week.

So I'm assuming that I should disregard this patch and wait for an
update, right?

Thanks,
Rafael

Re: [PATCH v2] device property: Get rid of union aliasing

[Andy Shevchenko]

On Mon, May 14, 2018 at 11:44 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, May 14, 2018 at 6:13 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Mon, 2018-05-14 at 17:40 +0200, Lukas Wunner wrote:
>>> On Mon, May 14, 2018 at 03:48:09PM +0300, Andy Shevchenko wrote:

>>> Well if that is your concern then you need to prevent functions which
>>> retrieve properties to use the wrong type.
>>>
>>> E.g. to prevent retrieval of the u8 array as string, you'd have to
>>> amend drivers/base/property.c:pset_prop_read_string_array() to
>>> check the type of the property found and return -EINVAL if it's not
>>> string.
>>
>> I think it's doable. I will hack a new version later this week.
>
> So I'm assuming that I should disregard this patch and wait for an
> update, right?

Right.

-- 
With Best Regards,
Andy Shevchenko