On 6/1/22 23:58, Clément Léger wrote:
> 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.
Right, Rob clarified for me in the v1 patch.
>
>>
>> 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).
So, that was the part I was missing. I think a comment would be helpful so its
clear value can be either trailing or user assigned.
-Tyrel
>
>>
>> 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);
>>> +
>
>
