* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" @ 2019-09-04 9:22 Faiz Abbas 2019-09-04 10:12 ` Alexey Brodkin 0 siblings, 1 reply; 12+ messages in thread From: Faiz Abbas @ 2019-09-04 9:22 UTC (permalink / raw) To: u-boot This reverts commit 8639e34d2c5e12cc2e45c95b1a2e97c22bf6a711. The blk_dread() call following the allocation will read one block from the device. This will lead to overflow if the blocksize is greater than the size of legacy_mbr. Fix this by allocating one block size. Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> --- disk/part_dos.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/disk/part_dos.c b/disk/part_dos.c index aae9d95906..24d23ad247 100644 --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -93,7 +93,7 @@ static int test_block_type(unsigned char *buffer) static int part_test_dos(struct blk_desc *dev_desc) { #ifndef CONFIG_SPL_BUILD - ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, 1); + ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz); if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) return -1; -- 2.19.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 9:22 [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" Faiz Abbas @ 2019-09-04 10:12 ` Alexey Brodkin 2019-09-04 11:43 ` Faiz Abbas 0 siblings, 1 reply; 12+ messages in thread From: Alexey Brodkin @ 2019-09-04 10:12 UTC (permalink / raw) To: u-boot Hi Faiz, > -----Original Message----- > From: Faiz Abbas <faiz_abbas@ti.com> > Sent: Wednesday, September 4, 2019 12:22 PM > To: u-boot at lists.denx.de > Cc: paulemge at forallsecure.com; faiz_abbas at ti.com; Alexey Brodkin <abrodkin@synopsys.com>; > trini at konsulko.com > Subject: [PATCH] Revert "part: Allocate only one legacy_mbr buffer" > > This reverts commit 8639e34d2c5e12cc2e45c95b1a2e97c22bf6a711. > > The blk_dread() call following the allocation will read one block from > the device. This will lead to overflow if the blocksize is greater than > the size of legacy_mbr. Fix this by allocating one block size. Did you read justification of my change that you're reverting now? With your change in place we'll allocate a buffer of size = (sizeof(legacy_mbr) * dev_desc->blksz). Is that something you really want? 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). -Alexey ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 10:12 ` Alexey Brodkin @ 2019-09-04 11:43 ` Faiz Abbas 2019-09-04 11:46 ` Alexey Brodkin 0 siblings, 1 reply; 12+ messages in thread From: Faiz Abbas @ 2019-09-04 11:43 UTC (permalink / raw) To: u-boot Hi Alexey, On 04/09/19 3:42 PM, Alexey Brodkin wrote: > Hi Faiz, > >> -----Original Message----- >> From: Faiz Abbas <faiz_abbas@ti.com> >> Sent: Wednesday, September 4, 2019 12:22 PM >> To: u-boot at lists.denx.de >> Cc: paulemge at forallsecure.com; faiz_abbas at ti.com; Alexey Brodkin <abrodkin@synopsys.com>; >> trini at konsulko.com >> Subject: [PATCH] Revert "part: Allocate only one legacy_mbr buffer" >> >> This reverts commit 8639e34d2c5e12cc2e45c95b1a2e97c22bf6a711. >> >> The blk_dread() call following the allocation will read one block from >> the device. This will lead to overflow if the blocksize is greater than >> the size of legacy_mbr. Fix this by allocating one block size. > > Did you read justification of my change that you're reverting now? > With your change in place we'll allocate a buffer > of size = (sizeof(legacy_mbr) * dev_desc->blksz). > > Is that something you really want? Oops. You're right. I thought your patch was changing buffer size from blksz to legacy_mbr. > > 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))); Will send a proper fix. Thanks, Faiz ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 11:43 ` Faiz Abbas @ 2019-09-04 11:46 ` Alexey Brodkin 2019-09-04 12:49 ` Faiz Abbas 0 siblings, 1 reply; 12+ messages in thread From: Alexey Brodkin @ 2019-09-04 11:46 UTC (permalink / raw) To: u-boot Hi Faiz, > -----Original Message----- > From: Faiz Abbas <faiz_abbas@ti.com> > Sent: Wednesday, September 4, 2019 2:44 PM > To: Alexey Brodkin <abrodkin@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 3:42 PM, Alexey Brodkin wrote: > > Hi Faiz, > > > >> -----Original Message----- > >> From: Faiz Abbas <faiz_abbas@ti.com> > >> Sent: Wednesday, September 4, 2019 12:22 PM > >> To: u-boot at lists.denx.de > >> Cc: paulemge at forallsecure.com; faiz_abbas at ti.com; Alexey Brodkin <abrodkin@synopsys.com>; > >> trini at konsulko.com > >> Subject: [PATCH] Revert "part: Allocate only one legacy_mbr buffer" > >> > >> This reverts commit 8639e34d2c5e12cc2e45c95b1a2e97c22bf6a711. > >> > >> The blk_dread() call following the allocation will read one block from > >> the device. This will lead to overflow if the blocksize is greater than > >> the size of legacy_mbr. Fix this by allocating one block size. > > > > Did you read justification of my change that you're reverting now? > > With your change in place we'll allocate a buffer > > of size = (sizeof(legacy_mbr) * dev_desc->blksz). > > > > Is that something you really want? > > Oops. You're right. I thought your patch was changing buffer size from > blksz to legacy_mbr. > > > > > 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. -Alexey ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 11:46 ` Alexey Brodkin @ 2019-09-04 12:49 ` Faiz Abbas 2019-09-04 12:57 ` Alexey Brodkin 0 siblings, 1 reply; 12+ messages in thread From: Faiz Abbas @ 2019-09-04 12:49 UTC (permalink / raw) To: u-boot Hi Alexey, On 04/09/19 5:16 PM, Alexey Brodkin wrote: > Hi Faiz, > >> -----Original Message----- >> From: Faiz Abbas <faiz_abbas@ti.com> >> Sent: Wednesday, September 4, 2019 2:44 PM >> To: Alexey Brodkin <abrodkin@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 3:42 PM, Alexey Brodkin wrote: >>> Hi Faiz, >>> >>>> -----Original Message----- >>>> From: Faiz Abbas <faiz_abbas@ti.com> >>>> Sent: Wednesday, September 4, 2019 12:22 PM >>>> To: u-boot at lists.denx.de >>>> Cc: paulemge at forallsecure.com; faiz_abbas at ti.com; Alexey Brodkin <abrodkin@synopsys.com>; >>>> trini at konsulko.com >>>> Subject: [PATCH] Revert "part: Allocate only one legacy_mbr buffer" >>>> >>>> This reverts commit 8639e34d2c5e12cc2e45c95b1a2e97c22bf6a711. >>>> >>>> The blk_dread() call following the allocation will read one block from >>>> the device. This will lead to overflow if the blocksize is greater than >>>> the size of legacy_mbr. Fix this by allocating one block size. >>> >>> Did you read justification of my change that you're reverting now? >>> With your change in place we'll allocate a buffer >>> of size = (sizeof(legacy_mbr) * dev_desc->blksz). >>> >>> Is that something you really want? >> >> Oops. You're right. I thought your patch was changing buffer size from >> blksz to legacy_mbr. >> >>> >>> 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. 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. Thanks, Faiz ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 12:49 ` Faiz Abbas @ 2019-09-04 12:57 ` Alexey Brodkin 2019-09-04 13:09 ` Faiz Abbas 0 siblings, 1 reply; 12+ messages in thread From: Alexey Brodkin @ 2019-09-04 12:57 UTC (permalink / raw) To: u-boot 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? [1] https://elinux.org/images/d/d6/U-Boot-Bootloader-for-IoT-Platform-Alexey-Brodkin-Synopsys-2.pdf -Alexey ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 12:57 ` Alexey Brodkin @ 2019-09-04 13:09 ` Faiz Abbas 2019-09-04 13:23 ` Alexey Brodkin 0 siblings, 1 reply; 12+ messages in thread From: Faiz Abbas @ 2019-09-04 13:09 UTC (permalink / raw) To: u-boot 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); Thanks, Faiz ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 13:09 ` Faiz Abbas @ 2019-09-04 13:23 ` Alexey Brodkin 2019-09-04 13:46 ` Alexey Brodkin 2019-09-04 14:08 ` Faiz Abbas 0 siblings, 2 replies; 12+ messages in thread From: Alexey Brodkin @ 2019-09-04 13:23 UTC (permalink / raw) To: u-boot 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 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 13:23 ` Alexey Brodkin @ 2019-09-04 13:46 ` Alexey Brodkin 2019-09-04 13:49 ` Tom Rini 2019-09-04 14:08 ` Faiz Abbas 1 sibling, 1 reply; 12+ messages in thread From: Alexey Brodkin @ 2019-09-04 13:46 UTC (permalink / raw) To: u-boot Hi Faiz, > -----Original Message----- > From: Alexey Brodkin > Sent: Wednesday, September 4, 2019 4:23 PM > To: Faiz Abbas <faiz_abbas@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@ti.com> > > Sent: Wednesday, September 4, 2019 4:09 PM > > To: Alexey Brodkin <abrodkin@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. -Alexey ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 13:46 ` Alexey Brodkin @ 2019-09-04 13:49 ` Tom Rini 2019-09-04 14:07 ` Faiz Abbas 0 siblings, 1 reply; 12+ messages in thread From: Tom Rini @ 2019-09-04 13:49 UTC (permalink / raw) To: u-boot 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 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@ti.com> > > > Sent: Wednesday, September 4, 2019 4:09 PM > > > To: Alexey Brodkin <abrodkin@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! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190904/5ed42448/attachment.sig> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 13:49 ` Tom Rini @ 2019-09-04 14:07 ` Faiz Abbas 0 siblings, 0 replies; 12+ messages in thread From: Faiz Abbas @ 2019-09-04 14:07 UTC (permalink / raw) To: u-boot 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@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@ti.com> >>>> Sent: Wednesday, September 4, 2019 4:09 PM >>>> To: Alexey Brodkin <abrodkin@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" 2019-09-04 13:23 ` Alexey Brodkin 2019-09-04 13:46 ` Alexey Brodkin @ 2019-09-04 14:08 ` Faiz Abbas 1 sibling, 0 replies; 12+ messages in thread From: Faiz Abbas @ 2019-09-04 14:08 UTC (permalink / raw) To: u-boot Alexey, On 04/09/19 6:53 PM, Alexey Brodkin wrote: > 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 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-09-04 14:08 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-04 9:22 [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" Faiz Abbas 2019-09-04 10:12 ` Alexey Brodkin 2019-09-04 11:43 ` Faiz Abbas 2019-09-04 11:46 ` Alexey Brodkin 2019-09-04 12:49 ` Faiz Abbas 2019-09-04 12:57 ` Alexey Brodkin 2019-09-04 13:09 ` Faiz Abbas 2019-09-04 13:23 ` Alexey Brodkin 2019-09-04 13:46 ` Alexey Brodkin 2019-09-04 13:49 ` Tom Rini 2019-09-04 14:07 ` Faiz Abbas 2019-09-04 14:08 ` Faiz Abbas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.