[PATCH] mac80211: don't clear all tx flags when requeing

[PATCH] mac80211: don't clear all tx flags when requeing

[Michal Kazior]

When acting as AP and a PS-Poll frame is received
associated station is marked as one in a Service
Period. This state is kept until Tx status for
released frame is reported. While a station is in
Service Period PS-Poll frames are ignored.

However if PS-Poll was received during A-MPDU
teardown it was possible to have the to-be
released frame re-queued back to pending queue.
In such case the frame was stripped of 2 important
flags:

 (a) IEEE80211_TX_CTL_NO_PS_BUFFER
 (b) IEEE80211_TX_STATUS_EOSP

Stripping of (a) led to the frame that was to be
released to be queued back to ps_tx_buf queue. If
station remained to use only PS-Poll frames the
re-queued frame (and new ones) was never actually
transmitted because mac80211 would ignore
subsequent PS-Poll frames due to station being in
Service Period. There was nothing left to clear
the Service Period bit (no xmit -> no tx status ->
no SP end), i.e. the AP would have the station
stuck in Service Period. Beacon TIM would
repeatedly prompt station to poll for frames but
it would get none.

Once (a) is not stripped (b) becomes important
because it's the main condition to clear the
Service Period bit of the station when Tx status
for the released frame is reported back.

