bss table corruption

bss table corruption

[Emmanuel Grumbach]

For quite a while now (not sure I can tell exactly for how long) we
see issues in association and scan list.
We send probe before authentication, get the probe response but never
send the authentication.
Moreover a lot of entries in the BSS list are duplicated.

I began to look at it and ended up to understand that these 2 issues
are related: we just can't find an existing BSS in the BSS table.
Obviously this causes the second issue. The reason was it breaks the
association is that ieee80211_probe_auth will never be able to find
the IEs of the probe response since we couldn't fetch the BSS when we
parsed the probe response. In short:

	if (auth_data->bss->proberesp_ies) {
always return false.... and we fall back to send yet another probe request.

As you probably know, the BSS table is implemented with an Red Black
Tree which requires its elements to be comparable. The compare
function compares the BSSID which is not always unique (there can be
several SSIDs on the same BSSID), so all the IEs are also compared.
But is this a good idea ?
It seems that since the IEs of an BSS may change from time to time
this compare function is not consistent...

Just for playing I always return a positive value in cmp_bss (to have
all the nodes serialized and avoid the possibility to miss a existing
node) and don't rebalance the tree after insertion... the bug
disappeared.

Thought ?

Emmanuel Grumbach
egrumbach@gmail.com

Re: bss table corruption

[Emmanuel Grumbach]

> For quite a while now (not sure I can tell exactly for how long) we
> see issues in association and scan list.
> We send probe before authentication, get the probe response but never
> send the authentication.
> Moreover a lot of entries in the BSS list are duplicated.
>
> I began to look at it and ended up to understand that these 2 issues
> are related: we just can't find an existing BSS in the BSS table.
> Obviously this causes the second issue. The reason was it breaks the
> association is that ieee80211_probe_auth will never be able to find
> the IEs of the probe response since we couldn't fetch the BSS when we
> parsed the probe response. In short:
>
>        if (auth_data->bss->proberesp_ies) {
> always return false.... and we fall back to send yet another probe request.
>
> As you probably know, the BSS table is implemented with an Red Black
> Tree which requires its elements to be comparable. The compare
> function compares the BSSID which is not always unique (there can be
> several SSIDs on the same BSSID), so all the IEs are also compared.
> But is this a good idea ?
> It seems that since the IEs of an BSS may change from time to time
> this compare function is not consistent...

Oops. Stupid me. Just noticed that we compare the SSID only...
So how come ?

>
> Just for playing I always return a positive value in cmp_bss (to have
> all the nodes serialized and avoid the possibility to miss a existing
> node) and don't rebalance the tree after insertion... the bug
> disappeared.
>
> Thought ?
>
> Emmanuel Grumbach
> egrumbach@gmail.com

Re: bss table corruption

[Mohammed Shafi]

Hi Emmanuel,

On Mon, Apr 30, 2012 at 10:58 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> For quite a while now (not sure I can tell exactly for how long) we
> see issues in association and scan list.
> We send probe before authentication, get the probe response but never
> send the authentication.
> Moreover a lot of entries in the BSS list are duplicated.
>
> I began to look at it and ended up to understand that these 2 issues
> are related: we just can't find an existing BSS in the BSS table.
> Obviously this causes the second issue. The reason was it breaks the
> association is that ieee80211_probe_auth will never be able to find
> the IEs of the probe response since we couldn't fetch the BSS when we
> parsed the probe response. In short:
>
>        if (auth_data->bss->proberesp_ies) {
> always return false.... and we fall back to send yet another probe request.
>
> As you probably know, the BSS table is implemented with an Red Black
> Tree which requires its elements to be comparable. The compare
> function compares the BSSID which is not always unique (there can be
> several SSIDs on the same BSSID), so all the IEs are also compared.
> But is this a good idea ?
> It seems that since the IEs of an BSS may change from time to time
> this compare function is not consistent...
>
> Just for playing I always return a positive value in cmp_bss (to have
> all the nodes serialized and avoid the possibility to miss a existing
> node) and don't rebalance the tree after insertion... the bug
> disappeared.
>
> Thought ?
>
> Emmanuel Grumbach
> egrumbach@gmail.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

we are comparing the BSSID of the new entry with the old entry (ie) we
would return positive (with your latest fix)
if the BSSID of the new entry > BSSID of the old entry, should not we
do the same for comparing frequency, ie length and
ie content.
just got a doubt if this is by design we have things like this.


net/wireless/scan.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 70faadf..fda4eef 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -271,7 +271,7 @@ static int cmp_ies(u8 num, u8 *ies1, size_t len1,
u8 *ies2, size_t len2)

      /* sort by length first, then by contents */
      if (ie1[1] != ie2[1])
-        return ie2[1] - ie1[1];
+        return ie1[1] - ie2[1];
      return memcmp(ie1 + 2, ie2 + 2, ie1[1]);
  }

@@ -361,7 +361,7 @@ static int cmp_bss_core(struct cfg80211_bss *a,
      int r;

      if (a->channel != b->channel)
-        return b->channel->center_freq - a->channel->center_freq;
+        return a->channel->center_freq - b->channel->center_freq;

      if (is_mesh_bss(a)&&  is_mesh_bss(b)) {
          r = cmp_ies(WLAN_EID_MESH_ID,
@@ -433,7 +433,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a,

      /* sort by length first, then by contents */
      if (ie1[1] != ie2[1])
-        return ie2[1] - ie1[1];
+        return ie1[1] - ie2[1];

      /* zeroed SSID ie is another indication of a hidden bss */
      for (i = 0; i<  ie2[1]; i++)


-- 
thanks,
shafi

Re: bss table corruption

[Emmanuel Grumbach]

On Mon, Apr 30, 2012 at 10:30, Mohammed Shafi <shafi.wireless@gmail.com> wrote:
> Hi Emmanuel,
>
> On Mon, Apr 30, 2012 at 10:58 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>> For quite a while now (not sure I can tell exactly for how long) we
>> see issues in association and scan list.
>> We send probe before authentication, get the probe response but never
>> send the authentication.
>> Moreover a lot of entries in the BSS list are duplicated.
>>
>> I began to look at it and ended up to understand that these 2 issues
>> are related: we just can't find an existing BSS in the BSS table.
>> Obviously this causes the second issue. The reason was it breaks the
>> association is that ieee80211_probe_auth will never be able to find
>> the IEs of the probe response since we couldn't fetch the BSS when we
>> parsed the probe response. In short:
>>
>>        if (auth_data->bss->proberesp_ies) {
>> always return false.... and we fall back to send yet another probe request.
>>
>> As you probably know, the BSS table is implemented with an Red Black
>> Tree which requires its elements to be comparable. The compare
>> function compares the BSSID which is not always unique (there can be
>> several SSIDs on the same BSSID), so all the IEs are also compared.
>> But is this a good idea ?
>> It seems that since the IEs of an BSS may change from time to time
>> this compare function is not consistent...
>>
>> Just for playing I always return a positive value in cmp_bss (to have
>> all the nodes serialized and avoid the possibility to miss a existing
>> node) and don't rebalance the tree after insertion... the bug
>> disappeared.
>>
>> Thought ?
>>
>> Emmanuel Grumbach
>> egrumbach@gmail.com
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> we are comparing the BSSID of the new entry with the old entry (ie) we
> would return positive (with your latest fix)
> if the BSSID of the new entry > BSSID of the old entry, should not we
> do the same for comparing frequency, ie length and
> ie content.
> just got a doubt if this is by design we have things like this.
>
>

Frankly I didn't think about this :-), but I think the current
behavior is correct even if it is not intuitive. All we need a
consistent comparison operator. But I don't mind folding you
suggestion retest and resubmit.

> net/wireless/scan.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 70faadf..fda4eef 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -271,7 +271,7 @@ static int cmp_ies(u8 num, u8 *ies1, size_t len1,
> u8 *ies2, size_t len2)
>
>      /* sort by length first, then by contents */
>      if (ie1[1] != ie2[1])
> -        return ie2[1] - ie1[1];
> +        return ie1[1] - ie2[1];
>      return memcmp(ie1 + 2, ie2 + 2, ie1[1]);
>  }
>
> @@ -361,7 +361,7 @@ static int cmp_bss_core(struct cfg80211_bss *a,
>      int r;
>
>      if (a->channel != b->channel)
> -        return b->channel->center_freq - a->channel->center_freq;
> +        return a->channel->center_freq - b->channel->center_freq;
>
>      if (is_mesh_bss(a)&&  is_mesh_bss(b)) {
>          r = cmp_ies(WLAN_EID_MESH_ID,
> @@ -433,7 +433,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a,
>
>      /* sort by length first, then by contents */
>      if (ie1[1] != ie2[1])
> -        return ie2[1] - ie1[1];
> +        return ie1[1] - ie2[1];
>
>      /* zeroed SSID ie is another indication of a hidden bss */
>      for (i = 0; i<  ie2[1]; i++)
>
>
> --
> thanks,
> shafi

Re: bss table corruption

[Mohammed Shafi]

On Mon, Apr 30, 2012 at 1:07 PM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
> On Mon, Apr 30, 2012 at 10:30, Mohammed Shafi <shafi.wireless@gmail.com> wrote:
>> Hi Emmanuel,
>>
>> On Mon, Apr 30, 2012 at 10:58 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote:
>>> For quite a while now (not sure I can tell exactly for how long) we
>>> see issues in association and scan list.
>>> We send probe before authentication, get the probe response but never
>>> send the authentication.
>>> Moreover a lot of entries in the BSS list are duplicated.
>>>
>>> I began to look at it and ended up to understand that these 2 issues
>>> are related: we just can't find an existing BSS in the BSS table.
>>> Obviously this causes the second issue. The reason was it breaks the
>>> association is that ieee80211_probe_auth will never be able to find
>>> the IEs of the probe response since we couldn't fetch the BSS when we
>>> parsed the probe response. In short:
>>>
>>>        if (auth_data->bss->proberesp_ies) {
>>> always return false.... and we fall back to send yet another probe request.
>>>
>>> As you probably know, the BSS table is implemented with an Red Black
>>> Tree which requires its elements to be comparable. The compare
>>> function compares the BSSID which is not always unique (there can be
>>> several SSIDs on the same BSSID), so all the IEs are also compared.
>>> But is this a good idea ?
>>> It seems that since the IEs of an BSS may change from time to time
>>> this compare function is not consistent...
>>>
>>> Just for playing I always return a positive value in cmp_bss (to have
>>> all the nodes serialized and avoid the possibility to miss a existing
>>> node) and don't rebalance the tree after insertion... the bug
>>> disappeared.
>>>
>>> Thought ?
>>>
>>> Emmanuel Grumbach
>>> egrumbach@gmail.com
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> we are comparing the BSSID of the new entry with the old entry (ie) we
>> would return positive (with your latest fix)
>> if the BSSID of the new entry > BSSID of the old entry, should not we
>> do the same for comparing frequency, ie length and
>> ie content.
>> just got a doubt if this is by design we have things like this.
>>
>>
>
> Frankly I didn't think about this :-), but I think the current
> behavior is correct even if it is not intuitive. All we need a
> consistent comparison operator. But I don't mind folding you
> suggestion retest and resubmit.

when i submitted the RFC internally Jouni was commenting whether this
fixes something.
i could not find one. thanks for your comments!


>
>> net/wireless/scan.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
>> index 70faadf..fda4eef 100644
>> --- a/net/wireless/scan.c
>> +++ b/net/wireless/scan.c
>> @@ -271,7 +271,7 @@ static int cmp_ies(u8 num, u8 *ies1, size_t len1,
>> u8 *ies2, size_t len2)
>>
>>      /* sort by length first, then by contents */
>>      if (ie1[1] != ie2[1])
>> -        return ie2[1] - ie1[1];
>> +        return ie1[1] - ie2[1];
>>      return memcmp(ie1 + 2, ie2 + 2, ie1[1]);
>>  }
>>
>> @@ -361,7 +361,7 @@ static int cmp_bss_core(struct cfg80211_bss *a,
>>      int r;
>>
>>      if (a->channel != b->channel)
>> -        return b->channel->center_freq - a->channel->center_freq;
>> +        return a->channel->center_freq - b->channel->center_freq;
>>
>>      if (is_mesh_bss(a)&&  is_mesh_bss(b)) {
>>          r = cmp_ies(WLAN_EID_MESH_ID,
>> @@ -433,7 +433,7 @@ static int cmp_hidden_bss(struct cfg80211_bss *a,
>>
>>      /* sort by length first, then by contents */
>>      if (ie1[1] != ie2[1])
>> -        return ie2[1] - ie1[1];
>> +        return ie1[1] - ie2[1];
>>
>>      /* zeroed SSID ie is another indication of a hidden bss */
>>      for (i = 0; i<  ie2[1]; i++)
>>
>>
>> --
>> thanks,
>> shafi



-- 
thanks,
shafi