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

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

[Andreas Christoforou]

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);
 
 /* 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;
-- 
2.7.4

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

-- 
Thanks
Himanshu Jha

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

[Kalle Valo]

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.

-- 
Kalle Valo

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

[Kalle Valo]

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.

-- 
Kalle Valo

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

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

-- 
Kees Cook
Pixel Security

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

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

[David Laight]

RnJvbTogRGFuaWVsIE1pY2F5DQo+IFNlbnQ6IDEwIE1hcmNoIDIwMTggMjM6NDUNCj4gDQo+ID4g
SnVzdCB3b25kZXJpbmcuIElzIHRoaXMgYWN0dWFsbHkgYSBWTEEuIEZGVF9OVU1fU0FNUExFUyB3
YXMgc3RhdGljIGNvbnN0IHNvDQo+ID4gbm90IHJlYWxseSBnb2luZyB0byBzaG93IGEgbG90IG9m
IHZhcmlhdGlvbi4gVGhpcyBhcnJheSB3aWxsIGFsd2F5cyBoYXZlIHRoZQ0KPiA+IHNhbWUgc2l6
ZSBvbiB0aGUgc3RhY2suDQo+IA0KPiBUaGUgaXNzdWUgaXMgdGhhdCB1bmxpa2UgaW4gQysrLCBh
IGBzdGF0aWMgY29uc3RgIGNhbid0IGJlIHVzZWQgaW4gYQ0KPiBjb25zdGFudCBleHByZXNzaW9u
IGluIEMuIEl0J3MgdW5jbGVhciB3aHkgQyBpcyBkZWZpbmVkIHRoYXQgd2F5IGJ1dA0KPiBpdCdz
IGhvdyBpdCBpcyBhbmQgdGhlcmUgaXNuJ3QgY3VycmVudGx5IGEgR0NDIGV4dGVuc2lvbiBtYWtp
bmcgbW9yZQ0KPiB0aGluZ3MgaW50byBjb25zdGFudCBleHByZXNzaW9ucyBsaWtlIEMrKy4NCg0K
J3N0YXRpYycgYW5kICdjb25zdCcgYXJlIGJvdGgganVzdCBxdWFsaWZpZXJzIHRvIGEgJ3Zhcmlh
YmxlJw0KWW91IGNhbiBzdGlsbCB0YWtlIGl0J3MgYWRkcmVzcy4NClRoZSBsYW5ndWFnZSBhbGxv
d3MgeW91IHRvIGNhc3QgYXdheSB0aGUgJ2NvbnN0JyBhbmQgd3JpdGUgdG8NCnRoZSB2YXJpYWJs
ZSAtIHRoZSBlZmZlY3QgaXMgcHJvYmFibHkgJ2ltcGxlbWVudGF0aW9uIGRlZmluZWQnLg0KDQpJ
dCBpcyBwcm9iYWJseSByZXF1aXJlZCB0byBiZSB2YWxpZCBmb3IgJ3N0YXRpYyBjb25zdCcgaXRl
bXMNCnRvIGJlIHBhdGNoYWJsZS4NCg0KCURhdmlkDQoNCg==

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

[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

[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

[Miguel Ojeda]

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

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

[Miguel Ojeda]

On Sun, Mar 11, 2018 at 12:12 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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