[PATCH v2] cfg80211: merge in beacon ies of hidden bss.

[PATCH v2] cfg80211: merge in beacon ies of hidden bss.

[Dmitry Tarnyagin]

The problem with PSM when a hidden SSID was used was originally
reported by Juuso Oikarinen.

 - When generally scanning, the AP is getting a bss entry with
   a zero SSID.
 - When associationg, a probe-req is sent to the AP with the SSID,
   and as a result a probe-responseis received with the hidden
   SSID in place. As a consequence, a second bss entry is created
   for the AP, now with the real SSID.
 - After association, mac80211 executes ieee80211_recalc_ps(),
   but does not switch to powersave becuse the beacon-ies are missing.

As result, the STA does not ever enter PSM.

The patch merges in beacon ies of hidden bss from beacon to the probe
response, creating a consistant set of ies in place.

Change-Id: I7d10f8546484f51b803b5c003c8208e471b7522b
Signed-off-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
---
 net/wireless/scan.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 112 insertions(+), 3 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index dd0d4c5..817c79c 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -325,8 +325,8 @@ static bool is_mesh(struct cfg80211_bss *a,
 	    sizeof(struct ieee80211_meshconf_ie) - 2) == 0;
 }

-static int cmp_bss(struct cfg80211_bss *a,
-		   struct cfg80211_bss *b)
+static int cmp_bss_core(struct cfg80211_bss *a,
+			struct cfg80211_bss *b)
 {
 	int r;

@@ -348,7 +348,15 @@ static int cmp_bss(struct cfg80211_bss *a,
 			       b->len_information_elements);
 	}

-	r = memcmp(a->bssid, b->bssid, ETH_ALEN);
+	return memcmp(a->bssid, b->bssid, ETH_ALEN);
+}
+
+static int cmp_bss(struct cfg80211_bss *a,
+		   struct cfg80211_bss *b)
+{
+	int r;
+
+	r = cmp_bss_core(a, b);
 	if (r)
 		return r;

@@ -359,6 +367,50 @@ static int cmp_bss(struct cfg80211_bss *a,
 		       b->len_information_elements);
 }

+static int cmp_hidden_bss(struct cfg80211_bss *a,
+		   struct cfg80211_bss *b)
+{
+	const u8 *ie1;
+	const u8 *ie2;
+	size_t ielen;
+	int i;
+	int r;
+
+	r = cmp_bss_core(a, b);
+	if (r)
+		return r;
+
+	ie1 = cfg80211_find_ie(WLAN_EID_SSID,
+			a->information_elements,
+			a->len_information_elements);
+	ie2 = cfg80211_find_ie(WLAN_EID_SSID,
+			b->information_elements,
+			b->len_information_elements);
+
+	/* Absence of SSID or zero-sized SSID is used as
+	 * an indication of the hidden bss. */
+	if (!ie2 || !ie2[1])
+		return 0;
+
+	/* It should not happen, but let's try to keep semantics
+	 * (-1 means "lesser"). */
+	if (WARN_ON(!ie1))
+		return -1;
+
+	/* Key comparator must use same algorithm in any rb-tree
+	 * search function (order is important), otherwise ordering
+	 * of items in the tree is broken and search gives incorrect
+	 * results. This code uses same order as cmp_ies() does.
+	 *
+	 * The only difference is that this code searchs for zeroed
+	 * SSID ie (another indication of the hidden bss). */
+	ielen = min(ie1[1], ie2[1]);
+	for (i = 0; i < ielen; i++)
+		if (ie2[i + 2])
+			return -1;
+	return ie2[1] - ie1[1];
+}
+
 struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
 				      struct ieee80211_channel *channel,
 				      const u8 *bssid,
@@ -475,6 +527,48 @@ rb_find_bss(struct cfg80211_registered_device *dev,
 }

 static struct cfg80211_internal_bss *
