Userspace (wpa_supplicant) not notified on interface down with 2.6.29/wireless-testing

Userspace (wpa_supplicant) not notified on interface down with 2.6.29/wireless-testing

[Herton Ronaldo Krzesinski]

I have the problem when using 2.6.29 and judging by the code the situation
stays the same with wireless-testing, even after the fix "mac80211: deauth when
interface is marked down" (the fix doesn't work with kernel 2.6.28 or later, as
I'll explain).

The problem is, with latest kernels wpa_supplicant isn't still notified when
interface goes down, even after commit "mac80211: deauth when interface is
marked down" (e327b847 on Linus tree). There isn't a problem with this commit,
but because of other code changes it doesn't work on latest kernels (but works
if I apply same/similar change on 2.6.27 for example).

The issue is as follows: after commit "mac80211: restructure disassoc/deauth
flows" in 2.6.28, the call to ieee80211_sta_deauthenticate added by commit
e327b847 will not work: because we do sta_info_flush(local, sdata); inside
ieee80211_stop (iface.c), all stations in interface are cleared, so when calling
 ieee80211_sta_deauthenticate->ieee80211_set_disassoc (mlme.c), inside
ieee80211_set_disassoc we have this in the beginning:

        sta = sta_info_get(local, ifsta->bssid);
        if (!sta) {

the !sta check triggers, so the function returns early and
ieee80211_sta_send_apinfo(sdata, ifsta); later isn't called, so wpa_supplicant
isn't notified with SIOCGIWAP

One possible fix is moving sta_info_flush, to avoid !sta, and seems good, like
the following:

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 2acc416..0929581 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -370,25 +370,6 @@ static int ieee80211_stop(struct net_device *dev)
 	rcu_read_unlock();
 
 	/*
-	 * Remove all stations associated with this interface.
-	 *
-	 * This must be done before calling ops->remove_interface()
-	 * because otherwise we can later invoke ops->sta_notify()
-	 * whenever the STAs are removed, and that invalidates driver
-	 * assumptions about always getting a vif pointer that is valid
-	 * (because if we remove a STA after ops->remove_interface()
-	 * the driver will have removed the vif info already!)
-	 *
-	 * We could relax this and only unlink the stations from the
-	 * hash table and list but keep them on a per-sdata list that
-	 * will be inserted back again when the interface is brought
-	 * up again, but I don't currently see a use case for that,
-	 * except with WDS which gets a STA entry created when it is
-	 * brought up.
-	 */
-	sta_info_flush(local, sdata);
-
-	/*
 	 * Don't count this interface for promisc/allmulti while it
 	 * is down. dev_mc_unsync() will invoke set_multicast_list
 	 * on the master interface which will sync these down to the
@@ -539,6 +520,26 @@ static int ieee80211_stop(struct net_device *dev)
 		conf.mac_addr = dev->dev_addr;
 		/* disable all keys for as long as this netdev is down */
 		ieee80211_disable_keys(sdata);
+
+		/*
+		 * Remove all stations associated with this interface.
+		 *
+		 * This must be done before calling ops->remove_interface()
+		 * because otherwise we can later invoke ops->sta_notify()
+		 * whenever the STAs are removed, and that invalidates driver
+		 * assumptions about always getting a vif pointer that is valid
+		 * (because if we remove a STA after ops->remove_interface()
+		 * the driver will have removed the vif info already!)
+		 *
+		 * We could relax this and only unlink the stations from the
+		 * hash table and list but keep them on a per-sdata list that
+		 * will be inserted back again when the interface is brought
+		 * up again, but I don't currently see a use case for that,
+		 * except with WDS which gets a STA entry created when it is
+		 * brought up.
+		 */
+		sta_info_flush(local, sdata);
+
 		local->ops->remove_interface(local_to_hw(local), &conf);
 	}
 
My question is, is the patch above safe? that is, do we need to always
call sta_info_flush on interface down like currently, or just for the case we
call remove_interface, like the comment states? if the latter, patch above
is good, and solves the wpa_supplicant/userspace issue.

--
[]'s
Herton.

Re: Userspace (wpa_supplicant) not notified on interface down with 2.6.29/wireless-testing

[Johannes Berg]

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

