Re: [PATCH] mac80211: Fix mesh estab_plinks counting in STA removal case

[Johannes Berg]

On Sun, 2016-06-19 at 23:51 +0300, Jouni Malinen wrote:
> If a user space program (e.g., wpa_supplicant) deletes a STA entry
> that
> is currently in NL80211_PLINK_ESTAB state, the number of established
> plinks counter was not decremented and this could result in rejecting
> new plink establishment before really hitting the real maximum plink
> limit. For !user_mpm case, this decrementation is handled by
> mesh_plink_deactive().
> 
> Fix this by decrementing estab_plinks on STA deletion
> (mesh_sta_cleanup() gets called from there) so that the counter has a
> correct value and the Beacon frame advertisement in Mesh
> Configuration
> element shows the proper value for capability to accept additional
> peers.
> 

Applied.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mac80211: Fix mesh estab_plinks counting in STA removal case

[Bob Copeland]

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

-- 
Bob Copeland %% http://bobcopeland.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mac80211: Fix mesh estab_plinks counting in STA removal case

[Bob Copeland]

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(>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(-)

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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mac80211: Fix mesh estab_plinks counting in STA removal case

[Johannes Berg]

>    */
> - 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(>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.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mac80211: Fix mesh estab_plinks counting in STA removal case

[Jouni Malinen]

If a user space program (e.g., wpa_supplicant) deletes a STA entry that
is currently in NL80211_PLINK_ESTAB state, the number of established
plinks counter was not decremented and this could result in rejecting
new plink establishment before really hitting the real maximum plink
limit. For !user_mpm case, this decrementation is handled by
mesh_plink_deactive().

Fix this by decrementing estab_plinks on STA deletion
(mesh_sta_cleanup() gets called from there) so that the counter has a
correct value and the Beacon frame advertisement in Mesh Configuration
element shows the proper value for capability to accept additional
peers.

Signed-off-by: Jouni Malinen 
---
 net/mac80211/mesh.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 21b1fdf..6a1603b 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -148,14 +148,17 @@ 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;
+   u32 changed = 0;
 
/*
 * 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 &&
+   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(>mesh->plink_timer);
-- 
1.9.1


-- 
Jouni MalinenPGP id EFC895FA
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html