This problem was observed with ath9k acting as P2P
GO in some testing scenarios but isn't limited to
it. AP operation with mac80211 based Tx A-MPDU
control combined with clients using PS-Poll frames
is subject to this race.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/tx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index d14f3618069f..2079d480cd7b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1113,7 +1113,9 @@ static bool ieee80211_tx_prep_agg(struct ieee80211_tx_data *tx,
 			queued = true;
 			info->control.vif = &tx->sdata->vif;
 			info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
-			info->flags &= ~IEEE80211_TX_TEMPORARY_FLAGS;
+			info->flags &= ~IEEE80211_TX_TEMPORARY_FLAGS |
+					IEEE80211_TX_CTL_NO_PS_BUFFER |
+					IEEE80211_TX_STATUS_EOSP;
 			__skb_queue_tail(&tid_tx->pending, skb);
 			if (skb_queue_len(&tid_tx->pending) > STA_MAX_TX_BUFFER)
 				purge_skb = __skb_dequeue(&tid_tx->pending);
-- 
2.1.4

Re: [PATCH] mac80211: don't clear all tx flags when requeing

[Johannes Berg]

On Thu, 2015-07-02 at 09:59 +0200, Michal Kazior wrote:
> When acting as AP and a PS-Poll frame is received
> associated station is marked as one in a Service
> Period. This state is kept until Tx status for
> released frame is reported. While a station is in
> Service Period PS-Poll frames are ignored.
> 
> However if PS-Poll was received during A-MPDU
> teardown it was possible to have the to-be
> released frame re-queued back to pending queue.
> In such case the frame was stripped of 2 important
> flags:
> 
>  (a) IEEE80211_TX_CTL_NO_PS_BUFFER
>  (b) IEEE80211_TX_STATUS_EOSP
> 
> Stripping of (a) led to the frame that was to be
> released to be queued back to ps_tx_buf queue. If
> station remained to use only PS-Poll frames the
> re-queued frame (and new ones) was never actually
> transmitted because mac80211 would ignore
> subsequent PS-Poll frames due to station being in
> Service Period. There was nothing left to clear
> the Service Period bit (no xmit -> no tx status ->
> no SP end), i.e. the AP would have the station
> stuck in Service Period. Beacon TIM would
> repeatedly prompt station to poll for frames but
> it would get none.
> 
> Once (a) is not stripped (b) becomes important
> because it's the main condition to clear the
> Service Period bit of the station when Tx status
> for the released frame is reported back.
> 
> This problem was observed with ath9k acting as P2P
> GO in some testing scenarios but isn't limited to
> it. AP operation with mac80211 based Tx A-MPDU
> control combined with clients using PS-Poll frames
> is subject to this race.

I'm not sure I quite understand - how is the aggregation teardown
causing frame filtering?

johannes

Re: [PATCH] mac80211: don't clear all tx flags when requeing

[Johannes Berg]

On Fri, 2015-07-17 at 11:05 +0200, Johannes Berg wrote:
> On Thu, 2015-07-02 at 09:59 +0200, Michal Kazior wrote:
> > When acting as AP and a PS-Poll frame is received
> > associated station is marked as one in a Service
> > Period. This state is kept until Tx status for
> > released frame is reported. While a station is in
> > Service Period PS-Poll frames are ignored.
> > 
> > However if PS-Poll was received during A-MPDU
> > teardown it was possible to have the to-be
> > released frame re-queued back to pending queue.
> > In such case the frame was stripped of 2 important
> > flags:
> > 
> >  (a) IEEE80211_TX_CTL_NO_PS_BUFFER
> >  (b) IEEE80211_TX_STATUS_EOSP
> > 
> > Stripping of (a) led to the frame that was to be
> > released to be queued back to ps_tx_buf queue. If
> > station remained to use only PS-Poll frames the
> > re-queued frame (and new ones) was never actually
> > transmitted because mac80211 would ignore
> > subsequent PS-Poll frames due to station being in
> > Service Period. There was nothing left to clear
> > the Service Period bit (no xmit -> no tx status ->
> > no SP end), i.e. the AP would have the station
> > stuck in Service Period. Beacon TIM would
> > repeatedly prompt station to poll for frames but
> > it would get none.
> > 
> > Once (a) is not stripped (b) becomes important
> > because it's the main condition to clear the
> > Service Period bit of the station when Tx status
> > for the released frame is reported back.
> > 
> > This problem was observed with ath9k acting as P2P
> > GO in some testing scenarios but isn't limited to
> > it. AP operation with mac80211 based Tx A-MPDU
> > control combined with clients using PS-Poll frames
> > is subject to this race.
> 
> I'm not sure I quite understand - how is the aggregation teardown
> causing frame filtering?
> 

Never mind, I was looking at the wrong code. I'll apply this.

johannes

Re: [PATCH] mac80211: don't clear all tx flags when requeing

[Johannes Berg]

On Fri, 2015-07-17 at 11:07 +0200, Johannes Berg wrote:
> 
> > > Once (a) is not stripped (b) becomes important
> > > because it's the main condition to clear the
> > > Service Period bit of the station when Tx status
> > > for the released frame is reported back.
> > > 
> > > This problem was observed with ath9k acting as P2P
> > > GO in some testing scenarios but isn't limited to
> > > it. AP operation with mac80211 based Tx A-MPDU
> > > control combined with clients using PS-Poll frames
> > > is subject to this race.
> > 
> > I'm not sure I quite understand - how is the aggregation teardown
> > causing frame filtering?
> > 
> 
> Never mind, I was looking at the wrong code. I'll apply this.
> 

However, I'd like to ask you to look at this again - I can see how it
fixes the problem now, but it seems like a fairly unreliable fix since
the frame is sent through TX processing again, and you're relying on
that preserving a flag that's otherwise marked temporary...

So I think it may be better to adjust the station flags in this case to
let a new (the same) frame be the ps-poll response again.

johannes

Re: [PATCH] mac80211: don't clear all tx flags when requeing

[Michal Kazior]

On 17 July 2015 at 11:09, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2015-07-17 at 11:07 +0200, Johannes Berg wrote:
>>
>> > > Once (a) is not stripped (b) becomes important
>> > > because it's the main condition to clear the
>> > > Service Period bit of the station when Tx status
>> > > for the released frame is reported back.
>> > >
>> > > This problem was observed with ath9k acting as P2P
>> > > GO in some testing scenarios but isn't limited to
>> > > it. AP operation with mac80211 based Tx A-MPDU
>> > > control combined with clients using PS-Poll frames
>> > > is subject to this race.
>> >
>> > I'm not sure I quite understand - how is the aggregation teardown
>> > causing frame filtering?
>> >
>>
>> Never mind, I was looking at the wrong code. I'll apply this.
>>
>
> However, I'd like to ask you to look at this again - I can see how it
> fixes the problem now, but it seems like a fairly unreliable fix since
> the frame is sent through TX processing again, and you're relying on
> that preserving a flag that's otherwise marked temporary...
>
> So I think it may be better to adjust the station flags in this case to
> let a new (the same) frame be the ps-poll response again.

Hmm.. I did contemplate on clearing the station flag. I didn't explore
the idea thoroughly but my feeling was it could introduce new u-APSD
corner-case bug(s) since you'd be changing the station state before
it'd normally be done and hence would behave differently when handling
Rx. I'll look into it more.


Michał