Patchwork Use correct BSSID for deauth/disconnect in mac80211 EALREADY workaround

login
register
mail settings
Submitter Mykyta Iziumtsev
Date Sept. 26, 2012, 2:52 p.m.
Message ID <CAHc5FiVHh=wTEPjdYQnnCzXN4tLf1a0qJpikUMdm5sMgqQJM8w@mail.gmail.com>
Download mbox | patch
Permalink /patch/187093/
State Changes Requested
Headers show

Comments

Mykyta Iziumtsev - Sept. 26, 2012, 2:52 p.m.
If auth/connect request to mac80211 fails with EALREADY -
there'll be retry after forced deauth/disconnect. The problem is that
wrong BSSID is used in this deauth/disconnect request. Instead of
BSSID of currently associated AP - BSSID of new AP will be used.

For NL80211_CMD_DEAUTHENTICATE use correct cached BSSID value.

For NL80211_CMD_DISCONNECT BSSID is not needed at all,
because mac80211 uses locally saved value.

Signed-hostap: Mykyta Iziumtsev <mykyta.iziumtsev@gmail.com>
---
 src/drivers/driver_nl80211.c |   56 +++++++-----------------------------------
 1 file changed, 9 insertions(+), 47 deletions(-)


@@ -6843,7 +6805,7 @@ skip_auth_type:
 		 * disconnection.
 		 */
 		if (ret == -EALREADY)
-			nl80211_disconnect(drv, params->bssid);
+			nl80211_disconnect(drv);
 		goto nla_put_failure;
 	}
 	ret = 0;
Johannes Berg - Sept. 26, 2012, 3:16 p.m.
On Wed, 2012-09-26 at 16:52 +0200, Mykyta Iziumtsev wrote:
> If auth/connect request to mac80211 fails with EALREADY -
> there'll be retry after forced deauth/disconnect. The problem is that
> wrong BSSID is used in this deauth/disconnect request. Instead of
> BSSID of currently associated AP - BSSID of new AP will be used.
> 
> For NL80211_CMD_DEAUTHENTICATE use correct cached BSSID value.
> 
> For NL80211_CMD_DISCONNECT BSSID is not needed at all,
> because mac80211 uses locally saved value.

What kernel version are you testisng this against? The kernel behaviour
changed significantly in this area, recently authentication isn't
tracked in the kernel any more at all, only association.

johannes
Mykyta Iziumtsev - Sept. 26, 2012, 3:56 p.m.
Hi Johannes,

I'm using compat-wireless-3.2.5-1. I see that in nl80211_deauthenticate()
NL80211_ATTR_MAC is mandatory. The same in compat-wireless-3.6-rc6-1.

To clear possible misunderstanding:
>> For NL80211_CMD_DEAUTHENTICATE use correct cached BSSID value.
This caching is done in hostap.git: src/drivers/driver_nl80211.c ,
struct wpa_driver_nl80211_data->auth_bssid .

/Mykyta

On Wed, Sep 26, 2012 at 5:16 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2012-09-26 at 16:52 +0200, Mykyta Iziumtsev wrote:
>> If auth/connect request to mac80211 fails with EALREADY -
>> there'll be retry after forced deauth/disconnect. The problem is that
>> wrong BSSID is used in this deauth/disconnect request. Instead of
>> BSSID of currently associated AP - BSSID of new AP will be used.
>>
>> For NL80211_CMD_DEAUTHENTICATE use correct cached BSSID value.
>>
>> For NL80211_CMD_DISCONNECT BSSID is not needed at all,
>> because mac80211 uses locally saved value.
>
> What kernel version are you testisng this against? The kernel behaviour
> changed significantly in this area, recently authentication isn't
> tracked in the kernel any more at all, only association.
>
> johannes
>
Johannes Berg - Sept. 28, 2012, 7:26 a.m.
Hi,

> I'm using compat-wireless-3.2.5-1. I see that in nl80211_deauthenticate()
> NL80211_ATTR_MAC is mandatory. The same in compat-wireless-3.6-rc6-1.

Yes, but 3.2 and 3.5 behave quite differently in this area. In 3.5, you
don't actually need to deauthenticate all the time, the whole "state
mismatch" handling isn't necessary. Therefore, I was curious if you
tested this before those changes, but then you did.

