[PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Pointer arithmetic already adjusts by the size of the struct,
so the sizeof() calculation is wrong. This is basically the
same as Colin King's patch for similar code in the iwlwifi
driver.

Fixes: 230ebaa189af ("cfg80211: read wmm rules from regulatory database")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/reg.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 4fc66a117b7d..283902974fbf 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain *src_regd)
 			continue;
 
 		regd->reg_rules[i].wmm_rule = d_wmm +
-			(src_regd->reg_rules[i].wmm_rule - s_wmm) /
-			sizeof(struct ieee80211_wmm_rule);
+			(src_regd->reg_rules[i].wmm_rule - s_wmm);
 	}
 	return regd;
 }
-- 
2.14.4

Re: [PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Stanislaw Gruszka]

On Mon, Aug 20, 2018 at 09:37:05AM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Pointer arithmetic already adjusts by the size of the struct,
> so the sizeof() calculation is wrong. This is basically the
> same as Colin King's patch for similar code in the iwlwifi
> driver.
> 
> Fixes: 230ebaa189af ("cfg80211: read wmm rules from regulatory database")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/wireless/reg.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 4fc66a117b7d..283902974fbf 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain *src_regd)
>  			continue;
>  
>  		regd->reg_rules[i].wmm_rule = d_wmm +
> -			(src_regd->reg_rules[i].wmm_rule - s_wmm) /
> -			sizeof(struct ieee80211_wmm_rule);
> +			(src_regd->reg_rules[i].wmm_rule - s_wmm);
>  	}
>  	return regd;
>  }

As side note those pointer aritmetics related with rule->wmm_rule is
really involuted in various places in reg.c . Seems would be better to
just make wmm_rule part of iee80211_reg_rule structure like this:

struct ieee80211_reg_rule {
        struct ieee80211_freq_range freq_range;
        struct ieee80211_power_rule power_rule;
        struct ieee80211_wmm_rule wmm_rule;
        u32 flags;
        u32 dfs_cac_ms;
};

and use a flag to intdicate wmm_rule is valid. There should be no
big memory overhead sice there are only few reg_rules for the regdomain.
Or I'm wrong?

Regards
Stanislaw

Re: [PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Grzegorz Duszyński]

On 20.08.2018 12:27, Stanislaw Gruszka wrote:
> On Mon, Aug 20, 2018 at 09:37:05AM +0200, Johannes Berg wrote:
>> From: Johannes Berg <johannes.berg@intel.com>
>>
>> Pointer arithmetic already adjusts by the size of the struct,
>> so the sizeof() calculation is wrong. This is basically the
>> same as Colin King's patch for similar code in the iwlwifi
>> driver.
>>
>> Fixes: 230ebaa189af ("cfg80211: read wmm rules from regulatory database")
>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>> ---
>>   net/wireless/reg.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 4fc66a117b7d..283902974fbf 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain *src_regd)
>>   			continue;
>>   
>>   		regd->reg_rules[i].wmm_rule = d_wmm +
>> -			(src_regd->reg_rules[i].wmm_rule - s_wmm) /
>> -			sizeof(struct ieee80211_wmm_rule);
>> +			(src_regd->reg_rules[i].wmm_rule - s_wmm);
>>   	}
>>   	return regd;
>>   }
> As side note those pointer aritmetics related with rule->wmm_rule is
> really involuted in various places in reg.c . Seems would be better to
> just make wmm_rule part of iee80211_reg_rule structure like this:
>
> struct ieee80211_reg_rule {
>          struct ieee80211_freq_range freq_range;
>          struct ieee80211_power_rule power_rule;
>          struct ieee80211_wmm_rule wmm_rule;
>          u32 flags;
>          u32 dfs_cac_ms;
> };
>
> and use a flag to intdicate wmm_rule is valid. There should be no
> big memory overhead sice there are only few reg_rules for the regdomain.
> Or I'm wrong?
>
> Regards
> Stanislaw

Thank you for the patch.

I've applied the patch, but it does not seem to help.
Spawning second instance of hostapd causes same error as before.

I don't follow why this issue would not occur with single NIC.
It's not like I am an expert in this field, but those rules should be 
applied to both interfaces, right?
Both interfaces use ath10k, that should imply same code path for setting 
rules.

Re: [PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Grzegorz Duszyński]

