[PATCH] mac80211: fix sequence number assignment for PS response frames

[PATCH] mac80211: fix sequence number assignment for PS response frames

[Felix Fietkau]

When using intermediate queues, sequence number allocation is deferred
until dequeue. This doesn't work for PS response frames, which bypass
those queues.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/tx.c | 65 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5023966..cc8e955 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -796,6 +796,36 @@ static __le16 ieee80211_tx_next_seq(struct sta_info *sta, int tid)
 	return ret;
 }
 
+static struct txq_info *ieee80211_get_txq(struct ieee80211_local *local,
+					  struct ieee80211_vif *vif,
+					  struct ieee80211_sta *pubsta,
+					  struct sk_buff *skb)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+	struct ieee80211_txq *txq = NULL;
+
+	if ((info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) ||
+	    (info->control.flags & IEEE80211_TX_CTRL_PS_RESPONSE))
+		return NULL;
+
+	if (!ieee80211_is_data(hdr->frame_control))
+		return NULL;
+
+	if (pubsta) {
+		u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
+
+		txq = pubsta->txq[tid];
+	} else if (vif) {
+		txq = vif->txq;
+	}
+
+	if (!txq)
+		return NULL;
+
+	return to_txq_info(txq);
+}
+
 static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
 {
@@ -853,7 +883,8 @@ ieee80211_tx_h_sequence(struct ieee80211_tx_data *tx)
 	tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
 	tx->sta->tx_stats.msdu[tid]++;
 
-	if (!tx->sta->sta.txq[0])
+	if (!ieee80211_get_txq(tx->local, info->control.vif, &tx->sta->sta,
+			       tx->skb))
 		hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
 
 	return TX_CONTINUE;
@@ -1243,36 +1274,6 @@ ieee80211_tx_prepare(struct ieee80211_sub_if_data *sdata,
 	return TX_CONTINUE;
 }
 
-static struct txq_info *ieee80211_get_txq(struct ieee80211_local *local,
-					  struct ieee80211_vif *vif,
-					  struct ieee80211_sta *pubsta,
-					  struct sk_buff *skb)
-{
-	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
-	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
-	struct ieee80211_txq *txq = NULL;
-
-	if ((info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) ||
-	    (info->control.flags & IEEE80211_TX_CTRL_PS_RESPONSE))
-		return NULL;
-
-	if (!ieee80211_is_data(hdr->frame_control))
-		return NULL;
-
-	if (pubsta) {
-		u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
-
-		txq = pubsta->txq[tid];
-	} else if (vif) {
-		txq = vif->txq;
-	}
-
-	if (!txq)
-		return NULL;
-
-	return to_txq_info(txq);
-}
-
 static void ieee80211_set_skb_enqueue_time(struct sk_buff *skb)
 {
 	IEEE80211_SKB_CB(skb)->control.enqueue_time = codel_get_time();
@@ -3264,7 +3265,7 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
 
 	if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
 		*ieee80211_get_qos_ctl(hdr) = tid;
-		if (!sta->sta.txq[0])
+		if (!ieee80211_get_txq(local, &sdata->vif, &sta->sta, skb))
 			hdr->seq_ctrl = ieee80211_tx_next_seq(sta, tid);
 	} else {
 		info->flags |= IEEE80211_TX_CTL_ASSIGN_SEQ;
-- 
2.8.4

Re: [PATCH] mac80211: fix sequence number assignment for PS response frames

[Johannes Berg]

On Sun, 2016-09-04 at 18:00 +0200, Felix Fietkau wrote:
> When using intermediate queues, sequence number allocation is
> deferred
> until dequeue. This doesn't work for PS response frames, which bypass
> those queues.
> 
Applied.

This worries me a bit though - there's nothing, afaict, that guarantees
that dequeue and real TX are not concurrent, and that would corrupt a
number of things, no?

johannes

Re: [PATCH] mac80211: fix sequence number assignment for PS response frames

[Felix Fietkau]

On 2016-09-12 11:58, Johannes Berg wrote:
> On Sun, 2016-09-04 at 18:00 +0200, Felix Fietkau wrote:
>> When using intermediate queues, sequence number allocation is
>> deferred
>> until dequeue. This doesn't work for PS response frames, which bypass
>> those queues.
>> 
> Applied.
> 
> This worries me a bit though - there's nothing, afaict, that guarantees
> that dequeue and real TX are not concurrent, and that would corrupt a
> number of things, no?
Hm, I guess I didn't think of that. I guess this potential issue will go
away once we get Toke's tx handler reorder patch fixed, rebased and
integrated.

- Felix

Re: [PATCH] mac80211: fix sequence number assignment for PS response frames

[Johannes Berg]

> Hm, I guess I didn't think of that. I guess this potential issue will
> go away once we get Toke's tx handler reorder patch fixed, rebased
> and integrated.
> 

I don't really see how that helps?

johannes

Re: [PATCH] mac80211: fix sequence number assignment for PS response frames

[Felix Fietkau]

On 2016-09-12 12:03, Johannes Berg wrote:
> 
>> Hm, I guess I didn't think of that. I guess this potential issue will
>> go away once we get Toke's tx handler reorder patch fixed, rebased
>> and integrated.
>> 
> 
> I don't really see how that helps?
It replaces the changes that I made.

- Felix

Re: [PATCH] mac80211: fix sequence number assignment for PS response frames

[Johannes Berg]

On Mon, 2016-09-12 at 12:05 +0200, Felix Fietkau wrote:
> On 2016-09-12 12:03, Johannes Berg wrote:
> > 
> > 
> > > 
> > > Hm, I guess I didn't think of that. I guess this potential issue
> > > will
> > > go away once we get Toke's tx handler reorder patch fixed,
> > > rebased
> > > and integrated.
> > > 
> > 
> > I don't really see how that helps?
> It replaces the changes that I made.
> 

But this is a more general problem, no?

johannes

Re: [PATCH] mac80211: fix sequence number assignment for PS response frames

[Felix Fietkau]

On 2016-09-12 12:07, Johannes Berg wrote:
> On Mon, 2016-09-12 at 12:05 +0200, Felix Fietkau wrote:
>> On 2016-09-12 12:03, Johannes Berg wrote:
>> > 
>> > 
>> > > 
>> > > Hm, I guess I didn't think of that. I guess this potential issue
>> > > will
>> > > go away once we get Toke's tx handler reorder patch fixed,
>> > > rebased
>> > > and integrated.
>> > > 
>> > 
>> > I don't really see how that helps?
>> It replaces the changes that I made.
>> 
> 
> But this is a more general problem, no?
Will look into it some more soon.

- Felix

Re: [PATCH] mac80211: fix sequence number assignment for PS response frames

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd-Vt+b4OUoWG0@public.gmane.org> writes:

> On 2016-09-12 12:07, Johannes Berg wrote:
>> On Mon, 2016-09-12 at 12:05 +0200, Felix Fietkau wrote:
>>> On 2016-09-12 12:03, Johannes Berg wrote:
>>> > 
>>> > 
>>> > > 
>>> > > Hm, I guess I didn't think of that. I guess this potential issue
>>> > > will
>>> > > go away once we get Toke's tx handler reorder patch fixed,
>>> > > rebased
>>> > > and integrated.
>>> > > 
>>> > 
>>> > I don't really see how that helps?
>>> It replaces the changes that I made.
>>> 
>> 
>> But this is a more general problem, no?
> Will look into it some more soon.
>
> - Felix

Well, ath9k calls ieee80211_tx_dequeue while holding the (driver) TXQ
lock. Which means that a packet going through the old TX path can block
waiting for the driver to finish pulling packets from the mac80211
queue. So that could definitely lead to reordering of sequence numbers.
And the obvious fix of taking a lock in mac80211 could then lead to
deadlock. Fun times! :)

Hmm, is there a reason why those packets *have* to go through the old TX
path? My reordering patchset introduces a queue that takes priority over
the FQ (for fragments created after dequeue). Would sticking the PS
response frame on there and having the driver pull it work?

-Toke