[PATCH] mac80211: fix alignment issue with compare_ether_addr()

[PATCH] mac80211: fix alignment issue with compare_ether_addr()

[Senthil Balasubramanian]

This addresses an alignment issue with compare_ether_addr().
The addresses passed to compare_ether_addr should be two bytes aligned.
It may function properly in x86 platform. However may not work properly
on IA-64 or ARM processor.

This also fixes a typo in mlme.c where the sk_buff struct name is incorect.
Though sizeof() works for any incorrect structure pointer name as its just
a pointer length that we want, lets just fix it.

Signed-off-by: Senthil Balasubramanian <senthilkumar@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/mlme.c |    4 ++--
 net/mac80211/rx.c   |    6 ++++--
 net/mac80211/util.c |    4 ++--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index c7819fd..4be7736 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1316,7 +1316,7 @@ static void ieee80211_sta_process_addba_request(struct net_device *dev,
 
 	/* prepare reordering buffer */
 	tid_agg_rx->reorder_buf =
-		kmalloc(buf_size * sizeof(struct sk_buf *), GFP_ATOMIC);
+		kmalloc(buf_size * sizeof(struct sk_buff *), GFP_ATOMIC);
 	if (!tid_agg_rx->reorder_buf) {
 		if (net_ratelimit())
 			printk(KERN_ERR "can not allocate reordering buffer "
@@ -1325,7 +1325,7 @@ static void ieee80211_sta_process_addba_request(struct net_device *dev,
 		goto end;
 	}
 	memset(tid_agg_rx->reorder_buf, 0,
-		buf_size * sizeof(struct sk_buf *));
+		buf_size * sizeof(struct sk_buff *));
 
 	if (local->ops->ampdu_action)
 		ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_RX_START,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 9400a97..4b83ed1 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1116,7 +1116,9 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
 	u16 fc, hdrlen, ethertype;
 	u8 *payload;
 	u8 dst[ETH_ALEN];
-	u8 src[ETH_ALEN];
+	/* Should be aligned on 2 bytes for compare_ether_addr() */
+	u16 src_aligned[ETH_ALEN >> 1];
+	u8 *src = (u8 *)src_aligned;
 	struct sk_buff *skb = rx->skb;
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	DECLARE_MAC_BUF(mac);
@@ -1259,7 +1261,7 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
  */
 static bool ieee80211_frame_allowed(struct ieee80211_rx_data *rx)
 {
-	static const u8 pae_group_addr[ETH_ALEN]
+	static const u8 pae_group_addr[ETH_ALEN] __attribute__ ((aligned(2)))
 		= { 0x01, 0x80, 0xC2, 0x00, 0x00, 0x03 };
 	struct ethhdr *ehdr = (struct ethhdr *) rx->skb->data;
 
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 5a77e2c..416214c 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -34,11 +34,11 @@ void *mac80211_wiphy_privid = &mac80211_wiphy_privid;
 
 /* See IEEE 802.1H for LLC/SNAP encapsulation/decapsulation */
 /* Ethernet-II snap header (RFC1042 for most EtherTypes) */
-const unsigned char rfc1042_header[] =
+const unsigned char rfc1042_header[] __attribute__ ((aligned(2))) =
 	{ 0xaa, 0xaa, 0x03, 0x00, 0x00, 0x00 };
 
 /* Bridge-Tunnel header (for EtherTypes ETH_P_AARP and ETH_P_IPX) */
-const unsigned char bridge_tunnel_header[] =
+const unsigned char bridge_tunnel_header[] __attribute__ ((aligned(2))) =
 	{ 0xaa, 0xaa, 0x03, 0x00, 0x00, 0xf8 };
 
 
-- 
1.5.2.2

Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

[John W. Linville]

On Wed, May 28, 2008 at 08:20:48PM +0530, Senthil Balasubramanian wrote:
> This addresses an alignment issue with compare_ether_addr().
> The addresses passed to compare_ether_addr should be two bytes aligned.
> It may function properly in x86 platform. However may not work properly
> on IA-64 or ARM processor.

> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1116,7 +1116,9 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>  	u16 fc, hdrlen, ethertype;
>  	u8 *payload;
>  	u8 dst[ETH_ALEN];
> -	u8 src[ETH_ALEN];
> +	/* Should be aligned on 2 bytes for compare_ether_addr() */
> +	u16 src_aligned[ETH_ALEN >> 1];
> +	u8 *src = (u8 *)src_aligned;
>  	struct sk_buff *skb = rx->skb;
>  	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>  	DECLARE_MAC_BUF(mac);

Any reason you couldn't just do this?

@@ -1116,7 +1116,7 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
 	u16 fc, hdrlen, ethertype;
 	u8 *payload;
 	u8 dst[ETH_ALEN];
-	u8 src[ETH_ALEN];
+	u8 src[ETH_ALEN] __attribute__ ((aligned(2)));
 	struct sk_buff *skb = rx->skb;
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	DECLARE_MAC_BUF(mac);

It seems to compile w/o errors on i686, and it seems more clear to me.
Will that not work?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

[Johannes Berg]

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


> +	/* Should be aligned on 2 bytes for compare_ether_addr() */
> +	u16 src_aligned[ETH_ALEN >> 1];
> +	u8 *src = (u8 *)src_aligned;

why not use aligned attribute here as well?

> -	static const u8 pae_group_addr[ETH_ALEN]
> +	static const u8 pae_group_addr[ETH_ALEN] __attribute__ ((aligned(2)))

I think we should use __aligned(2)

johannes

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

Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

[Johannes Berg]

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


> -	u8 src[ETH_ALEN];
> +	u8 src[ETH_ALEN] __attribute__ ((aligned(2)));

> It seems to compile w/o errors on i686, and it seems more clear to me.
> Will that not work?

I think it should be __aligned(2) anyway :)

Other than that, yeah, we definitely want this patch. And the previous
one too, thanks.

johannes

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

RE: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

[Senthilkumar Balasubramanian]

________________________________________
From: John W. Linville [linville@tuxdriver.com]
Sent: Wednesday, May 28, 2008 8:46 PM
To: Senthilkumar Balasubramanian
Cc: johannes@sipsolutions.net; ron.rindjunsky@intel.com; linux-wireless@vger.kernel.org; Luis Rodriguez
Subject: Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

On Wed, May 28, 2008 at 08:20:48PM +0530, Senthil Balasubramanian wrote:
> This addresses an alignment issue with compare_ether_addr().
> The addresses passed to compare_ether_addr should be two bytes aligned.
> It may function properly in x86 platform. However may not work properly
> on IA-64 or ARM processor.

> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1116,7 +1116,9 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>       u16 fc, hdrlen, ethertype;
>       u8 *payload;
>       u8 dst[ETH_ALEN];
> -     u8 src[ETH_ALEN];
> +     /* Should be aligned on 2 bytes for compare_ether_addr() */
> +     u16 src_aligned[ETH_ALEN >> 1];
> +     u8 *src = (u8 *)src_aligned;
>       struct sk_buff *skb = rx->skb;
>       struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>       DECLARE_MAC_BUF(mac);

Any reason you couldn't just do this?

I just thought of using the type u16 instead of attribute here. that's all. No specific reason.

@@ -1116,7 +1116,7 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
        u16 fc, hdrlen, ethertype;
        u8 *payload;
        u8 dst[ETH_ALEN];
-       u8 src[ETH_ALEN];
+       u8 src[ETH_ALEN] __attribute__ ((aligned(2)));
        struct sk_buff *skb = rx->skb;
        struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
        DECLARE_MAC_BUF(mac);

It seems to compile w/o errors on i686, and it seems more clear to me.
Will that not work?

Yes. It should also work. I will re-create the patch and send it to you. I will also include Johannes suggestion of just using __aligned macro.

John
--
John W. Linville
linville@tuxdriver.com

Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

[Tomas Winkler]

On Wed, May 28, 2008 at 7:50 PM, Senthilkumar Balasubramanian
<Senthilkumar.Balasubramanian@atheros.com> wrote:
> ________________________________________
> From: John W. Linville [linville@tuxdriver.com]
> Sent: Wednesday, May 28, 2008 8:46 PM
> To: Senthilkumar Balasubramanian
> Cc: johannes@sipsolutions.net; ron.rindjunsky@intel.com; linux-wireless@vger.kernel.org; Luis Rodriguez
> Subject: Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()
>
> On Wed, May 28, 2008 at 08:20:48PM +0530, Senthil Balasubramanian wrote:
>> This addresses an alignment issue with compare_ether_addr().
>> The addresses passed to compare_ether_addr should be two bytes aligned.
>> It may function properly in x86 platform. However may not work properly
>> on IA-64 or ARM processor.
>
>> --- a/net/mac80211/rx.c
>> +++ b/net/mac80211/rx.c
>> @@ -1116,7 +1116,9 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>>       u16 fc, hdrlen, ethertype;
>>       u8 *payload;
>>       u8 dst[ETH_ALEN];
>> -     u8 src[ETH_ALEN];
>> +     /* Should be aligned on 2 bytes for compare_ether_addr() */
>> +     u16 src_aligned[ETH_ALEN >> 1];
>> +     u8 *src = (u8 *)src_aligned;
>>       struct sk_buff *skb = rx->skb;
>>       struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>>       DECLARE_MAC_BUF(mac);
>
> Any reason you couldn't just do this?
>
> I just thought of using the type u16 instead of attribute here. that's all. No specific reason.
>
> @@ -1116,7 +1116,7 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>        u16 fc, hdrlen, ethertype;
>        u8 *payload;
>        u8 dst[ETH_ALEN];
> -       u8 src[ETH_ALEN];
> +       u8 src[ETH_ALEN] __attribute__ ((aligned(2)));
>        struct sk_buff *skb = rx->skb;
>        struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>        DECLARE_MAC_BUF(mac);
>
> It seems to compile w/o errors on i686, and it seems more clear to me.
> Will that not work?
>
> Yes. It should also work. I will re-create the patch and send it to you. I will also include Johannes suggestion of just using __aligned macro.
>
Can we split the two issues in seperate patches. It's better for tracking.

Thanks
Tomas

RE: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

[Senthilkumar Balasubramanian]

________________________________________
From: Tomas Winkler [tomasw@gmail.com]
Sent: Wednesday, May 28, 2008 10:37 PM
To: Senthilkumar Balasubramanian
Cc: John W. Linville; johannes@sipsolutions.net; ron.rindjunsky@intel.com; linux-wireless@vger.kernel.org; Luis Rodriguez
Subject: Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

On Wed, May 28, 2008 at 7:50 PM, Senthilkumar Balasubramanian
<Senthilkumar.Balasubramanian@atheros.com> wrote:
> ________________________________________
> From: John W. Linville [linville@tuxdriver.com]
> Sent: Wednesday, May 28, 2008 8:46 PM
> To: Senthilkumar Balasubramanian
> Cc: johannes@sipsolutions.net; ron.rindjunsky@intel.com; linux-wireless@vger.kernel.org; Luis Rodriguez
> Subject: Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()
>
> On Wed, May 28, 2008 at 08:20:48PM +0530, Senthil Balasubramanian wrote:
>> This addresses an alignment issue with compare_ether_addr().
>> The addresses passed to compare_ether_addr should be two bytes aligned.
>> It may function properly in x86 platform. However may not work properly
>> on IA-64 or ARM processor.
>
>> --- a/net/mac80211/rx.c
>> +++ b/net/mac80211/rx.c
>> @@ -1116,7 +1116,9 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>>       u16 fc, hdrlen, ethertype;
>>       u8 *payload;
>>       u8 dst[ETH_ALEN];
>> -     u8 src[ETH_ALEN];
>> +     /* Should be aligned on 2 bytes for compare_ether_addr() */
>> +     u16 src_aligned[ETH_ALEN >> 1];
>> +     u8 *src = (u8 *)src_aligned;
>>       struct sk_buff *skb = rx->skb;
>>       struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>>       DECLARE_MAC_BUF(mac);
>
> Any reason you couldn't just do this?
>
> I just thought of using the type u16 instead of attribute here. that's all. No specific reason.
>
> @@ -1116,7 +1116,7 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>        u16 fc, hdrlen, ethertype;
>        u8 *payload;
>        u8 dst[ETH_ALEN];
> -       u8 src[ETH_ALEN];
> +       u8 src[ETH_ALEN] __attribute__ ((aligned(2)));
>        struct sk_buff *skb = rx->skb;
>        struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>        DECLARE_MAC_BUF(mac);
>
> It seems to compile w/o errors on i686, and it seems more clear to me.
> Will that not work?
>
> Yes. It should also work. I will re-create the patch and send it to you. I will also include Johannes suggestion of just using __aligned macro.
>
Can we split the two issues in seperate patches. It's better for tracking.

They are not two different issues. All of them addresses the same alignment issue only.  Initially I thought of using u16 to get it aligned on 2 byte and then I started using the attribute stuff as in page_group_addr[] and others required initializations for the u8 array.

Thanks
Tomas

Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()

[Tomas Winkler]

On Wed, May 28, 2008 at 8:33 PM, Senthilkumar Balasubramanian
<Senthilkumar.Balasubramanian@atheros.com> wrote:
>
> ________________________________________
> From: Tomas Winkler [tomasw@gmail.com]
> Sent: Wednesday, May 28, 2008 10:37 PM
> To: Senthilkumar Balasubramanian
> Cc: John W. Linville; johannes@sipsolutions.net; ron.rindjunsky@intel.com; linux-wireless@vger.kernel.org; Luis Rodriguez
> Subject: Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()
>
> On Wed, May 28, 2008 at 7:50 PM, Senthilkumar Balasubramanian
> <Senthilkumar.Balasubramanian@atheros.com> wrote:
>> ________________________________________
>> From: John W. Linville [linville@tuxdriver.com]
>> Sent: Wednesday, May 28, 2008 8:46 PM
>> To: Senthilkumar Balasubramanian
>> Cc: johannes@sipsolutions.net; ron.rindjunsky@intel.com; linux-wireless@vger.kernel.org; Luis Rodriguez
>> Subject: Re: [PATCH] mac80211: fix alignment issue with compare_ether_addr()
>>
>> On Wed, May 28, 2008 at 08:20:48PM +0530, Senthil Balasubramanian wrote:
>>> This addresses an alignment issue with compare_ether_addr().
>>> The addresses passed to compare_ether_addr should be two bytes aligned.
>>> It may function properly in x86 platform. However may not work properly
>>> on IA-64 or ARM processor.
>>
>>> --- a/net/mac80211/rx.c
>>> +++ b/net/mac80211/rx.c
>>> @@ -1116,7 +1116,9 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>>>       u16 fc, hdrlen, ethertype;
>>>       u8 *payload;
>>>       u8 dst[ETH_ALEN];
>>> -     u8 src[ETH_ALEN];
>>> +     /* Should be aligned on 2 bytes for compare_ether_addr() */
>>> +     u16 src_aligned[ETH_ALEN >> 1];
>>> +     u8 *src = (u8 *)src_aligned;
>>>       struct sk_buff *skb = rx->skb;
>>>       struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>>>       DECLARE_MAC_BUF(mac);
>>
>> Any reason you couldn't just do this?
>>
>> I just thought of using the type u16 instead of attribute here. that's all. No specific reason.
>>
>> @@ -1116,7 +1116,7 @@ ieee80211_data_to_8023(struct ieee80211_rx_data *rx)
>>        u16 fc, hdrlen, ethertype;
>>        u8 *payload;
>>        u8 dst[ETH_ALEN];
>> -       u8 src[ETH_ALEN];
>> +       u8 src[ETH_ALEN] __attribute__ ((aligned(2)));
>>        struct sk_buff *skb = rx->skb;
>>        struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
>>        DECLARE_MAC_BUF(mac);
>>
>> It seems to compile w/o errors on i686, and it seems more clear to me.
>> Will that not work?
>>
>> Yes. It should also work. I will re-create the patch and send it to you. I will also include Johannes suggestion of just using __aligned macro.
>>
> Can we split the two issues in seperate patches. It's better for tracking.
>
> They are not two different issues. All of them addresses the same alignment issue only.  Initially I thought of using u16 to get it aligned on 2 byte and then I started using the attribute stuff as in page_group_addr[] and others required initializations for the u8 array.
>

I mean reordering buffer allocation and alignment are not the same issues.

Thanks
Tomas