On 21.08.2018 08:23, Grzegorz Duszyński wrote:
> On 20.08.2018 12:27, Stanislaw Gruszka wrote:
>> On Mon, Aug 20, 2018 at 09:37:05AM +0200, Johannes Berg wrote:
>>> From: Johannes Berg <johannes.berg@intel.com>
>>>
>>> Pointer arithmetic already adjusts by the size of the struct,
>>> so the sizeof() calculation is wrong. This is basically the
>>> same as Colin King's patch for similar code in the iwlwifi
>>> driver.
>>>
>>> Fixes: 230ebaa189af ("cfg80211: read wmm rules from regulatory 
>>> database")
>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>> ---
>>>   net/wireless/reg.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>>> index 4fc66a117b7d..283902974fbf 100644
>>> --- a/net/wireless/reg.c
>>> +++ b/net/wireless/reg.c
>>> @@ -452,8 +452,7 @@ reg_copy_regd(const struct ieee80211_regdomain 
>>> *src_regd)
>>>               continue;
>>>             regd->reg_rules[i].wmm_rule = d_wmm +
>>> -            (src_regd->reg_rules[i].wmm_rule - s_wmm) /
>>> -            sizeof(struct ieee80211_wmm_rule);
>>> +            (src_regd->reg_rules[i].wmm_rule - s_wmm);
>>>       }
>>>       return regd;
>>>   }
>> As side note those pointer aritmetics related with rule->wmm_rule is
>> really involuted in various places in reg.c . Seems would be better to
>> just make wmm_rule part of iee80211_reg_rule structure like this:
>>
>> struct ieee80211_reg_rule {
>>          struct ieee80211_freq_range freq_range;
>>          struct ieee80211_power_rule power_rule;
>>          struct ieee80211_wmm_rule wmm_rule;
>>          u32 flags;
>>          u32 dfs_cac_ms;
>> };
>>
>> and use a flag to intdicate wmm_rule is valid. There should be no
>> big memory overhead sice there are only few reg_rules for the regdomain.
>> Or I'm wrong?
>>
>> Regards
>> Stanislaw
>
> Thank you for the patch.
>
> I've applied the patch, but it does not seem to help.
> Spawning second instance of hostapd causes same error as before.
>
> I don't follow why this issue would not occur with single NIC.
> It's not like I am an expert in this field, but those rules should be 
> applied to both interfaces, right?
> Both interfaces use ath10k, that should imply same code path for 
> setting rules.
>

Issue has also been experienced by other Arch user:
https://bbs.archlinux.org/viewtopic.php?pid=1803445#p1803445

User chr0mag also reports that LTS version of Arch kernel does not 
exhibit this issue.

Re: [PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Johannes Berg]

On Tue, 2018-08-21 at 08:23 +0200, Grzegorz Duszyński wrote:
> 
> I've applied the patch, but it does not seem to help.
> Spawning second instance of hostapd causes same error as before.

Ok, it was worth a try.

> I don't follow why this issue would not occur with single NIC.
> It's not like I am an expert in this field, but those rules should be 
> applied to both interfaces, right?
> Both interfaces use ath10k, that should imply same code path for setting 
> rules.

Yeah that's why I thought maybe the copy code was involved ...

I guess we'll need to try to reproduce. Maybe it can be made to happen
with hwsim too?

johannes

Re: [PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Grzegorz Duszyński]

On 21.08.2018 11:03, Johannes Berg wrote:
> On Tue, 2018-08-21 at 08:23 +0200, Grzegorz Duszyński wrote:
>> I've applied the patch, but it does not seem to help.
>> Spawning second instance of hostapd causes same error as before.
> Ok, it was worth a try.
>
>> I don't follow why this issue would not occur with single NIC.
>> It's not like I am an expert in this field, but those rules should be
>> applied to both interfaces, right?
>> Both interfaces use ath10k, that should imply same code path for setting
>> rules.
> Yeah that's why I thought maybe the copy code was involved ...
>
> I guess we'll need to try to reproduce. Maybe it can be made to happen
> with hwsim too?
>
> johannes

I can try to recreate the issue with hwsim, it will take me some time 
probably as I have no prior experience.

Re: [PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Johannes Berg]

> I can try to recreate the issue with hwsim, it will take me some time 
> probably as I have no prior experience.

No worries, I'm compiling it now. Do you have a special hostapd
configuration?

johannes

Re: [PATCH] cfg80211: remove division by size of sizeof(struct ieee80211_wmm_rule)

[Grzegorz Duszyński]

On 21.08.2018 11:07, Johannes Berg wrote:
>> I can try to recreate the issue with hwsim, it will take me some time
>> probably as I have no prior experience.
> No worries, I'm compiling it now. Do you have a special hostapd
> configuration?
>
> johannes

I think they are pretty standard:

Compex wle900vx:
https://pastebin.com/pkThTvWH

Killer 1535 - didn't have a chance to finish this one
https://pastebin.com/eJ3t36ra