diff mbox

[v6,5/5] mac80211: only set CSA beacon when at least one beacon must be transmitted

Message ID 1386078502-19845-6-git-send-email-luciano.coelho@intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Luca Coelho Dec. 3, 2013, 1:48 p.m. UTC
A beacon should never have a Channel Switch Announcement information
element with a count of 0, because a count of 1 means switch just
before the next beacon.  So, if a count of 0 was valid in a beacon, it
would have been transmitted in the next channel already, which is
useless.  A CSA count equal to zero is only meaningful in action
frames or probe_responses.

Fix the ieee80211_csa_is_complete() and ieee80211_update_csa()
functions accordingly.

With a CSA count of 0, we won't transmit any CSA beacons, because the
switch will happen before the next TBTT.  To avoid extra work and
potential confusion in the drivers, complete the CSA immediately,
instead of waiting for the driver to call ieee80211_csa_finish().

To keep things simpler, we also switch immediately when the CSA count
is 1, while in theory we should delay the switch until just before the
next TBTT.

Additionally, move the ieee80211_csa_finish() function to cfg.c,
where it makes more sense.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 include/net/mac80211.h     |  10 ++--
 net/mac80211/cfg.c         | 114 ++++++++++++++++++++++++++++++++++-----------
 net/mac80211/ibss.c        |   6 ---
 net/mac80211/ieee80211_i.h |   3 +-
 net/mac80211/mesh.c        |   6 +--
 net/mac80211/tx.c          |  19 +++-----
 6 files changed, 102 insertions(+), 56 deletions(-)

Comments

Chun-Yeow Yeoh Dec. 4, 2013, 9:35 a.m. UTC | #1
>  #ifdef CONFIG_MAC80211_MESH
>         case NL80211_IFTYPE_MESH_POINT:
> @@ -3159,12 +3208,18 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>                 if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_NONE)
>                         ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_INIT;
>
> -               err = ieee80211_mesh_csa_beacon(sdata, params,
> -                       (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT));
> -               if (err < 0) {
> -                       ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
> -                       return err;
> +               if (params->count > 1) {
> +                       err = ieee80211_mesh_csa_beacon(sdata, params);
> +                       if (err < 0) {
> +                               ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
> +                               return err;
> +                       }
> +                       changed |= err;
>                 }

Since the ieee80211_mesh_csa_beacon is not called, the kfree_rcu of
tmp_csa_settings in ieee80211_mesh_finish_csa has caused kernel oops.

Anyway, I would like to suggest if the params->count is less than 1,
the mesh channel switch is not triggered since there maybe mesh STA
located two hops away from the initiator.

if (params->count > 1) {
.....
} else
return -EINVAL;




> +
> +               if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT)
> +                       ieee80211_send_action_csa(sdata, params);
> +
>                 break;

----
Chun-Yeow
--
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
Chun-Yeow Yeoh Dec. 4, 2013, 9:45 a.m. UTC | #2
On Wed, Dec 4, 2013 at 5:35 PM, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
>>  #ifdef CONFIG_MAC80211_MESH
>>         case NL80211_IFTYPE_MESH_POINT:
>> @@ -3159,12 +3208,18 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>>                 if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_NONE)
>>                         ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_INIT;
>>
>> -               err = ieee80211_mesh_csa_beacon(sdata, params,
>> -                       (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT));
>> -               if (err < 0) {
>> -                       ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
>> -                       return err;
>> +               if (params->count > 1) {
>> +                       err = ieee80211_mesh_csa_beacon(sdata, params);
>> +                       if (err < 0) {
>> +                               ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
>> +                               return err;
>> +                       }
>> +                       changed |= err;
>>                 }
>
> Since the ieee80211_mesh_csa_beacon is not called, the kfree_rcu of
> tmp_csa_settings in ieee80211_mesh_finish_csa has caused kernel oops.
>
> Anyway, I would like to suggest if the params->count is less than 1,
> the mesh channel switch is not triggered since there maybe mesh STA
> located two hops away from the initiator.
>
> if (params->count > 1) {
> .....
> } else
> return -EINVAL;

maybe try to push the checking of params->count earlier, such as follow:

if (params->count < 2)
      return -EINVAL;

----
Chun-Yeow
--
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
Luca Coelho Dec. 5, 2013, 9:03 a.m. UTC | #3
On Wed, 2013-12-04 at 17:45 +0800, Yeoh Chun-Yeow wrote:
> On Wed, Dec 4, 2013 at 5:35 PM, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:

> >>  #ifdef CONFIG_MAC80211_MESH

> >>         case NL80211_IFTYPE_MESH_POINT:

> >> @@ -3159,12 +3208,18 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,

> >>                 if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_NONE)

