Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Miguel Ojeda]

On Sun, Mar 11, 2018 at 12:12 AM, Kees Cook  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

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Daniel Micay]

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

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Daniel Micay]

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.

RE: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[David Laight]

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

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Gustavo A. R. Silva]

On 03/10/2018 05:12 PM, Kees Cook wrote:

On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel
 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 
---
   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

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Daniel Micay]

> 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++.

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Kees Cook]

On Sat, Mar 10, 2018 at 3:06 PM, Arend van Spriel
 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 
>> ---
>>   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

-- 
Kees Cook
Pixel Security

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Arend van Spriel]

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 
---
  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

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Kalle Valo]

Himanshu Jha  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 
>> ---
>>  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.

-- 
Kalle Valo

Re: [PATCH] drivers: net: wireless: ath: ath9: dfs: remove VLA usage

[Himanshu Jha]

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 
> ---
>  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


-- 
Thanks
Himanshu Jha