[PATCH 1/4] mac80211: leave mesh when stopping the netdev

[PATCH 1/4] mac80211: leave mesh when stopping the netdev

[Thomas Pedersen]

Previously, we would merely stop the beacon and fail to free any mesh
specific data structures on netdev down or unregister. Split mesh
beacon, data, timer, and work cancel steps into ieee80211_mesh_stop() to
avoid decrementing the filter counters twice, and call this in the
ndo_stop handler.

Also some improvements to ieee80211_mesh_stop():

 - flush mpath entries.
 - flush sta entries per-sdata so we don't remove entries belonging to
   other vifs on the same hw.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/iface.c |   12 ++++--------
 net/mac80211/mesh.c  |   16 +++++++++++++---
 net/mac80211/mesh.h  |    1 +
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index bfb57dc..80e0765 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -689,6 +689,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		local->fif_probe_req--;
 	} else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) {
 		local->fif_probe_req--;
+	} else if (ieee80211_vif_is_mesh(&sdata->vif)) {
+		local->fif_other_bss--;
 	}
 
 	netif_addr_lock_bh(sdata->dev);
@@ -733,6 +735,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		skb_queue_purge(&sdata->u.ap.ps_bc_buf);
 	} else if (sdata->vif.type == NL80211_IFTYPE_STATION) {
 		ieee80211_mgd_stop(sdata);
+	} else if (ieee80211_vif_is_mesh(&sdata->vif)) {
+		ieee80211_mesh_stop(sdata);
 	}
 
 	if (going_down)
@@ -771,14 +775,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 		skb_queue_purge(&sdata->skb_queue);
 
 		/*
-		 * Disable beaconing here for mesh only, AP and IBSS
-		 * are already taken care of.
-		 */
-		if (sdata->vif.type == NL80211_IFTYPE_MESH_POINT)
-			ieee80211_bss_info_change_notify(sdata,
-				BSS_CHANGED_BEACON_ENABLED);
-
-		/*
 		 * Free all remaining keys, there shouldn't be any,
 		 * except maybe group keys in AP more or WDS?
 		 */
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 764593d..4dd74a0 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -611,14 +611,17 @@ void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata)
 						BSS_CHANGED_BEACON_INT);
 }
 
-void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
+void ieee80211_mesh_stop(struct ieee80211_sub_if_data *sdata)
 {
-	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 
+	/* stop the beacon */
 	ifmsh->mesh_id_len = 0;
 	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED);
-	sta_info_flush(local, NULL);
+
+	/* flush STAs and mpaths on this iface */
+	sta_info_flush(sdata->local, sdata);
+	mesh_path_flush_by_iface(sdata);
 
 	del_timer_sync(&sdata->u.mesh.housekeeping_timer);
 	del_timer_sync(&sdata->u.mesh.mesh_path_root_timer);
@@ -630,6 +633,13 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
 	 * it no longer is.
 	 */
 	cancel_work_sync(&sdata->work);
+}
+
+void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+
+	ieee80211_mesh_stop(sdata);
 
 	local->fif_other_bss--;
 	atomic_dec(&local->iff_allmultis);
diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h
index faaa39b..2a21da1 100644
--- a/net/mac80211/mesh.h
+++ b/net/mac80211/mesh.h
@@ -252,6 +252,7 @@ void ieee80211s_stop(void);
 void ieee80211_mesh_init_sdata(struct ieee80211_sub_if_data *sdata);
 void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata);
 void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata);
+void ieee80211_mesh_stop(struct ieee80211_sub_if_data *sdata);
 void ieee80211_mesh_root_setup(struct ieee80211_if_mesh *ifmsh);
 struct ieee80211_mesh_sync_ops *ieee80211_mesh_sync_ops_get(u8 method);
 
-- 
1.7.5.4

[PATCH 2/4] mac80211: remove ieee80211_clean_sdata()

[Thomas Pedersen]

This function was only used by mesh, and not really needed since any
interface-specific cleanup already happens in the netdev handlers.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/iface.c |   17 -----------------
 1 files changed, 0 insertions(+), 17 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 80e0765..844a83f 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1152,18 +1152,6 @@ static void ieee80211_setup_sdata(struct ieee80211_sub_if_data *sdata,
 	ieee80211_debugfs_add_netdev(sdata);
 }
 