> To clear possible misunderstanding:
> >> For NL80211_CMD_DEAUTHENTICATE use correct cached BSSID value.
> This caching is done in hostap.git: src/drivers/driver_nl80211.c ,
> struct wpa_driver_nl80211_data->auth_bssid .

Right, I understand.

johannes
Johannes Berg - Sept. 28, 2012, 7:32 a.m.
On Wed, 2012-09-26 at 16:52 +0200, Mykyta Iziumtsev wrote:

> @@ -4648,7 +4650,7 @@ retry:
>  			"nl80211: MLME command failed (auth): ret=%d (%s)",
>  			ret, strerror(-ret));
>  		count++;
> -		if (ret == -EALREADY && count == 1 && params->bssid &&
> +		if (ret == -EALREADY && count == 1 &&
>  		    !params->local_state_change) {
>  			/*
>  			 * mac80211 does not currently accept new
> @@ -4658,7 +4660,7 @@ retry:
>  			wpa_printf(MSG_DEBUG, "nl80211: Retry authentication "
>  				   "after forced deauthentication");
>  			wpa_driver_nl80211_deauthenticate(
> -				bss, params->bssid,
> +				bss, prev_auth_bssid,
>  				WLAN_REASON_PREV_AUTH_NOT_VALID);
>  			nlmsg_free(msg);
>  			goto retry;

I'm not convinced this is correct. If cfg80211 returns -EALREADY, it
means "already authenticated with this BSS". So it seems using
params->bssid here would be correct in order to re-authenticate.

The case where we can't authenticate because it's already authenticated
with more APs than it would like to track will return -ENOSPC. Note like
I said, starting from kernel 3.4 all this went away.

> @@ -6617,51 +6619,11 @@ nla_put_failure:
>  }
> 
> 
> -static unsigned int nl80211_get_assoc_bssid(struct
> wpa_driver_nl80211_data *drv,
> -					    u8 *bssid)
> +static int nl80211_disconnect(struct wpa_driver_nl80211_data *drv)
>  {
> -	struct nl_msg *msg;
> -	int ret;
> -	struct nl80211_bss_info_arg arg;
> -
> -	os_memset(&arg, 0, sizeof(arg));
> -	msg = nlmsg_alloc();
> -	if (!msg)
> -		goto nla_put_failure;
> -
> -	nl80211_cmd(drv, msg, NLM_F_DUMP, NL80211_CMD_GET_SCAN);
> -	NLA_PUT_U32(msg, NL80211_ATTR_IFINDEX, drv->ifindex);
> -
> -	arg.drv = drv;
> -	ret = send_and_recv_msgs(drv, msg, bss_info_handler, &arg);
> -	msg = NULL;
> -	if (ret == 0) {
> -		if (is_zero_ether_addr(arg.assoc_bssid))
> -			return -ENOTCONN;
> -		os_memcpy(bssid, arg.assoc_bssid, ETH_ALEN);
> -		return 0;
> -	}
> -	wpa_printf(MSG_DEBUG, "nl80211: Scan result fetch failed: ret=%d "
> -		   "(%s)", ret, strerror(-ret));
> -nla_put_failure:
> -	nlmsg_free(msg);
> -	return drv->assoc_freq;
> -}
> -
> -
> -static int nl80211_disconnect(struct wpa_driver_nl80211_data *drv,
> -			      const u8 *bssid)
> -{
> -	u8 addr[ETH_ALEN];
> -
> -	if (bssid == NULL) {
> -		int res = nl80211_get_assoc_bssid(drv, addr);
> -		if (res)
> -			return res;
> -		bssid = addr;
> -	}
> -
> -	return wpa_driver_nl80211_disconnect(drv, bssid,
> +	u8 zero_addr[ETH_ALEN];
> +	os_memset(zero_addr, 0, ETH_ALEN);
> +	return wpa_driver_nl80211_disconnect(drv, zero_addr,
>  					     WLAN_REASON_PREV_AUTH_NOT_VALID);
>  }

Maybe this part should be a separate patch since it's a simplification
due to the fact that DISCONNECT doesn't care about the BSSID?

johannes
Mykyta Iziumtsev - Sept. 28, 2012, 7:52 a.m.
On Fri, Sep 28, 2012 at 9:32 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2012-09-26 at 16:52 +0200, Mykyta Iziumtsev wrote:
>
>> @@ -4648,7 +4650,7 @@ retry:
>>                       "nl80211: MLME command failed (auth): ret=%d (%s)",
>>                       ret, strerror(-ret));
>>               count++;
>> -             if (ret == -EALREADY && count == 1 && params->bssid &&
>> +             if (ret == -EALREADY && count == 1 &&
>>                   !params->local_state_change) {
>>                       /*
>>                        * mac80211 does not currently accept new
>> @@ -4658,7 +4660,7 @@ retry:
>>                       wpa_printf(MSG_DEBUG, "nl80211: Retry authentication "
>>                                  "after forced deauthentication");
>>                       wpa_driver_nl80211_deauthenticate(
>> -                             bss, params->bssid,
>> +                             bss, prev_auth_bssid,
>>                               WLAN_REASON_PREV_AUTH_NOT_VALID);
>>                       nlmsg_free(msg);
>>                       goto retry;
>
> I'm not convinced this is correct. If cfg80211 returns -EALREADY, it
> means "already authenticated with this BSS". So it seems using
> params->bssid here would be correct in order to re-authenticate.
>
> The case where we can't authenticate because it's already authenticated
> with more APs than it would like to track will return -ENOSPC. Note like
> I said, starting from kernel 3.4 all this went away.

This code is actually workaround for older versions of mac80211 returning
-EALREADY when we're already connected to some BSS. Please refer to
commit 6d6f4bb87f33278aed133875d0d561eb55d7ae59 in hostap.git .

So, we should expect -EALREADY not only in "already authenticated with this BSS"
case, but in "already authenticated with *any* BSS" case.

Of course, mac80211 doesn't have this problem any more when userspace
SME is in use, but it still returns -EALREADY when "connect" API is used
(in compat-wireless-3.2.5-1).

>> @@ -6617,51 +6619,11 @@ nla_put_failure:
>>  }
>>
>>
>> -static unsigned int nl80211_get_assoc_bssid(struct
>> wpa_driver_nl80211_data *drv,
>> -                                         u8 *bssid)
>> +static int nl80211_disconnect(struct wpa_driver_nl80211_data *drv)
>>  {
>> -     struct nl_msg *msg;
>> -     int ret;
>> -     struct nl80211_bss_info_arg arg;
>> -
>> -     os_memset(&arg, 0, sizeof(arg));
>> -     msg = nlmsg_alloc();
>> -     if (!msg)
>> -             goto nla_put_failure;
>> -
>> -     nl80211_cmd(drv, msg, NLM_F_DUMP, NL80211_CMD_GET_SCAN);
>> -     NLA_PUT_U32(msg, NL80211_ATTR_IFINDEX, drv->ifindex);
>> -
>> -     arg.drv = drv;
>> -     ret = send_and_recv_msgs(drv, msg, bss_info_handler, &arg);
>> -     msg = NULL;
>> -     if (ret == 0) {
>> -             if (is_zero_ether_addr(arg.assoc_bssid))
>> -                     return -ENOTCONN;
>> -             os_memcpy(bssid, arg.assoc_bssid, ETH_ALEN);
>> -             return 0;
>> -     }
>> -     wpa_printf(MSG_DEBUG, "nl80211: Scan result fetch failed: ret=%d "
>> -                "(%s)", ret, strerror(-ret));
>> -nla_put_failure:
>> -     nlmsg_free(msg);
>> -     return drv->assoc_freq;
>> -}
>> -
>> -
>> -static int nl80211_disconnect(struct wpa_driver_nl80211_data *drv,
>> -                           const u8 *bssid)
>> -{
>> -     u8 addr[ETH_ALEN];
>> -
>> -     if (bssid == NULL) {
>> -             int res = nl80211_get_assoc_bssid(drv, addr);
>> -             if (res)
>> -                     return res;
>> -             bssid = addr;
>> -     }
>> -
>> -     return wpa_driver_nl80211_disconnect(drv, bssid,
>> +     u8 zero_addr[ETH_ALEN];
>> +     os_memset(zero_addr, 0, ETH_ALEN);
>> +     return wpa_driver_nl80211_disconnect(drv, zero_addr,
>>                                            WLAN_REASON_PREV_AUTH_NOT_VALID);
>>  }
>
> Maybe this part should be a separate patch since it's a simplification
> due to the fact that DISCONNECT doesn't care about the BSSID?

Right, I'll split the patch into two.

> johannes
>
Johannes Berg - Sept. 28, 2012, 10:36 a.m.
On Fri, 2012-09-28 at 09:52 +0200, Mykyta Iziumtsev wrote:

> >> @@ -4658,7 +4660,7 @@ retry:
> >>                       wpa_printf(MSG_DEBUG, "nl80211: Retry authentication "
> >>                                  "after forced deauthentication");
> >>                       wpa_driver_nl80211_deauthenticate(
> >> -                             bss, params->bssid,
> >> +                             bss, prev_auth_bssid,
> >>                               WLAN_REASON_PREV_AUTH_NOT_VALID);
> >>                       nlmsg_free(msg);
> >>                       goto retry;
> >
> > I'm not convinced this is correct. If cfg80211 returns -EALREADY, it
> > means "already authenticated with this BSS". So it seems using
> > params->bssid here would be correct in order to re-authenticate.
> >
> > The case where we can't authenticate because it's already authenticated
> > with more APs than it would like to track will return -ENOSPC. Note like
> > I said, starting from kernel 3.4 all this went away.
> 
> This code is actually workaround for older versions of mac80211 returning
> -EALREADY when we're already connected to some BSS. Please refer to
> commit 6d6f4bb87f33278aed133875d0d561eb55d7ae59 in hostap.git .

Yes, I know.

> So, we should expect -EALREADY not only in "already authenticated with this BSS"
> case, but in "already authenticated with *any* BSS" case.

No, that's not true.

> Of course, mac80211 doesn't have this problem any more when userspace
> SME is in use, but it still returns -EALREADY when "connect" API is used
> (in compat-wireless-3.2.5-1).

Well you're not modifying the CONNECT API use though, you're modifying
the AUTHENTICATE API in a way that actually breaks the -EALREADY
workaround.

I could see a bit of value in doing the clear_state_mismatch()
workaround if you get -EALREADY or -ENOSPC, but what you're doing here
seems to break it.

johannes
Mykyta Iziumtsev - Sept. 30, 2012, 7:50 p.m.
Hi Johannes,

Maybe I'm missing something obvious.
I'll try to analyse code deeper, and get back with proof links.
Thank you!

/Mykyta

On Fri, Sep 28, 2012 at 12:36 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2012-09-28 at 09:52 +0200, Mykyta Iziumtsev wrote:
>
>> >> @@ -4658,7 +4660,7 @@ retry:
>> >>                       wpa_printf(MSG_DEBUG, "nl80211: Retry authentication "
>> >>                                  "after forced deauthentication");
>> >>                       wpa_driver_nl80211_deauthenticate(
>> >> -                             bss, params->bssid,
>> >> +                             bss, prev_auth_bssid,
>> >>                               WLAN_REASON_PREV_AUTH_NOT_VALID);
>> >>                       nlmsg_free(msg);
>> >>                       goto retry;
>> >
>> > I'm not convinced this is correct. If cfg80211 returns -EALREADY, it
>> > means "already authenticated with this BSS". So it seems using
>> > params->bssid here would be correct in order to re-authenticate.
>> >
>> > The case where we can't authenticate because it's already authenticated
>> > with more APs than it would like to track will return -ENOSPC. Note like
>> > I said, starting from kernel 3.4 all this went away.
>>
>> This code is actually workaround for older versions of mac80211 returning
>> -EALREADY when we're already connected to some BSS. Please refer to
>> commit 6d6f4bb87f33278aed133875d0d561eb55d7ae59 in hostap.git .
>
> Yes, I know.
>
>> So, we should expect -EALREADY not only in "already authenticated with this BSS"
>> case, but in "already authenticated with *any* BSS" case.
>
> No, that's not true.
>
>> Of course, mac80211 doesn't have this problem any more when userspace
>> SME is in use, but it still returns -EALREADY when "connect" API is used
>> (in compat-wireless-3.2.5-1).
>
> Well you're not modifying the CONNECT API use though, you're modifying
> the AUTHENTICATE API in a way that actually breaks the -EALREADY
> workaround.
>
> I could see a bit of value in doing the clear_state_mismatch()
> workaround if you get -EALREADY or -ENOSPC, but what you're doing here
> seems to break it.
>
> johannes
>
Johannes Berg - Oct. 1, 2012, 10:56 a.m.
On Sun, 2012-09-30 at 21:50 +0200, Mykyta Iziumtsev wrote:
> Hi Johannes,
> 
> Maybe I'm missing something obvious.
> I'll try to analyse code deeper, and get back with proof links.

Alright, sure, I'm always happy to be proven wrong :-)

johannes

Patch

diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index a5659c9..0a9f836 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -4565,11 +4565,13 @@  static int wpa_driver_nl80211_authenticate(
 	enum nl80211_iftype nlmode;
 	int count = 0;
 	int is_retry;
+	u8 prev_auth_bssid[ETH_ALEN];

 	is_retry = drv->retry_auth;
 	drv->retry_auth = 0;

 	drv->associated = 0;
+	os_memcpy(prev_auth_bssid, drv->auth_bssid, ETH_ALEN);
 	os_memset(drv->auth_bssid, 0, ETH_ALEN);
 	/* FIX: IBSS mode */
 	nlmode = params->p2p ?
@@ -4648,7 +4650,7 @@  retry:
 			"nl80211: MLME command failed (auth): ret=%d (%s)",
 			ret, strerror(-ret));
 		count++;
-		if (ret == -EALREADY && count == 1 && params->bssid &&
+		if (ret == -EALREADY && count == 1 &&
 		    !params->local_state_change) {
 			/*
 			 * mac80211 does not currently accept new
@@ -4658,7 +4660,7 @@  retry:
 			wpa_printf(MSG_DEBUG, "nl80211: Retry authentication "
 				   "after forced deauthentication");
 			wpa_driver_nl80211_deauthenticate(
-				bss, params->bssid,
+				bss, prev_auth_bssid,
 				WLAN_REASON_PREV_AUTH_NOT_VALID);
 			nlmsg_free(msg);
 			goto retry;
@@ -6617,51 +6619,11 @@  nla_put_failure:
 }


-static unsigned int nl80211_get_assoc_bssid(struct
wpa_driver_nl80211_data *drv,
-					    u8 *bssid)
+static int nl80211_disconnect(struct wpa_driver_nl80211_data *drv)
 {
-	struct nl_msg *msg;
-	int ret;
-	struct nl80211_bss_info_arg arg;
-
-	os_memset(&arg, 0, sizeof(arg));
-	msg = nlmsg_alloc();
-	if (!msg)
-		goto nla_put_failure;
-
-	nl80211_cmd(drv, msg, NLM_F_DUMP, NL80211_CMD_GET_SCAN);
-	NLA_PUT_U32(msg, NL80211_ATTR_IFINDEX, drv->ifindex);
-
-	arg.drv = drv;
-	ret = send_and_recv_msgs(drv, msg, bss_info_handler, &arg);
-	msg = NULL;
-	if (ret == 0) {
-		if (is_zero_ether_addr(arg.assoc_bssid))
-			return -ENOTCONN;
-		os_memcpy(bssid, arg.assoc_bssid, ETH_ALEN);
-		return 0;
-	}
-	wpa_printf(MSG_DEBUG, "nl80211: Scan result fetch failed: ret=%d "
-		   "(%s)", ret, strerror(-ret));
-nla_put_failure:
-	nlmsg_free(msg);
-	return drv->assoc_freq;
-}
-
-
-static int nl80211_disconnect(struct wpa_driver_nl80211_data *drv,
-			      const u8 *bssid)
-{
-	u8 addr[ETH_ALEN];
-
-	if (bssid == NULL) {
-		int res = nl80211_get_assoc_bssid(drv, addr);
-		if (res)
-			return res;
-		bssid = addr;
-	}
-
-	return wpa_driver_nl80211_disconnect(drv, bssid,
+	u8 zero_addr[ETH_ALEN];
+	os_memset(zero_addr, 0, ETH_ALEN);
+	return wpa_driver_nl80211_disconnect(drv, zero_addr,
 					     WLAN_REASON_PREV_AUTH_NOT_VALID);
 }