Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()

[Tyrel Datwyler <tyreld@linux.ibm.com>]

On 6/1/22 01:17, Clément Léger wrote:
> Add function which allows to dynamically allocate and free properties.
> Use this function internally for all code that used the same logic
> (mainly __of_prop_dup()).
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/of/dynamic.c    | 82 ++++++++++++++++++++++++-----------------
>  drivers/of/of_private.h | 21 ++++++++++-
>  include/linux/of.h      | 14 +++++++
>  3 files changed, 82 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..c0dcbea31d28 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
> 
>  	for (prop = prop_list; prop != NULL; prop = next) {
>  		next = prop->next;
> -		kfree(prop->name);
> -		kfree(prop->value);
> -		kfree(prop);
> +		of_property_free(prop);
>  	}
>  }
> 
> @@ -367,48 +365,66 @@ void of_node_release(struct kobject *kobj)
>  }
> 
>  /**
> - * __of_prop_dup - Copy a property dynamically.
> - * @prop:	Property to copy
> + * of_property_free - Free a property allocated dynamically.
> + * @prop:	Property to be freed
> + */
> +void of_property_free(const struct property *prop)
> +{
> +	if (!of_property_check_flag(prop, OF_DYNAMIC))
> +		return;
> +

This looks wrong to me. From what I understand the value data is allocated as
trailing memory that is part of the property allocation itself. (ie. prop =
kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
of the trailing value data. Calling kfree(prop->value) is bogus since
prop->value wasn't dynamically allocated on its own.

Also, this condition will always fail. You explicitly set prop->value = prop + 1
in alloc.

Maybe I need to go back and look at v1 again.

-Tyrel

> +	if (prop->value != prop + 1)
> +		kfree(prop->value);
> +
> +	kfree(prop->name);
> +	kfree(prop);
> +}
> +EXPORT_SYMBOL(of_property_free);
> +
> +/**
> + * of_property_alloc - Allocate a property dynamically.
> + * @name:	Name of the new property
> + * @value:	Value that will be copied into the new property value or NULL
> + *		if only @len allocation is needed.
> + * @len:	Length of new property value and if @value is provided, the
> + * 		length of the value to be copied
>   * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
>   *
> - * Copy a property by dynamically allocating the memory of both the
> + * Create a property by dynamically allocating the memory of both the
>   * property structure and the property name & contents. The property's
>   * flags have the OF_DYNAMIC bit set so that we can differentiate between
>   * dynamically allocated properties and not.
>   *
>   * Return: The newly allocated property or NULL on out of memory error.
>   */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +struct property *of_property_alloc(const char *name, const void *value,
> +				   size_t len, gfp_t allocflags)
>  {
> -	struct property *new;
> +	struct property *prop;
> 
> -	new = kzalloc(sizeof(*new), allocflags);
> -	if (!new)
> +	prop = kzalloc(sizeof(*prop) + len, allocflags);
> +	if (!prop)
>  		return NULL;
> 
> -	/*
> -	 * NOTE: There is no check for zero length value.
> -	 * In case of a boolean property, this will allocate a value
> -	 * of zero bytes. We do this to work around the use
> -	 * of of_get_property() calls on boolean values.
> -	 */
> -	new->name = kstrdup(prop->name, allocflags);
> -	new->value = kmemdup(prop->value, prop->length, allocflags);
> -	new->length = prop->length;
> -	if (!new->name || !new->value)
> -		goto err_free;
> -
> -	/* mark the property as dynamic */
> -	of_property_set_flag(new, OF_DYNAMIC);
> -
> -	return new;
> -
> - err_free:
> -	kfree(new->name);
> -	kfree(new->value);
> -	kfree(new);
> +	prop->name = kstrdup(name, allocflags);
> +	if (!prop->name)
> +		goto out_err;
> +
> +	prop->value = prop + 1;
> +	if (value)
> +		memcpy(prop->value, value, len);
> +
> +	prop->length = len;
> +	of_property_set_flag(prop, OF_DYNAMIC);
> +
> +	return prop;
> +
> +out_err:
> +	of_property_free(prop);
> +
>  	return NULL;
>  }
> +EXPORT_SYMBOL(of_property_alloc);
> 
>  /**
>   * __of_node_dup() - Duplicate or create an empty device node dynamically.
> @@ -447,9 +463,7 @@ struct device_node *__of_node_dup(const struct device_node *np,
>  			if (!new_pp)
>  				goto err_prop;
>  			if (__of_add_property(node, new_pp)) {
> -				kfree(new_pp->name);
> -				kfree(new_pp->value);
> -				kfree(new_pp);
> +				of_property_free(new_pp);
>  				goto err_prop;
>  			}
>  		}
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 9324483397f6..1d6459bf705d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -115,7 +115,26 @@ extern void *__unflatten_device_tree(const void *blob,
>   * without taking node references, so you either have to
>   * own the devtree lock or work on detached trees only.
>   */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
> +
> +/**
> + * __of_prop_dup - Copy a property dynamically.
> + * @prop:	Property to copy
> + * @allocflags:	Allocation flags (typically pass GFP_KERNEL)
> + *
> + * Copy a property by dynamically allocating the memory of both the
> + * property structure and the property name & contents. The property's
> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
> + * dynamically allocated properties and not.
> + *
> + * Return: The newly allocated property or NULL on out of memory error.
> + */
> +static inline
> +struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +{
> +	return of_property_alloc(prop->name, prop->value, prop->length,
> +				 allocflags);
> +}
> +
>  struct device_node *__of_node_dup(const struct device_node *np,
>  				  const char *full_name);
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index d74fd82a6963..f1966f3c3847 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1464,6 +1464,10 @@ enum of_reconfig_change {
>  };
> 
>  #ifdef CONFIG_OF_DYNAMIC
> +struct property *of_property_alloc(const char *name, const void *value,
> +				   size_t len, gfp_t allocflags);
> +void of_property_free(const struct property *prop);
> +
>  extern int of_reconfig_notifier_register(struct notifier_block *);
>  extern int of_reconfig_notifier_unregister(struct notifier_block *);
>  extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
> @@ -1508,6 +1512,16 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>  }
>  #else /* CONFIG_OF_DYNAMIC */
> +
> +static inline
> +struct property *of_property_alloc(const char *name, const void *value,
> +				   size_t len, gfp_t allocflags)
> +{
> +	return NULL;
> +}
> +
> +static inline  void of_property_free(const struct property *prop) {}
> +
>  static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>  {
>  	return -EINVAL;