[PATCH v2] cfg80211: add work for processing invalid user reg hints

[PATCH v2] cfg80211: add work for processing invalid user reg hints

[Luis R. Rodriguez]

We have no other option but to inform userspace that we
have queued up their regulatory hint request when we are
given one given that nl80211 operates atomically on user
requests. The best we can do is accept the request, and
add a delayed work item for processing failures and cancel it
if we succeeed. Upon failure we restore the regulatory
settings and ignore the supplied user input.

This fixes this reported bug:

https://bugzilla.kernel.org/show_bug.cgi?id=28112

Reported-by: gregoryx.alagnou@intel.com
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---

This simplifies the timer/work implementation by 9 lines :D

 net/wireless/reg.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 3332d5b..cb917de 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -106,6 +106,9 @@ struct reg_beacon {
 static void reg_todo(struct work_struct *work);
 static DECLARE_WORK(reg_work, reg_todo);
 
+static void reg_timeout_work(struct work_struct *work);
+static DECLARE_DELAYED_WORK(reg_timeout, reg_timeout_work);
+
 /* We keep a static world regulatory domain in case of the absence of CRDA */
 static const struct ieee80211_regdomain world_regdom = {
 	.n_reg_rules = 5,
@@ -1330,6 +1333,9 @@ static void reg_set_request_processed(void)
 		need_more_processing = true;
 	spin_unlock(&reg_requests_lock);
 
+	if (last_request->initiator == NL80211_REGDOM_SET_BY_USER)
+		cancel_delayed_work_sync(&reg_timeout);
+
 	if (need_more_processing)
 		schedule_work(&reg_work);
 }
@@ -1584,6 +1590,7 @@ int regulatory_hint_user(const char *alpha2)
 	request->initiator = NL80211_REGDOM_SET_BY_USER;
 
 	queue_regulatory_request(request);
+	schedule_delayed_work(&reg_timeout, msecs_to_jiffies(3142));
 
 	return 0;
 }
@@ -1787,7 +1794,6 @@ static void restore_regulatory_settings(bool reset_user)
 		regulatory_hint_user(user_alpha2);
 }
 
-
 void regulatory_hint_disconnect(void)
 {
 	REG_DBG_PRINT("All devices are disconnected, going to "
@@ -2125,6 +2131,13 @@ out:
 	mutex_unlock(&reg_mutex);
 }
 
+static void reg_timeout_work(struct work_struct *work)
+{
+	REG_DBG_PRINT("Timeout while waiting for CRDA to reply,"
+		      "restoring regulatory settings");
+	restore_regulatory_settings(true);
+}
+
 int __init regulatory_init(void)
 {
 	int err = 0;
@@ -2178,6 +2191,7 @@ void /* __init_or_exit */ regulatory_exit(void)
 	struct reg_beacon *reg_beacon, *btmp;
 
 	cancel_work_sync(&reg_work);
+	cancel_delayed_work_sync(&reg_timeout);
 
 	mutex_lock(&cfg80211_mutex);
 	mutex_lock(&reg_mutex);
-- 
1.7.4.15.g7811d

Re: [PATCH v2] cfg80211: add work for processing invalid user reg hints

[Johannes Berg]

On Thu, 2011-03-31 at 16:29 -0700, Luis R. Rodriguez wrote:
>  
> +static void reg_timeout_work(struct work_struct *work)
> +{
> +	REG_DBG_PRINT("Timeout while waiting for CRDA to reply,"
> +		      "restoring regulatory settings");
> +	restore_regulatory_settings(true);
> +}

We have a queue of requests but you're timing out only the last one.
Wouldn't that still get another one stuck on the queue? TBH, I don't
quite understand why we have last _and_ a queue to start with.

johannes

Re: [PATCH v2] cfg80211: add work for processing invalid user reg hints

[Luis R. Rodriguez]

On Fri, Apr 1, 2011 at 1:17 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2011-03-31 at 16:29 -0700, Luis R. Rodriguez wrote:
>>
>> +static void reg_timeout_work(struct work_struct *work)
>> +{
>> +     REG_DBG_PRINT("Timeout while waiting for CRDA to reply,"
>> +                   "restoring regulatory settings");
>> +     restore_regulatory_settings(true);
>> +}
>
> We have a queue of requests but you're timing out only the last one.
> Wouldn't that still get another one stuck on the queue?

The queue will sit still without processing any queued up regulatory
requests until the last_request was processed. The last_request will
be restored to the world regulatory domain request if we timeout on a
bogus request. There is however one issue here though and that is that
I did not clear out the pending requests when restoring regulatory
settings, and then add the pending stuff back on.

> TBH, I don't quite understand why we have last _and_ a queue to start with.

Well so the queue has the list of pending requests, the last_request
is actually the last request that was processed, this is always
non-null when after bootup and after restoring regulatory settings.
I'll send a v3 with the changes I noted.

Thanks for the review.

  Luis