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]
---
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -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);
On 2010-02-01 6:05 AM, Sujith wrote:
> Felix Fietkau wrote:
>> 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
Works for me. Tested it with a really short rekey interval, and I'm
getting no packet loss or connection interruption.
- Felix
John W. Linville wrote:
> Sujith, are you going to post this formally?
Sorry, will send a patch in a moment.
Sujith
Felix Fietkau wrote:
> 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
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index a6893cf..cb6d982 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -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.
On 2010-02-01 6:05 AM, Sujith wrote:
> Felix Fietkau wrote:
>> 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
>
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index a6893cf..cb6d982 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -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)) {
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
On Mon, Feb 01, 2010 at 10:35:09AM +0530, Sujith wrote:
> Felix Fietkau wrote:
> > 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
Sujith, are you going to post this formally?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On 2010-02-16 8:42 PM, Felix Fietkau wrote:
> On 2010-02-01 6:05 AM, Sujith wrote:
>> Felix Fietkau wrote:
>>> 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
>>
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> index a6893cf..cb6d982 100644
>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> @@ -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)) {
> 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.
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
Kalle Valo wrote:
> 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
Sujith <[email protected]> writes:
>> 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.
--
Kalle Valo