mt76x0: Remove VLA usage
PATCH 1/1 mt76x0: Remove VLA usage
Even with "const" variables, the compiler will generate warnings about VLA usage. In the quest to remove all VLAs from the kernel[1], this uses a #define instead of a const to do the array sizing.
---
Please include this for the v4.19 merge window. The VLA was introduced with the new source file (which I also note is missing a SPDX line), so I'd like to avoid the kernel ever getting released with a VLA here.
--- drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c +3 -3
@@ -81,15 +81,15 @@ mt76x0_efuse_read(struct mt76x0_dev *dev, u16 addr, u8 *data,return 0 ; } +#define MT_MAP_READS DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)static int mt76x0_efuse_physical_size_check (struct mt76x0_dev *dev) { -const int map_reads =DIV_ROUND_UP (MT_EFUSE_USAGE_MAP_SIZE ,16 ); -u8 data[map_reads *16 ]; +u8 data[MT_MAP_READS *16 ];int ret, i;u32 start =0 , end =0 , cnt_free; -for (i =0 ; i < map_reads; i++) { +for (i =0 ; i <MT_MAP_READS ; i++) { ret =mt76x0_efuse_read (dev,MT_EE_USAGE_MAP_START + i *16 , data + i *16 ,MT_EE_PHYSICAL_READ );if (ret)
Signature · 6 lines
2.17.1 -- Kees Cook Pixel Security
Quoted 4 lines · Kees Cook
Even with "const" variables, the compiler will generate warnings about VLA usage. In the quest to remove all VLAs from the kernel[1], this uses a #define instead of a const to do the array sizing.
Quoted 8 lines
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Fixes e87b5039511a ("mt76x0: eeprom files")Signed-off-by Kees Cook <[email protected]>---
Please include this for the v4.19 merge window. The VLA was introduced with the new source file (which I also note is missing a SPDX line), so I'd like to avoid the kernel ever getting released with a VLA here.
Ok, I'll push this to v4.19.
Signature · 1 line
Kalle Valo
Quoted 11 lines · Kees Cook
Even with "const" variables, the compiler will generate warnings about VLA usage. In the quest to remove all VLAs from the kernel[1], this uses a #define instead of a const to do the array sizing.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Fixes e87b5039511a ("mt76x0: eeprom files")Signed-off-by Kees Cook <[email protected]>---
Please include this for the v4.19 merge window. The VLA was introduced with the new source file (which I also note is missing a SPDX line), so
I thought SPDX line is needed only if file has no license and eeprom.c file and other mt76x0 files have specified the license. Is SPDX still needed in that case ?
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c mt76x0_efuse_read(struct mt76x0_dev *dev, u16 addr, u8 *data, · line 88
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c +1
-2+#define MT_MAP_READS DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)static int mt76x0_efuse_physical_size_check (struct mt76x0_dev *dev) { -const int map_reads =DIV_ROUND_UP (MT_EFUSE_USAGE_MAP_SIZE ,16 ); -u8 data[map_reads *16 ];
Why this is variable length array? DIV_ROUND_UP can not be calculated at compile time? But if so, macro do not change the situation either.
Thanks
Stanislaw
Quoted 18 lines · Stanislaw Gruszka
Quoted 4 lines · Kees Cook
Even with "const" variables, the compiler will generate warnings about VLA usage. In the quest to remove all VLAs from the kernel[1], this uses a #define instead of a const to do the array sizing.
Quoted 8 lines
Fixes e87b5039511a ("mt76x0: eeprom files")Signed-off-by Kees Cook <[email protected]>---
Please include this for the v4.19 merge window. The VLA was introduced with the new source file (which I also note is missing a SPDX line), soI thought SPDX line is needed only if file has no license and
eeprom.cfile and other mt76x0 files have specified the license. Is SPDX still needed in that case ?
Quoted 9 lines
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c mt76x0_efuse_read(struct mt76x0_dev *dev, u16 addr, u8 *data, · line 88
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c +1
-2+#define MT_MAP_READS DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)static int mt76x0_efuse_physical_size_check (struct mt76x0_dev *dev) { -const int map_reads =DIV_ROUND_UP (MT_EFUSE_USAGE_MAP_SIZE ,16 ); -u8 data[map_reads *16 ];Why this is variable length array?
DIV_ROUND_UPcan not be calculated at compile time? But if so, macro do not change the situation either.
The commit log mentioned:
"Even with "const" variables, the compiler will generate warnings about
VLA usage."
So I guess the compiler (gcc?) is just not smart enough in this case?
Signature · 1 line
Kalle Valo
Quoted 16 lines · Kalle Valo
Quoted 14 lines · Stanislaw Gruszka
Quoted 12 lines · Kees Cook
Even with "const" variables, the compiler will generate warnings about VLA usage. In the quest to remove all VLAs from the kernel[1], this uses a #define instead of a const to do the array sizing.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.comFixes e87b5039511a ("mt76x0: eeprom files")Signed-off-by Kees Cook <[email protected]>---
Please include this for the v4.19 merge window. The VLA was introduced with the new source file (which I also note is missing a SPDX line), so
Quoted 3 lines
Quoted 3 lines
I thought SPDX line is needed only if file has no license and
eeprom.cfile and other mt76x0 files have specified the license. Is SPDX still needed in that case ?
I thought all source files needed SPDX: https://lwn.net/Articles/739183/
Quoted 11 lines
Quoted 10 lines
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c mt76x0_efuse_read(struct mt76x0_dev *dev, u16 addr, u8 *data, · line 88
Quoted 7 lines
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c +1
-2+#define MT_MAP_READS DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)static int mt76x0_efuse_physical_size_check (struct mt76x0_dev *dev) { -const int map_reads =DIV_ROUND_UP (MT_EFUSE_USAGE_MAP_SIZE ,16 ); -u8 data[map_reads *16 ];Why this is variable length array?
DIV_ROUND_UPcan not be calculated at compile time? But if so, macro do not change the situation either.
Quoted 6 lines
The commit log mentioned:
"Even with "const" variables, the compiler will generate warnings about
VLA usage."So I guess the compiler (gcc?) is just not smart enough in this case?
Correct. This is technically a false positive, but with the goal of adding -Wvla to the build globally, we have to get rid of these as well. It's a little frustrating, I agree, but with all others fixed now, these stand out. :)
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Stanislaw Gruszka
Acked-byQuoted 5 lines · Kees Cook
Quoted 3 lines
Quoted 3 lines
I thought SPDX line is needed only if file has no license and
eeprom.cfile and other mt76x0 files have specified the license. Is SPDX still needed in that case ?I thought all source files needed SPDX: https://lwn.net/Articles/739183/
Ok, goal is to have all kernel source files with SPDX header.
Quoted 15 lines
Quoted 15 lines
Quoted 9 lines
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c mt76x0_efuse_read(struct mt76x0_dev *dev, u16 addr, u8 *data, · line 88
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c +1
-2+#define MT_MAP_READS DIV_ROUND_UP(MT_EFUSE_USAGE_MAP_SIZE, 16)static int mt76x0_efuse_physical_size_check (struct mt76x0_dev *dev) { -const int map_reads =DIV_ROUND_UP (MT_EFUSE_USAGE_MAP_SIZE ,16 ); -u8 data[map_reads *16 ];Why this is variable length array?
DIV_ROUND_UPcan not be calculated at compile time? But if so, macro do not change the situation either.The commit log mentioned:
"Even with "const" variables, the compiler will generate warnings about
VLA usage."
Quoted 6 lines
Quoted 1 line
So I guess the compiler (gcc?) is just not smart enough in this case?
Correct. This is technically a false positive, but with the goal of adding -Wvla to the build globally, we have to get rid of these as well. It's a little frustrating, I agree, but with all others fixed now, these stand out. :)
Then:
Quoted 8 lines · Stanislaw Gruszka
Quoted 5 lines · Kees Cook
Quoted 3 lines
Quoted 3 lines
I thought SPDX line is needed only if file has no license and
eeprom.cfile and other mt76x0 files have specified the license. Is SPDX still needed in that case ?I thought all source files needed SPDX: https://lwn.net/Articles/739183/
Ok, goal is to have all kernel source files with SPDX header.
Yeah, it's a goal but I don't think it's a hard requirement yet. At least I don't see it as a reason to reject patches.
And besides, mt76 uses ISC license and that's not in LICENSES directory. So someone should add that first, and I think that patch should go via Jonathan Corbet's tree (CCed).
Signature · 1 line
Kalle Valo
Quoted 9 lines · Kees Cook
Even with "const" variables, the compiler will generate warnings about VLA usage. In the quest to remove all VLAs from the kernel[1], this uses a #define instead of a const to do the array sizing.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Fixes e87b5039511a ("mt76x0: eeprom files")Signed-off-by Kees Cook <[email protected]>
Patch applied to wireless-drivers-next.git, thanks.
17ad18fd12a3 mt76x0: Remove VLA usage
Signature · 3 lines
https://patchwork.kernel.org/patch/10559297/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches