[PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg]

From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

There is a race between the TX path and the STA wakeup: while
a station is sleeping, mac80211 buffers frames until it wakes
up, then the frames are transmitted. However, the RX and TX
path are concurrent, so the packet indicating wakeup can be
processed while a packet is being transmitted.

This can lead to a situation where the buffered frames list
is emptied on the one side, while a frame is being added on
the other side, as the station is still seen as sleeping in
the TX path.

As a result, the newly added frame will not be send anytime
soon. It might be sent much later (and out of order) when the
station goes to sleep and wakes up the next time.

Additionally, it can lead to the crash below.

Fix all this by synchronising both paths with a new lock.
Both path are not fastpath since they handle PS situations.

In a later patch we'll remove the extra skb queue locks to
reduce locking overhead.

BUG: unable to handle kernel
NULL pointer dereference at 000000b0
IP: [<ff6f1791>] ieee80211_report_used_skb+0x11/0x3e0 [mac80211]
*pde = 00000000
Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
EIP: 0060:[<ff6f1791>] EFLAGS: 00210282 CPU: 1
EIP is at ieee80211_report_used_skb+0x11/0x3e0 [mac80211]
EAX: e5900da0 EBX: 00000000 ECX: 00000001 EDX: 00000000
ESI: e41d00c0 EDI: e5900da0 EBP: ebe458e4 ESP: ebe458b0
 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
CR0: 8005003b CR2: 000000b0 CR3: 25a78000 CR4: 000407d0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
Process iperf (pid: 3934, ti=ebe44000 task=e757c0b0 task.ti=ebe44000)
iwlwifi 0000:02:00.0: I iwl_pcie_enqueue_hcmd Sending command LQ_CMD (#4e), seq: 0x0903, 92 bytes at 3[3]:9
Stack:
 e403b32c ebe458c4 00200002 00200286 e403b338 ebe458cc c10960bb e5900da0
 ff76a6ec ebe458d8 00000000 e41d00c0 e5900da0 ebe458f0 ff6f1b75 e403b210
 ebe4598c ff723dc1 00000000 ff76a6ec e597c978 e403b758 00000002 00000002
Call Trace:
 [<ff6f1b75>] ieee80211_free_txskb+0x15/0x20 [mac80211]
 [<ff723dc1>] invoke_tx_handlers+0x1661/0x1780 [mac80211]
 [<ff7248a5>] ieee80211_tx+0x75/0x100 [mac80211]
 [<ff7249bf>] ieee80211_xmit+0x8f/0xc0 [mac80211]
 [<ff72550e>] ieee80211_subif_start_xmit+0x4fe/0xe20 [mac80211]
 [<c149ef70>] dev_hard_start_xmit+0x450/0x950
 [<c14b9aa9>] sch_direct_xmit+0xa9/0x250
 [<c14b9c9b>] __qdisc_run+0x4b/0x150
 [<c149f732>] dev_queue_xmit+0x2c2/0xca0

Cc: stable@vger.kernel.org
Reported-by: Yaara Rozenblum <yaara.rozenblum@intel.com>
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
[reword commit log, use a separate lock]
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/sta_info.c |  4 ++++
 net/mac80211/sta_info.h |  7 +++----
 net/mac80211/tx.c       | 14 ++++++++++++++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index decd30c1e290..62a5f0889583 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -330,6 +330,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 	rcu_read_unlock();
 
 	spin_lock_init(&sta->lock);
+	spin_lock_init(&sta->ps_lock);
 	INIT_WORK(&sta->drv_unblock_wk, sta_unblock);
 	INIT_WORK(&sta->ampdu_mlme.work, ieee80211_ba_session_work);
 	mutex_init(&sta->ampdu_mlme.mtx);
@@ -1109,6 +1110,8 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
 
 	skb_queue_head_init(&pending);
 
+	/* sync with ieee80211_tx_h_unicast_ps_buf */
+	spin_lock(&sta->ps_lock);
 	/* Send all buffered frames to the station */
 	for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
 		int count = skb_queue_len(&pending), tmp;
@@ -1128,6 +1131,7 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
 	}
 
 	ieee80211_add_pending_skbs_fn(local, &pending, clear_sta_ps_flags, sta);
+	spin_unlock(&sta->ps_lock);
 
 	/* This station just woke up and isn't aware of our SMPS state */
 	if (!ieee80211_smps_is_restrictive(sta->known_smps_mode,
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index d77ff7090630..d3a6d8208f2f 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -267,6 +267,7 @@ struct ieee80211_tx_latency_stat {
  * @drv_unblock_wk: used for driver PS unblocking
  * @listen_interval: listen interval of this station, when we're acting as AP
  * @_flags: STA flags, see &enum ieee80211_sta_info_flags, do not use directly
+ * @ps_lock: used for powersave (when mac80211 is the AP) related locking
  * @ps_tx_buf: buffers (per AC) of frames to transmit to this station
  *	when it leaves power saving state or polls
  * @tx_filtered: buffers (per AC) of frames we already tried to
@@ -356,10 +357,8 @@ struct sta_info {
 	/* use the accessors defined below */
 	unsigned long _flags;
 
-	/*
-	 * STA powersave frame queues, no more than the internal
-	 * locking required.
-	 */
+	/* STA powersave lock and frame queues */
+	spinlock_t ps_lock;
 	struct sk_buff_head ps_tx_buf[IEEE80211_NUM_ACS];
 	struct sk_buff_head tx_filtered[IEEE80211_NUM_ACS];
 	unsigned long driver_buffered_tids;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 97a02d3f7d87..c2ca9313595b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -478,6 +478,19 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
 		       sta->sta.addr, sta->sta.aid, ac);
 		if (tx->local->total_ps_buffered >= TOTAL_MAX_TX_BUFFER)
 			purge_old_ps_buffers(tx->local);
+
+		/* sync with ieee80211_sta_ps_deliver_wakeup */
+		spin_lock(&sta->ps_lock);
+		/*
+		 * STA woke up the meantime and all the frames on ps_tx_buf have
+		 * been queued to pending queue. No reordering can happen, go
+		 * ahead and Tx the packet.
+		 */
+		if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
+			spin_unlock(&sta->ps_lock);
+			return TX_CONTINUE;
+		}
+
 		if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER) {
 			struct sk_buff *old = skb_dequeue(&sta->ps_tx_buf[ac]);
 			ps_dbg(tx->sdata,
@@ -492,6 +505,7 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
 		info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
 		info->flags &= ~IEEE80211_TX_TEMPORARY_FLAGS;
 		skb_queue_tail(&sta->ps_tx_buf[ac], tx->skb);
+		spin_unlock(&sta->ps_lock);
 
 		if (!timer_pending(&local->sta_cleanup))
 			mod_timer(&local->sta_cleanup,
-- 
1.8.5.3

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Stanislaw Gruszka]

On Thu, Feb 20, 2014 at 09:52:45AM +0100, Johannes Berg wrote:
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Stanislaw Gruszka]

On Thu, Feb 20, 2014 at 09:52:45AM +0100, Johannes Berg wrote:
> +
> +		/* sync with ieee80211_sta_ps_deliver_wakeup */
> +		spin_lock(&sta->ps_lock);
> +		/*
> +		 * STA woke up the meantime and all the frames on ps_tx_buf have
> +		 * been queued to pending queue. No reordering can happen, go
> +		 * ahead and Tx the packet.
> +		 */
> +		if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> +			spin_unlock(&sta->ps_lock);

Actually I'm not sure if we should check if both WLAN_STA_PS_DRIVER 
and WLAN_STA_PS_STA flags are clear ?

Stanislaw

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg]

On Thu, 2014-02-20 at 10:21 +0100, Stanislaw Gruszka wrote:
> On Thu, Feb 20, 2014 at 09:52:45AM +0100, Johannes Berg wrote:
> > +
> > +		/* sync with ieee80211_sta_ps_deliver_wakeup */
> > +		spin_lock(&sta->ps_lock);
> > +		/*
> > +		 * STA woke up the meantime and all the frames on ps_tx_buf have
> > +		 * been queued to pending queue. No reordering can happen, go
> > +		 * ahead and Tx the packet.
> > +		 */
> > +		if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> > +			spin_unlock(&sta->ps_lock);
> 
> Actually I'm not sure if we should check if both WLAN_STA_PS_DRIVER 
> and WLAN_STA_PS_STA flags are clear ?

This area is a bit confusing, but I don't think WLAN_STA_PS_STA will be
clear until after WLAN_STA_PS_DRIVER is?

johannes

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg]

