Re: bss table corruption

[Mohammed Shafi <shafi.wireless@gmail.com>]

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