[PATCH v3 1/2] mac80211: rename AP variable num_sta_authorized to num_mcast_sta

[PATCH v3 1/2] mac80211: rename AP variable num_sta_authorized to num_mcast_sta

[Felix Fietkau]

It is only used to test for BSS multicast receivers.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 net/mac80211/debugfs_netdev.c |    4 ++--
 net/mac80211/ieee80211_i.h    |    2 +-
 net/mac80211/sta_info.c       |    4 ++--
 net/mac80211/tx.c             |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index e7af522..ea0122d 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -394,7 +394,7 @@ static ssize_t ieee80211_if_parse_uapsd_max_sp_len(
 __IEEE80211_IF_FILE_W(uapsd_max_sp_len);
 
 /* AP attributes */
-IEEE80211_IF_FILE(num_sta_authorized, u.ap.num_sta_authorized, ATOMIC);
+IEEE80211_IF_FILE(num_mcast_sta, u.ap.num_mcast_sta, ATOMIC);
 IEEE80211_IF_FILE(num_sta_ps, u.ap.num_sta_ps, ATOMIC);
 IEEE80211_IF_FILE(dtim_count, u.ap.dtim_count, DEC);
 
@@ -540,7 +540,7 @@ static void add_sta_files(struct ieee80211_sub_if_data *sdata)
 
 static void add_ap_files(struct ieee80211_sub_if_data *sdata)
 {
-	DEBUGFS_ADD(num_sta_authorized);
+	DEBUGFS_ADD(num_mcast_sta);
 	DEBUGFS_ADD(num_sta_ps);
 	DEBUGFS_ADD(dtim_count);
 	DEBUGFS_ADD(num_buffered_multicast);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index bd7a451..4404753 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -282,7 +282,7 @@ struct ieee80211_if_ap {
 	u8 tim[sizeof(unsigned long) * BITS_TO_LONGS(IEEE80211_MAX_AID + 1)];
 	struct sk_buff_head ps_bc_buf;
 	atomic_t num_sta_ps; /* number of stations in PS mode */
-	atomic_t num_sta_authorized; /* number of authorized stations */
+	atomic_t num_mcast_sta; /* number of stations receiving multicast */
 	int dtim_count;
 	bool dtim_bc_mc;
 };
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 7fd7ac4..4c04eb5 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1418,14 +1418,14 @@ int sta_info_move_state(struct sta_info *sta,
 			set_bit(WLAN_STA_ASSOC, &sta->_flags);
 		} else if (sta->sta_state == IEEE80211_STA_AUTHORIZED) {
 			if (sta->sdata->vif.type == NL80211_IFTYPE_AP)
-				atomic_dec(&sta->sdata->u.ap.num_sta_authorized);
+				atomic_dec(&sta->sdata->u.ap.num_mcast_sta);
 			clear_bit(WLAN_STA_AUTHORIZED, &sta->_flags);
 		}
 		break;
 	case IEEE80211_STA_AUTHORIZED:
 		if (sta->sta_state == IEEE80211_STA_ASSOC) {
 			if (sta->sdata->vif.type == NL80211_IFTYPE_AP)
-				atomic_inc(&sta->sdata->u.ap.num_sta_authorized);
+				atomic_inc(&sta->sdata->u.ap.num_mcast_sta);
 			set_bit(WLAN_STA_AUTHORIZED, &sta->_flags);
 		}
 		break;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 0abbef95..44001c7 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -306,7 +306,7 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
 		}
 	} else if (unlikely(tx->sdata->vif.type == NL80211_IFTYPE_AP &&
 			    ieee80211_is_data(hdr->frame_control) &&
-			    !atomic_read(&tx->sdata->u.ap.num_sta_authorized))) {
+			    !atomic_read(&tx->sdata->u.ap.num_mcast_sta))) {
 		/*
 		 * No associated STAs - no need to send multicast
 		 * frames.
-- 
1.7.3.2

[PATCH v3 2/2] mac80211: fix num_mcast_sta counting issues

[Felix Fietkau]

Moving a STA to an AP VLAN prevents num_mcast_sta from being decremented
once the STA leaves, because sta->sdata changes. Fix this by checking
for AP VLANs as well.

Also exclude 4-addr VLAN stations from num_mcast_sta - remote 4-addr
stations ignore 3-address multicast frames anyway. In a typical bridge
configuration they receive the same packets as 4-address unicast.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 net/mac80211/cfg.c      |   17 +++++++++++++++++
 net/mac80211/sta_info.c |   12 ++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 510a745..a675509 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1005,6 +1005,9 @@ static int ieee80211_change_station(struct wiphy *wiphy,
 	}
 
 	if (params->vlan && params->vlan != sta->sdata->dev) {
+		bool prev_4addr = false;
+		bool new_4addr = false;
+
 		vlansdata = IEEE80211_DEV_TO_SUB_IF(params->vlan);
 
 		if (vlansdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
@@ -1020,9 +1023,23 @@ static int ieee80211_change_station(struct wiphy *wiphy,
 			}
 
 			rcu_assign_pointer(vlansdata->u.vlan.sta, sta);
+			new_4addr = true;
 		}
 
+		if (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
+		    sta->sdata->u.vlan.sta)
+			prev_4addr = true;
+
 		sta->sdata = vlansdata;
+
+		if (sta->sta_state == IEEE80211_STA_AUTHORIZED &&
+		    prev_4addr != new_4addr) {
+			if (new_4addr)
+				atomic_dec(&sta->sdata->bss->num_mcast_sta);
+			else
+				atomic_inc(&sta->sdata->bss->num_mcast_sta);
+		}
+
 		ieee80211_send_layer2_update(sta);
 	}
 
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 4c04eb5..97a9d66 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1417,15 +1417,19 @@ int sta_info_move_state(struct sta_info *sta,
 		if (sta->sta_state == IEEE80211_STA_AUTH) {
 			set_bit(WLAN_STA_ASSOC, &sta->_flags);
 		} else if (sta->sta_state == IEEE80211_STA_AUTHORIZED) {
-			if (sta->sdata->vif.type == NL80211_IFTYPE_AP)
-				atomic_dec(&sta->sdata->u.ap.num_mcast_sta);
+			if (sta->sdata->vif.type == NL80211_IFTYPE_AP ||
+			    (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
+			     !sta->sdata->u.vlan.sta))
+				atomic_dec(&sta->sdata->bss->num_mcast_sta);
 			clear_bit(WLAN_STA_AUTHORIZED, &sta->_flags);
 		}
 		break;
 	case IEEE80211_STA_AUTHORIZED:
 		if (sta->sta_state == IEEE80211_STA_ASSOC) {
-			if (sta->sdata->vif.type == NL80211_IFTYPE_AP)
-				atomic_inc(&sta->sdata->u.ap.num_mcast_sta);
+			if (sta->sdata->vif.type == NL80211_IFTYPE_AP ||
+			    (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
+			     !sta->sdata->u.vlan.sta))
+				atomic_inc(&sta->sdata->bss->num_mcast_sta);
 			set_bit(WLAN_STA_AUTHORIZED, &sta->_flags);
 		}
 		break;
-- 
1.7.3.2

Re: [PATCH v3 2/2] mac80211: fix num_mcast_sta counting issues

[Johannes Berg]

On Sat, 2012-04-21 at 23:42 +0200, Felix Fietkau wrote:

>  			rcu_assign_pointer(vlansdata->u.vlan.sta, sta);
> +			new_4addr = true;
>  		}
>  
> +		if (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
> +		    sta->sdata->u.vlan.sta)
> +			prev_4addr = true;

Doesn't that have to be before the assignment or something, otherwise
it's always just true unless it was moved back to the AP iface, but
still true if it was just assigned to an AP_VLAN iface?

johannes

Re: [PATCH v3 2/2] mac80211: fix num_mcast_sta counting issues

[Johannes Berg]

On Sat, 2012-04-21 at 23:51 +0200, Johannes Berg wrote:
> On Sat, 2012-04-21 at 23:42 +0200, Felix Fietkau wrote:
> 
> >  			rcu_assign_pointer(vlansdata->u.vlan.sta, sta);
> > +			new_4addr = true;
> >  		}
> >  
> > +		if (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
> > +		    sta->sdata->u.vlan.sta)
> > +			prev_4addr = true;
> 
> Doesn't that have to be before the assignment or something, otherwise
> it's always just true unless it was moved back to the AP iface, but
> still true if it was just assigned to an AP_VLAN iface?

There also seems to be another existing bug, if the station is moved out
of the VLAN interface then the u.vlan.sta isn't cleared.

johannes

Re: [PATCH v3 1/2] mac80211: rename AP variable num_sta_authorized to num_mcast_sta

[Johannes Berg]

On Sat, 2012-04-21 at 23:42 +0200, Felix Fietkau wrote:
> It is only used to test for BSS multicast receivers.

This is obviously a good thing :-)

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  net/mac80211/debugfs_netdev.c |    4 ++--
>  net/mac80211/ieee80211_i.h    |    2 +-
>  net/mac80211/sta_info.c       |    4 ++--
>  net/mac80211/tx.c             |    2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
> index e7af522..ea0122d 100644
> --- a/net/mac80211/debugfs_netdev.c
> +++ b/net/mac80211/debugfs_netdev.c
> @@ -394,7 +394,7 @@ static ssize_t ieee80211_if_parse_uapsd_max_sp_len(
>  __IEEE80211_IF_FILE_W(uapsd_max_sp_len);
>  
>  /* AP attributes */
> -IEEE80211_IF_FILE(num_sta_authorized, u.ap.num_sta_authorized, ATOMIC);
> +IEEE80211_IF_FILE(num_mcast_sta, u.ap.num_mcast_sta, ATOMIC);
>  IEEE80211_IF_FILE(num_sta_ps, u.ap.num_sta_ps, ATOMIC);
>  IEEE80211_IF_FILE(dtim_count, u.ap.dtim_count, DEC);
>  
> @@ -540,7 +540,7 @@ static void add_sta_files(struct ieee80211_sub_if_data *sdata)
>  
>  static void add_ap_files(struct ieee80211_sub_if_data *sdata)
>  {
> -	DEBUGFS_ADD(num_sta_authorized);
> +	DEBUGFS_ADD(num_mcast_sta);
>  	DEBUGFS_ADD(num_sta_ps);
>  	DEBUGFS_ADD(dtim_count);
>  	DEBUGFS_ADD(num_buffered_multicast);
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index bd7a451..4404753 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -282,7 +282,7 @@ struct ieee80211_if_ap {
>  	u8 tim[sizeof(unsigned long) * BITS_TO_LONGS(IEEE80211_MAX_AID + 1)];
>  	struct sk_buff_head ps_bc_buf;
>  	atomic_t num_sta_ps; /* number of stations in PS mode */
> -	atomic_t num_sta_authorized; /* number of authorized stations */
> +	atomic_t num_mcast_sta; /* number of stations receiving multicast */
>  	int dtim_count;
>  	bool dtim_bc_mc;
>  };
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 7fd7ac4..4c04eb5 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -1418,14 +1418,14 @@ int sta_info_move_state(struct sta_info *sta,
>  			set_bit(WLAN_STA_ASSOC, &sta->_flags);
>  		} else if (sta->sta_state == IEEE80211_STA_AUTHORIZED) {
>  			if (sta->sdata->vif.type == NL80211_IFTYPE_AP)
> -				atomic_dec(&sta->sdata->u.ap.num_sta_authorized);
> +				atomic_dec(&sta->sdata->u.ap.num_mcast_sta);
>  			clear_bit(WLAN_STA_AUTHORIZED, &sta->_flags);
>  		}
>  		break;
>  	case IEEE80211_STA_AUTHORIZED:
>  		if (sta->sta_state == IEEE80211_STA_ASSOC) {
>  			if (sta->sdata->vif.type == NL80211_IFTYPE_AP)
> -				atomic_inc(&sta->sdata->u.ap.num_sta_authorized);
> +				atomic_inc(&sta->sdata->u.ap.num_mcast_sta);
>  			set_bit(WLAN_STA_AUTHORIZED, &sta->_flags);
>  		}
>  		break;
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 0abbef95..44001c7 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -306,7 +306,7 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
>  		}
>  	} else if (unlikely(tx->sdata->vif.type == NL80211_IFTYPE_AP &&
>  			    ieee80211_is_data(hdr->frame_control) &&
> -			    !atomic_read(&tx->sdata->u.ap.num_sta_authorized))) {
> +			    !atomic_read(&tx->sdata->u.ap.num_mcast_sta))) {
>  		/*
>  		 * No associated STAs - no need to send multicast
>  		 * frames.

Re: [PATCH v3 2/2] mac80211: fix num_mcast_sta counting issues

[Felix Fietkau]

On 2012-04-21 11:51 PM, Johannes Berg wrote:
> On Sat, 2012-04-21 at 23:42 +0200, Felix Fietkau wrote:
> 
>>  			rcu_assign_pointer(vlansdata->u.vlan.sta, sta);
>> +			new_4addr = true;
>>  		}
>>  
>> +		if (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
>> +		    sta->sdata->u.vlan.sta)
>> +			prev_4addr = true;
> 
> Doesn't that have to be before the assignment or something, otherwise
> it's always just true unless it was moved back to the AP iface, but
> still true if it was just assigned to an AP_VLAN iface?
This is just before the 'sta->sdata = vlansdata;' line, so it should be
OK. Or am I missing something here?

- Felix

Re: [PATCH v3 2/2] mac80211: fix num_mcast_sta counting issues

[Johannes Berg]

On Sun, 2012-04-22 at 21:54 +0200, Felix Fietkau wrote:
> On 2012-04-21 11:51 PM, Johannes Berg wrote:
> > On Sat, 2012-04-21 at 23:42 +0200, Felix Fietkau wrote:
> > 
> >>  			rcu_assign_pointer(vlansdata->u.vlan.sta, sta);
> >> +			new_4addr = true;
> >>  		}
> >>  
> >> +		if (sta->sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
> >> +		    sta->sdata->u.vlan.sta)
> >> +			prev_4addr = true;
> > 
> > Doesn't that have to be before the assignment or something, otherwise
> > it's always just true unless it was moved back to the AP iface, but
> > still true if it was just assigned to an AP_VLAN iface?

> This is just before the 'sta->sdata = vlansdata;' line, so it should be
> OK. Or am I missing something here?

Ah, I guess I missed that. Or maybe I was thinking what I also said on
IRC -- when you move a station out of the 4addr VLAN u.vlan.sta is never
reset to NULL.

johannes