SIOCGIWSCAN-race

SIOCGIWSCAN-race

[Lars Ericsson]

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

Hi Johannes,

I have discovered and patched a race in the scanning function since a couple
of releases.
To day I checked the current Linux git and the problem is still there.

The problem is the sequence of events when the scan result is reported back.
The wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL); 
is called before ieee80211_hw_config(local);

ieee80211_hw_config(local) will trig the wpa_supplicant to select an AP. 
That may happen before the ieee80211_hw_config() is executed since the
wpa_supplicant
generated actions is executed by an other thread (wpa_supplicant).

The result is that:
- wpa_supplicant setup for an association to an ap using correct channel.
- ieee80211_hw_config() reset the channel to the value before the SCAN
started.
- the association request will be sent out using the wrong channel.


Attached you will find the patch for 2.6.27. 
It is not a perfect patch since the code is duplicated but it works :)


Regards
Lars

[-- Attachment #2: lae-mac80211-mlme-SIOCGIWSCAN-race.patch --]
[-- Type: application/octet-stream, Size: 1264 bytes --]

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
old mode 100644
new mode 100755
index b404537..fd9e726
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -3788,10 +3788,6 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
 	struct ieee80211_sub_if_data *sdata;
 	union iwreq_data wrqu;
 
-	local->last_scan_completed = jiffies;
-	memset(&wrqu, 0, sizeof(wrqu));
-	wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL);
-
 	if (local->sta_hw_scanning) {
 		local->sta_hw_scanning = 0;
 		if (ieee80211_hw_config(local))
@@ -3803,6 +3799,10 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
 			ieee80211_restart_sta_timer(sdata);
 		rcu_read_unlock();
 
+		local->last_scan_completed = jiffies;
+		memset(&wrqu, 0, sizeof(wrqu));
+		wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL);
+
 		goto done;
 	}
 
@@ -3811,6 +3811,9 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
 		printk(KERN_DEBUG "%s: failed to restore operational "
 		       "channel after scan\n", dev->name);
 
+	local->last_scan_completed = jiffies;
+	memset(&wrqu, 0, sizeof(wrqu));
+	wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL);
 
 	netif_tx_lock_bh(local->mdev);
 	local->filter_flags &= ~FIF_BCN_PRBRESP_PROMISC;

Re: SIOCGIWSCAN-race

[John W. Linville]

On Sat, Feb 21, 2009 at 08:33:06AM +0100, Lars Ericsson wrote:
> Hi Johannes,
> 
> I have discovered and patched a race in the scanning function since a couple
> of releases.
> To day I checked the current Linux git and the problem is still there.
> 
> The problem is the sequence of events when the scan result is reported back.
> The wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL); 
> is called before ieee80211_hw_config(local);
> 
> ieee80211_hw_config(local) will trig the wpa_supplicant to select an AP. 
> That may happen before the ieee80211_hw_config() is executed since the
> wpa_supplicant
> generated actions is executed by an other thread (wpa_supplicant).
> 
> The result is that:
> - wpa_supplicant setup for an association to an ap using correct channel.
> - ieee80211_hw_config() reset the channel to the value before the SCAN
> started.
> - the association request will be sent out using the wrong channel.
> 
> 
> Attached you will find the patch for 2.6.27. 
> It is not a perfect patch since the code is duplicated but it works :)

Looks like the patch would need to be reworked -- the code in 2.6.29
and later is different.  Also, any reason we can't just move the
wireless_send_event() down to done:?

Still, this doesn't feel quite right.  Shouldn't we be able to queue
the userland-driven channel change until after the scan completes?

John

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index f5c7c33..8c13a91 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -437,15 +437,6 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
 	local->last_scan_completed = jiffies;
 	memset(&wrqu, 0, sizeof(wrqu));
 
-	/*
-	 * local->scan_sdata could have been NULLed by the interface
-	 * down code in case we were scanning on an interface that is
-	 * being taken down.
-	 */
-	sdata = local->scan_sdata;
-	if (sdata)
-		wireless_send_event(sdata->dev, SIOCGIWSCAN, &wrqu, NULL);
-
 	if (local->hw_scanning) {
 		local->hw_scanning = false;
 		/*
@@ -486,6 +477,15 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
 	rcu_read_unlock();
 
  done:
+	/*
+	 * local->scan_sdata could have been NULLed by the interface
+	 * down code in case we were scanning on an interface that is
+	 * being taken down.
+	 */
+	sdata = local->scan_sdata;
+	if (sdata)
+		wireless_send_event(sdata->dev, SIOCGIWSCAN, &wrqu, NULL);
+
 	ieee80211_mlme_notify_scan_completed(local);
 	ieee80211_mesh_notify_scan_completed(local);
 }

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

Re: SIOCGIWSCAN-race

[Johannes Berg]

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

On Mon, 2009-02-23 at 16:11 -0500, John W. Linville wrote:
> On Sat, Feb 21, 2009 at 08:33:06AM +0100, Lars Ericsson wrote:
> > Hi Johannes,
> > 
> > I have discovered and patched a race in the scanning function since a couple
> > of releases.
> > To day I checked the current Linux git and the problem is still there.
> > 
> > The problem is the sequence of events when the scan result is reported back.
> > The wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL); 
> > is called before ieee80211_hw_config(local);
> > 
> > ieee80211_hw_config(local) will trig the wpa_supplicant to select an AP. 
> > That may happen before the ieee80211_hw_config() is executed since the
> > wpa_supplicant
> > generated actions is executed by an other thread (wpa_supplicant).
> > 
> > The result is that:
> > - wpa_supplicant setup for an association to an ap using correct channel.
> > - ieee80211_hw_config() reset the channel to the value before the SCAN
> > started.
> > - the association request will be sent out using the wrong channel.
> > 
> > 
> > Attached you will find the patch for 2.6.27. 
> > It is not a perfect patch since the code is duplicated but it works :)
> 
> Looks like the patch would need to be reworked -- the code in 2.6.29
> and later is different.  