> >>                         ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_INIT;

> >>

> >> -               err = ieee80211_mesh_csa_beacon(sdata, params,

> >> -                       (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT));

> >> -               if (err < 0) {

> >> -                       ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;

> >> -                       return err;

> >> +               if (params->count > 1) {

> >> +                       err = ieee80211_mesh_csa_beacon(sdata, params);

> >> +                       if (err < 0) {

> >> +                               ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;

> >> +                               return err;

> >> +                       }

> >> +                       changed |= err;

> >>                 }

> >

> > Since the ieee80211_mesh_csa_beacon is not called, the kfree_rcu of

> > tmp_csa_settings in ieee80211_mesh_finish_csa has caused kernel oops.


Ugh, right, my bad.



> > Anyway, I would like to suggest if the params->count is less than 1,

> > the mesh channel switch is not triggered since there maybe mesh STA

> > located two hops away from the initiator.

> >

> > if (params->count > 1) {

> > .....

> > } else

> > return -EINVAL;

> 

> maybe try to push the checking of params->count earlier, such as follow:

> 

> if (params->count < 2)

>       return -EINVAL;


Actually, I just looked at the specs again and the count value for mesh
is completely different.  Instead of specifying the number of TBTTs
until the switch happens, it specifies the time in TUs until the switch
happens and 0 means at any time after the frame was sent (the same as
with nonmesh).  In our implementation, we just decrease the count on
every beacon, which is wrong in the mesh case.

The count implementation for mesh is bogus.  We need to fix that.

What if, for now, I just fix the oops because of the rcu_derefence when
the rcu is not initialized and we fix the count implementation later?

Thanks a lot for testing!

--
Cheers,
Luca.
Chun-Yeow Yeoh Dec. 5, 2013, 10:35 a.m. UTC | #4
> Actually, I just looked at the specs again and the count value for mesh
> is completely different.  Instead of specifying the number of TBTTs
> until the switch happens, it specifies the time in TUs until the switch
> happens and 0 means at any time after the frame was sent (the same as
> with nonmesh).  In our implementation, we just decrease the count on
> every beacon, which is wrong in the mesh case.

Yes, you are absolutely correct.

>
> The count implementation for mesh is bogus.  We need to fix that.
>
> What if, for now, I just fix the oops because of the rcu_derefence when
> the rcu is not initialized

Sure

> and we fix the count implementation later?

Hmm, even with unit of 100TU, we can't use the current count approach
since the default TBTT for mesh is 1000 TU.

----
Chun-Yeow
--
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
diff mbox

Patch

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 73d99bc..a72d408 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2717,11 +2717,13 @@  enum ieee80211_roc_type {
  * @channel_switch_beacon: Starts a channel switch to a new channel.
  *	Beacons are modified to include CSA or ECSA IEs before calling this
  *	function. The corresponding count fields in these IEs must be
- *	decremented, and when they reach zero the driver must call
+ *	decremented, and when they reach 1 the driver must call
  *	ieee80211_csa_finish(). Drivers which use ieee80211_beacon_get()
  *	get the csa counter decremented by mac80211, but must check if it is
- *	zero using ieee80211_csa_is_complete() after the beacon has been
+ *	1 using ieee80211_csa_is_complete() after the beacon has been
  *	transmitted and then call ieee80211_csa_finish().
+ *	If the CSA count starts as zero or 1, this function will not be called,
+ *	since there won't be any time to beacon before the switch anyway.
  *
  * @join_ibss: Join an IBSS (on an IBSS interface); this is called after all
  *	information in bss_conf is set up and the beacon can be retrieved. A
@@ -3416,13 +3418,13 @@  static inline struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
  *
  * After a channel switch announcement was scheduled and the counter in this
- * announcement hit zero, this function must be called by the driver to
+ * announcement hits 1, this function must be called by the driver to
  * notify mac80211 that the channel can be changed.
  */
 void ieee80211_csa_finish(struct ieee80211_vif *vif);
 
 /**
- * ieee80211_csa_is_complete - find out if counters reached zero
+ * ieee80211_csa_is_complete - find out if counters reached 1
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
  *
  * This function returns whether the channel switch counters reached zero.
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 790c15d..0ebce55 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1,4 +1,4 @@ 
-/*
+ /*
  * mac80211 configuration hooks for cfg80211
  *
  * Copyright 2006-2010	Johannes Berg <johannes@sipsolutions.net>
@@ -2982,21 +2982,19 @@  cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
 	return new_beacon;
 }
 
-void ieee80211_csa_finalize_work(struct work_struct *work)
+void ieee80211_csa_finish(struct ieee80211_vif *vif)
 {
-	struct ieee80211_sub_if_data *sdata =
-		container_of(work, struct ieee80211_sub_if_data,
-			     csa_finalize_work);
-	struct ieee80211_local *local = sdata->local;
-	int err, changed = 0;
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
 
-	sdata_lock(sdata);
-	/* AP might have been stopped while waiting for the lock. */
-	if (!sdata->vif.csa_active)
-		goto unlock;
+	ieee80211_queue_work(&sdata->local->hw,
+			     &sdata->csa_finalize_work);
+}
+EXPORT_SYMBOL(ieee80211_csa_finish);
 
-	if (!ieee80211_sdata_running(sdata))
-		goto unlock;
+static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	int err, changed = 0;
 
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
@@ -3048,6 +3046,29 @@  unlock:
 	sdata_unlock(sdata);
 }
 
