mac80211 : fix unaligned rx skb

PATCH matthieu castet 5 participants 8 messages Jun 4, 2009 – Jun 7, 2009 mac80211

Share & tools

matthieu castet

author

2009-06-04 20:16 UTC #0 lore raw

mac80211 is checking is the skb is aligned on 32 bit boundary.
But it is checking against ethernet header, whereas Linux expect IP
header aligned.
And ethernet ether size is 6*2+2=14, so aligning ethernet header make IP header unaligned.

Signed-off-by Matthieu CASTET <[email protected]>

net/mac80211/rx.c +1 -1

index 9776f73..0845fb3 100644
@@ -1397,7 +1397,7 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx)
 		 * mac80211. That also explains the __skb_push()
 		 * below.
 		 */
-		align = (unsigned long)skb->data & 3;
+		align = ((unsigned long)(skb->data + sizeof(struct ethhdr))) & 3;
 		if (align) {
 			if (WARN_ON(skb_headroom(skb) < 3)) {
 				dev_kfree_skb(skb);

Luis R. Rodriguez

2009-06-04 20:53 UTC #1 lore raw

Quoted 1 line · matthieu castet

This is all I see, can you resend with patch inline?

Luis

Georgy Berdyshev

2009-06-04 21:16 UTC #2 lore raw

Hi,

that's the inline version:
-------
mac80211 is checking is the skb is aligned on 32 bit boundary. But it is checking against ethernet header, whereas Linux expect IP header aligned.
And ethernet ether size is 6*2+2=14, so aligning ethernet header make IP header unaligned.

Signed-off-by Matthieu CASTET <[email protected]>

net/mac80211/rx.c +1 -1

index 9776f73..0845fb3 100644
@@ -1397,7 +1397,7 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx)
 		 * mac80211. That also explains the __skb_push()
 		 * below.
 		 */
-		align = (unsigned long)skb->data & 3;
+		align = ((unsigned long)(skb->data + sizeof(struct ethhdr))) & 3;
 		if (align) {
 			if (WARN_ON(skb_headroom(skb) < 3)) {
 				dev_kfree_skb(skb);

------------

Quoted 11 lines · Luis R. Rodriguez

Quoted 1 line · matthieu castet
This is all I see, can you resend with patch inline?
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 4 lines

Georgy Berdyshev

GPG key: 830F68C5
Fingerprint: 0379 ED5A BEE5 65A8 7BD5  31E7 F5B4 1EC7 830F 68C5

Michael Buesch

2009-06-04 21:40 UTC #3 lore raw

Quoted 16 lines · On Thursday 04 June 2009 23:16:13 Georgy Berdyshev wrote:

Hi,
that's the inline version:
-------
mac80211 is checking is the skb is aligned on 32 bit boundary. But it is checking against ethernet header, whereas Linux expect IP header aligned.
And ethernet ether size is 6*2+2=14, so aligning ethernet header make IP header unaligned.
Signed-off-by Matthieu CASTET <[email protected]>
net/mac80211/rx.c +0 -0
index 9776f73..0845fb3 100644

net/mac80211/rx.c ieee80211_deliver_skb(struct ieee80211_rx_data *rx) · line 1403

Quoted 8 lines

net/mac80211/rx.c +1 -1

@@ -1397,7 +1397,7 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx)
 		 * mac80211. That also explains the __skb_push()
 		 * below.
 		 */
-		align = (unsigned long)skb->data & 3;
+		align = ((unsigned long)(skb->data + sizeof(struct ethhdr))) & 3;
 		if (align) {
 			if (WARN_ON(skb_headroom(skb) < 3)) {
 				dev_kfree_skb(skb);

Uhm, can you give a more verbose explanation? Without that I'd say this patch is plain wrong. What the hell does struct ethhdr have to do with wireless?
This is not ethernet. It's 802.11. There is no such thing as an ethernet header in a 802.11 packet.

Signature · 1 line

Greetings, Michael.

matthieu castet

author

2009-06-05 05:57 UTC #4 lore raw

Quoted 22 lines · Michael Buesch

Quoted 21 lines · On Thursday 04 June 2009 23:16:13 Georgy Berdyshev wrote:
Hi,
that's the inline version:
-------
mac80211 is checking is the skb is aligned on 32 bit boundary. But it is checking against ethernet header, whereas Linux expect IP header aligned.
And ethernet ether size is 6*2+2=14, so aligning ethernet header make IP header unaligned.
Signed-off-by Matthieu CASTET <[email protected]>
net/mac80211/rx.c +1 -1
index 9776f73..0845fb3 100644
@@ -1397,7 +1397,7 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx)
 		 * mac80211. That also explains the __skb_push()
 		 * below.
 		 */
-		align = (unsigned long)skb->data & 3;
+		align = ((unsigned long)(skb->data + sizeof(struct ethhdr))) & 3;

Quoted 9 lines

net/mac80211/rx.c ieee80211_deliver_skb(struct ieee80211_rx_data *rx) · line 1403
Quoted 3 lines
net/mac80211/rx.c +0 -0
 		if (align) {
 			if (WARN_ON(skb_headroom(skb) < 3)) {
 				dev_kfree_skb(skb);
Uhm, can you give a more verbose explanation? Without that I'd say this patch is plain wrong. What the hell does struct ethhdr have to do with wireless?
This is not ethernet. It's 802.11. There is no such thing as an
ethernet header in a 802.11 packet.

Where are in ieee80211_deliver_skb that is called after ieee80211_data_to_8023. So it is not 802.11.

Matthieu

matthieu castet

author

2009-06-07 09:24 UTC #5 lore raw

Quoted 34 lines · matthieu castet

Quoted 24 lines · Michael Buesch
Quoted 23 lines · On Thursday 04 June 2009 23:16:13 Georgy Berdyshev wrote:
Hi,
that's the inline version:
-------
mac80211 is checking is the skb is aligned on 32 bit boundary. But it is checking against ethernet header, whereas Linux expect IP header aligned.
And ethernet ether size is 6*2+2=14, so aligning ethernet header make IP header unaligned.
Signed-off-by Matthieu CASTET <[email protected]>
net/mac80211/rx.c +1 -1
index 9776f73..0845fb3 100644
@@ -1397,7 +1397,7 @@ ieee80211_deliver_skb(struct ieee80211_rx_data 
 *rx)
          * mac80211. That also explains the __skb_push()
          * below.
          */
-        align = (unsigned long)skb->data & 3;
+        align = ((unsigned long)(skb->data + sizeof(struct ethhdr))) 
 & 3;
Quoted 9 lines
Quoted 3 lines
net/mac80211/rx.c +0 -0
         if (align) {
             if (WARN_ON(skb_headroom(skb) < 3)) {
                 dev_kfree_skb(skb);
Uhm, can you give a more verbose explanation? Without that I'd say this patch is plain wrong. What the hell does struct ethhdr have to do with wireless?
This is not ethernet. It's 802.11. There is no such thing as an
ethernet header in a 802.11 packet.

Quoted 2 lines

Where are in ieee80211_deliver_skb that is called after ieee80211_data_to_8023. So it is not 802.11.

Do you want more explanation ?

Matthieu

Michael Buesch

2009-06-07 09:26 UTC #6 lore raw

Quoted 33 lines · On Sunday 07 June 2009 11:24:48 matthieu castet wrote:

Quoted 32 lines · matthieu castet
Quoted 31 lines · Michael Buesch
Quoted 24 lines · On Thursday 04 June 2009 23:16:13 Georgy Berdyshev wrote:
Hi,
that's the inline version:
-------
mac80211 is checking is the skb is aligned on 32 bit boundary. But it is checking against ethernet header, whereas Linux expect IP header aligned.
And ethernet ether size is 6*2+2=14, so aligning ethernet header make IP header unaligned.
Signed-off-by Matthieu CASTET <[email protected]>
net/mac80211/rx.c +1 -1
index 9776f73..0845fb3 100644
@@ -1397,7 +1397,7 @@ ieee80211_deliver_skb(struct ieee80211_rx_data 
 *rx)
          * mac80211. That also explains the __skb_push()
          * below.
          */
-        align = (unsigned long)skb->data & 3;
+        align = ((unsigned long)(skb->data + sizeof(struct ethhdr))) 
 & 3;
         if (align) {
Quoted 2 lines
             if (WARN_ON(skb_headroom(skb) < 3)) {
                 dev_kfree_skb(skb);
Uhm, can you give a more verbose explanation? Without that I'd say this patch is plain wrong.
What the hell does struct ethhdr have to do with wireless?

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
This is not ethernet. It's 802.11. There is no such thing as an ethernet header in a 802.11 packet.
Where are in ieee80211_deliver_skb that is called after ieee80211_data_to_8023. So it is not 802.11.
Do you want more explanation ?

No reply always means "ok" on this list ;)

Signature · 1 line

Greetings, Michael.

John W. Linville

2009-06-07 15:30 UTC #7 lore raw

Quoted 9 lines · Michael Buesch

Quoted 6 lines · On Sunday 07 June 2009 11:24:48 matthieu castet wrote:
Quoted 3 lines · matthieu castet
Where are in ieee80211_deliver_skb that is called after ieee80211_data_to_8023. So it is not 802.11.
Do you want more explanation ?
No reply always means "ok" on this list ;)

Probably true...however, I was hoping to see an ACK from Johannes...

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.