| Message ID | 20190710173651.15770-1-sergey.matyukevich.os@quantenna.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | cfg80211: fix duplicated scan entries after channel switch | expand |
> Suggested approach to handle non-transmitting BSS entries is simplified in the > following sense. If new entries have been already created after channel switch, > only transmitting bss will be updated using IEs of new entry for the same > transmitting bss. Non-transmitting bss entries will be updated as soon as > new mgmt frames are received. Updating non-transmitting bss entries seems > too expensive: nested nontrans_list traversing is needed since we can not > rely on the same order of old and new non-transmitting entries. That sounds like a reasonable trade-off. I do wonder though what happens if we're connected to a non-transmitting BSS? johannes
On Fri, Jul 12, 2019 at 11:11:19AM +0200, Johannes Berg wrote: > > [External Email]: This email arrived from an external source - Please exercise caution when opening any attachments or clicking on links. > > > Suggested approach to handle non-transmitting BSS entries is simplified in the > > following sense. If new entries have been already created after channel switch, > > only transmitting bss will be updated using IEs of new entry for the same > > transmitting bss. Non-transmitting bss entries will be updated as soon as > > new mgmt frames are received. Updating non-transmitting bss entries seems > > too expensive: nested nontrans_list traversing is needed since we can not > > rely on the same order of old and new non-transmitting entries. > > That sounds like a reasonable trade-off. I do wonder though what happens > if we're connected to a non-transmitting BSS? Well, here I rely upon the assumption that CSA IEs of non-transmitting BSS are handled correctly by mac80211 or any FullMAC firmware. And if we are connected to non-transmitting BSS rather than transmitting one, the following code in the beginning of new cfg80211_update_assoc_bss_entry function is supposed to care about this use-case: + /* use transmitting bss */ + if (cbss->pub.transmitted_bss) + cbss = container_of(cbss->pub.transmitted_bss, + struct cfg80211_internal_bss, + pub); In other words, regardless of which BSS we are connected to, the whole hierarchy of non-transmitting BSS entries will be updated, including their channels and location in rb-tree. Regards, Sergey
On Fri, 2019-07-12 at 09:27 +0000, Sergey Matyukevich wrote: > On Fri, Jul 12, 2019 at 11:11:19AM +0200, Johannes Berg wrote: > > > > [External Email]: This email arrived from an external source - Please exercise caution when opening any attachments or clicking on links. Heh, you have a not so fun email system that rewrites mails ... > > > Suggested approach to handle non-transmitting BSS entries is simplified in the > > > following sense. If new entries have been already created after channel switch, > > > only transmitting bss will be updated using IEs of new entry for the same > > > transmitting bss. Non-transmitting bss entries will be updated as soon as > > > new mgmt frames are received. Updating non-transmitting bss entries seems > > > too expensive: nested nontrans_list traversing is needed since we can not > > > rely on the same order of old and new non-transmitting entries. > > > > That sounds like a reasonable trade-off. I do wonder though what happens > > if we're connected to a non-transmitting BSS? > > Well, here I rely upon the assumption that CSA IEs of non-transmitting BSS > are handled correctly by mac80211 or any FullMAC firmware. And if we are > connected to non-transmitting BSS rather than transmitting one, the > following code in the beginning of new cfg80211_update_assoc_bss_entry > function is supposed to care about this use-case: Right, it will be updated on RX. But then if we chanswitch, we would probably (mac80211 using a pointer to the non-transmitting BSS) update only one of the nontransmitting BSSes? Just saying that maybe we need to be careful there - or your wording might be incorrect. We might end up updating a *nontransmitting* BSS, and then its transmitting/other non-tx ones only later? johannes
> > > [External Email]: This email arrived from an external source - Please exercise caution when opening any attachments or clicking on links. > > Heh, you have a not so fun email system that rewrites mails ... :( > > > > Suggested approach to handle non-transmitting BSS entries is simplified in the > > > > following sense. If new entries have been already created after channel switch, > > > > only transmitting bss will be updated using IEs of new entry for the same > > > > transmitting bss. Non-transmitting bss entries will be updated as soon as > > > > new mgmt frames are received. Updating non-transmitting bss entries seems > > > > too expensive: nested nontrans_list traversing is needed since we can not > > > > rely on the same order of old and new non-transmitting entries. > > > > > > That sounds like a reasonable trade-off. I do wonder though what happens > > > if we're connected to a non-transmitting BSS? > > > > Well, here I rely upon the assumption that CSA IEs of non-transmitting BSS > > are handled correctly by mac80211 or any FullMAC firmware. And if we are > > connected to non-transmitting BSS rather than transmitting one, the > > following code in the beginning of new cfg80211_update_assoc_bss_entry > > function is supposed to care about this use-case: > > Right, it will be updated on RX. But then if we chanswitch, we would > probably (mac80211 using a pointer to the non-transmitting BSS) update > only one of the nontransmitting BSSes? > > Just saying that maybe we need to be careful there - or your wording > might be incorrect. We might end up updating a *nontransmitting* BSS, > and then its transmitting/other non-tx ones only later? Hmmm... I am not sure we are on the same page here. Could you please clarify your concerns here ? The normal (non multi-BSSID) BSS usecase seem to be clear: keep old and remove new (if any), since it is not easy to update ifmgd->associated. Now let me take another look at the usecase when STA is connected to a transmitting or non-transmitting BSS of a multi-BSS AP. At the moment suggested code does the following. If STA is connected to the non-transmitting BSS, then we switch to its transmitting BSS, instead of working with current_bss directly. So we look for the new entry (with new channel) of the transmitting BSS. If it exists, then we remove it and _all_ of its non-transmitting BSSs. Finally, we update channel and location in rb-tree of the existing (old) transmitting BSS as well as _all_ of its non-transmitting entries. Regards, Sergey
Hi Sergey, Sorry for dropping the ball on this thread. > > Right, it will be updated on RX. But then if we chanswitch, we would > > probably (mac80211 using a pointer to the non-transmitting BSS) update > > only one of the nontransmitting BSSes? > > > > Just saying that maybe we need to be careful there - or your wording > > might be incorrect. We might end up updating a *nontransmitting* BSS, > > and then its transmitting/other non-tx ones only later? > > Hmmm... I am not sure we are on the same page here. Could you please > clarify your concerns here ? I'm trying to say we might have this: cfg80211 * transmitting BSS 0 - nontx BSS 1 - nontx BSS 2 - nontx BSS 3 mac80211 * ifmgd->associated (and cfg80211's wdev->current_bss?) = nontx BSS 2 Now, things like the channel information etc. will always be identical between the 4 BSSes, by definition. However, if you chanswitch and mac80211 just lets cfg80211 know about the current_bss, then you may end up in a situation where the channel information is no longer the same, which is very surprising. > The normal (non multi-BSSID) BSS usecase seem to be clear: keep old and > remove new (if any), since it is not easy to update ifmgd->associated. Right. > Now let me take another look at the usecase when STA is connected to > a transmitting or non-transmitting BSS of a multi-BSS AP. At the moment > suggested code does the following. If STA is connected to the non-transmitting > BSS, then we switch to its transmitting BSS, instead of working with > current_bss directly. We switch? Where? Maybe I missed that. > So we look for the new entry (with new channel) of the transmitting BSS. > If it exists, then we remove it and _all_ of its non-transmitting BSSs. > Finally, we update channel and location in rb-tree of the existing (old) > transmitting BSS as well as _all_ of its non-transmitting entries. That would indeed address the scenario I was thinking of ... johannes
Hello Johannes, > > > Right, it will be updated on RX. But then if we chanswitch, we would > > > probably (mac80211 using a pointer to the non-transmitting BSS) update > > > only one of the nontransmitting BSSes? > > > > > > Just saying that maybe we need to be careful there - or your wording > > > might be incorrect. We might end up updating a *nontransmitting* BSS, > > > and then its transmitting/other non-tx ones only later? > > > > Hmmm... I am not sure we are on the same page here. Could you please > > clarify your concerns here ? > > I'm trying to say we might have this: > > cfg80211 > * transmitting BSS 0 > - nontx BSS 1 > - nontx BSS 2 > - nontx BSS 3 > mac80211 > * ifmgd->associated (and cfg80211's wdev->current_bss?) = nontx BSS 2 Yes, this is the use-case that I tried to address in the last revision of the patch. Suggested approach is similar to what is done for normal case: - to keep this hierarchy updating channels and location in rb-tree - remove newly added hierarchy of the same transmitting BSS on the new channel Note that here we update/remove not only transmitting BSSs, but their nontx BSS hierarchies as well. > > > Now, things like the channel information etc. will always be identical > between the 4 BSSes, by definition. > > However, if you chanswitch and mac80211 just lets cfg80211 know about > the current_bss, then you may end up in a situation where the channel > information is no longer the same, which is very surprising. > > > > The normal (non multi-BSSID) BSS usecase seem to be clear: keep old and > > remove new (if any), since it is not easy to update ifmgd->associated. > > Right. > > > Now let me take another look at the usecase when STA is connected to > > a transmitting or non-transmitting BSS of a multi-BSS AP. At the moment > > suggested code does the following. If STA is connected to the non-transmitting > > BSS, then we switch to its transmitting BSS, instead of working with > > current_bss directly. > > We switch? Where? Maybe I missed that. If you take a look at the top of new cfg80211_update_assoc_bss_entry function: + /* use transmitting bss */ + if (cbss->pub.transmitted_bss) + cbss = container_of(cbss->pub.transmitted_bss, + struct cfg80211_internal_bss, + pub); > > So we look for the new entry (with new channel) of the transmitting BSS. > > If it exists, then we remove it and _all_ of its non-transmitting BSSs. > > Finally, we update channel and location in rb-tree of the existing (old) > > transmitting BSS as well as _all_ of its non-transmitting entries. > > That would indeed address the scenario I was thinking of ... Ok! Let me know if you have any other concerns or questions. Actually one of the major concerns is the lack of testing for the 'multi-BSSID' scenario. I verified the 'normal' scenario using both mac80211 (iwlwifi) and FullMAC (qtnfmac) cards. But at the moment I don't have any mac80211 card supporting multi-BSSID. Regards, Sergey
Hi Sergey, > Yes, this is the use-case that I tried to address in the last revision > of the patch. OK! I didn't see it here and I guess I didn't look at the latest version yet, or I missed it. > If you take a look at the top of new cfg80211_update_assoc_bss_entry > function: > > + /* use transmitting bss */ > + if (cbss->pub.transmitted_bss) > + cbss = container_of(cbss->pub.transmitted_bss, > + struct cfg80211_internal_bss, > + pub); Right, makes sense! > Actually one of the major concerns is the lack of testing for the 'multi-BSSID' > scenario. I verified the 'normal' scenario using both mac80211 (iwlwifi) and > FullMAC (qtnfmac) cards. But at the moment I don't have any mac80211 card > supporting multi-BSSID. You might be able to do that with hwsim? There are multi-bssid test cases in the hostap repository, and CSA test cases as well, so I guess it'd be possible to come up with a combined one. I'm not *too* worried about this though - we're still all testing and developing this. johannes