* Re: [PATCH] ath9k: fix PAE frame handling
2010-02-01 5:05 ` Sujith
@ 2010-02-01 5:16 ` Felix Fietkau
2010-02-04 10:07 ` Kalle Valo
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Felix Fietkau @ 2010-02-01 5:16 UTC (permalink / raw)
To: Sujith; +Cc: linux-wireless, Luis Rodriguez, John W. Linville
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ath9k: fix PAE frame handling
2010-02-01 5:05 ` Sujith
2010-02-01 5:16 ` Felix Fietkau
@ 2010-02-04 10:07 ` Kalle Valo
2010-02-09 4:40 ` Sujith
2010-02-08 21:39 ` John W. Linville
2010-02-16 19:42 ` Felix Fietkau
3 siblings, 1 reply; 9+ messages in thread
From: Kalle Valo @ 2010-02-04 10:07 UTC (permalink / raw)
To: Sujith; +Cc: Felix Fietkau, linux-wireless, Luis Rodriguez, John W. Linville
Sujith <Sujith.Manoharan@atheros.com> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ath9k: fix PAE frame handling
2010-02-04 10:07 ` Kalle Valo
@ 2010-02-09 4:40 ` Sujith
0 siblings, 0 replies; 9+ messages in thread
From: Sujith @ 2010-02-09 4:40 UTC (permalink / raw)
To: Kalle Valo
Cc: Felix Fietkau, linux-wireless, Luis Rodriguez, John W. Linville
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ath9k: fix PAE frame handling
2010-02-01 5:05 ` Sujith
2010-02-01 5:16 ` Felix Fietkau
2010-02-04 10:07 ` Kalle Valo
@ 2010-02-08 21:39 ` John W. Linville
2010-02-09 4:35 ` Sujith
2010-02-16 19:42 ` Felix Fietkau
3 siblings, 1 reply; 9+ messages in thread
From: John W. Linville @ 2010-02-08 21:39 UTC (permalink / raw)
To: Sujith; +Cc: Felix Fietkau, linux-wireless, Luis Rodriguez
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
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ath9k: fix PAE frame handling
2010-02-01 5:05 ` Sujith
` (2 preceding siblings ...)
2010-02-08 21:39 ` John W. Linville
@ 2010-02-16 19:42 ` Felix Fietkau
2010-02-16 19:50 ` Felix Fietkau
3 siblings, 1 reply; 9+ messages in thread
From: Felix Fietkau @ 2010-02-16 19:42 UTC (permalink / raw)
To: Sujith; +Cc: linux-wireless, Luis Rodriguez, John W. Linville
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ath9k: fix PAE frame handling
2010-02-16 19:42 ` Felix Fietkau
@ 2010-02-16 19:50 ` Felix Fietkau
0 siblings, 0 replies; 9+ messages in thread
From: Felix Fietkau @ 2010-02-16 19:50 UTC (permalink / raw)
To: Sujith; +Cc: linux-wireless, Luis Rodriguez, John W. Linville
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
^ permalink raw reply [flat|nested] 9+ messages in thread