[RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich]

Hi Johannes and all,

This is v3 of RFC patch aimed at fixing duplicated scan entries after channel
switch. The major change is updating non-transmitting bss entries. Since such
a bss cannot change channel without its transmitting bss (and vice versa),
the whole hierarchy of transmitting bss is updated, including channel and
location in rb-tree.

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.

Basic use-case tested using both iwlwifi and qtnfmac.
However multi-BSSID support has not yet been tested. 

Regards,
Sergey

v1 -> v2
- use IEs of new BSS entry to update known BSS entry
  for this purpose extract BSS update code from cfg80211_bss_update
  into a separate function cfg80211_update_known_bss

v2 -> v3
- minor cleanup according to review comments
- split cfg80211_update_known_bss function into a separate patch
- update channel and location in rb-tree for nontransmit bss entries

Sergey Matyukevich (2):
  cfg80211: refactor cfg80211_bss_update
  cfg80211: fix duplicated scan entries after channel switch

 net/wireless/core.h    |   2 +
 net/wireless/nl80211.c |   2 +-
 net/wireless/scan.c    | 250 +++++++++++++++++++++++++++++++++----------------
 3 files changed, 171 insertions(+), 83 deletions(-)

-- 
2.11.0

[RFC PATCH v3 1/2] cfg80211: refactor cfg80211_bss_update

[Sergey Matyukevich]

This patch implements minor refactoring for cfg80211_bss_update function.
Code path for updating known BSS is extracted into dedicated
cfg80211_update_known_bss function.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
 net/wireless/scan.c | 171 +++++++++++++++++++++++++++-------------------------
 1 file changed, 89 insertions(+), 82 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index d66e6d4b7555..9f21162f05e9 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1091,6 +1091,93 @@ struct cfg80211_non_tx_bss {
 	u8 bssid_index;
 };
 