On Thu, 2014-02-20 at 10:27 +0100, Johannes Berg wrote:
> On Thu, 2014-02-20 at 10:21 +0100, Stanislaw Gruszka wrote:
> > On Thu, Feb 20, 2014 at 09:52:45AM +0100, Johannes Berg wrote:
> > > +
> > > +		/* sync with ieee80211_sta_ps_deliver_wakeup */
> > > +		spin_lock(&sta->ps_lock);
> > > +		/*
> > > +		 * STA woke up the meantime and all the frames on ps_tx_buf have
> > > +		 * been queued to pending queue. No reordering can happen, go
> > > +		 * ahead and Tx the packet.
> > > +		 */
> > > +		if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> > > +			spin_unlock(&sta->ps_lock);
> > 
> > Actually I'm not sure if we should check if both WLAN_STA_PS_DRIVER 
> > and WLAN_STA_PS_STA flags are clear ?
> 
> This area is a bit confusing, but I don't think WLAN_STA_PS_STA will be
> clear until after WLAN_STA_PS_DRIVER is?

OTOH, it will certainly not hurt to test the flag, and we do it in other
places, so I'll change this patch to do that as well.

johannes

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Stanislaw Gruszka]

On Thu, Feb 20, 2014 at 10:27:48AM +0100, Johannes Berg wrote:
> On Thu, 2014-02-20 at 10:21 +0100, Stanislaw Gruszka wrote:
> > On Thu, Feb 20, 2014 at 09:52:45AM +0100, Johannes Berg wrote:
> > > +
> > > +		/* sync with ieee80211_sta_ps_deliver_wakeup */
> > > +		spin_lock(&sta->ps_lock);
> > > +		/*
> > > +		 * STA woke up the meantime and all the frames on ps_tx_buf have
> > > +		 * been queued to pending queue. No reordering can happen, go
> > > +		 * ahead and Tx the packet.
> > > +		 */
> > > +		if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> > > +			spin_unlock(&sta->ps_lock);
> > 
> > Actually I'm not sure if we should check if both WLAN_STA_PS_DRIVER 
> > and WLAN_STA_PS_STA flags are clear ?
> 
> This area is a bit confusing, but I don't think WLAN_STA_PS_STA will be
> clear until after WLAN_STA_PS_DRIVER is?

Hmm, actually looks like we call ps_deliver_wakeup() with WLAN_STA_PS_STA
flag currently cleared.

tatic void sta_unblock(struct work_struct *wk)
{
        if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
                local_bh_disable();
                ieee80211_sta_ps_deliver_wakeup(sta);
                local_bh_enable();

so on TX we should rather check WLAN_STA_PS_DRIVER flag .

Stanislaw

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg]

On Thu, 2014-02-20 at 10:40 +0100, Stanislaw Gruszka wrote:

> > This area is a bit confusing, but I don't think WLAN_STA_PS_STA will be
> > clear until after WLAN_STA_PS_DRIVER is?
> 
> Hmm, actually looks like we call ps_deliver_wakeup() with WLAN_STA_PS_STA
> flag currently cleared.
> 
> tatic void sta_unblock(struct work_struct *wk)
> {
>         if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
>                 local_bh_disable();
>                 ieee80211_sta_ps_deliver_wakeup(sta);
>                 local_bh_enable();
> 
> so on TX we should rather check WLAN_STA_PS_DRIVER flag .

But that flag may never be set if the driver doesn't support/use it
(which means it's racy, but anyway), so we need to check both.

Now I'm confused though. It seems the intent here was to clear the
WLAN_STA_PS_STA flag in something like sta_ps_end() in the RX path, but
we don't do that. I'm not sure how it works now, but I know from testing
that it does ;-)

Do you think the v2 patch is fine?

johannes

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg]

