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

[Rob Herring <robh@xxxxxxxxxx>]

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.

Rob