Le Wed, 1 Jun 2022 15:32:29 -0700,
Tyrel Datwyler <tyreld@xxxxxxxxxxxxx> a écrit :
> > /**
> > - * __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.
kfree(prop->value) is only called if the value is not the trailing data
of the property so I don't see what is wrong there. In that case, only
kfree(prop) is called.
>
> Also, this condition will always fail. You explicitly set prop->value = prop + 1
> in alloc.
The user that did allocated the property might want to provide its own
"value". In that case, prop->value would be overwritten by the user
allocated value and thus the check would be true, hence calling
kfree(prop->value).
>
> 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);
> > +
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