-static void ieee80211_clean_sdata(struct ieee80211_sub_if_data *sdata)
-{
-	switch (sdata->vif.type) {
-	case NL80211_IFTYPE_MESH_POINT:
-		mesh_path_flush_by_iface(sdata);
-		break;
-
-	default:
-		break;
-	}
-}
-
 static int ieee80211_runtime_change_iftype(struct ieee80211_sub_if_data *sdata,
 					   enum nl80211_iftype type)
 {
@@ -1499,9 +1487,6 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata)
 	list_del_rcu(&sdata->list);
 	mutex_unlock(&sdata->local->iflist_mtx);
 
-	/* clean up type-dependent data */
-	ieee80211_clean_sdata(sdata);
-
 	synchronize_rcu();
 	unregister_netdevice(sdata->dev);
 }
@@ -1521,8 +1506,6 @@ void ieee80211_remove_interfaces(struct ieee80211_local *local)
 	list_for_each_entry_safe(sdata, tmp, &local->interfaces, list) {
 		list_del(&sdata->list);
 
-		ieee80211_clean_sdata(sdata);
-
 		unregister_netdevice_queue(sdata->dev, &unreg_list);
 	}
 	mutex_unlock(&local->iflist_mtx);
-- 
1.7.5.4

[PATCH 3/4] mac80211: don't request ack for peering close

[Thomas Pedersen]

It doesn't make a lot of sense to wait for an ack in response to a
peering close frame since either peer in this exchange could be going
down.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/mesh_plink.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 4256859..fba8d86 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -217,6 +217,7 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 		u8 *da, __le16 llid, __le16 plid, __le16 reason) {
 	struct ieee80211_local *local = sdata->local;
 	struct sk_buff *skb;
+	struct ieee80211_tx_info *info;
 	struct ieee80211_mgmt *mgmt;
 	bool include_plid = false;
 	u16 peering_proto = 0;
@@ -238,6 +239,7 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 			    sdata->u.mesh.ie_len);
 	if (!skb)
 		return -1;
+	info = IEEE80211_SKB_CB(skb);
 	skb_reserve(skb, local->tx_headroom);
 	mgmt = (struct ieee80211_mgmt *) skb_put(skb, hdr_len);
 	memset(mgmt, 0, hdr_len);
@@ -265,6 +267,7 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 		    mesh_add_meshconf_ie(skb, sdata))
 			return -1;
 	} else {	/* WLAN_SP_MESH_PEERING_CLOSE */
+		info->flags |= IEEE80211_TX_CTL_NO_ACK;
 		if (mesh_add_meshid_ie(skb, sdata))
 			return -1;
 	}
-- 
1.7.5.4

[PATCH 4/4] mac80211: skb leak in mesh_plink_frame_tx()

[Thomas Pedersen]

Although adding an IE is almost guaranteed to succeed since we already
accounted for its length while allocating the skb, we should still free
the skb in case of failure.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/mesh_plink.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index fba8d86..5a68097 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -224,6 +224,7 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 	u8 *pos, ie_len = 4;
 	int hdr_len = offsetof(struct ieee80211_mgmt, u.action.u.self_prot) +
 		      sizeof(mgmt->u.action.u.self_prot);
+	int err = -ENOMEM;
 
 	skb = dev_alloc_skb(local->tx_headroom +
 			    hdr_len +
@@ -265,11 +266,11 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 		    mesh_add_rsn_ie(skb, sdata) ||
 		    mesh_add_meshid_ie(skb, sdata) ||
 		    mesh_add_meshconf_ie(skb, sdata))
-			return -1;
+			goto free;
 	} else {	/* WLAN_SP_MESH_PEERING_CLOSE */
 		info->flags |= IEEE80211_TX_CTL_NO_ACK;
 		if (mesh_add_meshid_ie(skb, sdata))
-			return -1;
+			goto free;
 	}
 
 	/* Add Mesh Peering Management element */
@@ -288,11 +289,12 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 		ie_len += 2;	/* reason code */
 		break;
 	default:
-		return -EINVAL;
+		err = -EINVAL;
+		goto free;
 	}
 
 	if (WARN_ON(skb_tailroom(skb) < 2 + ie_len))
