[PATCH 0/4] cfg80211: two reg fixes

[PATCH 0/4] cfg80211: two reg fixes

[Luis R. Rodriguez]

This addresses two very unlikely but yet possible races
with cfg80211 reg code.

Luis R. Rodriguez (4):
  cfg80211: return immediately if num reg rules >
    NL80211_MAX_SUPP_REG_RULES
  cfg80211: cleanup return calls on nl80211_set_reg()
  cfg80211: fix in nl80211_set_reg()
  cfg80211: fix race between core hint and driver's custom apply

 net/wireless/nl80211.c |   26 ++++++++++++++++++--------
 net/wireless/reg.c     |    9 +++++++++
 2 files changed, 27 insertions(+), 8 deletions(-)

[PATCH 1/4] cfg80211: return immediately if num reg rules > NL80211_MAX_SUPP_REG_RULES

[Luis R. Rodriguez]

This has no functional change except we save a kfree(rd) and
allows us to clean this code up a bit after this. We do
avoid an unnecessary kfree(NULL) but calling that was OK too.

Cc: stable@kernel.org
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/nl80211.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f88dbbe..88cb4ab 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2542,7 +2542,7 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
 			rem_reg_rules) {
 		num_rules++;
 		if (num_rules > NL80211_MAX_SUPP_REG_RULES)
-			goto bad_reg;
+			return -EINVAL;
 	}
 
 	if (!reg_is_valid_request(alpha2))
-- 
1.6.0.6

[PATCH 2/4] cfg80211: cleanup return calls on nl80211_set_reg()

[Luis R. Rodriguez]

This has no functional change, but it will make the race
fix easier to spot in my next patch.

Cc: stable@kernel.org
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/nl80211.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 88cb4ab..bb2c37a 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2545,15 +2545,19 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
 			return -EINVAL;
 	}
 
-	if (!reg_is_valid_request(alpha2))
-		return -EINVAL;
+	if (!reg_is_valid_request(alpha2)) {
+		r = -EINVAL;
+		goto bad_reg;
+	}
 
 	size_of_regd = sizeof(struct ieee80211_regdomain) +
 		(num_rules * sizeof(struct ieee80211_reg_rule));
 
 	rd = kzalloc(size_of_regd, GFP_KERNEL);
-	if (!rd)
-		return -ENOMEM;
+	if (!rd) {
+		r = -ENOMEM;
+		goto bad_reg;
+	}
 
 	rd->n_reg_rules = num_rules;
 	rd->alpha2[0] = alpha2[0];
@@ -2570,8 +2574,10 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
 
 		rule_idx++;
 
-		if (rule_idx > NL80211_MAX_SUPP_REG_RULES)
+		if (rule_idx > NL80211_MAX_SUPP_REG_RULES) {
+			r = -EINVAL;
 			goto bad_reg;
+		}
 	}
 
 	BUG_ON(rule_idx != num_rules);
@@ -2579,11 +2585,12 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
 	mutex_lock(&cfg80211_mutex);
 	r = set_regdom(rd);
 	mutex_unlock(&cfg80211_mutex);
+
 	return r;
 
  bad_reg:
 	kfree(rd);
-	return -EINVAL;
+	return r;
 }
 
 static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info)
-- 
1.6.0.6

[PATCH 3/4] cfg80211: fix in nl80211_set_reg()

[Luis R. Rodriguez]

There is a race on access to last_request and its alpha2
through reg_is_valid_request() and us possibly processing
first another regulatory request on another CPU. We avoid
this improbably race by locking with the cfg80211_mutex as
we should have done in the first place. While at it add
the assert on locking on reg_is_valid_request().

Cc: stable@kernel.org
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/nl80211.c |    5 ++++-
 net/wireless/reg.c     |    2 ++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index bb2c37a..d67f891 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2545,6 +2545,8 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
 			return -EINVAL;
 	}
 
