[PATCH 1/3] mac80211: Fix incorrect num_sta_ps decrement in __sta_info_destroy

[PATCH 1/3] mac80211: Fix incorrect num_sta_ps decrement in __sta_info_destroy

[Helmut Schaa]

When WLAN_STA_PS_DRIVER is set by ieee80211_sta_block_awake the
num_sta_ps counter is not incremented. Hence, we shouldn't decrement
it in __sta_info_destroy if only WLAN_STA_PS_DRIVER is set. This
could result in an incorrect num_sta_ps counter leading to strange side
effects with associated powersaving clients.

Fix this by only decrementing num_sta_ps when WLAN_STA_PS_STA was set
before.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/sta_info.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 5b4ab21..70a0de3 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -750,12 +750,10 @@ static int __must_check __sta_info_destroy(struct sta_info *sta)
 
 	sta->dead = true;
 
-	if (test_sta_flag(sta, WLAN_STA_PS_STA) ||
-	    test_sta_flag(sta, WLAN_STA_PS_DRIVER)) {
+	if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
 		BUG_ON(!sdata->bss);
 
 		clear_sta_flag(sta, WLAN_STA_PS_STA);
-		clear_sta_flag(sta, WLAN_STA_PS_DRIVER);
 
 		atomic_dec(&sdata->bss->num_sta_ps);
 		sta_info_recalc_tim(sta);
-- 
1.7.7

[PATCH 2/3] mac80211: Fix incorrect num_sta_ps decrement in ap_sta_ps_end

[Helmut Schaa]

If the driver blocked this specific STA with the help of
ieee80211_sta_block_awake we won't clear WLAN_STA_PS_STA later but
still decrement num_sta_ps. Hence, the next data frame from this
STA will trigger ap_sta_ps_end again and also decrement num_sta_ps
again leading to an incorrect num_sta_ps counter.

This can result in problems with powersaving clients not waking up
from PS because the TIM calculation might be skipped due to the
incorrect num_sta_ps counter.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/rx.c       |    1 -
 net/mac80211/sta_info.c |    4 +++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 3567586..f89617a 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1147,7 +1147,6 @@ static void ap_sta_ps_end(struct sta_info *sta)
 {
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 
-	atomic_dec(&sdata->bss->num_sta_ps);
 
 #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
 	printk(KERN_DEBUG "%s: STA %pM aid %d exits power save mode\n",
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 70a0de3..92b894c 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1007,9 +1007,11 @@ EXPORT_SYMBOL(ieee80211_find_sta);
 static void clear_sta_ps_flags(void *_sta)
 {
 	struct sta_info *sta = _sta;
+	struct ieee80211_sub_if_data *sdata = sta->sdata;
 
 	clear_sta_flag(sta, WLAN_STA_PS_DRIVER);
-	clear_sta_flag(sta, WLAN_STA_PS_STA);
+	if (test_and_clear_sta_flag(sta, WLAN_STA_PS_STA))
+		atomic_dec(&sdata->bss->num_sta_ps);
 }
 
 /* powersave support code */
-- 
1.7.7

[PATCH 3/3] mac80211: Move num_sta_ps counter decrement after synchronize_rcu

[Helmut Schaa]

Unted the assumption that the sta struct is still accessible before the
synchronize_rcu call we should move the num_sta_ps counter decrement
after synchronize_rcu to avoid incorrect decrements if num_sta_ps.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---

Not sure if this really triggers in reality but it looks a bit more correct to me.

 net/mac80211/sta_info.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 92b894c..3af737c 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -750,15 +750,6 @@ static int __must_check __sta_info_destroy(struct sta_info *sta)
 
 	sta->dead = true;
 
-	if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
-		BUG_ON(!sdata->bss);
-
-		clear_sta_flag(sta, WLAN_STA_PS_STA);
-
-		atomic_dec(&sdata->bss->num_sta_ps);
-		sta_info_recalc_tim(sta);
-	}
-
 	local->num_sta--;
 	local->sta_generation++;
 
@@ -785,6 +776,15 @@ static int __must_check __sta_info_destroy(struct sta_info *sta)
 	 */
 	synchronize_rcu();
 
+	if (test_sta_flag(sta, WLAN_STA_PS_STA)) {
+		BUG_ON(!sdata->bss);
+
+		clear_sta_flag(sta, WLAN_STA_PS_STA);
+
+		atomic_dec(&sdata->bss->num_sta_ps);
+		sta_info_recalc_tim(sta);
+	}
+
 	for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
 		local->total_ps_buffered -= skb_queue_len(&sta->ps_tx_buf[ac]);
 		__skb_queue_purge(&sta->ps_tx_buf[ac]);
-- 
1.7.7

Re: [PATCH 2/3] mac80211: Fix incorrect num_sta_ps decrement in ap_sta_ps_end

[Helmut Schaa]

On Fri, Jan 27, 2012 at 11:02 AM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> If the driver blocked this specific STA with the help of
> ieee80211_sta_block_awake we won't clear WLAN_STA_PS_STA later but
> still decrement num_sta_ps. Hence, the next data frame from this
> STA will trigger ap_sta_ps_end again and also decrement num_sta_ps
> again leading to an incorrect num_sta_ps counter.
>
> This can result in problems with powersaving clients not waking up
> from PS because the TIM calculation might be skipped due to the
> incorrect num_sta_ps counter.

John, I need to send a v2 for this since it introduces a warning:

net/mac80211/rx.c: In function ‘ap_sta_ps_end’:
net/mac80211/rx.c:1148:32: warning: unused variable ‘sdata’ [-Wunused-variable]

Helmut

> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
>  net/mac80211/rx.c       |    1 -
>  net/mac80211/sta_info.c |    4 +++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 3567586..f89617a 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1147,7 +1147,6 @@ static void ap_sta_ps_end(struct sta_info *sta)
>  {
>        struct ieee80211_sub_if_data *sdata = sta->sdata;
>
> -       atomic_dec(&sdata->bss->num_sta_ps);
>
>  #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
>        printk(KERN_DEBUG "%s: STA %pM aid %d exits power save mode\n",
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 70a0de3..92b894c 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -1007,9 +1007,11 @@ EXPORT_SYMBOL(ieee80211_find_sta);
>  static void clear_sta_ps_flags(void *_sta)
>  {
>        struct sta_info *sta = _sta;
> +       struct ieee80211_sub_if_data *sdata = sta->sdata;
>
>        clear_sta_flag(sta, WLAN_STA_PS_DRIVER);
> -       clear_sta_flag(sta, WLAN_STA_PS_STA);
> +       if (test_and_clear_sta_flag(sta, WLAN_STA_PS_STA))
> +               atomic_dec(&sdata->bss->num_sta_ps);
>  }
>
>  /* powersave support code */
> --
> 1.7.7
>

[PATCHv2 2/3] mac80211: Fix incorrect num_sta_ps decrement in ap_sta_ps_end

[Helmut Schaa]

If the driver blocked this specific STA with the help of
ieee80211_sta_block_awake we won't clear WLAN_STA_PS_STA later but
still decrement num_sta_ps. Hence, the next data frame from this
STA will trigger ap_sta_ps_end again and also decrement num_sta_ps
again leading to an incorrect num_sta_ps counter.

This can result in problems with powersaving clients not waking up
from PS because the TIM calculation might be skipped due to the
incorrect num_sta_ps counter.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---

v2: Fix a compiler warning
net/mac80211/rx.c: In function 'ap_sta_ps_end':
net/mac80211/rx.c:1148:32: warning: unused variable 'sdata' [-Wunused-variable]

 net/mac80211/rx.c       |    8 ++------
 net/mac80211/sta_info.c |    4 +++-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 3567586..fe82881 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1145,19 +1145,15 @@ static void ap_sta_ps_start(struct sta_info *sta)
 
 static void ap_sta_ps_end(struct sta_info *sta)
 {
-	struct ieee80211_sub_if_data *sdata = sta->sdata;
-
-	atomic_dec(&sdata->bss->num_sta_ps);
-
 #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
 	printk(KERN_DEBUG "%s: STA %pM aid %d exits power save mode\n",
-	       sdata->name, sta->sta.addr, sta->sta.aid);
+	       sta->sdata->name, sta->sta.addr, sta->sta.aid);
 #endif /* CONFIG_MAC80211_VERBOSE_PS_DEBUG */
 
 	if (test_sta_flag(sta, WLAN_STA_PS_DRIVER)) {
 #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
 		printk(KERN_DEBUG "%s: STA %pM aid %d driver-ps-blocked\n",
-		       sdata->name, sta->sta.addr, sta->sta.aid);
+		       sta->sdata->name, sta->sta.addr, sta->sta.aid);
 #endif /* CONFIG_MAC80211_VERBOSE_PS_DEBUG */
 		return;
 	}
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 70a0de3..92b894c 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1007,9 +1007,11 @@ EXPORT_SYMBOL(ieee80211_find_sta);
 static void clear_sta_ps_flags(void *_sta)
 {
 	struct sta_info *sta = _sta;
+	struct ieee80211_sub_if_data *sdata = sta->sdata;
 
 	clear_sta_flag(sta, WLAN_STA_PS_DRIVER);
-	clear_sta_flag(sta, WLAN_STA_PS_STA);
+	if (test_and_clear_sta_flag(sta, WLAN_STA_PS_STA))
+		atomic_dec(&sdata->bss->num_sta_ps);
 }
 
 /* powersave support code */
-- 
1.7.7