[PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Zhu Yi]

I'm not sure if this is best choice, someone might have better
solutions. But this patch fixed the connection problem when switching
from a WPA enabled AP (using wpa_supplicant) to an open AP (using
iwconfig). The root cause is when we connect to a WPA enabled AP,
wpa_supplicant sets the ifsta->extra_ie thru SIOCSIWGENIE. But if we
stop wpa_supplicant and connect to an open AP with iwconfig, there is
no way to clear the extra_ie so that mac80211 keeps connecting with that.

Someone could argue wpa_supplicant should clear the extra_ie during
its shutdown. But mac80211 should also handle the unexpected shutdown
case (ie. killall -9 wpa_supplicant).

Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
 net/mac80211/ieee80211.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index e0ee65a..7cf2f14 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -334,6 +334,10 @@ static int ieee80211_stop(struct net_device *dev)
 			cancel_delayed_work(&local->scan_work);
 		}
 		flush_workqueue(local->hw.workqueue);
+
+		kfree(sdata->u.sta.extra_ie);
+		sdata->u.sta.extra_ie = NULL;
+		sdata->u.sta.extra_ie_len = 0;
 		/* fall through */
 	default:
 		conf.if_id = dev->ifindex;
-- 
1.5.2

Re: [PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Johannes Berg]

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


On Tue, 2007-11-20 at 14:55 +0800, Zhu Yi wrote:
> I'm not sure if this is best choice, someone might have better
> solutions. But this patch fixed the connection problem when switching
> from a WPA enabled AP (using wpa_supplicant) to an open AP (using
> iwconfig). The root cause is when we connect to a WPA enabled AP,
> wpa_supplicant sets the ifsta->extra_ie thru SIOCSIWGENIE. But if we
> stop wpa_supplicant and connect to an open AP with iwconfig, there is
> no way to clear the extra_ie so that mac80211 keeps connecting with that.

Fun.

> Someone could argue wpa_supplicant should clear the extra_ie during
> its shutdown. But mac80211 should also handle the unexpected shutdown
> case (ie. killall -9 wpa_supplicant).

Yes. But if you do that, the interface won't be down either. Maybe this
should also be done when removing keys?

johannes

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

Re: [PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Dan Williams]

On Tue, 2007-11-20 at 14:37 +0100, Johannes Berg wrote:
> On Tue, 2007-11-20 at 14:55 +0800, Zhu Yi wrote:
> > I'm not sure if this is best choice, someone might have better
> > solutions. But this patch fixed the connection problem when switching
> > from a WPA enabled AP (using wpa_supplicant) to an open AP (using
> > iwconfig). The root cause is when we connect to a WPA enabled AP,
> > wpa_supplicant sets the ifsta->extra_ie thru SIOCSIWGENIE. But if we
> > stop wpa_supplicant and connect to an open AP with iwconfig, there is
> > no way to clear the extra_ie so that mac80211 keeps connecting with that.
> 
> Fun.
> 
> > Someone could argue wpa_supplicant should clear the extra_ie during
> > its shutdown. But mac80211 should also handle the unexpected shutdown
> > case (ie. killall -9 wpa_supplicant).
> 
> Yes. But if you do that, the interface won't be down either. Maybe this
> should also be done when removing keys?

That seems better to me; although couldn't it also be cleared when
setting static WEP keys?  Dynamic WEP shouldn't care about the IE bits
either, so any time a WEP key is set and you're _not_ doing WPA the IE
could get cleared.

Though on the other hand, the interface isn't going to work much longer
after wpa_supplicant goes away anyway, depending on how long the
group/multicast rekey interval is.

Dan

Re: [PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Johannes Berg]

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


> > Yes. But if you do that, the interface won't be down either. Maybe this
> > should also be done when removing keys?
> 
> That seems better to me; although couldn't it also be cleared when
> setting static WEP keys?  Dynamic WEP shouldn't care about the IE bits
> either, so any time a WEP key is set and you're _not_ doing WPA the IE
> could get cleared.

Yeah, this is a bit icky. Also, we need to reset the state of the "use
key management" variable.

