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@ti.com Cc: paulemge@forallsecure.com; trini@konsulko.com; u-boot@lists.denx.de Subject: RE: [PATCH] Revert "part: Allocate only one legacy_mbr buffer"
Hi Faiz,
-----Original Message----- From: Faiz Abbas faiz_abbas@ti.com Sent: Wednesday, September 4, 2019 4:09 PM To: Alexey Brodkin abrodkin@synopsys.com Cc: paulemge@forallsecure.com; trini@konsulko.com; u-boot@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!