* [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.