AW: [PATCH] spl: fix error handling of spl_fit_read

[mailinglist1 at johanneskirchmair.de]

Hey,

> -----Ursprüngliche Nachricht-----
> Von: Simon Glass <sjg at chromium.org>
> Gesendet: Samstag, 17. August 2024 17:58
> An: mailinglist1 at johanneskirchmair.de; Sean Anderson
> <seanga2 at gmail.com>
> Cc: u-boot at lists.denx.de; trini at konsulko.com; Johannes Kirchmair
> <johannes.kirchmair at 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 at johanneskirchmair.de> wrote:
> >
> > From: Johannes Kirchmair <johannes.kirchmair at 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 at 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/