[PATCH 7/7] fs: Fix a confusing error about overlapping regions

[Tom Rini]

On Thu, Sep 07, 2023 at 06:23:06AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 6 Sept 2023 at 11:58, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Tue, Sep 05, 2023 at 12:16:56PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Sun, 3 Sept 2023 at 13:25, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Sun, Sep 03, 2023 at 12:06:13PM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Sun, 3 Sept 2023 at 10:42, Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Aug 31, 2023 at 06:15:19PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Wed, 23 Aug 2023 at 09:14, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Aug 23, 2023 at 07:42:03AM -0600, Simon Glass wrote:
> > > > > > > >
> > > > > > > > > When lmb runs out of space in its internal tables, it gives
> > > errors on
> > > > > > > > > every fs load operation. This is horribly confusing, as the
> > > poor user
> > > > > > > > > tries different memory regions one at a time.
> > > > > > > > >
> > > > > > > > > Use the updated API to check the error code and print a helpful
> > > message.
> > > > > > > > > Also, allow the operation to proceed.
> > > > > > > > >
> > > > > > > > > Update the tests accordingly.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > [snip]
> > > > > > > > > +     if (ret == -E2BIG) {
> > > > > > > > > +             log_warning("Reservation tables exhausted: see
> > > CONFIG_LMB_USE_MAX_REGIONS\n");
> > > > > > > > > +             return 0;
> > > > > > > > > +     }
> > > > > > > >
> > > > > > > > This isn't the right option.  Everyone has
> > > CONFIG_LMB_USE_MAX_REGIONS
> > > > > > > > set.  You would want to increase CONFIG_LMB_MAX_REGIONS.
> > > > > > > >
> > > > > > > > But it sounds like what you've found and fixed is an underlying
> > > problem
> > > > > > > > as to why 16 regions isn't enough in some common cases now?  So
> > > we could
> > > > > > >
> > > > > > > I don't think I have fixed anything. But I'll send v2 and perhaps it
> > > > > > > will be clearer what is going on here.
> > > > > > >
> > > > > > > > possibly avoid the string size growth here and have a comment
> > > that also
> > > > > > > > matches up with the help under LMB_MAX_REGIONS?
> > > > > > >
> > > > > > > I don't know, sorry. The size of struct(lmb) on 64-bit sandbox is
> > > > > > > about 400 bytes. There seems to be a lot of code to save not much
> > > > > > > memory.
> > > > > >
> > > > > > What do you mean here?  The alternative is not unlimited ranges but
> > > > > > instead to define the limit of memory regions and limit of reserved
> > > > > > ranges.
> > > > >
> > > > > A better alternative would be not to limit the number of regions, IMO,
> > > > > i.e. have an array of regions that can grow as needed.
> > > >
> > > > That's not something that we have in the code today, however.
> > >
> > > No, I do have an arraylist thing that I plan to upstream, which could
> > > handle that.
> > >
> > > But for this series, what do you think of the memregion idea? I am still
> > > unsure of the saming we should use - see Heinrich's comments too.
> >
> > My biggest worry here is that we're papering over a real issue, and
> > should either dig at that and see what's going on (should something be
> > coalescing adjacent allocations? Were many platforms just right at the
> > previous limit?) or just bump the default from 16 to "64 if EFI_LOADER"
> > and then see if anything else really needs tweaking / cleaning up in the
> > code itself.  I know Heinrich has previously said the LMB system could
> > be better (or something to that effect, I'm going from memory, sorry if
> > I'm mis-stating things) and I don't object to replacing what we have
> > with something more robust/smarter/etc.
> 
> I am not sure...my series was designed to rename the code to reduce
> confusion, and print a useful message when we run out of regions. It
> does not paper over the problem, but actually makes it more visible.

Well, "papering over" maybe wasn't the best choice of words on my part.
But since the series of events was something like:
- We nee to use LMB to cover my U-Boot regions to address a handful of
  CVEs over arbitrarily overwriting U-Boot at run-time.
  - AFAICT no platforms suddenly ran out of LMB reservation space, but
    maybe I'm wrong?
- Someone noted that the EFI subsystem was exposing the same kind of
  issue.
- Heinrich adds logic for EFI_LOADER (iirc) to also add allocations
  there to LMB
- Assorted platforms start changing the max allocation to 64 to fix the
  problems that get reported sometimes by booting.
- Heinrich notes that our memory reservation system (LMB) could be
  better designed than it is today.
  - And, iirc, EFI_LOADER or similar has something maybe we can
    leverage?

Which brings me to what I was trying to say.  I'm not sure it's worth
cleaning up the code a little, or refactoring/renaming things within it
without both:
- Understanding why EFI_LOADER being enabled causes us to run out of
  allocations and so if the answer is "increase the default" OR "fix
  some underlying issue such as coalescing being too strict or broken".
- Understanding if there's a better memory reservation system we can use
  instead.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230907/43fbf580/attachment.sig>