+void ieee80211_csa_finalize_work(struct work_struct *work)
+{
+	struct ieee80211_sub_if_data *sdata =
+		container_of(work, struct ieee80211_sub_if_data,
+			     csa_finalize_work);
+
+	sdata_lock(sdata);
+	/* AP might have been stopped while waiting for the lock. */
+	if (!sdata->vif.csa_active)
+		goto unlock;
+
+	if (!ieee80211_sdata_running(sdata))
+		goto unlock;
+
+	if (!ieee80211_sdata_running(sdata))
+		return;
+
+	ieee80211_csa_finalize(sdata);
+
+unlock:
+	sdata_unlock(sdata);
+}
+
 int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 			     struct cfg80211_csa_settings *params)
 {
@@ -3056,7 +3077,7 @@  int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	struct ieee80211_chanctx_conf *chanctx_conf;
 	struct ieee80211_chanctx *chanctx;
 	struct ieee80211_if_mesh __maybe_unused *ifmsh;
-	int err, num_chanctx;
+	int err, num_chanctx, changed = 0;
 
 	lockdep_assert_held(&sdata->wdev.mtx);
 
@@ -3097,19 +3118,40 @@  int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP:
-		sdata->csa_counter_offset_beacon =
-			params->counter_offset_beacon;
-		sdata->csa_counter_offset_presp = params->counter_offset_presp;
 		sdata->u.ap.next_beacon =
 			cfg80211_beacon_dup(&params->beacon_after);
 		if (!sdata->u.ap.next_beacon)
 			return -ENOMEM;
 
+		/*
+		 * With a count of 0, we don't have to wait for any
+		 * TBTT before switching, so complete the CSA
+		 * immediately.  In theory, with a count == 1 we
+		 * should delay the switch until just before the next
+		 * TBTT, but that would complicate things so we switch
+		 * immediately too.  If we would delay the switch
+		 * until the next TBTT, we would have to set the probe
+		 * response here.
+		 *
+		 * TODO: A channel switch with count <= 1 without
+		 * sending a CSA action frame is kind of useless,
+		 * because the clients won't know we're changing
+		 * channels.  The action frame must be implemented
+		 * either here or in the userspace.
+		 */
+		if (params->count <= 1)
+			break;
+
+		sdata->csa_counter_offset_beacon =
+			params->counter_offset_beacon;
+		sdata->csa_counter_offset_presp = params->counter_offset_presp;
 		err = ieee80211_assign_beacon(sdata, &params->beacon_csa);
 		if (err < 0) {
 			kfree(sdata->u.ap.next_beacon);
 			return err;
 		}
+		changed |= err;
+
 		break;
 	case NL80211_IFTYPE_ADHOC:
 		if (!sdata->vif.bss_conf.ibss_joined)
@@ -3137,9 +3179,16 @@  int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		    params->chandef.chan->band)
 			return -EINVAL;
 