+	mutex_lock(&cfg80211_mutex);
+
 	if (!reg_is_valid_request(alpha2)) {
 		r = -EINVAL;
 		goto bad_reg;
@@ -2582,13 +2584,14 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
 
 	BUG_ON(rule_idx != num_rules);
 
-	mutex_lock(&cfg80211_mutex);
 	r = set_regdom(rd);
+
 	mutex_unlock(&cfg80211_mutex);
 
 	return r;
 
  bad_reg:
+	mutex_unlock(&cfg80211_mutex);
 	kfree(rd);
 	return r;
 }
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 537af62..041300e 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -388,6 +388,8 @@ static int call_crda(const char *alpha2)
 /* Used by nl80211 before kmalloc'ing our regulatory domain */
 bool reg_is_valid_request(const char *alpha2)
 {
+	assert_cfg80211_lock();
+
 	if (!last_request)
 		return false;
 
-- 
1.6.0.6

Re: [PATCH 3/4] cfg80211: fix in nl80211_set_reg()

[John W. Linville]

On Wed, May 13, 2009 at 05:04:41PM -0400, Luis R. Rodriguez wrote:
> There is a race on access to last_request and its alpha2
> through reg_is_valid_request() and us possibly processing
> first another regulatory request on another CPU. We avoid
> this improbably race by locking with the cfg80211_mutex as
> we should have done in the first place. While at it add
> the assert on locking on reg_is_valid_request().
> 
> Cc: stable@kernel.org
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>

What is the effect of this race?  What justifies this for 2.6.30
and/or stable?  It is getting late in the cycle for 2.6.30...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 3/4] cfg80211: fix in nl80211_set_reg()

[Luis R. Rodriguez]

On Mon, May 18, 2009 at 11:46:41AM -0700, John W. Linville wrote:
> On Wed, May 13, 2009 at 05:04:41PM -0400, Luis R. Rodriguez wrote:
> > There is a race on access to last_request and its alpha2
> > through reg_is_valid_request() and us possibly processing
> > first another regulatory request on another CPU. We avoid
> > this improbably race by locking with the cfg80211_mutex as
> > we should have done in the first place. While at it add
> > the assert on locking on reg_is_valid_request().
> >
> > Cc: stable@kernel.org
> > Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> 
> What is the effect of this race?

The race is theoretical and I cannot confirm if it happens but
if it does I suspect we can allow / deny regutatory requests
when the race occurs when in fact we shouldn't.

> What justifies this for 2.6.30
> and/or stable?  It is getting late in the cycle for 2.6.30...

We could potentially be allowing two duplicate regulatory
requests to be processed when two requests are being processed
on different CPUs, I believe this is hard to trigger, you would
need the global workqueue to be hit pretty hard, but if the race
is hit I would expect two regulatory requests for the same
alpha2 to be tried to be processed. In order to trigger though
you'd need crda to send two regulutory responses instead of one
or someone in userspace hammering crda for the same alpha2.
Just checked and if the race happens we'd drop it anyway with
a WARN_ON() later in __set_regdom():

        if (WARN_ON(!reg_is_valid_request(rd->alpha2)))
                return -EINVAL;

I suppose its safe to say that's all the consequences I can think of,
unless I'm overlooking something.

  Luis

[PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[Luis R. Rodriguez]

Its possible for cfg80211 to have scheduled the work and for
the global workqueue to not have kicked in prior to a cfg80211
driver's regulatory hint or wiphy_apply_custom_regulatory().

Although this is very unlikely its possible and should fix
this race. When this race would happen you are expected to have
hit a null pointer dereference panic.

Cc: stable@kernel.org
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 041300e..a5fe1f3 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1554,6 +1554,13 @@ static int regulatory_hint_core(const char *alpha2)
 
 	queue_regulatory_request(request);
 
+	/*
+	 * This ensures last_request is populated once modules
+	 * come swinging in and calling regulatory hints and
+	 * wiphy_apply_custom_regulatory().
+	 */
+	flush_scheduled_work();
+
 	return 0;
 }
 