+static bool
+cfg80211_update_known_bss(struct cfg80211_registered_device *rdev,
+			  struct cfg80211_internal_bss *known,
+			  struct cfg80211_internal_bss *new,
+			  bool signal_valid)
+{
+	lockdep_assert_held(&rdev->bss_lock);
+
+	/* Update IEs */
+	if (rcu_access_pointer(new->pub.proberesp_ies)) {
+		const struct cfg80211_bss_ies *old;
+
+		old = rcu_access_pointer(known->pub.proberesp_ies);
+
+		rcu_assign_pointer(known->pub.proberesp_ies,
+				   new->pub.proberesp_ies);
+		/* Override possible earlier Beacon frame IEs */
+		rcu_assign_pointer(known->pub.ies,
+				   new->pub.proberesp_ies);
+		if (old)
+			kfree_rcu((struct cfg80211_bss_ies *)old, rcu_head);
+	} else if (rcu_access_pointer(new->pub.beacon_ies)) {
+		const struct cfg80211_bss_ies *old;
+		struct cfg80211_internal_bss *bss;
+
+		if (known->pub.hidden_beacon_bss &&
+		    !list_empty(&known->hidden_list)) {
+			const struct cfg80211_bss_ies *f;
+
+			/* The known BSS struct is one of the probe
+			 * response members of a group, but we're
+			 * receiving a beacon (beacon_ies in the new
+			 * bss is used). This can only mean that the
+			 * AP changed its beacon from not having an
+			 * SSID to showing it, which is confusing so
+			 * drop this information.
+			 */
+
+			f = rcu_access_pointer(new->pub.beacon_ies);
+			kfree_rcu((struct cfg80211_bss_ies *)f, rcu_head);
+			return false;
+		}
+
+		old = rcu_access_pointer(known->pub.beacon_ies);
+
+		rcu_assign_pointer(known->pub.beacon_ies, new->pub.beacon_ies);
+
+		/* Override IEs if they were from a beacon before */
+		if (old == rcu_access_pointer(known->pub.ies))
+			rcu_assign_pointer(known->pub.ies, new->pub.beacon_ies);
+
+		/* Assign beacon IEs to all sub entries */
+		list_for_each_entry(bss, &known->hidden_list, hidden_list) {
+			const struct cfg80211_bss_ies *ies;
+
+			ies = rcu_access_pointer(bss->pub.beacon_ies);
+			WARN_ON(ies != old);
+
+			rcu_assign_pointer(bss->pub.beacon_ies,
+					   new->pub.beacon_ies);
+		}
+
+		if (old)
+			kfree_rcu((struct cfg80211_bss_ies *)old, rcu_head);
+	}
+
+	known->pub.beacon_interval = new->pub.beacon_interval;
+
+	/* don't update the signal if beacon was heard on
+	 * adjacent channel.
+	 */
+	if (signal_valid)
+		known->pub.signal = new->pub.signal;
+	known->pub.capability = new->pub.capability;
+	known->ts = new->ts;
+	known->ts_boottime = new->ts_boottime;
+	known->parent_tsf = new->parent_tsf;
+	known->pub.chains = new->pub.chains;
+	memcpy(known->pub.chain_signal, new->pub.chain_signal,
+	       IEEE80211_MAX_CHAINS);
+	ether_addr_copy(known->parent_bssid, new->parent_bssid);
+	known->pub.max_bssid_indicator = new->pub.max_bssid_indicator;
+	known->pub.bssid_index = new->pub.bssid_index;
+
+	return true;
+}
+
 /* Returned bss is reference counted and must be cleaned up appropriately. */
 struct cfg80211_internal_bss *
 cfg80211_bss_update(struct cfg80211_registered_device *rdev,
@@ -1114,88 +1201,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *rdev,
 	found = rb_find_bss(rdev, tmp, BSS_CMP_REGULAR);
 
 	if (found) {
-		/* Update IEs */
-		if (rcu_access_pointer(tmp->pub.proberesp_ies)) {
-			const struct cfg80211_bss_ies *old;
-
-			old = rcu_access_pointer(found->pub.proberesp_ies);
-
-			rcu_assign_pointer(found->pub.proberesp_ies,
-					   tmp->pub.proberesp_ies);
-			/* Override possible earlier Beacon frame IEs */
-			rcu_assign_pointer(found->pub.ies,
-					   tmp->pub.proberesp_ies);
-			if (old)
-				kfree_rcu((struct cfg80211_bss_ies *)old,
-					  rcu_head);
-		} else if (rcu_access_pointer(tmp->pub.beacon_ies)) {
-			const struct cfg80211_bss_ies *old;
-			struct cfg80211_internal_bss *bss;
-
-			if (found->pub.hidden_beacon_bss &&
-			    !list_empty(&found->hidden_list)) {
-				const struct cfg80211_bss_ies *f;
-
-				/*
-				 * The found BSS struct is one of the probe
-				 * response members of a group, but we're
-				 * receiving a beacon (beacon_ies in the tmp
-				 * bss is used). This can only mean that the
-				 * AP changed its beacon from not having an
-				 * SSID to showing it, which is confusing so
-				 * drop this information.
-				 */
-
-				f = rcu_access_pointer(tmp->pub.beacon_ies);
-				kfree_rcu((struct cfg80211_bss_ies *)f,
-					  rcu_head);
-				goto drop;
-			}
-
-			old = rcu_access_pointer(found->pub.beacon_ies);
-
-			rcu_assign_pointer(found->pub.beacon_ies,
-					   tmp->pub.beacon_ies);
-
-			/* Override IEs if they were from a beacon before */
-			if (old == rcu_access_pointer(found->pub.ies))
-				rcu_assign_pointer(found->pub.ies,
-						   tmp->pub.beacon_ies);
-
-			/* Assign beacon IEs to all sub entries */
-			list_for_each_entry(bss, &found->hidden_list,
-					    hidden_list) {
-				const struct cfg80211_bss_ies *ies;
-
-				ies = rcu_access_pointer(bss->pub.beacon_ies);
-				WARN_ON(ies != old);
-
-				rcu_assign_pointer(bss->pub.beacon_ies,
-						   tmp->pub.beacon_ies);
-			}
-
-			if (old)
-				kfree_rcu((struct cfg80211_bss_ies *)old,
-					  rcu_head);
-		}
-
-		found->pub.beacon_interval = tmp->pub.beacon_interval;
-		/*
-		 * don't update the signal if beacon was heard on
-		 * adjacent channel.
-		 */
-		if (signal_valid)
-			found->pub.signal = tmp->pub.signal;
-		found->pub.capability = tmp->pub.capability;
-		found->ts = tmp->ts;
-		found->ts_boottime = tmp->ts_boottime;
-		found->parent_tsf = tmp->parent_tsf;
-		found->pub.chains = tmp->pub.chains;
-		memcpy(found->pub.chain_signal, tmp->pub.chain_signal,
-		       IEEE80211_MAX_CHAINS);
-		ether_addr_copy(found->parent_bssid, tmp->parent_bssid);
-		found->pub.max_bssid_indicator = tmp->pub.max_bssid_indicator;
-		found->pub.bssid_index = tmp->pub.bssid_index;
+		if (!cfg80211_update_known_bss(rdev, found, tmp, signal_valid))
+			goto drop;
 	} else {
 		struct cfg80211_internal_bss *new;
 		struct cfg80211_internal_bss *hidden;
-- 
2.11.0

Re: [RFC PATCH v3 1/2] cfg80211: refactor cfg80211_bss_update

[Johannes Berg]

On Wed, 2019-07-10 at 17:37 +0000, Sergey Matyukevich wrote:
> This patch implements minor refactoring for cfg80211_bss_update function.
> Code path for updating known BSS is extracted into dedicated
> cfg80211_update_known_bss function.
> 

Looks good, thanks for splitting this out.

I'm not going to apply anything until net-next also opens up again
though.

johannes

[RFC PATCH v3 2/2] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich]

When associated BSS completes channel switch procedure, its channel record
needs to be updated. The existing mac80211 solution was extended to
cfg80211 in commit 5dc8cdce1d72 ("mac80211/cfg80211: update bss
channel on channel switch")

However that solution still appears to be incomplete as it may lead
to duplicated scan entries for associated BSS after channel switch.
The root cause of the problem is as follows. Each BSS entry is
included into the following data structures:
- bss list rdev->bss_list
- bss search tree rdev->bss_tree
Updating BSS channel record without rebuilding bss_tree may break
tree search since cmp_bss considers all of the following: channel,
bssid, ssid. When BSS channel is updated, but its location in bss_tree
is not updated, then subsequent search operations may fail to locate
this BSS since they will be traversing bss_tree in wrong direction.
As a result, for scan performed after associated BSS channel switch,
cfg80211_bss_update may add the second entry for the same BSS to both
bss_list and bss_tree, rather then update the existing one.

To summarize, if BSS channel needs to be updated, then bss_tree should
be rebuilt in order to put updated BSS entry into a proper location.

This commit suggests the following straightforward solution:
- if new entry has been already created for BSS after channel switch,
  then use its IEs to update known BSS entry and then remove new
  entry completely
- use rb_erase/rb_insert_bss reinstall updated BSS in bss_tree
- update channel and location in rb-tree for non-transmitting bss entries

Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---

Cover email is not attached to patchwork, so duplicate it here as well...

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.

Basic use-case tested using both iwlwifi and qtnfmac. However multi-BSSID
support has not yet been tested. 

v1 -> v2
- use IEs of new BSS entry to update known BSS entry
  for this purpose extract BSS update code from cfg80211_bss_update
  into a separate function cfg80211_update_known_bss

v2 -> v3
- minor cleanup according to review comments
- split cfg80211_update_known_bss function into a separate patch
- update channel and location in rb-tree for nontransmit bss entries


Regards,
Sergey

---
 net/wireless/core.h    |  2 ++
 net/wireless/nl80211.c |  2 +-
 net/wireless/scan.c    | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/net/wireless/core.h b/net/wireless/core.h
index ee8388fe4a92..77556c58d9ac 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -306,6 +306,8 @@ void ieee80211_set_bitrate_flags(struct wiphy *wiphy);
 void cfg80211_bss_expire(struct cfg80211_registered_device *rdev);
 void cfg80211_bss_age(struct cfg80211_registered_device *rdev,
                       unsigned long age_secs);
+void cfg80211_update_assoc_bss_entry(struct wireless_dev *wdev,
+				     struct ieee80211_channel *channel);
 
 /* IBSS */
 int __cfg80211_join_ibss(struct cfg80211_registered_device *rdev,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fc83dd179c1a..6ebb427883d0 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -16092,7 +16092,7 @@ void cfg80211_ch_switch_notify(struct net_device *dev,
 
 	if (wdev->iftype == NL80211_IFTYPE_STATION &&
 	    !WARN_ON(!wdev->current_bss))
-		wdev->current_bss->pub.channel = chandef->chan;
+		cfg80211_update_assoc_bss_entry(wdev, chandef->chan);
 
 	nl80211_ch_switch_notify(rdev, dev, chandef, GFP_KERNEL,
 				 NL80211_CMD_CH_SWITCH_NOTIFY, 0);
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 9f21162f05e9..30932b955ebc 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -2002,6 +2002,85 @@ void cfg80211_bss_iter(struct wiphy *wiphy,
 }
 EXPORT_SYMBOL(cfg80211_bss_iter);
 
+void cfg80211_update_assoc_bss_entry(struct wireless_dev *wdev,
+				     struct ieee80211_channel *chan)
+{
+	struct wiphy *wiphy = wdev->wiphy;
+	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+	struct cfg80211_internal_bss *cbss = wdev->current_bss;
+	struct cfg80211_internal_bss *new = NULL;
+	struct cfg80211_internal_bss *bss;
+	struct cfg80211_bss *nontrans_bss;
+	struct cfg80211_bss *tmp;
+
+	spin_lock_bh(&rdev->bss_lock);
+
+	if (WARN_ON(cbss->pub.channel == chan))
+		goto done;
+
+	/* use transmitting bss */
+	if (cbss->pub.transmitted_bss)
+		cbss = container_of(cbss->pub.transmitted_bss,
+				    struct cfg80211_internal_bss,
+				    pub);
+
+	cbss->pub.channel = chan;
+
+	list_for_each_entry(bss, &rdev->bss_list, list) {
+		if (!cfg80211_bss_type_match(bss->pub.capability,
+					     bss->pub.channel->band,
+					     wdev->conn_bss_type))
+			continue;
+
+		if (bss == cbss)
+			continue;
+
+		if (!cmp_bss(&bss->pub, &cbss->pub, BSS_CMP_REGULAR)) {
+			new = bss;
+			break;
+		}
+	}
+
+	if (new) {
+		/* to save time, update IEs for trasmitted bss only */
+		if (cfg80211_update_known_bss(rdev, cbss, new, false)) {
+			new->pub.proberesp_ies = NULL;
+			new->pub.beacon_ies = NULL;
+		}
+
+		list_for_each_entry_safe(nontrans_bss, tmp,
+					 &new->pub.nontrans_list,
+					 nontrans_list) {
+			bss = container_of(nontrans_bss,
+					   struct cfg80211_internal_bss, pub);
+			if (__cfg80211_unlink_bss(rdev, bss))
+				rdev->bss_generation++;
+		}
+
+		WARN_ON(atomic_read(&new->hold));
+		if (!WARN_ON(!__cfg80211_unlink_bss(rdev, new)))
+			rdev->bss_generation++;
+	}
+
+	rb_erase(&cbss->rbn, &rdev->bss_tree);
+	rb_insert_bss(rdev, cbss);
+	rdev->bss_generation++;
+
+	list_for_each_entry_safe(nontrans_bss, tmp,
+				 &cbss->pub.nontrans_list,
+				 nontrans_list) {
+		bss = container_of(nontrans_bss,
+				   struct cfg80211_internal_bss, pub);
+		bss->pub.channel = chan;
+		rb_erase(&bss->rbn, &rdev->bss_tree);
+		rb_insert_bss(rdev, bss);
+		rdev->bss_generation++;
+	}
+
+done:
+	spin_unlock_bh(&rdev->bss_lock);
+}
+
 #ifdef CONFIG_CFG80211_WEXT
 static struct cfg80211_registered_device *
 cfg80211_get_dev_from_ifindex(struct net *net, int ifindex)
-- 
2.11.0

Re: [RFC PATCH v3 2/2] cfg80211: fix duplicated scan entries after channel switch

[Johannes Berg]

Umm, regarding multi-BSSID, I'm clearly just not paying any attention
... sorry about that.


This looks good to me, can you resend as just PATCH?

Thanks,
johannes

Re: [RFC PATCH v3 2/2] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich]

> Umm, regarding multi-BSSID, I'm clearly just not paying any attention
> ... sorry about that.
> 
> 
> This looks good to me, can you resend as just PATCH?

Sure, I will rebase on top of latest mac80211-next and resend.

Regards,
Sergey

Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Johannes Berg]

> 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

Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich]

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

Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Johannes Berg]

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

Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich]

> > > [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

Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Johannes Berg]

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

Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich]

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

Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Johannes Berg]

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