[U-Boot] [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.
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;
Hi Faiz,
-----Original Message----- From: Faiz Abbas faiz_abbas@ti.com Sent: Wednesday, September 4, 2019 12:22 PM To: u-boot@lists.denx.de Cc: paulemge@forallsecure.com; faiz_abbas@ti.com; Alexey Brodkin abrodkin@synopsys.com; trini@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
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@lists.denx.de Cc: paulemge@forallsecure.com; faiz_abbas@ti.com; Alexey Brodkin abrodkin@synopsys.com; trini@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
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@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 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@lists.denx.de Cc: paulemge@forallsecure.com; faiz_abbas@ti.com; Alexey Brodkin abrodkin@synopsys.com; trini@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
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@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 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@lists.denx.de Cc: paulemge@forallsecure.com; faiz_abbas@ti.com; Alexey Brodkin abrodkin@synopsys.com; trini@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
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-Bro...
-Alexey
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
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/
-Alexey
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.
-Alexey
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!
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@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!
Ok. Will post that fix.
Thanks, Faiz
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@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/
Interesting. This is news to me.
Thanks, Faiz
participants (3)
-
Alexey Brodkin -
Faiz Abbas -
Tom Rini