| Message ID | 20180807225040.GA2164@beast (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 17ad18fd12a35bf84109741415594e21f2577e5e |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [wireless-drivers] mt76x0: Remove VLA usage | expand |
Kees Cook <keescook@chromium.org> writes: > 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 <keescook@chromium.org> > --- > 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.
On Tue, Aug 07, 2018 at 03:50:40PM -0700, Kees Cook wrote: > 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 <keescook@chromium.org> > --- > 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 ? > +#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
Stanislaw Gruszka <sgruszka@redhat.com> writes: > On Tue, Aug 07, 2018 at 03:50:40PM -0700, Kees Cook wrote: >> 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 <keescook@chromium.org> >> --- >> 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 ? > >> +#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. 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?
On Wed, Aug 8, 2018 at 2:46 AM, Kalle Valo <kvalo@codeaurora.org> wrote: > Stanislaw Gruszka <sgruszka@redhat.com> writes: > >> On Tue, Aug 07, 2018 at 03:50:40PM -0700, Kees Cook wrote: >>> 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 <keescook@chromium.org> >>> --- >>> 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 ? I thought all source files needed SPDX: https://lwn.net/Articles/739183/ >> >>> +#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. > > 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
On Wed, Aug 08, 2018 at 08:41:28AM -0700, Kees Cook wrote: > >> 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 ? > > I thought all source files needed SPDX: https://lwn.net/Articles/739183/ Ok, goal is to have all kernel source files with SPDX header. > >>> +#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. > > > > 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. :) Then: Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>
Stanislaw Gruszka <sgruszka@redhat.com> writes: > On Wed, Aug 08, 2018 at 08:41:28AM -0700, Kees Cook wrote: >> >> 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 ? >> >> 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).
Kees Cook <keescook@chromium.org> wrote: > 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 <keescook@chromium.org> > Acked-by: Stanislaw Gruszka <sgruszka@redhat.com> Patch applied to wireless-drivers-next.git, thanks. 17ad18fd12a3 mt76x0: Remove VLA usage
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c b/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c index 1ecd018f12b8..af2fd6a1bb44 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c +++ b/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c @@ -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)
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 <keescook@chromium.org> --- 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(-)