[U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer"

[Alexey Brodkin]

Hi Faiz,

> -----Original Message-----
> From: Faiz Abbas <faiz_abbas at ti.com>
> Sent: Wednesday, September 4, 2019 4:09 PM
> To: Alexey Brodkin <abrodkin at synopsys.com>
> Cc: paulemge at forallsecure.com; trini at konsulko.com; u-boot at lists.denx.de
> Subject: Re: [PATCH] Revert "part: Allocate only one legacy_mbr buffer"
> 
> Hi Alexey,
> 
> On 04/09/19 6:27 PM, Alexey Brodkin wrote:
> > Hi Faiz,
> >
> > [snip]
> >
> >>>>> I guess what you really want to do is to allocate buffer for "mbr"
> >>>>> dynamically of size which is max(sizeof(legacy_mbr), dev_desc->blksz).
> >>>>>
> >>>>
> >>>> With the assumption that blksz is always greater than
> >>>> sizeof(legacy_mbr), this should work:
> >>>>
> >>>> ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, DIV_ROUND_UP(dev_desc->blksz,
> >>>> sizeof(legacy_mbr)));
> >>>
> >>> No this won't work. See ALLOC_CACHE_ALIGN_BUFFER does static allocation
> >>> in compile-time while blksz is set in runtime.
> >>>
> >>> That's why I mentioned switch to runtime allocation.
> >>>
> >>
> >> Hmm. So the problem isn't as much about allocating too much in the
> >> buffer but about using dynamically determined size for static array
> >> declaration.
> >
> > In fact it was a problem of huge static allocation I discovered just by
> > chance building U-Boot for a very memory-limited device, see [1].
> >
> >> This is being used all over this disk/part_dos.c file.
> >> Shouldn't we fix all of that? Not sure how it has been working all along.
> >
> > That part I don't quite understand. What's being used, where and how?
> > And what's the problem with dynamic allocation of the buffer for MBR?
> >
> 
> Isn't the following line (being used in different functions in
> disk/part_dos.c) also problematic because we don't know the value of
> blksz at compile time?
> 
> ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);

Ok I see now. Looks like I missed that Variable-Length Array (VLA) feature
added to C99 so up-to-date GCC apparently supports it.

But I would strongly recommend to get rid of VLA usage by all means,
see [1] & [2].

[1] https://lwn.net/Articles/749064/
[2] https://lwn.net/Articles/749089/

-Alexey