ath9k: fix PAE frame handling

PATCH New Felix Fietkau 4 participants 9 messages Jan 31, 2010 – Feb 16, 2010 Qualcomm Atheros ATH9K wireless driver

Share & tools

Felix Fietkau

author

2010-01-31 21:33 UTC #0 lore raw

PATCH 1/1 ath9k: fix PAE frame handling

ath9k's tx handling code contains a special case for PAE frames, which looks like it was intended to be improving reliability by excluding them from aggregates.
What it actually did is the opposite: By assigning a faulty sequence number, yet still keeping it as a qos-frame, it caused bogus packet reordering, which broke WPA rekeying.
The special case handling is completely unnecessary, so this patch removes it.

Signed-off-by Felix Fietkau <[email protected]>

Cc [email protected]

---

drivers/net/wireless/ath/ath9k/xmit.c +1 ~~-20~~

@@ -1353,25 +1353,6 @@ static enum ath9k_pkt_type get_hw_packet
 	return htype;
 }
 
-static bool is_pae(struct sk_buff *skb)
-{
-	struct ieee80211_hdr *hdr;
-	__le16 fc;
-
-	hdr = (struct ieee80211_hdr *)skb->data;
-	fc = hdr->frame_control;
-
-	if (ieee80211_is_data(fc)) {
-		if (ieee80211_is_nullfunc(fc) ||
-		    /* Port Access Entity (IEEE 802.1X) */
-		    (skb->protocol == cpu_to_be16(ETH_P_PAE))) {
-			return true;
-		}
-	}
-
-	return false;
-}
-
 static int get_hw_crypto_keytype(struct sk_buff *skb)
 {
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);

@@ -1610,7 +1591,7 @@ static int ath_tx_setup_buffer(struct ie
 		bf->bf_frmlen -= padsize;
 	}
 
-	if (conf_is_ht(&hw->conf) && !is_pae(skb))
+	if (conf_is_ht(&hw->conf))
 		bf->bf_state.bf_type |= BUF_HT;
 
 	bf->bf_flags = setup_tx_flags(sc, skb, txctl->txq);

Sujith

2010-02-01 04:47 UTC #1 lore raw

PATCH 1/1 ath9k: fix PAE frame handling

Quoted 8 lines · Felix Fietkau

ath9k's tx handling code contains a special case for PAE frames, which looks like it was intended to be improving reliability by excluding them from aggregates.
What it actually did is the opposite: By assigning a faulty sequence number, yet still keeping it as a qos-frame, it caused bogus packet reordering, which broke WPA rekeying.
The special case handling is completely unnecessary, so this patch removes it.

Sending PAE frames as part of an aggregate broke crypto with several APs. Assigning the correct seq. number should work, no ?

Something like the patch below. Can you check if it fixes your issue ? Though, removing the seq. number mess in the driver would be great. :D

drivers/net/wireless/ath/ath9k/xmit.c +2 -2

index a6893cf..cb6d982 100644
@@ -1610,7 +1610,7 @@ static int ath_tx_setup_buffer(struct ieee80211_hw *hw, struct ath_buf *bf,
 		bf->bf_frmlen -= padsize;
 	}
 
-	if (conf_is_ht(&hw->conf) && !is_pae(skb))
+	if (conf_is_ht(&hw->conf))
 		bf->bf_state.bf_type |= BUF_HT;
 
 	bf->bf_flags = setup_tx_flags(sc, skb, txctl->txq);

@@ -1696,7 +1696,7 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf,
 			goto tx_done;
 		}
 
