[PATCH] cfg80211: fix set_regdom() to cancel requests with same alpha2

[PATCH] cfg80211: fix set_regdom() to cancel requests with same alpha2

[Kalle Valo]

While adding regulatory support to ath6kl I noticed that I easily
got the regulatory code confused. The way to reproduce the bug was:

1. iw reg set FI (in userspace)
2. cfg80211 calls ath6kl_reg_notify(FI)
3. ath6kl sets regdomain in firmware
4. firmware sends regdomain event to notify about the new regdomain (FI)
5. ath6kl calls regulatory_hint(FI)

And this (from FI to FI transition) confuses cfg80211 and after that I
only get "Pending regulatory request, waiting for it to be
processed...." messages and regdomain changes won't work anymore.

The reason why ath6kl calls regulatory_hint() is that firmware can change
the regulatory domain by it's own, for example due to 11d IEs. I could
of course workaround this in ath6kl but I think it's better to handle
the case in cfg80211.

The fix is pretty simple, use a different error code if the regdomain is
same and then just set the request processed so that it doesn't block new
requests.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 net/wireless/reg.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 6d57e18..dbb01df 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2205,7 +2205,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 		 * checking if the alpha2 changes if CRDA was already called
 		 */
 		if (!regdom_changes(rd->alpha2))
-			return -EINVAL;
+			return -EALREADY;
 	}
 
 	/*
@@ -2325,6 +2325,9 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 	/* Note that this doesn't update the wiphys, this is done below */
 	r = __set_regdom(rd);
 	if (r) {
+		if (r == -EALREADY)
+			reg_set_request_processed();
+
 		kfree(rd);
 		mutex_unlock(&reg_mutex);
 		return r;

Re: [PATCH] cfg80211: fix set_regdom() to cancel requests with same alpha2

[Johannes Berg]

Luis?

>  		if (!regdom_changes(rd->alpha2))
> -			return -EINVAL;
> +			return -EALREADY;
>  	}
>  
>  	/*
> @@ -2325,6 +2325,9 @@ int set_regdom(const struct ieee80211_regdomain *rd)
>  	/* Note that this doesn't update the wiphys, this is done below */
>  	r = __set_regdom(rd);
>  	if (r) {
> +		if (r == -EALREADY)
> +			reg_set_request_processed();
> +

I'm not really sure I like the reliance on the error code ... would it
make more sense to invent a status enum? Or just return 0 to start with?

johannes

Re: [PATCH] cfg80211: fix set_regdom() to cancel requests with same alpha2

[Luis R. Rodriguez]

On Fri, Jul 13, 2012 at 5:50 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Luis?

It follows the -EALREADY convention of when a regdomain is already applied.

Acked-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com>

>>               if (!regdom_changes(rd->alpha2))
>> -                     return -EINVAL;
>> +                     return -EALREADY;
>>       }
>>
>>       /*
>> @@ -2325,6 +2325,9 @@ int set_regdom(const struct ieee80211_regdomain *rd)
>>       /* Note that this doesn't update the wiphys, this is done below */
>>       r = __set_regdom(rd);
>>       if (r) {
>> +             if (r == -EALREADY)
>> +                     reg_set_request_processed();
>> +
>
> I'm not really sure I like the reliance on the error code ... would it
> make more sense to invent a status enum? Or just return 0 to start with?

The code already did this with -EALREADY, if we want to treat this
differently we should go in and sweep change all that. But the big
sweep is better to happen on the regsim IMHO. I've been trying to do
as little changes as possible to existing code.

  Luis

Re: [PATCH] cfg80211: fix set_regdom() to cancel requests with same alpha2

[Johannes Berg]

On Thu, 2012-07-12 at 15:33 +0300, Kalle Valo wrote:
> While adding regulatory support to ath6kl I noticed that I easily
> got the regulatory code confused. The way to reproduce the bug was:
> 
> 1. iw reg set FI (in userspace)
> 2. cfg80211 calls ath6kl_reg_notify(FI)
> 3. ath6kl sets regdomain in firmware
> 4. firmware sends regdomain event to notify about the new regdomain (FI)
> 5. ath6kl calls regulatory_hint(FI)
> 
> And this (from FI to FI transition) confuses cfg80211 and after that I
> only get "Pending regulatory request, waiting for it to be
> processed...." messages and regdomain changes won't work anymore.
> 
> The reason why ath6kl calls regulatory_hint() is that firmware can change
> the regulatory domain by it's own, for example due to 11d IEs. I could
> of course workaround this in ath6kl but I think it's better to handle
> the case in cfg80211.
> 
> The fix is pretty simple, use a different error code if the regdomain is
> same and then just set the request processed so that it doesn't block new
> requests.
> 
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

Should this go to 3.5 and maybe be Cc: stable?

If so, John please pick it up, fwiw:

Acked-by: Johannes Berg <johannes@sipsolutions.net>

Otherwise I'll pick it up after Kalle decides :)

johannes

Re: [PATCH] cfg80211: fix set_regdom() to cancel requests with same alpha2

[Kalle Valo]

On 07/17/2012 12:52 PM, Johannes Berg wrote:
> On Thu, 2012-07-12 at 15:33 +0300, Kalle Valo wrote:
>> While adding regulatory support to ath6kl I noticed that I easily
>> got the regulatory code confused. The way to reproduce the bug was:
>>
>> 1. iw reg set FI (in userspace)
>> 2. cfg80211 calls ath6kl_reg_notify(FI)
>> 3. ath6kl sets regdomain in firmware
>> 4. firmware sends regdomain event to notify about the new regdomain (FI)
>> 5. ath6kl calls regulatory_hint(FI)
>>
>> And this (from FI to FI transition) confuses cfg80211 and after that I
>> only get "Pending regulatory request, waiting for it to be
>> processed...." messages and regdomain changes won't work anymore.
>>
>> The reason why ath6kl calls regulatory_hint() is that firmware can change
>> the regulatory domain by it's own, for example due to 11d IEs. I could
>> of course workaround this in ath6kl but I think it's better to handle
>> the case in cfg80211.
>>
>> The fix is pretty simple, use a different error code if the regdomain is
>> same and then just set the request processed so that it doesn't block new
>> requests.
>>
>> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
> 
> Should this go to 3.5 and maybe be Cc: stable?
> 
> If so, John please pick it up, fwiw:
> 
> Acked-by: Johannes Berg <johannes@sipsolutions.net>
> 
> Otherwise I'll pick it up after Kalle decides :)

IMHO this doesn't need to go 3.5 nor stable. I only noticed it with
ath6kl and the corresponding ath6kl patches will go to 3.6.

Kalle