> Though on the other hand, the interface isn't going to work much longer
> after wpa_supplicant goes away anyway, depending on how long the
> group/multicast rekey interval is.

Actually, we're talking about getting it to work on another AP anyway.

johannes

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

Re: [PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Zhu Yi]

On Tue, 2007-11-20 at 14:37 +0100, Johannes Berg wrote:
> Yes. But if you do that, the interface won't be down either. 

Indeed. But at least user has a way to let mac80211 forget the previous
setting (ifconfig wlan0 down and up). Otherwise, user has to reload the
modules to achieve this.

> Maybe this should also be done when removing keys?

I agree there are more places we need to clear the setting. Putting it
to ieee80211_stop() is a good start. Should we have a "reset" like
ioctl? So the second config tool can call it to let mac80211 forget all
the previous setting.

Thanks,
-yi

Re: [PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Johannes Berg]

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

Hi,

> Indeed. But at least user has a way to let mac80211 forget the previous
> setting (ifconfig wlan0 down and up). Otherwise, user has to reload the
> modules to achieve this.

Yeah. Can you amend the patch to also clear the
IEEE80211_STA_PRIVACY_INVOKED flag?

> > Maybe this should also be done when removing keys?
> 
> I agree there are more places we need to clear the setting. Putting it
> to ieee80211_stop() is a good start. Should we have a "reset" like
> ioctl? So the second config tool can call it to let mac80211 forget all
> the previous setting.

No. We're not adding any ioctls, we now have to live with the wext crap.

What I'm going to do in nl80211 is put *all* the required setting into
an "association" object. Then all settings can be deleted together when
the association object is removed which will probably be done by
userland tools automatigally when tell them to associate to something
else, i.e. add a new association (needs to remove the old because there
can be only one).

johannes

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

Re: [PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Zhu Yi]

On Wed, 2007-11-21 at 16:19 +0100, Johannes Berg wrote:
> Yeah. Can you amend the patch to also clear the
> IEEE80211_STA_PRIVACY_INVOKED flag?

Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---

diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 59350b8..69daf0a 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -357,6 +357,11 @@ static int ieee80211_stop(struct net_device *dev)
 			cancel_delayed_work(&local->scan_work);
 		}
 		flush_workqueue(local->hw.workqueue);
+
+		sdata->u.sta.flags &= ~IEEE80211_STA_PRIVACY_INVOKED;
+		kfree(sdata->u.sta.extra_ie);
+		sdata->u.sta.extra_ie = NULL;
+		sdata->u.sta.extra_ie_len = 0;
 		/* fall through */
 	default:
 		conf.if_id = dev->ifindex;

Re: [PATCH] mac80211: free ifsta->extra_ie in ieee80211_stop

[Johannes Berg]

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


On Thu, 2007-11-22 at 11:10 +0800, Zhu Yi wrote:
> On Wed, 2007-11-21 at 16:19 +0100, Johannes Berg wrote:
> > Yeah. Can you amend the patch to also clear the
> > IEEE80211_STA_PRIVACY_INVOKED flag?
> 
> Signed-off-by: Zhu Yi <yi.zhu@intel.com>

And if you adjust the patch subject now and fill in the description you
have my
Acked-by: Johannes Berg <johannes@sipsolutions.net>

> ---
> 
> diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
> index 59350b8..69daf0a 100644
> --- a/net/mac80211/ieee80211.c
> +++ b/net/mac80211/ieee80211.c
> @@ -357,6 +357,11 @@ static int ieee80211_stop(struct net_device *dev)
>  			cancel_delayed_work(&local->scan_work);
>  		}
>  		flush_workqueue(local->hw.workqueue);
> +
> +		sdata->u.sta.flags &= ~IEEE80211_STA_PRIVACY_INVOKED;
> +		kfree(sdata->u.sta.extra_ie);
> +		sdata->u.sta.extra_ie = NULL;
> +		sdata->u.sta.extra_ie_len = 0;
>  		/* fall through */
>  	default:
>  		conf.if_id = dev->ifindex;
> 

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