-		if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
+		if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) && !is_pae(skb)) {
 			/*
 			 * Try aggregation if it's a unicast data frame
 			 * and the destination is HT capable.

Felix Fietkau

author

2010-02-01 05:16 UTC #2 lore raw

Quoted 10 lines · Sujith

Quoted 8 lines · Felix Fietkau
ath9k's tx handling code contains a special case for PAE frames, which looks like it was intended to be improving reliability by excluding them from aggregates.
What it actually did is the opposite: By assigning a faulty sequence number, yet still keeping it as a qos-frame, it caused bogus packet reordering, which broke WPA rekeying.
The special case handling is completely unnecessary, so this patch removes it.

Quoted 5 lines

Sending PAE frames as part of an aggregate broke crypto with several APs. Assigning the correct seq. number should work, no ?
Something like the patch below. Can you check if it fixes your issue ? Though, removing the seq. number mess in the driver would be great. :D

Works for me. Tested it with a really short rekey interval, and I'm getting no packet loss or connection interruption.

- Felix

Kalle Valo

2010-02-04 10:07 UTC #3 lore raw

In reply to Sujith

Quoted 10 lines · Sujith

Quoted 8 lines
ath9k's tx handling code contains a special case for PAE frames, which looks like it was intended to be improving reliability by excluding them from aggregates.
What it actually did is the opposite: By assigning a faulty sequence number, yet still keeping it as a qos-frame, it caused bogus packet reordering, which broke WPA rekeying.
The special case handling is completely unnecessary, so this patch removes it.
Sending PAE frames as part of an aggregate broke crypto with several APs.

Any more information about the problem? For example, what APs are broken?

I'm just wondering if we should see this with other hardware also.

Signature · 1 line

Kalle Valo

Sujith

2010-02-09 04:22 UTC #4 lore raw

Quoted 4 lines · Kalle Valo

Any more information about the problem? For example, what APs are broken?
I'm just wondering if we should see this with other hardware also.

Internal test cycles exposed this problem where certain APs didn't handle properly PAE frames being part of an aggregate.

I'll see if I can dig up the APs from the test results.

Sujith

John W. Linville

2010-02-08 21:45 UTC #5 lore raw

In reply to Sujith

Quoted 10 lines · Sujith

Quoted 8 lines · Felix Fietkau
ath9k's tx handling code contains a special case for PAE frames, which looks like it was intended to be improving reliability by excluding them from aggregates.
What it actually did is the opposite: By assigning a faulty sequence number, yet still keeping it as a qos-frame, it caused bogus packet reordering, which broke WPA rekeying.
The special case handling is completely unnecessary, so this patch removes it.

Quoted 5 lines

Sending PAE frames as part of an aggregate broke crypto with several APs. Assigning the correct seq. number should work, no ?
Something like the patch below. Can you check if it fixes your issue ? Though, removing the seq. number mess in the driver would be great. :D

Sujith, are you going to post this formally?

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

Sujith

2010-02-09 04:17 UTC #6 lore raw

Quoted 1 line · John W. Linville

Sujith, are you going to post this formally?

Sorry, will send a patch in a moment.

Sujith

Felix Fietkau

author

2010-02-16 19:42 UTC #7 lore raw

In reply to Sujith

Quoted 31 lines · Sujith

Quoted 8 lines · Felix Fietkau
ath9k's tx handling code contains a special case for PAE frames, which looks like it was intended to be improving reliability by excluding them from aggregates.
What it actually did is the opposite: By assigning a faulty sequence number, yet still keeping it as a qos-frame, it caused bogus packet reordering, which broke WPA rekeying.
The special case handling is completely unnecessary, so this patch removes it.
Sending PAE frames as part of an aggregate broke crypto with several APs. Assigning the correct seq. number should work, no ?
Something like the patch below. Can you check if it fixes your issue ? Though, removing the seq. number mess in the driver would be great. :D
drivers/net/wireless/ath/ath9k/xmit.c +1 -1
index a6893cf..cb6d982 100644
@@ -1610,7 +1610,7 @@ static int ath_tx_setup_buffer(struct ieee80211_hw *hw, struct ath_buf *bf,
 		bf->bf_frmlen -= padsize;
 	}
 
-	if (conf_is_ht(&hw->conf) && !is_pae(skb))
+	if (conf_is_ht(&hw->conf))
 		bf->bf_state.bf_type |= BUF_HT;
 
 	bf->bf_flags = setup_tx_flags(sc, skb, txctl->txq);

drivers/net/wireless/ath/ath9k/xmit.c static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf, · line 1699

Quoted 5 lines

drivers/net/wireless/ath/ath9k/xmit.c +1 -1

@@ -1696,7 +1696,7 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf,
 			goto tx_done;
 		}
 
-		if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
+		if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) && !is_pae(skb)) {

I just stumbled upon the exact same issue again, though this time with a Windows based Ralink STA instead of the Mac OS Broadcom STA. Same symptoms, rekeying frequently leads to timeouts and thus causes reconnects with long timeouts inbetween.

Turns out that my original patch works better than what was merged - removing the is_pae() check entirely makes all the STA devices that I have here work properly.

I'll submit a patch that removes the is_pae() stuff entirely, as I believe that keeping these special cases there does more harm than good.

- Felix

Felix Fietkau

author

2010-02-16 19:50 UTC #8 lore raw

Quoted 42 lines · Felix Fietkau

Quoted 31 lines · Sujith
Quoted 8 lines · Felix Fietkau
ath9k's tx handling code contains a special case for PAE frames, which looks like it was intended to be improving reliability by excluding them from aggregates.
What it actually did is the opposite: By assigning a faulty sequence number, yet still keeping it as a qos-frame, it caused bogus packet reordering, which broke WPA rekeying.
The special case handling is completely unnecessary, so this patch removes it.
Sending PAE frames as part of an aggregate broke crypto with several APs. Assigning the correct seq. number should work, no ?
Something like the patch below. Can you check if it fixes your issue ? Though, removing the seq. number mess in the driver would be great. :D
drivers/net/wireless/ath/ath9k/xmit.c +1 -1
index a6893cf..cb6d982 100644
@@ -1610,7 +1610,7 @@ static int ath_tx_setup_buffer(struct ieee80211_hw *hw, struct ath_buf *bf,
 		bf->bf_frmlen -= padsize;
 	}
 
-	if (conf_is_ht(&hw->conf) && !is_pae(skb))
+	if (conf_is_ht(&hw->conf))
 		bf->bf_state.bf_type |= BUF_HT;
 
 	bf->bf_flags = setup_tx_flags(sc, skb, txctl->txq);
drivers/net/wireless/ath/ath9k/xmit.c static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf, · line 1699
Quoted 5 lines
drivers/net/wireless/ath/ath9k/xmit.c +1 -1
@@ -1696,7 +1696,7 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf,
 			goto tx_done;
 		}
 
-		if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
+		if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) && !is_pae(skb)) {
I just stumbled upon the exact same issue again, though this time with a Windows based Ralink STA instead of the Mac OS Broadcom STA. Same symptoms, rekeying frequently leads to timeouts and thus causes reconnects with long timeouts inbetween.

Quoted 6 lines

Turns out that my original patch works better than what was merged - removing the is_pae() check entirely makes all the STA devices that I have here work properly.
I'll submit a patch that removes the is_pae() stuff entirely, as I believe that keeping these special cases there does more harm than good.

Oops, sorry. It seems that I've been tricked by a Heisenbug. After I changed the code, it worked fine for 12 rekey intervals, whereas it had failed every second attempt before.
Now that I've restarted the interface it's failing frequently again. Must be something else going on here, I'll keep looking.

- Felix