ZD1211RW unaligned accesses...

ZD1211RW unaligned accesses...

[David Miller]

The problem is drivers/net/wireless/zd1211/zd_mac.c:update_qual_rssi().
Specifically the compare_ether_addr() call.  Now, ieee80211_hdr_3addr
is marked with attribute((unaligned)) but compare_ether_addr() does
not know that and does "u16 *" dereferences in the optimized
comparison.

Shaddy I attach a hack patch that you can use which should get
rid of the warnings.

Wireless folks, I would suggest we do some auditing of the
compare_ether_addr() calls and for the ones that are operating
on these potentially unaligned structs we change it to either
a straight memcmp() or some new routine which will more reflect
the issue (say something like "compare_ether_addr_unaligned()"
or "ieee80211_compare_ether_addr()").

diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
index a903645..4999869 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -1047,8 +1047,13 @@ static void update_qual_rssi(struct zd_mac *mac,
 	hdr = (struct ieee80211_hdr_3addr *)buffer;
 	if (length < offsetof(struct ieee80211_hdr_3addr, addr3))
 		return;
+#if 1
+	if (memcmp(hdr->addr2, zd_mac_to_ieee80211(mac)->bssid, ETH_ALEN))
+		return;
+#else
 	if (compare_ether_addr(hdr->addr2, zd_mac_to_ieee80211(mac)->bssid) != 0)
 		return;
+#endif
 
 	spin_lock_irqsave(&mac->lock, flags);
 	i = mac->stats_count % ZD_MAC_STATS_BUFFER_SIZE;

Re: ZD1211RW unaligned accesses...

[Shaddy Baddah]

Hi David,

David Miller wrote:
> Shaddy I attach a hack patch that you can use which should get
> rid of the warnings.

It hasn't seemed to. I patched the source (confirming the patched lines 
are in), compiled, installed and rebooted to effect the changes. My 
zd1211rw modules timestamp indicates that I have an updated module:

$ ls -l /lib/modules/2.6.22/kernel/drivers/net/wireless/zd1211rw/zd1211rw.ko
-rw-r--r-- 1 root root 84536 2007-11-21 23:18 
/lib/modules/2.6.22/kernel/drivers/net/wireless/zd1211rw/zd1211rw.ko

lsmod confirms the module is loaded. After activating the interface 
(without configuring it yet):

$ ifconfig eth2 up

I start getting the messages over and over on the console:

Kernel unaligned access at TPC[100ee624] do_rx+0x394/0x5ec [zd1211rw]
Kernel unaligned access at TPC[100ee62c] do_rx+0x39c/0x5ec [zd1211rw]
Kernel unaligned access at TPC[100ee638] do_rx+0x3a8/0x5ec [zd1211rw]

Sorry that this has not been successful this time, but thanks for your 
help. I will be trying to follow-up on some of the other questions put 
to me.

Regards,
Shaddy

Re: ZD1211RW unaligned accesses...

[Herbert Xu]

On Wed, Nov 21, 2007 at 01:00:44PM +0000, Shaddy Baddah wrote:
>
> It hasn't seemed to. I patched the source (confirming the patched lines 
> are in), compiled, installed and rebooted to effect the changes. My 
> zd1211rw modules timestamp indicates that I have an updated module:

Thanks for your quick response and sorry for my late answer :)

I think Dave's patch is definietly on the right track but there
are subsequent unaligned accesses of a similar kind which is
why it still appears to be broken if you look at the kernel
messages.

But there is definitely progress because those addresses are now
bigger (0x394/0x39c/0x3a8 vs. 0x2** earlier).