On Thu, 2014-02-20 at 10:42 +0100, Johannes Berg wrote:
> On Thu, 2014-02-20 at 10:40 +0100, Stanislaw Gruszka wrote:
> 
> > > This area is a bit confusing, but I don't think WLAN_STA_PS_STA will be
> > > clear until after WLAN_STA_PS_DRIVER is?
> > 
> > Hmm, actually looks like we call ps_deliver_wakeup() with WLAN_STA_PS_STA
> > flag currently cleared.
> > 
> > tatic void sta_unblock(struct work_struct *wk)
> > {
> >         if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> >                 local_bh_disable();
> >                 ieee80211_sta_ps_deliver_wakeup(sta);
> >                 local_bh_enable();
> > 
> > so on TX we should rather check WLAN_STA_PS_DRIVER flag .
> 
> But that flag may never be set if the driver doesn't support/use it
> (which means it's racy, but anyway), so we need to check both.
> 
> Now I'm confused though. It seems the intent here was to clear the
> WLAN_STA_PS_STA flag in something like sta_ps_end() in the RX path, but
> we don't do that. I'm not sure how it works now, but I know from testing
> that it does ;-)

Looks like I broke that in commit
50a9432daeece6fc1309bef1dc0a7b8fde8204cb ("mac80211: fix powersaving
clients races") and simply don't have a test case for the following
scenario:
 1. client goes to sleep with frames on HW queue
 2. driver blocks wakeup
 3. client wakes up
 4. driver unblocks wakeup

Instead, we always get the steps 3/4 reversed, so nothing happens.

I'll try to write a test for that, but the fix would have to be
something like the below.

johannes

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3701930c6649..5e44e3179e02 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1692,14 +1692,8 @@ void ieee80211_stop_queue_by_reason(struct ieee80211_hw *hw, int queue,
 void ieee80211_propagate_queue_wake(struct ieee80211_local *local, int queue);
 void ieee80211_add_pending_skb(struct ieee80211_local *local,
 			       struct sk_buff *skb);
-void ieee80211_add_pending_skbs_fn(struct ieee80211_local *local,
-				   struct sk_buff_head *skbs,
-				   void (*fn)(void *data), void *data);
-static inline void ieee80211_add_pending_skbs(struct ieee80211_local *local,
-					      struct sk_buff_head *skbs)
-{
-	ieee80211_add_pending_skbs_fn(local, skbs, NULL, NULL);
-}
+void ieee80211_add_pending_skbs(struct ieee80211_local *local,
+				struct sk_buff_head *skbs);
 void ieee80211_flush_queues(struct ieee80211_local *local,
 			    struct ieee80211_sub_if_data *sdata);
 
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index c24ca0d0f469..841636e657ab 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1127,6 +1127,8 @@ static void sta_ps_end(struct sta_info *sta)
 	ps_dbg(sta->sdata, "STA %pM aid %d exits power save mode\n",
 	       sta->sta.addr, sta->sta.aid);
 
+	ieee80211_clear_sta_ps_flag(sta);
+
 	if (test_sta_flag(sta, WLAN_STA_PS_DRIVER)) {
 		ps_dbg(sta->sdata, "STA %pM aid %d driver-ps-blocked\n",
 		       sta->sta.addr, sta->sta.aid);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index ffc1ee6a2ec1..86840c2f984a 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1090,9 +1090,8 @@ struct ieee80211_sta *ieee80211_find_sta(struct ieee80211_vif *vif,
 }
 EXPORT_SYMBOL(ieee80211_find_sta);
 
-static void clear_sta_ps_flags(void *_sta)
+void ieee80211_clear_sta_ps_flag(struct sta_info *sta)
 {
-	struct sta_info *sta = _sta;
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 	struct ps_data *ps;
 
@@ -1104,7 +1103,6 @@ static void clear_sta_ps_flags(void *_sta)
 	else
 		return;
 
-	clear_sta_flag(sta, WLAN_STA_PS_DRIVER);
 	if (test_and_clear_sta_flag(sta, WLAN_STA_PS_STA))
 		atomic_dec(&ps->num_sta_ps);
 }
@@ -1148,7 +1146,9 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
 		buffered += tmp - count;
 	}
 
-	ieee80211_add_pending_skbs_fn(local, &pending, clear_sta_ps_flags, sta);
+	ieee80211_add_pending_skbs(local, &pending);
+	ieee80211_clear_sta_ps_flag(sta);
+	clear_sta_flag(sta, WLAN_STA_PS_DRIVER);
 	spin_unlock(&sta->ps_lock);
 
 	/* This station just woke up and isn't aware of our SMPS state */
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index d3a6d8208f2f..afe8d43dff14 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -630,6 +630,7 @@ void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata,
 			  unsigned long exp_time);
 u8 sta_info_tx_streams(struct sta_info *sta);
 
+void ieee80211_clear_sta_ps_flag(struct sta_info *sta);
 void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta);
 void ieee80211_sta_ps_deliver_poll_response(struct sta_info *sta);
 void ieee80211_sta_ps_deliver_uapsd(struct sta_info *sta);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 1d1bb7084c05..b8700d417a9c 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -435,9 +435,8 @@ void ieee80211_add_pending_skb(struct ieee80211_local *local,
 	spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
 }
 
-void ieee80211_add_pending_skbs_fn(struct ieee80211_local *local,
-				   struct sk_buff_head *skbs,
-				   void (*fn)(void *data), void *data)
+void ieee80211_add_pending_skbs(struct ieee80211_local *local,
+				struct sk_buff_head *skbs)
 {
 	struct ieee80211_hw *hw = &local->hw;
 	struct sk_buff *skb;
@@ -461,9 +460,6 @@ void ieee80211_add_pending_skbs_fn(struct ieee80211_local *local,
 		__skb_queue_tail(&local->pending[queue], skb);
 	}
 
-	if (fn)
-		fn(data);
-
 	for (i = 0; i < hw->queues; i++)
 		__ieee80211_wake_queue(hw, i,
 			IEEE80211_QUEUE_STOP_REASON_SKB_ADD);

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg]

On Thu, 2014-02-20 at 11:09 +0100, Stanislaw Gruszka wrote:
> On Thu, Feb 20, 2014 at 10:42:48AM +0100, Johannes Berg wrote:
> > On Thu, 2014-02-20 at 10:40 +0100, Stanislaw Gruszka wrote:
> > 
> > > > This area is a bit confusing, but I don't think WLAN_STA_PS_STA will be
> > > > clear until after WLAN_STA_PS_DRIVER is?
> > > 
> > > Hmm, actually looks like we call ps_deliver_wakeup() with WLAN_STA_PS_STA
> > > flag currently cleared.
> > > 
> > > tatic void sta_unblock(struct work_struct *wk)
> > > {
> > >         if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> > >                 local_bh_disable();
> > >                 ieee80211_sta_ps_deliver_wakeup(sta);
> > >                 local_bh_enable();
> > > 
> > > so on TX we should rather check WLAN_STA_PS_DRIVER flag .
> > 
> > But that flag may never be set if the driver doesn't support/use it
> > (which means it's racy, but anyway), so we need to check both.
> > 
> > Now I'm confused though. It seems the intent here was to clear the
> > WLAN_STA_PS_STA flag in something like sta_ps_end() in the RX path, but
> > we don't do that. I'm not sure how it works now, but I know from testing
> > that it does ;-)
> 
> LOL. There is another call to ieee80211_sta_ps_deliver_wakeup() from
> sta_ps_end(), I missed this somehow. Anyway, we need to check both
> flags.

Yeah but in the case that the driver actually blocked, it's still
broken ... see my other email.

johannes

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Stanislaw Gruszka]

On Thu, Feb 20, 2014 at 10:42:48AM +0100, Johannes Berg wrote:
> On Thu, 2014-02-20 at 10:40 +0100, Stanislaw Gruszka wrote:
> 
> > > This area is a bit confusing, but I don't think WLAN_STA_PS_STA will be
> > > clear until after WLAN_STA_PS_DRIVER is?
> > 
> > Hmm, actually looks like we call ps_deliver_wakeup() with WLAN_STA_PS_STA
> > flag currently cleared.
> > 
> > tatic void sta_unblock(struct work_struct *wk)
> > {
> >         if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> >                 local_bh_disable();
> >                 ieee80211_sta_ps_deliver_wakeup(sta);
> >                 local_bh_enable();
> > 
> > so on TX we should rather check WLAN_STA_PS_DRIVER flag .
> 
> But that flag may never be set if the driver doesn't support/use it
> (which means it's racy, but anyway), so we need to check both.
> 
> Now I'm confused though. It seems the intent here was to clear the
> WLAN_STA_PS_STA flag in something like sta_ps_end() in the RX path, but
> we don't do that. I'm not sure how it works now, but I know from testing
> that it does ;-)

LOL. There is another call to ieee80211_sta_ps_deliver_wakeup() from
sta_ps_end(), I missed this somehow. Anyway, we need to check both
flags.

> Do you think the v2 patch is fine?

Yes, I think it's ok.

Stanislaw 

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Luis R. Rodriguez]

On Thu, Feb 20, 2014 at 12:52 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -478,6 +478,19 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
>                        sta->sta.addr, sta->sta.aid, ac);
>                 if (tx->local->total_ps_buffered >= TOTAL_MAX_TX_BUFFER)
>                         purge_old_ps_buffers(tx->local);
> +
> +               /* sync with ieee80211_sta_ps_deliver_wakeup */
> +               spin_lock(&sta->ps_lock);
> +               /*
> +                * STA woke up the meantime and all the frames on ps_tx_buf have
> +                * been queued to pending queue. No reordering can happen, go
> +                * ahead and Tx the packet.
> +                */
> +               if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> +                       spin_unlock(&sta->ps_lock);
> +                       return TX_CONTINUE;
> +               }
> +
>                 if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER) {
>                         struct sk_buff *old = skb_dequeue(&sta->ps_tx_buf[ac]);
>                         ps_dbg(tx->sdata,

This fixes the race for the case where the API introduced via commit
af81858172cc but that code also *moved* code, the API added simply
added a new way to let driver to hit a trigger to send buffered
frames, are we sure that through mechanisms that don't use this API
this race doesn't exist? That is can we not race between
ieee80211_rx_h_sta_process() and ieee80211_tx_h_unicast_ps_buf()?

  Luis

Re: [PATCH 3.14] mac80211: fix AP powersave TX vs. wakeup race

[Johannes Berg]

On Tue, 2014-06-03 at 19:05 -0700, Luis R. Rodriguez wrote:

> > +++ b/net/mac80211/tx.c
> > @@ -478,6 +478,19 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> >                        sta->sta.addr, sta->sta.aid, ac);
> >                 if (tx->local->total_ps_buffered >= TOTAL_MAX_TX_BUFFER)
> >                         purge_old_ps_buffers(tx->local);
> > +
> > +               /* sync with ieee80211_sta_ps_deliver_wakeup */
> > +               spin_lock(&sta->ps_lock);
> > +               /*
> > +                * STA woke up the meantime and all the frames on ps_tx_buf have
> > +                * been queued to pending queue. No reordering can happen, go
> > +                * ahead and Tx the packet.
> > +                */
> > +               if (!test_sta_flag(sta, WLAN_STA_PS_STA)) {
> > +                       spin_unlock(&sta->ps_lock);
> > +                       return TX_CONTINUE;
> > +               }
> > +
> >                 if (skb_queue_len(&sta->ps_tx_buf[ac]) >= STA_MAX_TX_BUFFER) {
> >                         struct sk_buff *old = skb_dequeue(&sta->ps_tx_buf[ac]);
> >                         ps_dbg(tx->sdata,
> 
> This fixes the race for the case where the API introduced via commit
> af81858172cc but that code also *moved* code, the API added simply
> added a new way to let driver to hit a trigger to send buffered
> frames, are we sure that through mechanisms that don't use this API
> this race doesn't exist? That is can we not race between
> ieee80211_rx_h_sta_process() and ieee80211_tx_h_unicast_ps_buf()?

Err, I have no idea what you're trying to say.

Did you mean sta_ps_end() vs. ieee80211_tx_h_unicast_ps_buf()? Those can
*race*, but any outcome will be valid, afaict, given the station flag
tricks. It all goes back to the same (locked) code for manipulating the
queues.

I recently fixed a bug where mac80211 would get into a confused state
and not buffer packets when it would really be required, but that's just
a bug that can be observed over the air, it has no bad consequences
except for that station losing traffic.

johannes