| Message ID | alpine.DEB.2.22.394.2203311740450.2910984@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | of: of_property_read_string return -ENODATA when !length | expand |
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;
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; >
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
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?
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
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
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;