From mboxrd@z Thu Jan 1 00:00:00 1970 From: Faiz Abbas Date: Wed, 4 Sep 2019 19:37:35 +0530 Subject: [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" In-Reply-To: <20190904134931.GO26850@bill-the-cat> References: <20190904092219.6045-1-faiz_abbas@ti.com> <554e1bc3-f6a1-a0cc-c724-16d00b249b70@ti.com> <51d70f01-0fb8-ef8a-3ca8-3621fcceb23e@ti.com> <20190904134931.GO26850@bill-the-cat> Message-ID: <3b0b67dd-a43d-ff0f-7984-2fd09d91ee14@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>> 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 >>>> Sent: Wednesday, September 4, 2019 4:09 PM >>>> To: Alexey Brodkin >>>> 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