and the new code with cfg80211 is even more different ;)

> Also, any reason we can't just move the
> wireless_send_event() down to done:?

seems fine

> Still, this doesn't feel quite right.  Shouldn't we be able to queue
> the userland-driven channel change until after the scan completes?

?
we do that, well, we set it here:
        local->sw_scanning = false;
        ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);

but we claim to be on the user requested channel at all times, so that
should be ok

johannes

> John
> 
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index f5c7c33..8c13a91 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -437,15 +437,6 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
>  	local->last_scan_completed = jiffies;
>  	memset(&wrqu, 0, sizeof(wrqu));
>  
> -	/*
> -	 * local->scan_sdata could have been NULLed by the interface
> -	 * down code in case we were scanning on an interface that is
> -	 * being taken down.
> -	 */
> -	sdata = local->scan_sdata;
> -	if (sdata)
> -		wireless_send_event(sdata->dev, SIOCGIWSCAN, &wrqu, NULL);
> -
>  	if (local->hw_scanning) {
>  		local->hw_scanning = false;
>  		/*
> @@ -486,6 +477,15 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
>  	rcu_read_unlock();
>  
>   done:
> +	/*
> +	 * local->scan_sdata could have been NULLed by the interface
> +	 * down code in case we were scanning on an interface that is
> +	 * being taken down.
> +	 */
> +	sdata = local->scan_sdata;
> +	if (sdata)
> +		wireless_send_event(sdata->dev, SIOCGIWSCAN, &wrqu, NULL);
> +
>  	ieee80211_mlme_notify_scan_completed(local);
>  	ieee80211_mesh_notify_scan_completed(local);
>  }
> 

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

RE: SIOCGIWSCAN-race

[Lars Ericsson]

> On Mon, 2009-02-23 at 16:11 -0500, John W. Linville wrote:
> > On Sat, Feb 21, 2009 at 08:33:06AM +0100, Lars Ericsson wrote:
> > > Hi Johannes,
> > > 
> > > I have discovered and patched a race in the scanning function since a
couple
> > > of releases.
> > > To day I checked the current Linux git and the problem is still there.
> > > 
> > > The problem is the sequence of events when the scan result is reported
back.
> > > The wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL); 
> > > is called before ieee80211_hw_config(local);
> > > 
> > > ieee80211_hw_config(local) will trig the wpa_supplicant to select an
AP. 
> > > That may happen before the ieee80211_hw_config() is executed since the
> > > wpa_supplicant
> > > generated actions is executed by an other thread (wpa_supplicant).
> > > 
> > > The result is that:
> > > - wpa_supplicant setup for an association to an ap using correct
channel.
> > > - ieee80211_hw_config() reset the channel to the value before the SCAN
started.
> > > - the association request will be sent out using the wrong channel.
> > > 
> > > 
> > > Attached you will find the patch for 2.6.27. 
> > > It is not a perfect patch since the code is duplicated but it works :)
> > 
> > Looks like the patch would need to be reworked -- the code in 2.6.29
> > and later is different.  
> 
> and the new code with cfg80211 is even more different ;)
> 

OK, I have not moved in to the 'cfg80211' world, yet.
Johannes; do you create the patch for this ?
 
> > Also, any reason we can't just move the
> > wireless_send_event() down to done:?
> 
> seems fine

Perfect, is it to late for 2.6.29 ?

Lars

> > Still, this doesn't feel quite right.  Shouldn't we be able to queue
> > the userland-driven channel change until after the scan completes?
> 
> ?
> we do that, well, we set it here:
>         local->sw_scanning = false;
>         ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
> 
> but we claim to be on the user requested channel at all times, so that
> should be ok
> 
> johannes
> 
> > John
> > 
> > diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> > index f5c7c33..8c13a91 100644
> > --- a/net/mac80211/scan.c
> > +++ b/net/mac80211/scan.c
> > @@ -437,15 +437,6 @@ void ieee80211_scan_completed(struct 
> ieee80211_hw *hw)
> >  	local->last_scan_completed = jiffies;
> >  	memset(&wrqu, 0, sizeof(wrqu));
> >  
> > -	/*
> > -	 * local->scan_sdata could have been NULLed by the interface
> > -	 * down code in case we were scanning on an interface that is
> > -	 * being taken down.
> > -	 */
> > -	sdata = local->scan_sdata;
> > -	if (sdata)
> > -		wireless_send_event(sdata->dev, SIOCGIWSCAN, 
> &wrqu, NULL);
> > -
> >  	if (local->hw_scanning) {
> >  		local->hw_scanning = false;
> >  		/*
> > @@ -486,6 +477,15 @@ void ieee80211_scan_completed(struct 
> ieee80211_hw *hw)
> >  	rcu_read_unlock();
> >  
> >   done:
> > +	/*
> > +	 * local->scan_sdata could have been NULLed by the interface
> > +	 * down code in case we were scanning on an interface that is
> > +	 * being taken down.
> > +	 */
> > +	sdata = local->scan_sdata;
> > +	if (sdata)
> > +		wireless_send_event(sdata->dev, SIOCGIWSCAN, 
> &wrqu, NULL);
> > +
> >  	ieee80211_mlme_notify_scan_completed(local);
> >  	ieee80211_mesh_notify_scan_completed(local);
> >  }
> > 
>