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

[Faiz Abbas]

Alexey,

On 04/09/19 6:53 PM, Alexey Brodkin wrote:
> 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/
> 

Interesting. This is news to me.

Thanks,
Faiz