So please try the following patch (instead of the original one)
which should fix all the unailgned accesses in do_rx.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
index a903645..d06b05b 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -1166,15 +1166,16 @@ static void do_rx(unsigned long mac_ptr)
 int zd_mac_rx_irq(struct zd_mac *mac, const u8 *buffer, unsigned int length)
 {
 	struct sk_buff *skb;
+	unsigned int hlen = ALIGN(sizeof(struct zd_rt_hdr), 16);
 
-	skb = dev_alloc_skb(sizeof(struct zd_rt_hdr) + length);
+	skb = dev_alloc_skb(hlen + length);
 	if (!skb) {
 		struct ieee80211_device *ieee = zd_mac_to_ieee80211(mac);
 		dev_warn(zd_mac_dev(mac), "Could not allocate skb.\n");
 		ieee->stats.rx_dropped++;
 		return -ENOMEM;
 	}
-	skb_reserve(skb, sizeof(struct zd_rt_hdr));
+	skb_reserve(skb, hlen - ZD_PLCP_HEADER_SIZE);
 	memcpy(__skb_put(skb, length), buffer, length);
 	skb_queue_tail(&mac->rx_queue, skb);
 	tasklet_schedule(&mac->rx_tasklet);

Re: ZD1211RW unaligned accesses...

[Ulrich Kunitz]

Herbert Xu wrote:

> So please try the following patch (instead of the original one)
> which should fix all the unailgned accesses in do_rx.
> 
> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
> index a903645..d06b05b 100644
> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
> @@ -1166,15 +1166,16 @@ static void do_rx(unsigned long mac_ptr)
>  int zd_mac_rx_irq(struct zd_mac *mac, const u8 *buffer, unsigned int length)
>  {
>  	struct sk_buff *skb;
> +	unsigned int hlen = ALIGN(sizeof(struct zd_rt_hdr), 16);
>  
> -	skb = dev_alloc_skb(sizeof(struct zd_rt_hdr) + length);
> +	skb = dev_alloc_skb(hlen + length);
>  	if (!skb) {
>  		struct ieee80211_device *ieee = zd_mac_to_ieee80211(mac);
>  		dev_warn(zd_mac_dev(mac), "Could not allocate skb.\n");
>  		ieee->stats.rx_dropped++;
>  		return -ENOMEM;
>  	}
> -	skb_reserve(skb, sizeof(struct zd_rt_hdr));
> +	skb_reserve(skb, hlen - ZD_PLCP_HEADER_SIZE);
>  	memcpy(__skb_put(skb, length), buffer, length);
>  	skb_queue_tail(&mac->rx_queue, skb);
>  	tasklet_schedule(&mac->rx_tasklet);

ACK. This patch should solve it and is better than my patch.

-- 
Uli Kunitz

Re: ZD1211RW unaligned accesses...

[John W. Linville]

So, did the patch below fix the problem?  Should I apply it?

John

On Sat, Nov 24, 2007 at 11:02:16PM +0800, Herbert Xu wrote:
> On Wed, Nov 21, 2007 at 01:00:44PM +0000, Shaddy Baddah wrote:
> >
> > It hasn't seemed to. I patched the source (confirming the patched lines 
> > are in), compiled, installed and rebooted to effect the changes. My 
> > zd1211rw modules timestamp indicates that I have an updated module:
> 
> Thanks for your quick response and sorry for my late answer :)
> 
> I think Dave's patch is definietly on the right track but there
> are subsequent unaligned accesses of a similar kind which is
> why it still appears to be broken if you look at the kernel
> messages.
> 
> But there is definitely progress because those addresses are now
> bigger (0x394/0x39c/0x3a8 vs. 0x2** earlier).
> 
> So please try the following patch (instead of the original one)
> which should fix all the unailgned accesses in do_rx.
> 
> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
> index a903645..d06b05b 100644
> --- a/drivers/net/wireless/zd1211rw/zd_mac.c
> +++ b/drivers/net/wireless/zd1211rw/zd_mac.c
> @@ -1166,15 +1166,16 @@ static void do_rx(unsigned long mac_ptr)
>  int zd_mac_rx_irq(struct zd_mac *mac, const u8 *buffer, unsigned int length)
>  {
>  	struct sk_buff *skb;
> +	unsigned int hlen = ALIGN(sizeof(struct zd_rt_hdr), 16);
>  
> -	skb = dev_alloc_skb(sizeof(struct zd_rt_hdr) + length);
> +	skb = dev_alloc_skb(hlen + length);
>  	if (!skb) {
>  		struct ieee80211_device *ieee = zd_mac_to_ieee80211(mac);
>  		dev_warn(zd_mac_dev(mac), "Could not allocate skb.\n");
>  		ieee->stats.rx_dropped++;
>  		return -ENOMEM;
>  	}
> -	skb_reserve(skb, sizeof(struct zd_rt_hdr));
> +	skb_reserve(skb, hlen - ZD_PLCP_HEADER_SIZE);
>  	memcpy(__skb_put(skb, length), buffer, length);
>  	skb_queue_tail(&mac->rx_queue, skb);
>  	tasklet_schedule(&mac->rx_tasklet);
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
John W. Linville
linville@tuxdriver.com

Re: ZD1211RW unaligned accesses...

[Herbert Xu]

On Thu, Nov 29, 2007 at 04:45:33PM -0500, John W. Linville wrote:
> So, did the patch below fix the problem?  Should I apply it?

I'm keen to find out the result too :)

Chances are it does make progress however we may still have the
general wireless/IP stack alignment issue that we are still discussing.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: ZD1211RW unaligned accesses...

[Shaddy Baddah]

Hi again,

Herbert Xu wrote:
> On Thu, Nov 29, 2007 at 04:45:33PM -0500, John W. Linville wrote:
>> So, did the patch below fix the problem?  Should I apply it?
> 
> I'm keen to find out the result too :)
> 
> Chances are it does make progress however we may still have the
> general wireless/IP stack alignment issue that we are still discussing.


OK... so I've applied patches left right and centre. As there have been 
a few, I'll in-line them all at the bottom of this email.

The result is that there are no more unaligned access messages at all. 
However, I still can only scan one (occasionally two) AP, using iwlist 
eth2 scanning command before a bus error. Jean, I missed your emails 
regarding compiling the wireless-tools, I will try these and see if they 
help.

Perhaps related to the scanning problems, I cannot setup any wireless 
links, with Open access points, WEP access points, anything at all. I am 
losing direction on what information to supply here-in, but am willing 
to take suggestions.

Thanks for all your help,
Shaddy

Patches applied follow:

diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c 
b/drivers/net/wireless/zd1211rw/zd_mac.c
index a903645..d06b05b 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -1166,15 +1166,16 @@ static void do_rx(unsigned long mac_ptr)
  int zd_mac_rx_irq(struct zd_mac *mac, const u8 *buffer, unsigned int 
length)
  {
  	struct sk_buff *skb;
+	unsigned int hlen = ALIGN(sizeof(struct zd_rt_hdr), 16);

-	skb = dev_alloc_skb(sizeof(struct zd_rt_hdr) + length);
+	skb = dev_alloc_skb(hlen + length);
  	if (!skb) {
  		struct ieee80211_device *ieee = zd_mac_to_ieee80211(mac);
  		dev_warn(zd_mac_dev(mac), "Could not allocate skb.\n");
  		ieee->stats.rx_dropped++;
  		return -ENOMEM;
  	}
-	skb_reserve(skb, sizeof(struct zd_rt_hdr));
+	skb_reserve(skb, hlen - ZD_PLCP_HEADER_SIZE);
  	memcpy(__skb_put(skb, length), buffer, length);
  	skb_queue_tail(&mac->rx_queue, skb);
  	tasklet_schedule(&mac->rx_tasklet);

diff --git a/net/ieee80211/ieee80211_tx.c b/net/ieee80211/ieee80211_tx.c
index a4c3c51..6d06f13 100644
--- a/net/ieee80211/ieee80211_tx.c
+++ b/net/ieee80211/ieee80211_tx.c
@@ -144,7 +144,8 @@ static int ieee80211_copy_snap(u8 * data, u16 h_proto)
  	snap->oui[1] = oui[1];
  	snap->oui[2] = oui[2];

-	*(u16 *) (data + SNAP_SIZE) = htons(h_proto);
+	h_proto = htons(h_proto);
+	memcpy(data + SNAP_SIZE, &h_proto, sizeof(u16));

  	return SNAP_SIZE + sizeof(u16);
  }

Index: linux-2.6.24-rc3-git1/drivers/net/wireless/zd1211rw/zd_mac.c
===================================================================
--- linux-2.6.24-rc3-git1.orig/drivers/net/wireless/zd1211rw/zd_mac.c
+++ linux-2.6.24-rc3-git1/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -974,14 +974,14 @@ static int is_data_packet_for_us(struct
  	switch (ieee->iw_mode) {
  	case IW_MODE_ADHOC:
  		if ((fc & (IEEE80211_FCTL_TODS|IEEE80211_FCTL_FROMDS)) != 0 ||
-		    compare_ether_addr(hdr->addr3, ieee->bssid) != 0)
+		    memcmp(hdr->addr3, ieee->bssid, ETH_ALEN) != 0)
  			return 0;
  		break;
  	case IW_MODE_AUTO:
  	case IW_MODE_INFRA:
  		if ((fc & (IEEE80211_FCTL_TODS|IEEE80211_FCTL_FROMDS)) !=
  		    IEEE80211_FCTL_FROMDS ||
-		    compare_ether_addr(hdr->addr2, ieee->bssid) != 0)
+		    memcmp(hdr->addr2, ieee->bssid, ETH_ALEN) != 0)
  			return 0;
  		break;
  	default:
@@ -989,9 +989,9 @@ static int is_data_packet_for_us(struct
  		return 0;
  	}

-	return compare_ether_addr(hdr->addr1, netdev->dev_addr) == 0 ||
+	return memcmp(hdr->addr1, netdev->dev_addr, ETH_ALEN) == 0 ||
  	       (is_multicast_ether_addr(hdr->addr1) &&
-		compare_ether_addr(hdr->addr3, netdev->dev_addr) != 0) ||
+		memcmp(hdr->addr3, netdev->dev_addr, ETH_ALEN) != 0) ||
  	       (netdev->flags & IFF_PROMISC);
  }