-		return -ENOMEM;
+		goto free;
 
 	pos = skb_put(skb, 2 + ie_len);
 	*pos++ = WLAN_EID_PEER_MGMT;
@@ -313,14 +315,18 @@ static int mesh_plink_frame_tx(struct ieee80211_sub_if_data *sdata,
 	if (action != WLAN_SP_MESH_PEERING_CLOSE) {
 		if (mesh_add_ht_cap_ie(skb, sdata) ||
 		    mesh_add_ht_oper_ie(skb, sdata))
-			return -1;
+			goto free;
 	}
 
 	if (mesh_add_vendor_ies(skb, sdata))
-		return -1;
+		goto free;
 
 	ieee80211_tx_skb(sdata, skb);
 	return 0;
+
+free:
+	kfree_skb(skb);
+	return err;
 }
 
 /* mesh_peer_init - initialize new mesh peer and return resulting sta_info
-- 
1.7.5.4

Re: [PATCH 1/4] mac80211: leave mesh when stopping the netdev

[Johannes Berg]

On Thu, 2012-08-02 at 16:47 -0700, Thomas Pedersen wrote:
> Previously, we would merely stop the beacon and fail to free any mesh
> specific data structures on netdev down or unregister. Split mesh
> beacon, data, timer, and work cancel steps into ieee80211_mesh_stop() to
> avoid decrementing the filter counters twice, and call this in the
> ndo_stop handler.

Now you're going overboard a bit :-)

This isn't needed, as cfg80211 will explicitly leave the mesh while the
netdev is going down (NETDEV_GOING_DOWN notification handler) without
mac80211 even knowing yet that it's be going down.

> +++ b/net/mac80211/iface.c
> @@ -689,6 +689,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>  		local->fif_probe_req--;
>  	} else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) {
>  		local->fif_probe_req--;
> +	} else if (ieee80211_vif_is_mesh(&sdata->vif)) {
> +		local->fif_other_bss--;

I think this may be wrong since it's only incremented when we join the
mesh, so it will cause the number to become negative if we never join
the mesh, or if we correctly leave it before going down (as we actually
do)

> @@ -733,6 +735,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>  		skb_queue_purge(&sdata->u.ap.ps_bc_buf);
>  	} else if (sdata->vif.type == NL80211_IFTYPE_STATION) {
>  		ieee80211_mgd_stop(sdata);
> +	} else if (ieee80211_vif_is_mesh(&sdata->vif)) {
> +		ieee80211_mesh_stop(sdata);
>  	}

This isn't needed, since we already called stop_mesh from cfg.c via
cfg80211.

> @@ -771,14 +775,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>  		skb_queue_purge(&sdata->skb_queue);
>  
>  		/*
> -		 * Disable beaconing here for mesh only, AP and IBSS
> -		 * are already taken care of.
> -		 */
> -		if (sdata->vif.type == NL80211_IFTYPE_MESH_POINT)
> -			ieee80211_bss_info_change_notify(sdata,
> -				BSS_CHANGED_BEACON_ENABLED);

This makes sense, it's already done anyway.


> --- a/net/mac80211/mesh.c
> +++ b/net/mac80211/mesh.c
> @@ -611,14 +611,17 @@ void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata)
>  						BSS_CHANGED_BEACON_INT);
>  }
>  
> -void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
> +void ieee80211_mesh_stop(struct ieee80211_sub_if_data *sdata)
>  {
> -	struct ieee80211_local *local = sdata->local;
>  	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
>  
> +	/* stop the beacon */
>  	ifmsh->mesh_id_len = 0;
>  	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED);
> -	sta_info_flush(local, NULL);
> +
> +	/* flush STAs and mpaths on this iface */
> +	sta_info_flush(sdata->local, sdata);
> +	mesh_path_flush_by_iface(sdata);
>  
>  	del_timer_sync(&sdata->u.mesh.housekeeping_timer);
>  	del_timer_sync(&sdata->u.mesh.mesh_path_root_timer);

This also makes sense, of course.

> @@ -630,6 +633,13 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
>  	 * it no longer is.
>  	 */
>  	cancel_work_sync(&sdata->work);
> +}
> +
> +void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +
> +	ieee80211_mesh_stop(sdata);
>  
>  	local->fif_other_bss--;
>  	atomic_dec(&local->iff_allmultis);

