| Message ID | 20160920171712.15528-1-paul.burton@imgtec.com |
|---|---|
| State | Accepted |
| Commit | bd86ef117d983a9f89cb1125cc93e63d07f03e02 |
| Delegated to: | Tom Rini |
| Headers | show |
On 09/20/2016 07:17 PM, Paul Burton wrote: > Commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT") changed > fit_get_node_from_config to return -ENOENT when a property doesn't > exist, but didn't change any of its callers which check return values. > Notably it didn't change boot_get_ramdisk, which leads to U-Boot failing > to boot FIT images which don't include ramdisks with the following > message: > > Ramdisk image is corrupt or invalid > > It also didn't take into account that by returning -ENOENT to denote the > lack of a property we lost the ability to determine from the return > value of fit_get_node_from_config whether it was the property or the > configuration node that was missing, which may potentially lead callers > to accept invalid FIT images. > > Fix this by having fit_get_node_from_config return -EINVAL when the > configuration node isn't found and -ENOENT when the property isn't > found, which seems to make semantic sense. Callers that previously > checked for -ENOLINK are adjusted to check for -ENOENT, which fixes the > breakage introduced by commit bac17b78dace ("image-fit: switch ENOLINK > to ENOENT"). > > The only other user of the return fit_get_node_from_config return value, > indirectly, is bootm_find_os which already checked for -ENOENT. From a > read-through of the code I suspect it ought to have been checking for > -ENOLINK prior to bac17b78dace ("image-fit: switch ENOLINK to ENOENT") > anyway, which would make it right after this patch, but this would be > good to get verified by someone who knows this x86 code or is able to > test it. > > Signed-off-by: Paul Burton <paul.burton@imgtec.com> > Cc: Jonathan Gray <jsg@jsg.id.au> > Cc: Marek Vasut <marex@denx.de> Acked-by: Marek Vasut <marex@denx.de> > > --- > > common/image-fdt.c | 2 +- > common/image-fit.c | 2 +- > common/image.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/common/image-fdt.c b/common/image-fdt.c > index d6ee225..3d23608 100644 > --- a/common/image-fdt.c > +++ b/common/image-fdt.c > @@ -285,7 +285,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, > fdt_noffset = fit_get_node_from_config(images, > FIT_FDT_PROP, > fdt_addr); > - if (fdt_noffset == -ENOLINK) > + if (fdt_noffset == -ENOENT) > return 0; > else if (fdt_noffset < 0) > return 1; > diff --git a/common/image-fit.c b/common/image-fit.c > index 9ce68f1..1b0234a 100644 > --- a/common/image-fit.c > +++ b/common/image-fit.c > @@ -1560,7 +1560,7 @@ int fit_get_node_from_config(bootm_headers_t *images, const char *prop_name, > cfg_noffset = fit_conf_get_node(fit_hdr, images->fit_uname_cfg); > if (cfg_noffset < 0) { > debug("* %s: no such config\n", prop_name); > - return -ENOENT; > + return -EINVAL; > } > > noffset = fit_conf_get_prop_node(fit_hdr, cfg_noffset, prop_name); > diff --git a/common/image.c b/common/image.c > index 7ad04ca..c8d9bc8 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1078,7 +1078,7 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, > rd_addr = map_to_sysmem(images->fit_hdr_os); > rd_noffset = fit_get_node_from_config(images, > FIT_RAMDISK_PROP, rd_addr); > - if (rd_noffset == -ENOLINK) > + if (rd_noffset == -ENOENT) > return 0; > else if (rd_noffset < 0) > return 1; >
On 21.09.2016 17:12, Marek Vasut wrote: > On 09/20/2016 07:17 PM, Paul Burton wrote: >> Commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT") changed >> fit_get_node_from_config to return -ENOENT when a property doesn't >> exist, but didn't change any of its callers which check return values. >> Notably it didn't change boot_get_ramdisk, which leads to U-Boot failing >> to boot FIT images which don't include ramdisks with the following >> message: >> >> Ramdisk image is corrupt or invalid >> >> It also didn't take into account that by returning -ENOENT to denote the >> lack of a property we lost the ability to determine from the return >> value of fit_get_node_from_config whether it was the property or the >> configuration node that was missing, which may potentially lead callers >> to accept invalid FIT images. >> >> Fix this by having fit_get_node_from_config return -EINVAL when the >> configuration node isn't found and -ENOENT when the property isn't >> found, which seems to make semantic sense. Callers that previously >> checked for -ENOLINK are adjusted to check for -ENOENT, which fixes the >> breakage introduced by commit bac17b78dace ("image-fit: switch ENOLINK >> to ENOENT"). >> >> The only other user of the return fit_get_node_from_config return value, >> indirectly, is bootm_find_os which already checked for -ENOENT. From a >> read-through of the code I suspect it ought to have been checking for >> -ENOLINK prior to bac17b78dace ("image-fit: switch ENOLINK to ENOENT") >> anyway, which would make it right after this patch, but this would be >> good to get verified by someone who knows this x86 code or is able to >> test it. >> >> Signed-off-by: Paul Burton <paul.burton@imgtec.com> >> Cc: Jonathan Gray <jsg@jsg.id.au> >> Cc: Marek Vasut <marex@denx.de> > > Acked-by: Marek Vasut <marex@denx.de> Acked-by: Stefan Roese <sr@denx.de> Thanks, Stefan
On Wed, Sep 21, 2016 at 10:15 AM, Stefan Roese <sr@denx.de> wrote: > On 21.09.2016 17:12, Marek Vasut wrote: >> >> On 09/20/2016 07:17 PM, Paul Burton wrote: >>> >>> Commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT") changed >>> fit_get_node_from_config to return -ENOENT when a property doesn't >>> exist, but didn't change any of its callers which check return values. >>> Notably it didn't change boot_get_ramdisk, which leads to U-Boot failing >>> to boot FIT images which don't include ramdisks with the following >>> message: >>> >>> Ramdisk image is corrupt or invalid >>> >>> It also didn't take into account that by returning -ENOENT to denote the >>> lack of a property we lost the ability to determine from the return >>> value of fit_get_node_from_config whether it was the property or the >>> configuration node that was missing, which may potentially lead callers >>> to accept invalid FIT images. >>> >>> Fix this by having fit_get_node_from_config return -EINVAL when the >>> configuration node isn't found and -ENOENT when the property isn't >>> found, which seems to make semantic sense. Callers that previously >>> checked for -ENOLINK are adjusted to check for -ENOENT, which fixes the >>> breakage introduced by commit bac17b78dace ("image-fit: switch ENOLINK >>> to ENOENT"). >>> >>> The only other user of the return fit_get_node_from_config return value, >>> indirectly, is bootm_find_os which already checked for -ENOENT. From a >>> read-through of the code I suspect it ought to have been checking for >>> -ENOLINK prior to bac17b78dace ("image-fit: switch ENOLINK to ENOENT") >>> anyway, which would make it right after this patch, but this would be >>> good to get verified by someone who knows this x86 code or is able to >>> test it. >>> >>> Signed-off-by: Paul Burton <paul.burton@imgtec.com> >>> Cc: Jonathan Gray <jsg@jsg.id.au> >>> Cc: Marek Vasut <marex@denx.de> >> >> >> Acked-by: Marek Vasut <marex@denx.de> > > > Acked-by: Stefan Roese <sr@denx.de> Acked-by: George McCollister <george.mccollister@gmail.com> Tested-by: George McCollister <george.mccollister@gmail.com>
On Tue, Sep 20, 2016 at 06:17:12PM +0100, Paul Burton wrote: > Commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT") changed > fit_get_node_from_config to return -ENOENT when a property doesn't > exist, but didn't change any of its callers which check return values. > Notably it didn't change boot_get_ramdisk, which leads to U-Boot failing > to boot FIT images which don't include ramdisks with the following > message: > > Ramdisk image is corrupt or invalid > > It also didn't take into account that by returning -ENOENT to denote the > lack of a property we lost the ability to determine from the return > value of fit_get_node_from_config whether it was the property or the > configuration node that was missing, which may potentially lead callers > to accept invalid FIT images. > > Fix this by having fit_get_node_from_config return -EINVAL when the > configuration node isn't found and -ENOENT when the property isn't > found, which seems to make semantic sense. Callers that previously > checked for -ENOLINK are adjusted to check for -ENOENT, which fixes the > breakage introduced by commit bac17b78dace ("image-fit: switch ENOLINK > to ENOENT"). > > The only other user of the return fit_get_node_from_config return value, > indirectly, is bootm_find_os which already checked for -ENOENT. From a > read-through of the code I suspect it ought to have been checking for > -ENOLINK prior to bac17b78dace ("image-fit: switch ENOLINK to ENOENT") > anyway, which would make it right after this patch, but this would be > good to get verified by someone who knows this x86 code or is able to > test it. > > Signed-off-by: Paul Burton <paul.burton@imgtec.com> > Cc: Jonathan Gray <jsg@jsg.id.au> > Cc: Marek Vasut <marex@denx.de> > Acked-by: Marek Vasut <marex@denx.de> > Acked-by: Stefan Roese <sr@denx.de> > Acked-by: George McCollister <george.mccollister@gmail.com> > Tested-by: George McCollister <george.mccollister@gmail.com> A few days ago now, applied to u-boot/master, thanks!
diff --git a/common/image-fdt.c b/common/image-fdt.c index d6ee225..3d23608 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -285,7 +285,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, fdt_noffset = fit_get_node_from_config(images, FIT_FDT_PROP, fdt_addr); - if (fdt_noffset == -ENOLINK) + if (fdt_noffset == -ENOENT) return 0; else if (fdt_noffset < 0) return 1; diff --git a/common/image-fit.c b/common/image-fit.c index 9ce68f1..1b0234a 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1560,7 +1560,7 @@ int fit_get_node_from_config(bootm_headers_t *images, const char *prop_name, cfg_noffset = fit_conf_get_node(fit_hdr, images->fit_uname_cfg); if (cfg_noffset < 0) { debug("* %s: no such config\n", prop_name); - return -ENOENT; + return -EINVAL; } noffset = fit_conf_get_prop_node(fit_hdr, cfg_noffset, prop_name); diff --git a/common/image.c b/common/image.c index 7ad04ca..c8d9bc8 100644 --- a/common/image.c +++ b/common/image.c @@ -1078,7 +1078,7 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, rd_addr = map_to_sysmem(images->fit_hdr_os); rd_noffset = fit_get_node_from_config(images, FIT_RAMDISK_PROP, rd_addr); - if (rd_noffset == -ENOLINK) + if (rd_noffset == -ENOENT) return 0; else if (rd_noffset < 0) return 1;
Commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT") changed fit_get_node_from_config to return -ENOENT when a property doesn't exist, but didn't change any of its callers which check return values. Notably it didn't change boot_get_ramdisk, which leads to U-Boot failing to boot FIT images which don't include ramdisks with the following message: Ramdisk image is corrupt or invalid It also didn't take into account that by returning -ENOENT to denote the lack of a property we lost the ability to determine from the return value of fit_get_node_from_config whether it was the property or the configuration node that was missing, which may potentially lead callers to accept invalid FIT images. Fix this by having fit_get_node_from_config return -EINVAL when the configuration node isn't found and -ENOENT when the property isn't found, which seems to make semantic sense. Callers that previously checked for -ENOLINK are adjusted to check for -ENOENT, which fixes the breakage introduced by commit bac17b78dace ("image-fit: switch ENOLINK to ENOENT"). The only other user of the return fit_get_node_from_config return value, indirectly, is bootm_find_os which already checked for -ENOENT. From a read-through of the code I suspect it ought to have been checking for -ENOLINK prior to bac17b78dace ("image-fit: switch ENOLINK to ENOENT") anyway, which would make it right after this patch, but this would be good to get verified by someone who knows this x86 code or is able to test it. Signed-off-by: Paul Burton <paul.burton@imgtec.com> Cc: Jonathan Gray <jsg@jsg.id.au> Cc: Marek Vasut <marex@denx.de> --- common/image-fdt.c | 2 +- common/image-fit.c | 2 +- common/image.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)