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;
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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.
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~} <[email protected]>
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);
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
> 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
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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~} <[email protected]>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
John W. Linville
[email protected]
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~} <[email protected]>
> 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
> 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 \
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