Ok so you're trying to split it up, but I don't think that makes sense
since we'll always call stop_mesh() before the interface goes down.

johannes

Re: [PATCH 1/4] mac80211: leave mesh when stopping the netdev

[Thomas Pedersen]

On Thu, Aug 2, 2012 at 11:43 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-08-02 at 16:47 -0700, Thomas Pedersen wrote:
>> Previously, we would merely stop the beacon and fail to free any mesh
>> specific data structures on netdev down or unregister. Split mesh
>> beacon, data, timer, and work cancel steps into ieee80211_mesh_stop() to
>> avoid decrementing the filter counters twice, and call this in the
>> ndo_stop handler.
>
> Now you're going overboard a bit :-)
>
> This isn't needed, as cfg80211 will explicitly leave the mesh while the
> netdev is going down (NETDEV_GOING_DOWN notification handler) without
> mac80211 even knowing yet that it's be going down.

Thanks for pointing that out. It seems we still need to avoid
decrementing the filter counters twice (once by cfg80211 in
ieee80211_stop_mesh() and again by mac80211 in the ndo_stop callback).
I'll work on this and submit a v2.

>> +++ b/net/mac80211/iface.c
>> @@ -689,6 +689,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>>               local->fif_probe_req--;
>>       } else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) {
>>               local->fif_probe_req--;
>> +     } else if (ieee80211_vif_is_mesh(&sdata->vif)) {
>> +             local->fif_other_bss--;
>
> I think this may be wrong since it's only incremented when we join the
> mesh, so it will cause the number to become negative if we never join
> the mesh, or if we correctly leave it before going down (as we actually
> do)
>
>> @@ -733,6 +735,8 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>>               skb_queue_purge(&sdata->u.ap.ps_bc_buf);
>>       } else if (sdata->vif.type == NL80211_IFTYPE_STATION) {
>>               ieee80211_mgd_stop(sdata);
>> +     } else if (ieee80211_vif_is_mesh(&sdata->vif)) {
>> +             ieee80211_mesh_stop(sdata);
>>       }
>
> This isn't needed, since we already called stop_mesh from cfg.c via
> cfg80211.
>
>> @@ -771,14 +775,6 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
>>               skb_queue_purge(&sdata->skb_queue);
>>
>>               /*
>> -              * Disable beaconing here for mesh only, AP and IBSS
>> -              * are already taken care of.
>> -              */
>> -             if (sdata->vif.type == NL80211_IFTYPE_MESH_POINT)
>> -                     ieee80211_bss_info_change_notify(sdata,
>> -                             BSS_CHANGED_BEACON_ENABLED);
>
> This makes sense, it's already done anyway.
>
>
>> --- a/net/mac80211/mesh.c
>> +++ b/net/mac80211/mesh.c
>> @@ -611,14 +611,17 @@ void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata)
>>                                               BSS_CHANGED_BEACON_INT);
>>  }
>>
>> -void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
>> +void ieee80211_mesh_stop(struct ieee80211_sub_if_data *sdata)
>>  {
>> -     struct ieee80211_local *local = sdata->local;
>>       struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
>>
>> +     /* stop the beacon */
>>       ifmsh->mesh_id_len = 0;
>>       ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED);
>> -     sta_info_flush(local, NULL);
>> +
>> +     /* flush STAs and mpaths on this iface */
>> +     sta_info_flush(sdata->local, sdata);
>> +     mesh_path_flush_by_iface(sdata);
>>
>>       del_timer_sync(&sdata->u.mesh.housekeeping_timer);
>>       del_timer_sync(&sdata->u.mesh.mesh_path_root_timer);
>
> This also makes sense, of course.
>
>> @@ -630,6 +633,13 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
>>        * it no longer is.
>>        */
>>       cancel_work_sync(&sdata->work);
>> +}
>> +
>> +void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
>> +{
>> +     struct ieee80211_local *local = sdata->local;
>> +
>> +     ieee80211_mesh_stop(sdata);
>>
>>       local->fif_other_bss--;
>>       atomic_dec(&local->iff_allmultis);
>
> Ok so you're trying to split it up, but I don't think that makes sense
> since we'll always call stop_mesh() before the interface goes down.
>
> johannes
>