On Mon, 2009-03-09 at 18:50 -0300, Herton Ronaldo Krzesinski wrote:
> I have the problem when using 2.6.29 and judging by the code the situation
> stays the same with wireless-testing, even after the fix "mac80211: deauth when
> interface is marked down" (the fix doesn't work with kernel 2.6.28 or later, as
> I'll explain).
> 
> The problem is, with latest kernels wpa_supplicant isn't still notified when
> interface goes down, even after commit "mac80211: deauth when interface is
> marked down" (e327b847 on Linus tree). There isn't a problem with this commit,
> but because of other code changes it doesn't work on latest kernels (but works
> if I apply same/similar change on 2.6.27 for example).
> 
> The issue is as follows: after commit "mac80211: restructure disassoc/deauth
> flows" in 2.6.28, the call to ieee80211_sta_deauthenticate added by commit
> e327b847 will not work: because we do sta_info_flush(local, sdata); inside
> ieee80211_stop (iface.c), all stations in interface are cleared, so when calling
>  ieee80211_sta_deauthenticate->ieee80211_set_disassoc (mlme.c), inside
> ieee80211_set_disassoc we have this in the beginning:
> 
>         sta = sta_info_get(local, ifsta->bssid);
>         if (!sta) {
> 
> the !sta check triggers, so the function returns early and
> ieee80211_sta_send_apinfo(sdata, ifsta); later isn't called, so wpa_supplicant
> isn't notified with SIOCGIWAP

Interesting problem.

> One possible fix is moving sta_info_flush, to avoid !sta, and seems good, like
> the following:
> 
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 2acc416..0929581 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -370,25 +370,6 @@ static int ieee80211_stop(struct net_device *dev)
>  	rcu_read_unlock();
>  
>  	/*
> -	 * Remove all stations associated with this interface.
> -	 *
> -	 * This must be done before calling ops->remove_interface()
> -	 * because otherwise we can later invoke ops->sta_notify()
> -	 * whenever the STAs are removed, and that invalidates driver
> -	 * assumptions about always getting a vif pointer that is valid
> -	 * (because if we remove a STA after ops->remove_interface()
> -	 * the driver will have removed the vif info already!)
> -	 *
> -	 * We could relax this and only unlink the stations from the
> -	 * hash table and list but keep them on a per-sdata list that
> -	 * will be inserted back again when the interface is brought
> -	 * up again, but I don't currently see a use case for that,
> -	 * except with WDS which gets a STA entry created when it is
> -	 * brought up.
> -	 */
> -	sta_info_flush(local, sdata);
> -
> -	/*
>  	 * Don't count this interface for promisc/allmulti while it
>  	 * is down. dev_mc_unsync() will invoke set_multicast_list
>  	 * on the master interface which will sync these down to the
> @@ -539,6 +520,26 @@ static int ieee80211_stop(struct net_device *dev)
>  		conf.mac_addr = dev->dev_addr;
>  		/* disable all keys for as long as this netdev is down */
>  		ieee80211_disable_keys(sdata);
> +
> +		/*
> +		 * Remove all stations associated with this interface.
> +		 *
> +		 * This must be done before calling ops->remove_interface()
> +		 * because otherwise we can later invoke ops->sta_notify()
> +		 * whenever the STAs are removed, and that invalidates driver
> +		 * assumptions about always getting a vif pointer that is valid
> +		 * (because if we remove a STA after ops->remove_interface()
> +		 * the driver will have removed the vif info already!)
> +		 *
> +		 * We could relax this and only unlink the stations from the
> +		 * hash table and list but keep them on a per-sdata list that
> +		 * will be inserted back again when the interface is brought
> +		 * up again, but I don't currently see a use case for that,
> +		 * except with WDS which gets a STA entry created when it is
> +		 * brought up.
> +		 */
> +		sta_info_flush(local, sdata);
> +
>  		local->ops->remove_interface(local_to_hw(local), &conf);
>  	}
>  
> My question is, is the patch above safe? that is, do we need to always
> call sta_info_flush on interface down like currently, or just for the case we
> call remove_interface, like the comment states? if the latter, patch above
> is good, and solves the wpa_supplicant/userspace issue.

The comment states it must be before remove_interface, not that it
doesn't need to be called unless that is called, and this code moving is
incorrect for AP_VLAN devices.

You could move the disassoc part out of the switch to before this.