-		err = ieee80211_ibss_csa_beacon(sdata, params);
-		if (err < 0)
-			return err;
+		/* see comments and TODO in the NL80211_IFTYPE_AP block */
+		if (params->count > 1) {
+			err = ieee80211_ibss_csa_beacon(sdata, params);
+			if (err < 0)
+				return err;
+			changed |= err;
+		}
+
+		ieee80211_send_action_csa(sdata, params);
+
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
@@ -3159,12 +3208,18 @@  int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_NONE)
 			ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_INIT;
 
-		err = ieee80211_mesh_csa_beacon(sdata, params,
-			(ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT));
-		if (err < 0) {
-			ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
-			return err;
+		if (params->count > 1) {
+			err = ieee80211_mesh_csa_beacon(sdata, params);
+			if (err < 0) {
+				ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
+				return err;
+			}
+			changed |= err;
 		}
+
+		if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT)
+			ieee80211_send_action_csa(sdata, params);
+
 		break;
 #endif
 	default:
@@ -3181,8 +3236,13 @@  int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	sdata->csa_chandef = params->chandef;
 	sdata->vif.csa_active = true;
 
-	ieee80211_bss_info_change_notify(sdata, err);
-	drv_channel_switch_beacon(sdata, &params->chandef);
+	if (changed) {
+		ieee80211_bss_info_change_notify(sdata, changed);
+		drv_channel_switch_beacon(sdata, &params->chandef);
+	} else {
+		/* if the beacon didn't change, we can finalize immediately */
+		ieee80211_csa_finalize(sdata);
+	}
 
 	return 0;
 }
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 23e035f..595902c 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -516,12 +516,6 @@  int ieee80211_ibss_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	if (old_presp)
 		kfree_rcu(old_presp, rcu_head);
 
-	/* it might not send the beacon for a while. send an action frame
-	 * immediately to announce the channel switch.
-	 */
-	if (csa_settings)
-		ieee80211_send_action_csa(sdata, csa_settings);
-
 	return BSS_CHANGED_BEACON;
  out:
 	return ret;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index cf38a74..9b5f1bc 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1400,8 +1400,7 @@  void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata);
 void ieee80211_mesh_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 				   struct sk_buff *skb);
 int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata,
-			      struct cfg80211_csa_settings *csa_settings,
-			      bool csa_action);
+			      struct cfg80211_csa_settings *csa_settings);
 int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata);
 
 /* scan/BSS handling */
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 5476ad9..e604b9e 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -1064,8 +1064,7 @@  int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
 }
 
 int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata,
-			      struct cfg80211_csa_settings *csa_settings,
-			      bool csa_action)
+			      struct cfg80211_csa_settings *csa_settings)
 {
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	struct mesh_csa_settings *tmp_csa_settings;
@@ -1089,9 +1088,6 @@  int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata,
 		return ret;
 	}
 
-	if (csa_action)
-		ieee80211_send_action_csa(sdata, csa_settings);
-
 	return BSS_CHANGED_BEACON;
 }
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 6d59e21..b86a679 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2397,15 +2397,6 @@  static int ieee80211_beacon_add_tim(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-void ieee80211_csa_finish(struct ieee80211_vif *vif)
-{
-	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
-
-	ieee80211_queue_work(&sdata->local->hw,
-			     &sdata->csa_finalize_work);
-}
-EXPORT_SYMBOL(ieee80211_csa_finish);
-
 static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
 				 struct beacon_data *beacon)
 {
@@ -2434,8 +2425,12 @@  static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
 	if (WARN_ON(counter_offset_beacon >= beacon_data_len))
 		return;
 
-	/* warn if the driver did not check for/react to csa completeness */
-	if (WARN_ON(beacon_data[counter_offset_beacon] == 0))
+	/* Warn if the driver did not check for/react to csa
+	 * completeness.  A beacon with CSA counter set to 0 should
+	 * never occur, because a counter of 1 means switch just
+	 * before the next beacon.
+	 */
+	if (WARN_ON(beacon_data[counter_offset_beacon] == 1))
 		return;
 
 	beacon_data[counter_offset_beacon]--;
@@ -2501,7 +2496,7 @@  bool ieee80211_csa_is_complete(struct ieee80211_vif *vif)
 	if (WARN_ON(counter_beacon > beacon_data_len))
 		goto out;
 
-	if (beacon_data[counter_beacon] == 0)
+	if (beacon_data[counter_beacon] == 1)
 		ret = true;
  out:
 	rcu_read_unlock();