+rb_find_hidden_bss(struct cfg80211_registered_device *dev,
+	    struct cfg80211_internal_bss *res)
+{
+	struct rb_node *n = dev->bss_tree.rb_node;
+	struct cfg80211_internal_bss *bss;
+	int r;
+
+	while (n) {
+		bss = rb_entry(n, struct cfg80211_internal_bss, rbn);
+		r = cmp_hidden_bss(&res->pub, &bss->pub);
+
+		if (r == 0)
+			return bss;
+		else if (r < 0)
+			n = n->rb_left;
+		else
+			n = n->rb_right;
+	}
+
+	return NULL;
+}
+
+static void
+copy_hidden_ies(struct cfg80211_internal_bss *res,
+		 struct cfg80211_internal_bss *hidden)
+{
+	if (unlikely(res->pub.beacon_ies))
+		return;
+	if (WARN_ON(!hidden->pub.beacon_ies))
+		return;
+
+	res->pub.beacon_ies = kmalloc(hidden->pub.len_beacon_ies, GFP_ATOMIC);
+	if (unlikely(!res->pub.beacon_ies))
+		return;
+
+	res->beacon_ies_allocated = true;
+	res->pub.len_beacon_ies = hidden->pub.len_beacon_ies;
+	memcpy(res->pub.beacon_ies, hidden->pub.beacon_ies,
+			res->pub.len_beacon_ies);
+}
+
+static struct cfg80211_internal_bss *
 cfg80211_bss_update(struct cfg80211_registered_device *dev,
 		    struct cfg80211_internal_bss *res)
 {
@@ -587,6 +681,21 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,

 		kref_put(&res->ref, bss_release);
 	} else {
+		struct cfg80211_internal_bss *hidden;
+
+		/* First check if the beacon is a probe response from
+		 * a hidden bss. If so, copy beacon ies (with nullified
+		 * ssid) into the probe response bss entry (with real ssid).
+		 * It is required basically for PSM implementation
+		 * (probe responses do not contain tim ie) */
+
+		/* TODO: The code is not trying to update existing probe
+		 * response bss entries when beacon ies are
+		 * getting changed. */
+		hidden = rb_find_hidden_bss(dev, res);
+		if (hidden)
+			copy_hidden_ies(res, hidden);
+
 		/* this "consumes" the reference */
 		list_add_tail(&res->list, &dev->bss_list);
 		rb_insert_bss(dev, res);
-- 
With best regards,
Dmitry

Re: [PATCH v2] cfg80211: merge in beacon ies of hidden bss.

[Vitaly Wool]

Hi Dmitry,

On Thu, Nov 3, 2011 at 10:59 PM, Dmitry Tarnyagin <abi.dmitryt@gmail.com> wrote:
> The problem with PSM when a hidden SSID was used was originally
> reported by Juuso Oikarinen.
>
>  - When generally scanning, the AP is getting a bss entry with
>   a zero SSID.
>  - When associationg, a probe-req is sent to the AP with the SSID,
>   and as a result a probe-responseis received with the hidden
>   SSID in place. As a consequence, a second bss entry is created
>   for the AP, now with the real SSID.
>  - After association, mac80211 executes ieee80211_recalc_ps(),
>   but does not switch to powersave becuse the beacon-ies are missing.
>
> As result, the STA does not ever enter PSM.
>
> The patch merges in beacon ies of hidden bss from beacon to the probe
> response, creating a consistant set of ies in place.
>
> Change-Id: I7d10f8546484f51b803b5c003c8208e471b7522b
> Signed-off-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>

as a matter of fact, I haven't had a chance to look into your patch in
great detail. But what I can say is that it does work for me, tested
against 2 access points and with 2 different chips (STE's CW1100 and
TI's WL1271L), so let me add

Tested-by: Vitaly Wool <vitalywool@gmail.com>

Thanks,
   Vitaly

Re: [PATCH v2] cfg80211: merge in beacon ies of hidden bss.

[Johannes Berg]

On Thu, 2011-11-03 at 22:59 +0100, Dmitry Tarnyagin wrote:

> +	/* Absence of SSID or zero-sized SSID is used as
> +	 * an indication of the hidden bss. */
> +	if (!ie2 || !ie2[1])
> +		return 0;

I don't think that's right -- I never saw anything w/o an SSID IE except
for mesh networks I think. Also this has different semantics from the
regular rb-tree search which will return -1 if not present (which is
actually a bug).

> +	/* Key comparator must use same algorithm in any rb-tree
> +	 * search function (order is important), otherwise ordering
> +	 * of items in the tree is broken and search gives incorrect
> +	 * results. This code uses same order as cmp_ies() does.
> +	 *
> +	 * The only difference is that this code searchs for zeroed
> +	 * SSID ie (another indication of the hidden bss). */
> +	ielen = min(ie1[1], ie2[1]);
> +	for (i = 0; i < ielen; i++)
> +		if (ie2[i + 2])
> +			return -1;
> +	return ie2[1] - ie1[1];

Since we need to fix cmp_ie() anyway, how about we change it there as
well. I'm going to post a patch.

> @@ -587,6 +681,21 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
> 
>  		kref_put(&res->ref, bss_release);
>  	} else {
> +		struct cfg80211_internal_bss *hidden;
> +
> +		/* First check if the beacon is a probe response from
> +		 * a hidden bss. If so, copy beacon ies (with nullified
> +		 * ssid) into the probe response bss entry (with real ssid).
> +		 * It is required basically for PSM implementation
> +		 * (probe responses do not contain tim ie) */
> +
> +		/* TODO: The code is not trying to update existing probe
> +		 * response bss entries when beacon ies are
> +		 * getting changed. */
> +		hidden = rb_find_hidden_bss(dev, res);
> +		if (hidden)
> +			copy_hidden_ies(res, hidden);
> +

This is nicer, though I'd prefer the TODO was addressed as well since
now we'll forever show stale data, this seems a bit bad.

johannes

Re: [PATCH v2] cfg80211: merge in beacon ies of hidden bss.

[Dmitry Tarnyagin]

Hi Johannes,

>> +     /* Absence of SSID or zero-sized SSID is used as
>> +      * an indication of the hidden bss. */
>> +     if (!ie2 || !ie2[1])
>> +             return 0;
>
> I don't think that's right -- I never saw anything w/o an SSID IE except
> for mesh networks I think. Also this has different semantics from the
> regular rb-tree search which will return -1 if not present (which is
> actually a bug).
Ok, agree.

> Since we need to fix cmp_ie() anyway, how about we change it there as
> well. I'm going to post a patch.
>
I assume there is some (unclear for me) idea behind the order of operation
in the cmp_ie comparator. Natural order of scan output?

>> +             /* TODO: The code is not trying to update existing probe
>> +              * response bss entries when beacon ies are
>> +              * getting changed. */
>
> This is nicer, though I'd prefer the TODO was addressed as well since
> now we'll forever show stale data, this seems a bit bad.
>
We'll discuss it and maybe will redesign bss cache a bit, but not in a scope
of this patch. I do not like stale data in IEs as well - from end-user
prospective
WPS likely will not work.

With best regards,
Dmitry

Re: [PATCH v2] cfg80211: merge in beacon ies of hidden bss.

[Johannes Berg]

Hi,

> > Since we need to fix cmp_ie() anyway, how about we change it there as
> > well. I'm going to post a patch.

> I assume there is some (unclear for me) idea behind the order of operation
> in the cmp_ie comparator. Natural order of scan output?

Well, the sort order here matters only for the tree search and is
clearly broken for that. I just sent a patch to fix that.

johannes