2008-11-28 06:10:55

by Shaddy Baddah

[permalink] [raw]
Subject: [PATCHv2] zd1211rw: use unaligned safe memcmp() in-place of compare_ether_addr()

Forgot the sign-off. Take two:

Under my 2.6.28-rc6 sparc64, when associating to an AP through my
zd1211rw device, I was seeing kernel log messages like (not exact output):

Kernel unaligned access at TPC[10129b68] zd_mac_rx+0x144/0x32c [zd1211rw]

For the zd1211rw module, on RX, the 80211 packet will be located after
the PLCP header in the skb data buffer. The PLCP header being 5 bytes
long, the 80211 header will start unaligned from an aligned skb
buffer.

As per Documentation/unaligned-memory-access.txt, we must replace the
not unaligned() safe compare_ether_addr() with memcmp() to protect
architectures that require alignment.

Signed-off-by: Shaddy Baddah <[email protected]>

diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c
b/drivers/net/wireless/zd1211rw/zd_mac.c
index e48d605..dd93f23 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -616,7 +616,7 @@ static int filter_ack(struct ieee80211_hw *hw,
struct ieee80211_hdr *rx_hdr,
struct ieee80211_hdr *tx_hdr;

tx_hdr = (struct ieee80211_hdr *)skb->data;
- if (likely(!compare_ether_addr(tx_hdr->addr2, rx_hdr->addr1)))
+ if (likely(!memcmp(tx_hdr->addr2, rx_hdr->addr1, ETH_ALEN)))
{
__skb_unlink(skb, q);
tx_status(hw, skb, IEEE80211_TX_STAT_ACK, stats->signal, 1);


2008-11-29 12:41:22

by Holger Schurig

[permalink] [raw]
Subject: Re: [PATCHv2] zd1211rw: use unaligned safe memcmp() in-place of compare_ether_addr()

> - if (likely(!compare_ether_addr(tx_hdr->addr2,
> rx_hdr->addr1))) + if (likely(!memcmp(tx_hdr->addr2,
> rx_hdr->addr1, ETH_ALEN))) {
> __skb_unlink(skb, q);

Wouldn't it be better to fix compile_ether_addr instead?

2008-11-29 13:00:55

by Shaddy Baddah

[permalink] [raw]
Subject: Re: [PATCHv2] zd1211rw: use unaligned safe memcmp() in-place of compare_ether_addr()

Hi,

On 29/11/08 23:40, Holger Schurig wrote:
>> - if (likely(!compare_ether_addr(tx_hdr->addr2,
>> rx_hdr->addr1))) + if (likely(!memcmp(tx_hdr->addr2,
>> rx_hdr->addr1, ETH_ALEN))) {
>> __skb_unlink(skb, q);
>
> Wouldn't it be better to fix compile_ether_addr instead?

From Documentation/unaligned-memory-access.txt in reference to
compare_ether_addr():

Despite the potential unaligned access problems with the above function, it
is included in the kernel anyway but is understood to only work on
16-bit-aligned addresses. It is up to the caller to ensure this alignment or
not use this function at all. This alignment-unsafe function is still useful
as it is a decent optimization for the cases when you can ensure alignment,
which is true almost all of the time in ethernet networking context.

Hope that helps,
Shaddy