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

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

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

When the length of the string is zero of_property_read_string should
return -ENODATA according to the description of the function.

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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 8e90071de6ed..da0f02c98bb2 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -439,7 +439,7 @@ int of_property_read_string(const struct device_node *np, const char *propname,
 	const struct property *prop = of_find_property(np, propname, NULL);
 	if (!prop)
 		return -EINVAL;
-	if (!prop->value)
+	if (!prop->value || !pp->length)
 		return -ENODATA;
 	if (strnlen(prop->value, prop->length) >= prop->length)
 		return -EILSEQ;

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

[Rob Herring]

On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
<sstabellini@kernel.org> wrote:
>
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> 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.

>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 8e90071de6ed..da0f02c98bb2 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -439,7 +439,7 @@ int of_property_read_string(const struct device_node *np, const char *propname,
>         const struct property *prop = of_find_property(np, propname, NULL);
>         if (!prop)
>                 return -EINVAL;
> -       if (!prop->value)
> +       if (!prop->value || !pp->length)
>                 return -ENODATA;
>         if (strnlen(prop->value, prop->length) >= prop->length)
>                 return -EILSEQ;

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

[Stefano Stabellini]

On Fri, 1 Apr 2022, Rob Herring wrote:
> On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> <sstabellini@kernel.org> wrote:
> >
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >
> > 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

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.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 8e90071de6ed..da0f02c98bb2 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -439,7 +439,7 @@ int of_property_read_string(const struct device_node *np, const char *propname,
> >         const struct property *prop = of_find_property(np, propname, NULL);
> >         if (!prop)
> >                 return -EINVAL;
> > -       if (!prop->value)
> > +       if (!prop->value || !pp->length)
> >                 return -ENODATA;
> >         if (strnlen(prop->value, prop->length) >= prop->length)
> >                 return -EILSEQ;
> 

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

[Rob Herring]

On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
<sstabellini@kernel.org> wrote:
>
> On Fri, 1 Apr 2022, Rob Herring wrote:
> > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > <sstabellini@kernel.org> wrote:
> > >
> > > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > >
> > > 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.

Rob

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

[Stefano Stabellini]

On Fri, 1 Apr 2022, Rob Herring wrote:
> On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
> <sstabellini@kernel.org> wrote:
> >
> > On Fri, 1 Apr 2022, Rob Herring wrote:
> > > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > > <sstabellini@kernel.org> wrote:
> > > >
> > > > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > >
> > > > 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?

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

[Rob Herring]

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@kernel.org> wrote:
> > >
> > > On Fri, 1 Apr 2022, Rob Herring wrote:
> > > > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > > > <sstabellini@kernel.org> wrote:
> > > > >
> > > > > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > > >
> > > > > 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.

Rob

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

[Frank Rowand]

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@kernel.org> wrote:
>>>>
>>>> On Fri, 1 Apr 2022, Rob Herring wrote:
>>>>> On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
>>>>> <sstabellini@kernel.org> wrote:
>>>>>>
>>>>>> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>>>>
>>>>>> 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