Re: [PATCH v2 4/4] powerpc/pseries: use of_property_alloc/free() and of_node_alloc()

[Clément Léger]

Le Fri, 3 Jun 2022 15:14:07 -0500,
Rob Herring <robh@xxxxxxxxxx> a écrit :

> >  static struct device_node *dlpar_parse_cc_node(struct cc_workarea *ccwa)
> >  {
> > -	struct device_node *dn;
> >  	const char *name;
> >  
> > -	dn = kzalloc(sizeof(*dn), GFP_KERNEL);
> > -	if (!dn)
> > -		return NULL;
> > -
> >  	name = (const char *)ccwa + be32_to_cpu(ccwa->name_offset);
> > -	dn->full_name = kstrdup(name, GFP_KERNEL);
> > -	if (!dn->full_name) {
> > -		kfree(dn);
> > -		return NULL;
> > -	}
> >  
> > -	of_node_set_flag(dn, OF_DYNAMIC);
> > -	of_node_init(dn);
> > -
> > -	return dn;
> > +	return of_node_alloc(name, GFP_KERNEL);  
> 
> Do you have any need for different flags? I can't really see a need for 
> atomic or dma allocs or ???, so drop it I think.

Hum no, i copied this behavior from an existing function. I'll remove
that.

> 
> >  }
> >  
> >  static void dlpar_free_one_cc_node(struct device_node *dn)
> > @@ -102,11 +66,10 @@ static void dlpar_free_one_cc_node(struct device_node *dn)
> >  	while (dn->properties) {
> >  		prop = dn->properties;
> >  		dn->properties = prop->next;
> > -		dlpar_free_cc_property(prop);
> > +		of_property_free(prop);  
> 
> We should be able to just put the node and all the properties already 
> attached will be freed.

Indeed !

> 
> Looking at the history of this code, it originally did the kref_init 
> much later in dlpar_attach_node(). So there was a window of allocating 
> the node and adding properties where you'd need to manually free 
> everything. Now that the node is referenced from the start, a put should 
> free everything.
> 
> > @@ -91,9 +82,7 @@ static void release_prop_list(const struct property *prop)
> >  	struct property *next;
> >  	for (; prop; prop = next) {
> >  		next = prop->next;
> > -		kfree(prop->name);
> > -		kfree(prop->value);
> > -		kfree(prop);
> > +		of_property_free(prop);  
> 
> Looks like you need this because code does: alloc properties, alloc 
> node, add properties, attach node. It would need to be refactored to 
> alloc the node first, but that's a bit more complex needing someone to 
> test on pSeries.

Acked.

> 
> >  	}
> >  
> >  }
> > @@ -167,27 +156,17 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
> >  static struct property *new_property(const char *name, const int length,
> >  				     const unsigned char *value, struct property *last)
> >  {
> > -	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
> > +	struct property *prop;
> >  
> > -	if (!new)
> > +	prop = of_property_alloc(name, NULL, length + 1, GFP_KERNEL);
> > +	if (!prop)
> >  		return NULL;
> >  
> > -	if (!(new->name = kstrdup(name, GFP_KERNEL)))
> > -		goto cleanup;
> > -	if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
> > -		goto cleanup;
> > -
> > -	memcpy(new->value, value, length);
> > -	*(((char *)new->value) + length) = 0;
> > -	new->length = length;
> > -	new->next = last;
> > -	return new;
> > -
> > -cleanup:
> > -	kfree(new->name);
> > -	kfree(new->value);
> > -	kfree(new);
> > -	return NULL;
> > +	memcpy(prop->value, value, length);
> > +	*(((char *)prop->value) + length) = 0;  
> 
> Looks to me like this could be avoided with this change:
> 
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index cad7a0c93117..614753fc5f27 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -148,7 +148,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
>         /* now we're on the value */
>         *value = tmp;
>         tmp += *length;
> -       if (tmp > end) {
> +       if (tmp >= end) {
>                 printk(KERN_ERR "property parse failed in %s at line %d\n",
>                        __func__, __LINE__);
>                 return NULL;
> @@ -158,6 +158,7 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
>                        __func__, __LINE__);
>                 return NULL;
>         }
> +       *tmp = '\0';
>         tmp++;
>  
>         /* and now we should be on the next name, or the end */
> 
> 
> Based on the comments, 'buf' should be nul terminated, so I would think 
> that tmp == end would be an error. But I really don't know.
> 
> Really need some pSeries people to comment on all this.
> 
> Another option is if value is NULL, then of_property_alloc() should 
> ensure the buffer is zeroed. Then you just need the memcpy.

Probably looks like a safe behavior anyway to zero the value buffer.
I'll add that.

Thanks,

Clément

> 
> Rob



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com