Re: [PATCH] of: of_property_read_string return -ENODATA when !length

[Frank Rowand <frowand.list@xxxxxxxxx>]

On 4/4/22 10:28, Rob Herring wrote:
> On Fri, Apr 01, 2022 at 03:43:42PM -0700, Stefano Stabellini wrote:
>> On Fri, 1 Apr 2022, Rob Herring wrote:
>>> On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
>>> <sstabellini@xxxxxxxxxx> wrote:
>>>>
>>>> On Fri, 1 Apr 2022, Rob Herring wrote:
>>>>> On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
>>>>> <sstabellini@xxxxxxxxxx> wrote:
>>>>>>
>>>>>> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>>>>>>
>>>>>> When the length of the string is zero of_property_read_string should
>>>>>> return -ENODATA according to the description of the function.
>>>>>
>>>>> Perhaps it is a difference of:
>>>>>
>>>>> prop;
>>>>>
>>>>> vs.
>>>>>
>>>>> prop = "";
>>>>>
>>>>> Both are 0 length by some definition. The description, '-ENODATA if
>>>>> property does not have a value', matches the first case.
>>>>>
>>>>>>
>>>>>> However, of_property_read_string doesn't check pp->length. If pp->length
>>>>>> is zero, return -ENODATA.
>>>>>>
>>>>>> Without this patch the following command in u-boot:
>>>>>>
>>>>>> fdt set /chosen/node property-name
>>>>>>
>>>>>> results in of_property_read_string returning -EILSEQ when attempting to
>>>>>> read property-name. With this patch, it returns -ENODATA as expected.
>>>>>
>>>>> Why do you care? Do you have a user? There could be an in tree user
>>>>> that doesn't like this change.
>>>>
>>>> During review of a Xen patch series (we have libfdt is Xen too, synced
>>>> with the kernel) Julien noticed a check for -EILSEQ. I added the check
>>>> so that Xen would behave correctly in cases like the u-boot example in
>>>> the patch description.
>>>>
>>>> Looking more into it, it seemed to be a mismatch between the description
>>>> of of_property_read_string and the behavior (e.g. -ENODATA would seem to
>>>> be the right return value, not -EILSEQ.)
>>>>
>>>> I added a printk to confirm what was going on when -EILSEQ was returned:
>>>>
>>>> printk("DEBUG %s %d value=%s value[0]=%d length=%u 
>>>> len=%lu\n",__func__,__LINE__,(char*)pp->value, 
>>>> *((char*)pp->value),pp->length,
>>>> strlen(pp->value));
>>>>
>>>> This is the output:
>>>> DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
>>>
>>> It turns out that we never set pp->value to NULL when unflattening
>>> (and libfdt always returns a value). This function is assuming we do.
>>>>
>>>> As the description says:
>>>>
>>>>  *
>>>>  * Return: 0 on success, -EINVAL if the property does not exist, -ENODATA 
>>>> if
>>>>  * property does not have a value, and -EILSEQ if the string is not
>>>>  * null-terminated within the length of the property data.
>>>>  *
>>>>
>>>> It seems that this case matches "property does not have a value" which
>>>> is expected to be -ENODATA instead of -EILSEQ. I guess one could also
>>>> say that length is zero, so the string cannot be null-terminated,
>>>> thus -EILSEQ?
>>>>
>>>> I am happy to go with your interpretation but -ENODATA seems to be the
>>>> best match in my opinion.
>>>
>>> I agree. I just think empty property should have a NULL value and 0
>>> length, but we should only have to check one. I don't want check
>>> length as that could be different for Sparc or non-FDT. So I think we
>>> need this instead:
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index ec315b060cd5..d6b2b0d49d89 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -165,7 +165,7 @@ static void populate_properties(const void *blob,
>>>
>>>                 pp->name   = (char *)pname;
>>>                 pp->length = sz;
>>> -               pp->value  = (__be32 *)val;
>>> +               pp->value  = sz ? (__be32 *)val : NULL;
>>>                 *pprev     = pp;
>>>                 pprev      = &pp->next;
>>>         }
>>>
>>>
>>> It looks like setting 'value' has been like this at least since 2010.
>>
>> Yep, fixing old bugs nobody cares about, that's me :-)
> 
> :)
> 
> 
>> I made the corresponding change to Xen to check that it fixes the
>> original issue (I am using Xen only for convenience because I already
>> have a reproducer setup for it.)
>>
>> Unfortunately it breaks the boot: the reason is that of_get_property is
>> implemented as:
>>
>>   return pp ? pp->value : NULL;
>>
>> and at least in Xen (maybe in Linux too) there are instances of callers
>> doing:
>>
>>         if (!of_get_property(node, "interrupt-controller", NULL))
>>             continue;
>>
>> This would now take the wrong code path because value is returned as
>> NULL.
>>
>> So, although your patch is technically correct, it comes with higher
>> risk of breaking existing code. How do you want to proceed?
> 
> We should just check 'length' is 0 and drop the !value part.

I agree with checking prop->length (not "pp->length" as in the original
patch because there is no "pp" in of_property_read_string()), and return
-ENODATA for that case.

I'm ok with dropping the prop->value check since we populate the field
with a non-zero value during unflattenning.

And update the function header documentation to mention that the empty
string "" has a length of 1.  Thus -ENODATA can not be interpreted as an
empty string.

-Frank

> 
> Rob