[RFC]: atk5k: fix FCS corruption for ACKs

[RFC]: atk5k: fix FCS corruption for ACKs

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

When running a monitor interface with ath5k, the FCS of ACK
frames is two bytes short. The reason is that unlike the
payload, the FCS doesn't seem to aligned to a 4 byte boundary
by the chip, the attempt to remove the padding corrupts it.

This patch checks whether there actually is any payload after
the header (besides the FCS) before removing the padding.

This might not be fully correct or not handle other cases where
this can occur, but it doesn't seem too hackish and fixes the
problem for me :)

http://bugzilla.kernel.org/show_bug.cgi?id=12101


[-- Attachment #2: 01.diff --]
[-- Type: text/x-patch, Size: 874 bytes --]

commit 8c1d598338077d397aed25fe4840e4d4b51ba79c
Author: Patrick McHardy <kaber@trash.net>
Date:   Tue Dec 9 21:52:00 2008 +0100

    atk5k: fix FCS corruption for ACKs
    
    The trailing FCS value is not aliged when no payload is present. Don't try
    to remove padding to avoid corrupting it.

    Kernel Bugzilla #12101
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index d9e1980..940c724 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1752,7 +1752,7 @@ accept:
 		 * not multiples of 4 - remove it
 		 */
 		hdrlen = ieee80211_get_hdrlen_from_skb(skb);
-		if (hdrlen & 3) {
+		if (hdrlen & 3 && hdrlen != rs.rs_datalen - FCS_LEN) {
 			pad = hdrlen % 4;
 			memmove(skb->data + pad, skb->data, hdrlen);
 			skb_pull(skb, pad);

Re: [RFC]: atk5k: fix FCS corruption for ACKs

[Bob Copeland]

On Wed, Dec 10, 2008 at 06:04:05AM +0100, Patrick McHardy wrote:
> This might not be fully correct or not handle other cases where
> this can occur, but it doesn't seem too hackish and fixes the
> problem for me :)

> -		if (hdrlen & 3) {
> +		if (hdrlen & 3 && hdrlen != rs.rs_datalen - FCS_LEN) {
>  			pad = hdrlen % 4;
>  			memmove(skb->data + pad, skb->data, hdrlen);
>  			skb_pull(skb, pad);

It seems very plausible to me.  Though, why doesn't ath9k also have this
problem?  Luis, it looks like in that case ath9k could trim two extra
bytes if ath9k hw behaves the same.

main.c:

   951          /* see if any padding is done by the hw and remove it */
   952          if (hdrlen & 3) {
   953                  padsize = hdrlen % 4;
   954                  memmove(skb->data + padsize, skb->data, hdrlen);
   955                  skb_pull(skb, padsize);
   956          }

   957          /* remove FCS before passing up to protocol stack */
   958          skb_trim(skb, (skb->len - FCS_LEN));

-- 
Bob Copeland %% www.bobcopeland.com

Re: [RFC]: atk5k: fix FCS corruption for ACKs

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Wed, 2008-12-10 at 08:24 -0500, Bob Copeland wrote:

> It seems very plausible to me.  Though, why doesn't ath9k also have this
> problem?  Luis, it looks like in that case ath9k could trim two extra
> bytes if ath9k hw behaves the same.
> 
> main.c:
> 
>    951          /* see if any padding is done by the hw and remove it */
>    952          if (hdrlen & 3) {
>    953                  padsize = hdrlen % 4;
>    954                  memmove(skb->data + padsize, skb->data, hdrlen);
>    955                  skb_pull(skb, padsize);
>    956          }
> 
>    957          /* remove FCS before passing up to protocol stack */
>    958          skb_trim(skb, (skb->len - FCS_LEN));

It also shouldn't remove the FCS, sometimes you want to see that on
wireshark and mac80211 handles removing the FCS itself just fine, just
set the HW flag.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC]: atk5k: fix FCS corruption for ACKs

[Sujith]

Bob Copeland wrote:
>    951          /* see if any padding is done by the hw and remove it */
>    952          if (hdrlen & 3) {
>    953                  padsize = hdrlen % 4;
>    954                  memmove(skb->data + padsize, skb->data, hdrlen);
>    955                  skb_pull(skb, padsize);
>    956          }
> 
>    957          /* remove FCS before passing up to protocol stack */
>    958          skb_trim(skb, (skb->len - FCS_LEN));
> 

I remember removing this piece of code...

Sujith

Re: [RFC]: atk5k: fix FCS corruption for ACKs

[Bob Copeland]

On Wed, Dec 10, 2008 at 8:43 AM, Sujith <m.sujith@gmail.com> wrote:
>>    957          /* remove FCS before passing up to protocol stack */
>>    958          skb_trim(skb, (skb->len - FCS_LEN));
>>
>
> I remember removing this piece of code...

Hmm, you're right.  I must've been looking at 2.6.27.  Sorry for the noise.

The padding still gets removed regardless of whether there is a payload;
does that truncate the FCS for ACK/CTS frames on ath9k?

>>    952          if (hdrlen & 3) {
>>    953                  padsize = hdrlen % 4;

While we're at it, (for ath5k also) -- hdrlen % 4 can just be hdrlen & 3
again.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [RFC]: atk5k: fix FCS corruption for ACKs

[Bob Copeland]

On Wed, Dec 10, 2008 at 8:24 AM, Bob Copeland <me@bobcopeland.com> wrote:
> On Wed, Dec 10, 2008 at 06:04:05AM +0100, Patrick McHardy wrote:
>> This might not be fully correct or not handle other cases where
>> this can occur, but it doesn't seem too hackish and fixes the
>> problem for me :)
>
>> -             if (hdrlen & 3) {
>> +             if (hdrlen & 3 && hdrlen != rs.rs_datalen - FCS_LEN) {
>>                       pad = hdrlen % 4;
>>                       memmove(skb->data + pad, skb->data, hdrlen);
>>                       skb_pull(skb, pad);
>
> It seems very plausible to me.  Though, why doesn't ath9k also have this
> problem?

So, I guess ath9k did too.  This patch gets my ack, though we should probably
do the hdrlen check the same way as Jouni's patch for consistency, and switch
to hdrlen & 3 when computing pad.

-- 
Bob Copeland %% www.bobcopeland.com