| Message ID | 20240816073332.279025-1-mailinglist1@johanneskirchmair.de |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | spl: fix error handling of spl_fit_read | expand |
+Sean Anderson too, for this Hi, On Fri, 16 Aug 2024 at 07:36, <mailinglist1@johanneskirchmair.de> wrote: > > From: Johannes Kirchmair <johannes.kirchmair@skidata.com> > > Returning negative values from spl_fit_read leads to u-boot crashing. > The return value of spl_fit_read is compared with an unsigned value. > Returning negative values leads to the check not detecting the error. > Not detecting the error leads to crashing. > > Returning zero in case of an reading error is fine. It indicates that > nothing was red. read > > Signed-off-by: Johannes Kirchmair <johannes.kirchmair@skidata.com> > --- > common/spl/spl_fat.c | 2 +- > include/spl_load.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > Perhaps instead this should be fixed in _spl_load()? We don't want to ignore errors, and returning -EIO is not as good as returning the actual error received. The docs of struct spl_load_info indicate that functions should return 0 on error, but that is somewhat surprising behaviour, which is probably why people are not following it? > diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c > index bd8aab253a..345bc55149 100644 > --- a/common/spl/spl_fat.c > +++ b/common/spl/spl_fat.c > @@ -53,7 +53,7 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset, > > ret = fat_read_file(filename, buf, file_offset, size, &actread); > if (ret) > - return ret; > + return 0; > > return actread; > } > diff --git a/include/spl_load.h b/include/spl_load.h > index 1c2b296c0a..b411d9daa1 100644 > --- a/include/spl_load.h > +++ b/include/spl_load.h > @@ -17,8 +17,8 @@ static inline int _spl_load(struct spl_image_info *spl_image, > { > struct legacy_img_hdr *header = > spl_get_load_buffer(-sizeof(*header), sizeof(*header)); > - ulong base_offset, image_offset, overhead; > - int read, ret; > + ulong base_offset, image_offset, overhead, read; > + int ret; > > read = info->read(info, offset, ALIGN(sizeof(*header), > spl_get_bl_len(info)), header); > -- > 2.34.1 > Regards, Simon
On 8/16/24 03:33, mailinglist1@johanneskirchmair.de wrote: > From: Johannes Kirchmair <johannes.kirchmair@skidata.com> > > Returning negative values from spl_fit_read leads to u-boot crashing. > The return value of spl_fit_read is compared with an unsigned value. > Returning negative values leads to the check not detecting the error. > Not detecting the error leads to crashing. > > Returning zero in case of an reading error is fine. It indicates that > nothing was red. > > Signed-off-by: Johannes Kirchmair <johannes.kirchmair@skidata.com> > --- > common/spl/spl_fat.c | 2 +- > include/spl_load.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c > index bd8aab253a..345bc55149 100644 > --- a/common/spl/spl_fat.c > +++ b/common/spl/spl_fat.c > @@ -53,7 +53,7 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset, > > ret = fat_read_file(filename, buf, file_offset, size, &actread); > if (ret) > - return ret; > + return 0; > > return actread; > } > diff --git a/include/spl_load.h b/include/spl_load.h > index 1c2b296c0a..b411d9daa1 100644 > --- a/include/spl_load.h > +++ b/include/spl_load.h > @@ -17,8 +17,8 @@ static inline int _spl_load(struct spl_image_info *spl_image, > { > struct legacy_img_hdr *header = > spl_get_load_buffer(-sizeof(*header), sizeof(*header)); > - ulong base_offset, image_offset, overhead; > - int read, ret; > + ulong base_offset, image_offset, overhead, read; > + int ret; > > read = info->read(info, offset, ALIGN(sizeof(*header), > spl_get_bl_len(info)), header); Does [1] fix your problem? --Sean [1] https://lore.kernel.org/u-boot/20240731130913.487121-1-fventuri@comcast.net/
Hey, > -----Ursprüngliche Nachricht----- > Von: Simon Glass <sjg@chromium.org> > Gesendet: Samstag, 17. August 2024 17:58 > An: mailinglist1@johanneskirchmair.de; Sean Anderson > <seanga2@gmail.com> > Cc: u-boot@lists.denx.de; trini@konsulko.com; Johannes Kirchmair > <johannes.kirchmair@skidata.com> > Betreff: Re: [PATCH] spl: fix error handling of spl_fit_read > > +Sean Anderson too, for this > > Hi, > > On Fri, 16 Aug 2024 at 07:36, <mailinglist1@johanneskirchmair.de> wrote: > > > > From: Johannes Kirchmair <johannes.kirchmair@skidata.com> > > > > Returning negative values from spl_fit_read leads to u-boot crashing. > > The return value of spl_fit_read is compared with an unsigned value. > > Returning negative values leads to the check not detecting the error. > > Not detecting the error leads to crashing. > > > > Returning zero in case of an reading error is fine. It indicates that > > nothing was red. > > read > > > > > Signed-off-by: Johannes Kirchmair <johannes.kirchmair@skidata.com> > > --- > > common/spl/spl_fat.c | 2 +- > > include/spl_load.h | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > Perhaps instead this should be fixed in _spl_load()? We don't want to ignore > errors, and returning -EIO is not as good as returning the actual error received. > The docs of struct spl_load_info indicate that functions should return 0 on > error, but that is somewhat surprising behaviour, which is probably why > people are not following it? Could also be a valid way of fixing, as Sean did in his patch [1] Too have less confusion in the future, we should also switch the signature from returning ulong to long and allow negative return values for errors. > > > diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c index > > bd8aab253a..345bc55149 100644 > > --- a/common/spl/spl_fat.c > > +++ b/common/spl/spl_fat.c > > @@ -53,7 +53,7 @@ static ulong spl_fit_read(struct spl_load_info > > *load, ulong file_offset, > > > > ret = fat_read_file(filename, buf, file_offset, size, &actread); > > if (ret) > > - return ret; > > + return 0; > > > > return actread; > > } > > diff --git a/include/spl_load.h b/include/spl_load.h index > > 1c2b296c0a..b411d9daa1 100644 > > --- a/include/spl_load.h > > +++ b/include/spl_load.h > > @@ -17,8 +17,8 @@ static inline int _spl_load(struct spl_image_info > > *spl_image, { > > struct legacy_img_hdr *header = > > spl_get_load_buffer(-sizeof(*header), sizeof(*header)); > > - ulong base_offset, image_offset, overhead; > > - int read, ret; > > + ulong base_offset, image_offset, overhead, read; > > + int ret; > > > > read = info->read(info, offset, ALIGN(sizeof(*header), > > spl_get_bl_len(info)), > > header); > > -- > > 2.34.1 > > > > Regards, > Simon Best regards, Johannes [1] https://lore.kernel.org/u-boot/20240731130913.487121-1-fventuri@comcast.net/
diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c index bd8aab253a..345bc55149 100644 --- a/common/spl/spl_fat.c +++ b/common/spl/spl_fat.c @@ -53,7 +53,7 @@ static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset, ret = fat_read_file(filename, buf, file_offset, size, &actread); if (ret) - return ret; + return 0; return actread; } diff --git a/include/spl_load.h b/include/spl_load.h index 1c2b296c0a..b411d9daa1 100644 --- a/include/spl_load.h +++ b/include/spl_load.h @@ -17,8 +17,8 @@ static inline int _spl_load(struct spl_image_info *spl_image, { struct legacy_img_hdr *header = spl_get_load_buffer(-sizeof(*header), sizeof(*header)); - ulong base_offset, image_offset, overhead; - int read, ret; + ulong base_offset, image_offset, overhead, read; + int ret; read = info->read(info, offset, ALIGN(sizeof(*header), spl_get_bl_len(info)), header);