mac80211: Fix mesh estab_plinks counting in STA removal case

Commit Message

Bob Copeland June 21, 2016, 10:26 p.m. UTC

On Tue, Jun 21, 2016 at 09:09:10PM +0200, Johannes Berg wrote:
> 
> >  	 */
> > -	changed = mesh_accept_plinks_update(sdata);
> > +	if (sdata->u.mesh.user_mpm &&
> > +	    sta->mesh->plink_state == NL80211_PLINK_ESTAB)
> > +		changed |= mesh_plink_dec_estab_count(sdata);
> > +	changed |= mesh_accept_plinks_update(sdata);
> >  	if (!sdata->u.mesh.user_mpm) {
> >  		changed |= mesh_plink_deactivate(sta);
> >  		del_timer_sync(&sta->mesh->plink_timer);
> > 
> 
> Does it have to be done before the mesh_accept_plinks_update()?
> 
> If not, you should put it with the existing u.mesh.user_mpm check. If
> yes, then the code is further buggy since only mesh_plink_deactivate()
> will call it when the kernel MPM is used.

Looks further buggy, so perhaps this untested patch would work, i.e.
move the accepting-plinks change closer to the decrement, and push
the user_mpm check down into mesh_plink_deactivate to just
avoid sending the peering frames.  [There's also a bit there for
power saving that we likely want to keep for secure networks.]

Then again maybe accepting_plinks flag should just be computed when
used instead of tracking the state separately.

---
 net/mac80211/mesh.c       | 18 ++----------------
 net/mac80211/mesh_plink.c | 16 ++++++++++++----
 2 files changed, 14 insertions(+), 20 deletions(-)

Comments

Bob Copeland June 21, 2016, 10:29 p.m. UTC | #1

On Tue, Jun 21, 2016 at 06:26:02PM -0400, Bob Copeland wrote:
> Looks further buggy, so perhaps this untested patch would work, i.e.
                                       ^^^^^^^^ (still)

> @@ -370,13 +372,19 @@ u32 mesh_plink_deactivate(struct sta_info *sta)
>  
>  	spin_lock_bh(&sta->mesh->plink_lock);
>  	changed = __mesh_plink_deactivate(sta);
> -	sta->mesh->reason = WLAN_REASON_MESH_PEER_CANCELED;
> -	mesh_plink_frame_tx(sdata, sta, WLAN_SP_MESH_PEERING_CLOSE,
> -			    sta->sta.addr, sta->mesh->llid, sta->mesh->plid,
> -			    sta->mesh->reason);
> +
> +	if (sdata->u.mesh.user_mpm) {

should be !sdata->u.mesh.user_mpm :)

> +		sta->mesh->reason = WLAN_REASON_MESH_PEER_CANCELED;
> +		mesh_plink_frame_tx(sdata, sta, WLAN_SP_MESH_PEERING_CLOSE,
> +				    sta->sta.addr, sta->mesh->llid, sta->mesh->plid,
> +				    sta->mesh->reason);
> +	}
>  	spin_unlock_bh(&sta->mesh->plink_lock);
>  	mesh_path_flush_by_nexthop(sta);
>  
> +	/* make sure no readers can access nexthop sta from here on */
> +	synchronize_net();
> +
>  	return changed;
>  }
>  
> -- 
> 2.9.0
>

Patch

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 21b1fdf..3c150f8 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -148,22 +148,8 @@  u32 mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata)
 void mesh_sta_cleanup(struct sta_info *sta)
 {
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
-	u32 changed;
-
-	/*
-	 * maybe userspace handles peer allocation and peering, but in either
-	 * case the beacon is still generated by the kernel and we might need
-	 * an update.
-	 */
-	changed = mesh_accept_plinks_update(sdata);
-	if (!sdata->u.mesh.user_mpm) {
-		changed |= mesh_plink_deactivate(sta);
-		del_timer_sync(&sta->mesh->plink_timer);
-	}
-
-	/* make sure no readers can access nexthop sta from here on */
-	mesh_path_flush_by_nexthop(sta);
-	synchronize_net();
+	u32 changed = mesh_plink_deactivate(sta);
+	del_timer_sync(&sta->mesh->plink_timer);
 
 	if (changed)
 		ieee80211_mbss_info_change_notify(sdata, changed);
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 79f2a0a..69ac7a8 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -349,6 +349,8 @@  static u32 __mesh_plink_deactivate(struct sta_info *sta)
 		changed = mesh_plink_dec_estab_count(sdata);
 	sta->mesh->plink_state = NL80211_PLINK_BLOCKED;
 
+	changed |= mesh_accept_plinks_update(sdata);
+
 	ieee80211_mps_sta_status_update(sta);
 	changed |= ieee80211_mps_set_sta_local_pm(sta,
 			NL80211_MESH_POWER_UNKNOWN);
@@ -370,13 +372,19 @@  u32 mesh_plink_deactivate(struct sta_info *sta)
 
 	spin_lock_bh(&sta->mesh->plink_lock);
 	changed = __mesh_plink_deactivate(sta);
-	sta->mesh->reason = WLAN_REASON_MESH_PEER_CANCELED;
-	mesh_plink_frame_tx(sdata, sta, WLAN_SP_MESH_PEERING_CLOSE,
-			    sta->sta.addr, sta->mesh->llid, sta->mesh->plid,
-			    sta->mesh->reason);
+
+	if (sdata->u.mesh.user_mpm) {
+		sta->mesh->reason = WLAN_REASON_MESH_PEER_CANCELED;
+		mesh_plink_frame_tx(sdata, sta, WLAN_SP_MESH_PEERING_CLOSE,
+				    sta->sta.addr, sta->mesh->llid, sta->mesh->plid,
+				    sta->mesh->reason);
+	}
 	spin_unlock_bh(&sta->mesh->plink_lock);
 	mesh_path_flush_by_nexthop(sta);
 
+	/* make sure no readers can access nexthop sta from here on */
+	synchronize_net();
+
 	return changed;
 }