-- 
1.6.0.6

Re: [PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Wed, 2009-05-13 at 17:04 -0400, Luis R. Rodriguez wrote:

>  	queue_regulatory_request(request);
>  
> +	/*
> +	 * This ensures last_request is populated once modules
> +	 * come swinging in and calling regulatory hints and
> +	 * wiphy_apply_custom_regulatory().
> +	 */
> +	flush_scheduled_work();
> +

Umm, this is a little stupid, first you queue it and then you flush the
workqueue... Why not call the function that runs off the workqueue
directly?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[Luis R. Rodriguez]

On Wed, May 13, 2009 at 3:08 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2009-05-13 at 17:04 -0400, Luis R. Rodriguez wrote:
>
>> =C2=A0 =C2=A0 =C2=A0 queue_regulatory_request(request);
>>
>> + =C2=A0 =C2=A0 /*
>> + =C2=A0 =C2=A0 =C2=A0* This ensures last_request is populated once =
modules
>> + =C2=A0 =C2=A0 =C2=A0* come swinging in and calling regulatory hint=
s and
>> + =C2=A0 =C2=A0 =C2=A0* wiphy_apply_custom_regulatory().
>> + =C2=A0 =C2=A0 =C2=A0*/
>> + =C2=A0 =C2=A0 flush_scheduled_work();
>> +
>
> Umm, this is a little stupid, first you queue it and then you flush t=
he
> workqueue... Why not call the function that runs off the workqueue
> directly?

That was the original approach -- but then we went on a bagwagon on
getting all these regulatory hints through the workqueue. I can
address for for wireless-testing but not sure what path we prefer for
stable. The above one liner seems small enough and would be nice to
get confirmation from a user on stable.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Wed, 2009-05-13 at 15:29 -0700, Luis R. Rodriguez wrote:

> >>       queue_regulatory_request(request);
> >>
> >> +     /*
> >> +      * This ensures last_request is populated once modules
> >> +      * come swinging in and calling regulatory hints and
> >> +      * wiphy_apply_custom_regulatory().
> >> +      */
> >> +     flush_scheduled_work();
> >> +
> >
> > Umm, this is a little stupid, first you queue it and then you flush the
> > workqueue... Why not call the function that runs off the workqueue
> > directly?
> 
> That was the original approach -- but then we went on a bagwagon on
> getting all these regulatory hints through the workqueue. I can
> address for for wireless-testing but not sure what path we prefer for
> stable. The above one liner seems small enough and would be nice to
> get confirmation from a user on stable.

Yeah definitely the way to go for stable, and we can als put it into
wireless-testing. But I think ultimately we should just refactor what is
necessary and just call the function that would otherwise run off the
workqueue. Can do that after this patch though.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[Luis R. Rodriguez]

On Wed, May 13, 2009 at 3:35 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2009-05-13 at 15:29 -0700, Luis R. Rodriguez wrote:
>
>> >> =C2=A0 =C2=A0 =C2=A0 queue_regulatory_request(request);
>> >>
>> >> + =C2=A0 =C2=A0 /*
>> >> + =C2=A0 =C2=A0 =C2=A0* This ensures last_request is populated on=
ce modules
>> >> + =C2=A0 =C2=A0 =C2=A0* come swinging in and calling regulatory h=
ints and
>> >> + =C2=A0 =C2=A0 =C2=A0* wiphy_apply_custom_regulatory().
>> >> + =C2=A0 =C2=A0 =C2=A0*/
>> >> + =C2=A0 =C2=A0 flush_scheduled_work();
>> >> +
>> >
>> > Umm, this is a little stupid, first you queue it and then you flus=
h the
>> > workqueue... Why not call the function that runs off the workqueue
>> > directly?
>>
>> That was the original approach -- but then we went on a bagwagon on
>> getting all these regulatory hints through the workqueue. I can
>> address for for wireless-testing but not sure what path we prefer fo=
r
>> stable. The above one liner seems small enough and would be nice to
>> get confirmation from a user on stable.
>
> Yeah definitely the way to go for stable, and we can als put it into
> wireless-testing. But I think ultimately we should just refactor what=
 is
> necessary and just call the function that would otherwise run off the
> workqueue. Can do that after this patch though.

OK -- will work on that next.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[Luis R. Rodriguez]

On Wed, May 13, 2009 at 3:37 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> On Wed, May 13, 2009 at 3:35 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Wed, 2009-05-13 at 15:29 -0700, Luis R. Rodriguez wrote:
>>
>>> >>       queue_regulatory_request(request);
>>> >>
>>> >> +     /*
>>> >> +      * This ensures last_request is populated once modules
>>> >> +      * come swinging in and calling regulatory hints and
>>> >> +      * wiphy_apply_custom_regulatory().
>>> >> +      */
>>> >> +     flush_scheduled_work();
>>> >> +
>>> >
>>> > Umm, this is a little stupid, first you queue it and then you flush the
>>> > workqueue... Why not call the function that runs off the workqueue
>>> > directly?
>>>
>>> That was the original approach -- but then we went on a bagwagon on
>>> getting all these regulatory hints through the workqueue. I can
>>> address for for wireless-testing but not sure what path we prefer for
>>> stable. The above one liner seems small enough and would be nice to
>>> get confirmation from a user on stable.
>>
>> Yeah definitely the way to go for stable, and we can als put it into
>> wireless-testing. But I think ultimately we should just refactor what is
>> necessary and just call the function that would otherwise run off the
>> workqueue. Can do that after this patch though.
>
> OK -- will work on that next.

BTW please feel free to add:

Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

  Luis

Re: [PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[John W. Linville]

On Wed, May 13, 2009 at 05:04:42PM -0400, Luis R. Rodriguez wrote:
> Its possible for cfg80211 to have scheduled the work and for
> the global workqueue to not have kicked in prior to a cfg80211
> driver's regulatory hint or wiphy_apply_custom_regulatory().
> 
> Although this is very unlikely its possible and should fix
> this race. When this race would happen you are expected to have
> hit a null pointer dereference panic.
> 
> Cc: stable@kernel.org
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>

(Same questions as for 3/4...)

What is the effect of this race?  What justifies this for 2.6.30
and/or stable?  It is getting late in the cycle for 2.6.30...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 4/4] cfg80211: fix race between core hint and driver's custom apply

[Luis R. Rodriguez]

On Mon, May 18, 2009 at 11:47:18AM -0700, John W. Linville wrote:
> On Wed, May 13, 2009 at 05:04:42PM -0400, Luis R. Rodriguez wrote:
> > Its possible for cfg80211 to have scheduled the work and for
> > the global workqueue to not have kicked in prior to a cfg80211
> > driver's regulatory hint or wiphy_apply_custom_regulatory().
> >
> > Although this is very unlikely its possible and should fix
> > this race. When this race would happen you are expected to have
> > hit a null pointer dereference panic.
> >
> > Cc: stable@kernel.org
> > Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> 
> (Same questions as for 3/4...)
> 
> What is the effect of this race?  What justifies this for 2.6.30
> and/or stable?  It is getting late in the cycle for 2.6.30...

It fixes an oops when your global workqueue is hammered during load
of ath9k, ath5k, or ar9170. I finally got confirmation from our team
it does fix the oops. Alan Jenkins also reported it fixed his oops.
He was getting an oops by adding the new RFKILL patches by johannes which
does add stuff ontop of the global workqueue.

  Luis
> 
> John
> --
> John W. Linville                Someday the world will need a hero, and you
> linville@tuxdriver.com                  might be all we have.  Be ready.