| Message ID | 1520598613-3641-1-git-send-email-andreaschristofo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 09, 2018 at 02:30:12PM +0200, Andreas Christoforou wrote: > The kernel would like to have all stack VLA usage removed. > > Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> > --- > drivers/net/wireless/ath/ath9k/dfs.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/dfs.c b/drivers/net/wireless/ath/ath9k/dfs.c > index 6fee9a4..cfb0f84 100644 > --- a/drivers/net/wireless/ath/ath9k/dfs.c > +++ b/drivers/net/wireless/ath/ath9k/dfs.c > @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX = 10; > > /* we need at least 3 deltas / 4 samples for a reliable chirp detection */ > #define NUM_DIFFS 3 > -static const int FFT_NUM_SAMPLES = (NUM_DIFFS + 1); Are you sure it is correct ? Look for other users of "FFT_NUM_SAMPLES". > /* Threshold for difference of delta peaks */ > static const int MAX_DIFF = 2; > @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 *data, > int datalen, bool is_ctl, bool is_ext) > { > int i; > - int max_bin[FFT_NUM_SAMPLES]; > + int max_bin[NUM_DIFFS + 1]; > struct ath_hw *ah = sc->sc_ah; > struct ath_common *common = ath9k_hw_common(ah); > int prev_delta; Always compile test the driver before sending a patch. Also, patch title seems incorrect *ath9k* himanshu@himanshu-Vostro-3559:~/linux-next$ git log --oneline drivers/net/wireless/ath/ath9k/dfs.c 626ab67 ath9k: dfs: use swap macro in ath9k_check_chirping 50c8cd4 ath9k: remove cast to void pointer 8fc2b61 ath9k: DFS - add pulse chirp detection for FCC ....
Himanshu Jha <morganfreeman6991@gmail.com> writes: > On Fri, Mar 09, 2018 at 02:30:12PM +0200, Andreas Christoforou wrote: >> The kernel would like to have all stack VLA usage removed. >> >> Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> >> --- >> drivers/net/wireless/ath/ath9k/dfs.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath9k/dfs.c >> b/drivers/net/wireless/ath/ath9k/dfs.c >> index 6fee9a4..cfb0f84 100644 >> --- a/drivers/net/wireless/ath/ath9k/dfs.c >> +++ b/drivers/net/wireless/ath/ath9k/dfs.c >> @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX = 10; >> >> /* we need at least 3 deltas / 4 samples for a reliable chirp detection */ >> #define NUM_DIFFS 3 >> -static const int FFT_NUM_SAMPLES = (NUM_DIFFS + 1); > > Are you sure it is correct ? > Look for other users of "FFT_NUM_SAMPLES". > >> /* Threshold for difference of delta peaks */ >> static const int MAX_DIFF = 2; >> @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 *data, >> int datalen, bool is_ctl, bool is_ext) >> { >> int i; >> - int max_bin[FFT_NUM_SAMPLES]; >> + int max_bin[NUM_DIFFS + 1]; >> struct ath_hw *ah = sc->sc_ah; >> struct ath_common *common = ath9k_hw_common(ah); >> int prev_delta; > > Always compile test the driver before sending a patch. > Also, patch title seems incorrect *ath9k* > > himanshu@himanshu-Vostro-3559:~/linux-next$ git log --oneline > drivers/net/wireless/ath/ath9k/dfs.c > 626ab67 ath9k: dfs: use swap macro in ath9k_check_chirping > 50c8cd4 ath9k: remove cast to void pointer > 8fc2b61 ath9k: DFS - add pulse chirp detection for FCC > .... Yeah, just "ath9k:" is enough as the prefix, no need to have full directory path in the title.
On 3/9/2018 1:30 PM, Andreas Christoforou wrote: > The kernel would like to have all stack VLA usage removed. I think there was a remark made earlier to give more explanation here. It should explain why we want "VLA on stack" removed. > Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> > --- > drivers/net/wireless/ath/ath9k/dfs.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/dfs.c b/drivers/net/wireless/ath/ath9k/dfs.c > index 6fee9a4..cfb0f84 100644 > --- a/drivers/net/wireless/ath/ath9k/dfs.c > +++ b/drivers/net/wireless/ath/ath9k/dfs.c > @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX = 10; > > /* we need at least 3 deltas / 4 samples for a reliable chirp detection */ > #define NUM_DIFFS 3 > -static const int FFT_NUM_SAMPLES = (NUM_DIFFS + 1); > > /* Threshold for difference of delta peaks */ > static const int MAX_DIFF = 2; > @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 *data, > int datalen, bool is_ctl, bool is_ext) > { > int i; > - int max_bin[FFT_NUM_SAMPLES]; > + int max_bin[NUM_DIFFS + 1]; Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so not really going to show a lot of variation. This array will always have the same size on the stack. Regards, Arend
On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > On 3/9/2018 1:30 PM, Andreas Christoforou wrote: >> >> The kernel would like to have all stack VLA usage removed. > > > I think there was a remark made earlier to give more explanation here. It > should explain why we want "VLA on stack" removed. > >> Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> >> --- >> drivers/net/wireless/ath/ath9k/dfs.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath9k/dfs.c >> b/drivers/net/wireless/ath/ath9k/dfs.c >> index 6fee9a4..cfb0f84 100644 >> --- a/drivers/net/wireless/ath/ath9k/dfs.c >> +++ b/drivers/net/wireless/ath/ath9k/dfs.c >> @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX = 10; >> >> /* we need at least 3 deltas / 4 samples for a reliable chirp detection >> */ >> #define NUM_DIFFS 3 >> -static const int FFT_NUM_SAMPLES = (NUM_DIFFS + 1); >> >> /* Threshold for difference of delta peaks */ >> static const int MAX_DIFF = 2; >> @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, >> u8 *data, >> int datalen, bool is_ctl, bool is_ext) >> { >> int i; >> - int max_bin[FFT_NUM_SAMPLES]; >> + int max_bin[NUM_DIFFS + 1]; > > > Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so > not really going to show a lot of variation. This array will always have the > same size on the stack. The problem is that it's not a "constant expression", so the compiler frontend still yells about it under -Wvla. I would characterize this mainly as a fix for "accidental VLA" or "misdetected VLA" or something like that. AIUI, there really isn't a functional change here. -Kees
On 03/10/2018 05:12 PM, Kees Cook wrote: > On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel > <arend.vanspriel@broadcom.com> wrote: >> On 3/9/2018 1:30 PM, Andreas Christoforou wrote: >>> >>> The kernel would like to have all stack VLA usage removed. >> >> >> I think there was a remark made earlier to give more explanation here. It >> should explain why we want "VLA on stack" removed. >> >>> Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> >>> --- >>> drivers/net/wireless/ath/ath9k/dfs.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath9k/dfs.c >>> b/drivers/net/wireless/ath/ath9k/dfs.c >>> index 6fee9a4..cfb0f84 100644 >>> --- a/drivers/net/wireless/ath/ath9k/dfs.c >>> +++ b/drivers/net/wireless/ath/ath9k/dfs.c >>> @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX = 10; >>> >>> /* we need at least 3 deltas / 4 samples for a reliable chirp detection >>> */ >>> #define NUM_DIFFS 3 >>> -static const int FFT_NUM_SAMPLES = (NUM_DIFFS + 1); >>> What about this instead? #define FFT_NUM_SAMPLES (NUM_DIFFS + 1) >>> /* Threshold for difference of delta peaks */ >>> static const int MAX_DIFF = 2; >>> @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, >>> u8 *data, >>> int datalen, bool is_ctl, bool is_ext) >>> { >>> int i; >>> - int max_bin[FFT_NUM_SAMPLES]; >>> + int max_bin[NUM_DIFFS + 1]; >> >> >> Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so >> not really going to show a lot of variation. This array will always have the >> same size on the stack. > > The problem is that it's not a "constant expression", so the compiler > frontend still yells about it under -Wvla. I would characterize this > mainly as a fix for "accidental VLA" or "misdetected VLA" or something > like that. AIUI, there really isn't a functional change here. > > -Kees > -- Gustavo
> Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so > not really going to show a lot of variation. This array will always have the > same size on the stack. The issue is that unlike in C++, a `static const` can't be used in a constant expression in C. It's unclear why C is defined that way but it's how it is and there isn't currently a GCC extension making more things into constant expressions like C++.
From: Daniel Micay > Sent: 10 March 2018 23:45 > > > Just wondering. Is this actually a VLA. FFT_NUM_SAMPLES was static const so > > not really going to show a lot of variation. This array will always have the > > same size on the stack. > > The issue is that unlike in C++, a `static const` can't be used in a > constant expression in C. It's unclear why C is defined that way but > it's how it is and there isn't currently a GCC extension making more > things into constant expressions like C++. 'static' and 'const' are both just qualifiers to a 'variable' You can still take it's address. The language allows you to cast away the 'const' and write to the variable - the effect is probably 'implementation defined'. It is probably required to be valid for 'static const' items to be patchable. David
No, it's undefined behavior to write to a const variable. The `static` and `const` on the variable both change the code generation in the real world as permitted / encouraged by the standard. It's placed in read-only memory. Trying to write to it will break. It's not "implemented defined" to write to it, it's "undefined behavior" i.e. it's considered incorrect. There a clear distinction between those in the standard. You're confusing having a real `const` for a variable with having it applied to a pointer. It's well-defined to cast away const from a pointer and write to what it points at if it's not actually const. If it is const, that's broken. There's nothing implementation defined about either case. The C standard could have considered `static const` variables to work as constant expressions just like the C++ standard. They borrowed it from there but made it less useful than const in what became the C++ standard. They also used stricter rules for the permitted implicit conversions of const pointers which made those much less usable, i.e. converting `int **` to `const int *const *` wasn't permitted like C++. I don't think the difference between C and C++ const pointer conversions, it's a quirk of them being standardized on different timelines and ending up with different versions of the same thing. On the other hand, they might have left out having ever so slightly more useful constant expressions on purpose since people can use #define.
> I don't think the difference between C and C++ const pointer > conversions I mean I don't think the difference between them was intended.
On Sun, Mar 11, 2018 at 12:12 AM, Kees Cook <keescook@chromium.org> wrote: > > The problem is that it's not a "constant expression", so the compiler > frontend still yells about it under -Wvla. I would characterize this > mainly as a fix for "accidental VLA" or "misdetected VLA" or something > like that. AIUI, there really isn't a functional change here. C++11's constexpr variables would be nice to have. Cheers, Miguel
diff --git a/drivers/net/wireless/ath/ath9k/dfs.c b/drivers/net/wireless/ath/ath9k/dfs.c index 6fee9a4..cfb0f84 100644 --- a/drivers/net/wireless/ath/ath9k/dfs.c +++ b/drivers/net/wireless/ath/ath9k/dfs.c @@ -41,7 +41,6 @@ static const int BIN_DELTA_MAX = 10; /* we need at least 3 deltas / 4 samples for a reliable chirp detection */ #define NUM_DIFFS 3 -static const int FFT_NUM_SAMPLES = (NUM_DIFFS + 1); /* Threshold for difference of delta peaks */ static const int MAX_DIFF = 2; @@ -101,7 +100,7 @@ static bool ath9k_check_chirping(struct ath_softc *sc, u8 *data, int datalen, bool is_ctl, bool is_ext) { int i; - int max_bin[FFT_NUM_SAMPLES]; + int max_bin[NUM_DIFFS + 1]; struct ath_hw *ah = sc->sc_ah; struct ath_common *common = ath9k_hw_common(ah); int prev_delta;
The kernel would like to have all stack VLA usage removed. Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> --- drivers/net/wireless/ath/ath9k/dfs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)