johannes

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

Re: Userspace (wpa_supplicant) not notified on interface down with 2.6.29/wireless-testing

[Herton Ronaldo Krzesinski]

Em Ter=E7a-feira 10 Mar=E7o 2009, =E0s 04:13:34, Johannes Berg escreveu=
:
> On Mon, 2009-03-09 at 18:50 -0300, Herton Ronaldo Krzesinski wrote:
> > I have the problem when using 2.6.29 and judging by the code the si=
tuation
> > stays the same with wireless-testing, even after the fix "mac80211:=
 deauth when
> > interface is marked down" (the fix doesn't work with kernel 2.6.28 =
or later, as
> > I'll explain).
> >=20
> > The problem is, with latest kernels wpa_supplicant isn't still noti=
fied when
> > interface goes down, even after commit "mac80211: deauth when inter=
face is
> > marked down" (e327b847 on Linus tree). There isn't a problem with t=
his commit,
> > but because of other code changes it doesn't work on latest kernels=
 (but works
> > if I apply same/similar change on 2.6.27 for example).
> >=20
> > The issue is as follows: after commit "mac80211: restructure disass=
oc/deauth
> > flows" in 2.6.28, the call to ieee80211_sta_deauthenticate added by=
 commit
> > e327b847 will not work: because we do sta_info_flush(local, sdata);=
 inside
> > ieee80211_stop (iface.c), all stations in interface are cleared, so=
 when calling
> >  ieee80211_sta_deauthenticate->ieee80211_set_disassoc (mlme.c), ins=
ide
> > ieee80211_set_disassoc we have this in the beginning:
> >=20
> >         sta =3D sta_info_get(local, ifsta->bssid);
> >         if (!sta) {
> >=20
> > the !sta check triggers, so the function returns early and
> > ieee80211_sta_send_apinfo(sdata, ifsta); later isn't called, so wpa=
_supplicant
> > isn't notified with SIOCGIWAP
>=20
> Interesting problem.
>=20
> > One possible fix is moving sta_info_flush, to avoid !sta, and seems=
 good, like
> > the following:
> >=20
> > diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> > index 2acc416..0929581 100644
> > --- a/net/mac80211/iface.c
> > +++ b/net/mac80211/iface.c
> > @@ -370,25 +370,6 @@ static int ieee80211_stop(struct net_device *d=
ev)
> >  	rcu_read_unlock();
> > =20
> >  	/*
> > -	 * Remove all stations associated with this interface.
> > -	 *
> > -	 * This must be done before calling ops->remove_interface()
> > -	 * because otherwise we can later invoke ops->sta_notify()
> > -	 * whenever the STAs are removed, and that invalidates driver
> > -	 * assumptions about always getting a vif pointer that is valid
> > -	 * (because if we remove a STA after ops->remove_interface()
> > -	 * the driver will have removed the vif info already!)
> > -	 *
> > -	 * We could relax this and only unlink the stations from the
> > -	 * hash table and list but keep them on a per-sdata list that
> > -	 * will be inserted back again when the interface is brought
> > -	 * up again, but I don't currently see a use case for that,
> > -	 * except with WDS which gets a STA entry created when it is
> > -	 * brought up.
> > -	 */
> > -	sta_info_flush(local, sdata);
> > -
> > -	/*
> >  	 * Don't count this interface for promisc/allmulti while it
> >  	 * is down. dev_mc_unsync() will invoke set_multicast_list
> >  	 * on the master interface which will sync these down to the
> > @@ -539,6 +520,26 @@ static int ieee80211_stop(struct net_device *d=
ev)
> >  		conf.mac_addr =3D dev->dev_addr;
> >  		/* disable all keys for as long as this netdev is down */
> >  		ieee80211_disable_keys(sdata);
> > +
> > +		/*
> > +		 * Remove all stations associated with this interface.
> > +		 *
> > +		 * This must be done before calling ops->remove_interface()
> > +		 * because otherwise we can later invoke ops->sta_notify()
> > +		 * whenever the STAs are removed, and that invalidates driver
> > +		 * assumptions about always getting a vif pointer that is valid
> > +		 * (because if we remove a STA after ops->remove_interface()
> > +		 * the driver will have removed the vif info already!)
> > +		 *
> > +		 * We could relax this and only unlink the stations from the
> > +		 * hash table and list but keep them on a per-sdata list that
> > +		 * will be inserted back again when the interface is brought
> > +		 * up again, but I don't currently see a use case for that,
> > +		 * except with WDS which gets a STA entry created when it is
> > +		 * brought up.
> > +		 */
> > +		sta_info_flush(local, sdata);
> > +
> >  		local->ops->remove_interface(local_to_hw(local), &conf);
> >  	}
> > =20
> > My question is, is the patch above safe? that is, do we need to alw=
ays
> > call sta_info_flush on interface down like currently, or just for t=
he case we
> > call remove_interface, like the comment states? if the latter, patc=
h above
> > is good, and solves the wpa_supplicant/userspace issue.
>=20
> The comment states it must be before remove_interface, not that it
> doesn't need to be called unless that is called, and this code moving=
 is
