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

[Faiz Abbas]

Hi Tom, Alexey,

On 04/09/19 7:19 PM, Tom Rini wrote:
> On Wed, Sep 04, 2019 at 01:46:52PM +0000, Alexey Brodkin wrote:
>> Hi Faiz,
>>
>>> -----Original Message-----
>>> From: Alexey Brodkin
>>> Sent: Wednesday, September 4, 2019 4:23 PM
>>> To: Faiz Abbas <faiz_abbas at ti.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 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/
>>
>> So if I add "-Wvla" to CFLAGS I see 22 usages of them:
>> -------------------------------->8---------------------------------
>> diff --git a/Makefile b/Makefile
>> index c02accfc26..c6e8d12809 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -389,7 +389,7 @@ CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
>>
>>  KBUILD_CPPFLAGS := -D__KERNEL__ -D__UBOOT__
>>
>> -KBUILD_CFLAGS   := -Wall -Wstrict-prototypes \
>> +KBUILD_CFLAGS   := -Wvla -Wall -Wstrict-prototypes \
>>                    -Wno-format-security \
>>                    -fno-builtin -ffreestanding $(CSTD_FLAG)
>>  KBUILD_CFLAGS  += -fshort-wchar -fno-strict-aliasing
>> -------------------------------->8---------------------------------
>>
>> So that's what we have:
>> -------------------------------->8---------------------------------
>> # make -j 48 2>&1 | grep "\-Wvla"
>> disk/part_dos.c:129:2: warning: ISO C90 forbids variable length array ‘__buffer’ [-Wvla]
>> disk/part_dos.c:200:2: warning: ISO C90 forbids variable length array ‘__buffer’ [-Wvla]
>> drivers/spi/spi-mem.c:350:2: warning: ISO C90 forbids variable length array ‘op_buf’ [-Wvla]
>> drivers/input/input.c:341:2: warning: ISO C90 forbids variable length array ‘temp’ [-Wvla]
>> drivers/input/input.c:513:2: warning: ISO C90 forbids variable length array ‘ch’ [-Wvla]
>> env/attr.c:124:2: warning: ISO C90 forbids variable length array ‘regex’ [-Wvla]
>> env/attr.c:129:10: warning: ISO C90 forbids variable length array ‘caps’ [-Wvla]
>> common/command.c:29:3: warning: ISO C90 forbids variable length array ‘cmd_array’ [-Wvla]
>> drivers/mmc/dw_mmc.c:248:34: warning: ISO C90 forbids variable length array ‘__cur_idmac’ [-Wvla]
>> fs/fat/fat.c:61:2: warning: ISO C90 forbids variable length array ‘__buffer’ [-Wvla]
>> fs/fat/fat.c:262:3: warning: ISO C90 forbids variable length array ‘__tmpbuf’ [-Wvla]
>> fs/fat/fat.c:290:3: warning: ISO C90 forbids variable length array ‘__tmpbuf’ [-Wvla]
>> fs/fat/fat_write.c:410:3: warning: ISO C90 forbids variable length array ‘__tmpbuf’ [-Wvla]
>> fs/fat/fat_write.c:439:3: warning: ISO C90 forbids variable length array ‘__tmpbuf’ [-Wvla]
>> fs/ext4/ext4_common.c:200:2: warning: ISO C90 forbids variable length array ‘__sec_buf’ [-Wvla]
>> fs/fs_internal.c:18:2: warning: ISO C90 forbids variable length array ‘__sec_buf’ [-Wvla]
>> fs/fs_internal.c:62:3: warning: ISO C90 forbids variable length array ‘__p’ [-Wvla]
>> fs/ext4/ext4_common.c:2075:4: warning: ISO C90 forbids variable length array ‘filename’ [-Wvla]
>> fs/ext4/ext4_common.c:2213:2: warning: ISO C90 forbids variable length array ‘fpath’ [-Wvla]
>> lib/fdtdec.c:395:2: warning: ISO C90 forbids variable length array ‘nodes’ [-Wvla]
>> lib/hashtable.c:601:9: warning: ISO C90 forbids variable length array ‘list’ [-Wvla]
>> lib/hashtable.c:792:2: warning: ISO C90 forbids variable length array ‘localvars’ [-Wvla]
>> -------------------------------->8---------------------------------
>>
>> Given that situation I think it should be fine for starters to implement
>> a fix proposed by you and later work on VLA removal as a separate project.
> 
> Agreed, thanks guys!
> 

Ok. Will post that fix.

Thanks,
Faiz