@@ -1047,7 +1047,7 @@ static void update_qual_rssi(struct zd_m
  	hdr = (struct ieee80211_hdr_3addr *)buffer;
  	if (length < offsetof(struct ieee80211_hdr_3addr, addr3))
  		return;
-	if (compare_ether_addr(hdr->addr2, zd_mac_to_ieee80211(mac)->bssid) != 0)
+	if (memcmp(hdr->addr2, zd_mac_to_ieee80211(mac)->bssid, ETH_ALEN) != 0)
  		return;

  	spin_lock_irqsave(&mac->lock, flags);


--- everything.orig/drivers/net/wireless/zd1211rw/Makefile	2007-11-23 
11:36:30.652094075 +0100
+++ everything/drivers/net/wireless/zd1211rw/Makefile	2007-11-23 
11:36:57.112090711 +0100
@@ -1,5 +1,7 @@
  obj-$(CONFIG_ZD1211RW) += zd1211rw.o

+EXTRA_CFLAGS += -fno-inline-functions-called-once
+
  zd1211rw-objs := zd_chip.o zd_ieee80211.o \
  		zd_mac.o zd_netdev.o \
  		zd_rf_al2230.o zd_rf_rf2959.o \


I believe that's it.

Re: ZD1211RW unaligned accesses...

[Herbert Xu]

On Fri, Nov 30, 2007 at 06:34:56PM +1100, Shaddy Baddah wrote:
> 
> OK... so I've applied patches left right and centre. As there have been 
> a few, I'll in-line them all at the bottom of this email.
> 
> The result is that there are no more unaligned access messages at all. 

Good stuff! I was sort of worried that you might end up getting unaligned
traps in the IP stack but it's good to see that you didn't.  Of course
it's still something that we need to fix up at some point for other
configurations.

John, you can apply my patch now.

> However, I still can only scan one (occasionally two) AP, using iwlist 
> eth2 scanning command before a bus error. Jean, I missed your emails 
> regarding compiling the wireless-tools, I will try these and see if they 
> help.

That's kind of expected because we haven't completely fixed up the
user-kernel wireless interface for the 32-in-64 case.  But rest assured
we will be working on that.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: ZD1211RW unaligned accesses...

[Johannes Berg]

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


> Chances are it does make progress however we may still have the
> general wireless/IP stack alignment issue that we are still discussing.

Yeah, it's still on my list. I'll make a patch to WARN_ON unaligned data
in a packet... although this is a bit complicated. ath5k actually had a
bug with this where they simply ignored the hardware-added padding in
QoS frames initially which would result in "corrupted" data...

johannes

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

Re: ZD1211RW unaligned accesses...

[Ulrich Kunitz]

John W. Linville wrote:

> So, did the patch below fix the problem?  Should I apply it?
> 
> John

John,

the patch would have worked, but I have sent a second one to the
list, which is based on Herbert's and has an assert to be able to test
the patch on x86.

You should be notify that the mac80211 driver, doesn't suffer
from the problem and Daniel has already provided a patch to
replace zd1211rw by the mac80211 driver. Daniel's patch must of
course break by the new patch.

-- 
Uli Kunitz

Re: ZD1211RW unaligned accesses...

[Johannes Berg]

> The problem is
> drivers/net/wireless/zd1211/zd_mac.c:update_qual_rssi().
> Specifically the compare_ether_addr() call

I don't believe this is true. Shaddy seems to back that up by the patch
not helping.

> Wireless folks, I would suggest we do some auditing of the
> compare_ether_addr() calls and for the ones that are operating
> on these potentially unaligned structs we change it to either
> a straight memcmp() or some new routine which will more reflect
> the issue (say something like "compare_ether_addr_unaligned()"
> or "ieee80211_compare_ether_addr()").

All MAC addresses in 802.11 headers are at least aligned on 16-bit
boundaries. Hence, if the some MAC address like the BSSID here was
unaligned we'd also have the IP header unaligned causing a lot more
trouble than this.

Shaddy, please rebuild zd1211 with the patch below, it should make the
compiler not inline all those static functions allowing us to pinpoint
much better where the problem occurs. You will probably need to delete
all *.o files in the zd1211rw/ directory to the them rebuilt after the
Makefile change.

--- everything.orig/drivers/net/wireless/zd1211rw/Makefile	2007-11-23 11:36:30.652094075 +0100
+++ everything/drivers/net/wireless/zd1211rw/Makefile	2007-11-23 11:36:57.112090711 +0100
@@ -1,5 +1,7 @@
 obj-$(CONFIG_ZD1211RW) += zd1211rw.o
 
+EXTRA_CFLAGS += -fno-inline-functions-called-once
+
 zd1211rw-objs := zd_chip.o zd_ieee80211.o \
 		zd_mac.o zd_netdev.o \
 		zd_rf_al2230.o zd_rf_rf2959.o \