> incorrect for AP_VLAN devices.
>=20
> You could move the disassoc part out of the switch to before this.

Ok, here is the new patch, if it's ok please apply (I can resubmit with=
 patch
in subject/standalone message if necessary):

=46rom eb6f79597b9e6af8a3618873a781fd6ebcbd50a0 Mon Sep 17 00:00:00 200=
1
=46rom: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Date: Tue, 10 Mar 2009 09:44:17 -0300
Subject: [PATCH] mac80211: deauth before flushing STA information

Even after commit "mac80211: deauth when interface is marked down"
(e327b847 on Linus tree), userspace still isn't notified when interface
goes down. There isn't a problem with this commit, but because of other
code changes it doesn't work on kernels >=3D 2.6.28 (works if same/simi=
lar
change applied on 2.6.27 for example).

The issue is as follows: after commit "mac80211: restructure disassoc/d=
eauth
flows" in 2.6.28, the call to ieee80211_sta_deauthenticate added by
commit e327b847 will not work: because we do sta_info_flush(local, sdat=
a)
inside ieee80211_stop (iface.c), all stations in interface are cleared,=
 so
when calling ieee80211_sta_deauthenticate->ieee80211_set_disassoc (mlme=
=2Ec),
inside ieee80211_set_disassoc we have this in the beginning:

         sta =3D sta_info_get(local, ifsta->bssid);
         if (!sta) {

The !sta check triggers, thus the function returns early and
ieee80211_sta_send_apinfo(sdata, ifsta) later isn't called, so
wpa_supplicant/userspace isn't notified with SIOCGIWAP.

This commit moves deauthentication to before flushing STA info
(sta_info_flush), thus the above can't happen and userspace is really
notified when interface goes down.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
---
 net/mac80211/iface.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 2acc416..f9f27b9 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -370,6 +370,18 @@ static int ieee80211_stop(struct net_device *dev)
 	rcu_read_unlock();
=20
 	/*
+	 * Announce that we are leaving the network, in case we are a
+	 * station interface type. This must be done before removing
+	 * all stations associated with sta_info_flush, otherwise STA
+	 * information will be gone and no announce being done.
+	 */
+	if (sdata->vif.type =3D=3D NL80211_IFTYPE_STATION) {
+		if (sdata->u.mgd.state !=3D IEEE80211_STA_MLME_DISABLED)
+			ieee80211_sta_deauthenticate(sdata,
+				WLAN_REASON_DEAUTH_LEAVING);
+	}
+
+	/*
 	 * Remove all stations associated with this interface.
 	 *
 	 * This must be done before calling ops->remove_interface()
@@ -454,10 +466,6 @@ static int ieee80211_stop(struct net_device *dev)
 		netif_addr_unlock_bh(local->mdev);
 		break;
 	case NL80211_IFTYPE_STATION:
-		/* Announce that we are leaving the network. */
-		if (sdata->u.mgd.state !=3D IEEE80211_STA_MLME_DISABLED)
-			ieee80211_sta_deauthenticate(sdata,
-						WLAN_REASON_DEAUTH_LEAVING);
 		memset(sdata->u.mgd.bssid, 0, ETH_ALEN);
 		del_timer_sync(&sdata->u.mgd.chswitch_timer);
 		del_timer_sync(&sdata->u.mgd.timer);
--=20
1.6.2

>=20
> johannes
>=20

--
[]'s
Herton
--
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