* [PATCH] wireless: Compare ethernet addresses by unaligned safe way @ 2009-06-16 10:54 Yauhen Kharuzhy 2009-06-16 11:14 ` Johannes Berg 0 siblings, 1 reply; 8+ messages in thread From: Yauhen Kharuzhy @ 2009-06-16 10:54 UTC (permalink / raw) To: linux-wireless; +Cc: Yauhen Kharuzhy When we try to run RTL8187 driver on AD BlackFin platform, we got messages from kernel about unaligned memory access at compare_ether_addr() calls. Replacing of compare_ether_addr() by memcmp() fixes this problem. Signed-off-by: Yauhen Kharuzhy <yauhen.kharuzhy@promwad.com> --- net/mac80211/ibss.c | 2 +- net/wireless/scan.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c index 0b30277..bcbb1ae 100644 --- a/net/mac80211/ibss.c +++ b/net/mac80211/ibss.c @@ -395,7 +395,7 @@ struct sta_info *ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata, return NULL; } - if (compare_ether_addr(bssid, sdata->u.ibss.bssid)) + if (memcmp(bssid, sdata->u.ibss.bssid, ETH_ALEN)) return NULL; #ifdef CONFIG_MAC80211_VERBOSE_DEBUG diff --git a/net/wireless/scan.c b/net/wireless/scan.c index e95b638..6ad9b59 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -134,7 +134,7 @@ static bool is_bss(struct cfg80211_bss *a, { const u8 *ssidie; - if (bssid && compare_ether_addr(a->bssid, bssid)) + if (bssid && memcmp(a->bssid, bssid, ETH_ALEN)) return false; if (!ssid) -- 1.6.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] wireless: Compare ethernet addresses by unaligned safe way 2009-06-16 10:54 [PATCH] wireless: Compare ethernet addresses by unaligned safe way Yauhen Kharuzhy @ 2009-06-16 11:14 ` Johannes Berg 2009-06-28 12:18 ` Ivan Kuten 0 siblings, 1 reply; 8+ messages in thread From: Johannes Berg @ 2009-06-16 11:14 UTC (permalink / raw) To: Yauhen Kharuzhy; +Cc: linux-wireless [-- Attachment #1: Type: text/plain, Size: 1353 bytes --] On Tue, 2009-06-16 at 13:54 +0300, Yauhen Kharuzhy wrote: > When we try to run RTL8187 driver on AD BlackFin platform, we got > messages from kernel about unaligned memory access at > compare_ether_addr() calls. > > Replacing of compare_ether_addr() by memcmp() fixes this problem. This shouldn't be necessary. Which operand is unaligned? > --- a/net/mac80211/ibss.c > +++ b/net/mac80211/ibss.c > @@ -395,7 +395,7 @@ struct sta_info *ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata, > return NULL; > } > > - if (compare_ether_addr(bssid, sdata->u.ibss.bssid)) > + if (memcmp(bssid, sdata->u.ibss.bssid, ETH_ALEN)) > return NULL; So in this case it seems that it is possible that u.ibss.bssid is not aligned, consider fixing by doing --- ieee80211_i.h +++ ieee80211_i.h - u8 bssid[ETH_ALEN]; + u8 bssid[ETH_ALEN] __align(2); or so instead. > --- a/net/wireless/scan.c > +++ b/net/wireless/scan.c > @@ -134,7 +134,7 @@ static bool is_bss(struct cfg80211_bss *a, > { > const u8 *ssidie; > > - if (bssid && compare_ether_addr(a->bssid, bssid)) > + if (bssid && memcmp(a->bssid, bssid, ETH_ALEN)) Since a->bssid is after a pointer I can't see how it would be unaligned, and bssid should be unaligned only if the call trace shows it's coming from the above u.ibss.bssid. johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wireless: Compare ethernet addresses by unaligned safe way 2009-06-16 11:14 ` Johannes Berg @ 2009-06-28 12:18 ` Ivan Kuten 2009-06-29 7:58 ` Johannes Berg 2009-06-29 8:23 ` Johannes Berg 0 siblings, 2 replies; 8+ messages in thread From: Ivan Kuten @ 2009-06-28 12:18 UTC (permalink / raw) To: linux-wireless; +Cc: Johannes Berg, Yauhen Kharuzhy Hello, In net/wireless/scan.c : cfg80211_wext_siwscan there seems also unaligned allocations for creq->ssids and creq->channels. Should it be something like that? Modified: trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c ============================================================================== --- trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c (original) +++ trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c Fri Jun 26 14:00:52 2009 @@ -619,7 +619,7 @@ if (wiphy->bands[band]) n_channels += wiphy->bands[band]->n_channels; - creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) + + creq = kzalloc(roundup(sizeof(*creq), 4) + roundup(sizeof(struct cfg80211_ssid), 4) + n_channels * sizeof(void *), GFP_ATOMIC); if (!creq) { @@ -629,8 +629,8 @@ creq->wiphy = wiphy; creq->ifidx = dev->ifindex; - creq->ssids = (void *)(creq + 1); - creq->channels = (void *)(creq->ssids + 1); + creq->ssids = (void *)creq + roundup(sizeof(*creq), 4); + creq->channels = (void *)creq->ssids + roundup(sizeof(*creq->ssids), 4); creq->n_channels = n_channels; creq->n_ssids = 1; Regards, Ivan > On Tue, 2009-06-16 at 13:54 +0300, Yauhen Kharuzhy wrote: >> When we try to run RTL8187 driver on AD BlackFin platform, we got >> messages from kernel about unaligned memory access at >> compare_ether_addr() calls. >> >> Replacing of compare_ether_addr() by memcmp() fixes this problem. > > This shouldn't be necessary. Which operand is unaligned? > >> --- a/net/mac80211/ibss.c >> +++ b/net/mac80211/ibss.c >> @@ -395,7 +395,7 @@ struct sta_info *ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata, >> return NULL; >> } >> >> - if (compare_ether_addr(bssid, sdata->u.ibss.bssid)) >> + if (memcmp(bssid, sdata->u.ibss.bssid, ETH_ALEN)) >> return NULL; > > So in this case it seems that it is possible that u.ibss.bssid is not > aligned, consider fixing by doing > > --- ieee80211_i.h > +++ ieee80211_i.h > - u8 bssid[ETH_ALEN]; > + u8 bssid[ETH_ALEN] __align(2); > > or so instead. > >> --- a/net/wireless/scan.c >> +++ b/net/wireless/scan.c >> @@ -134,7 +134,7 @@ static bool is_bss(struct cfg80211_bss *a, >> { >> const u8 *ssidie; >> >> - if (bssid && compare_ether_addr(a->bssid, bssid)) >> + if (bssid && memcmp(a->bssid, bssid, ETH_ALEN)) > > Since a->bssid is after a pointer I can't see how it would be unaligned, > and bssid should be unaligned only if the call trace shows it's coming > from the above u.ibss.bssid. > > johannes ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wireless: Compare ethernet addresses by unaligned safe way 2009-06-28 12:18 ` Ivan Kuten @ 2009-06-29 7:58 ` Johannes Berg 2009-07-23 17:56 ` Ivan Kuten 2009-06-29 8:23 ` Johannes Berg 1 sibling, 1 reply; 8+ messages in thread From: Johannes Berg @ 2009-06-29 7:58 UTC (permalink / raw) To: ivan.kuten; +Cc: linux-wireless, Yauhen Kharuzhy [-- Attachment #1: Type: text/plain, Size: 3071 bytes --] On Sun, 2009-06-28 at 15:18 +0300, Ivan Kuten wrote: > Hello, > > In net/wireless/scan.c : cfg80211_wext_siwscan there seems also unaligned allocations > for creq->ssids and creq->channels. Should it be something like that? Seems alright, but there is more than one instance of this, maybe you can make a function to allocate a scan request properly and have it be called from all the places it's needed. johannes > Modified: trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c > ============================================================================== > --- trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c (original) > +++ trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c Fri Jun 26 14:00:52 2009 > @@ -619,7 +619,7 @@ > if (wiphy->bands[band]) > n_channels += wiphy->bands[band]->n_channels; > > - creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) + > + creq = kzalloc(roundup(sizeof(*creq), 4) + roundup(sizeof(struct cfg80211_ssid), 4) + > n_channels * sizeof(void *), > GFP_ATOMIC); > if (!creq) { > @@ -629,8 +629,8 @@ > > creq->wiphy = wiphy; > creq->ifidx = dev->ifindex; > - creq->ssids = (void *)(creq + 1); > - creq->channels = (void *)(creq->ssids + 1); > + creq->ssids = (void *)creq + roundup(sizeof(*creq), 4); > + creq->channels = (void *)creq->ssids + roundup(sizeof(*creq->ssids), 4); > creq->n_channels = n_channels; > creq->n_ssids = 1; > > Regards, > Ivan > > > On Tue, 2009-06-16 at 13:54 +0300, Yauhen Kharuzhy wrote: > >> When we try to run RTL8187 driver on AD BlackFin platform, we got > >> messages from kernel about unaligned memory access at > >> compare_ether_addr() calls. > >> > >> Replacing of compare_ether_addr() by memcmp() fixes this problem. > > > > This shouldn't be necessary. Which operand is unaligned? > > > >> --- a/net/mac80211/ibss.c > >> +++ b/net/mac80211/ibss.c > >> @@ -395,7 +395,7 @@ struct sta_info *ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata, > >> return NULL; > >> } > >> > >> - if (compare_ether_addr(bssid, sdata->u.ibss.bssid)) > >> + if (memcmp(bssid, sdata->u.ibss.bssid, ETH_ALEN)) > >> return NULL; > > > > So in this case it seems that it is possible that u.ibss.bssid is not > > aligned, consider fixing by doing > > > > --- ieee80211_i.h > > +++ ieee80211_i.h > > - u8 bssid[ETH_ALEN]; > > + u8 bssid[ETH_ALEN] __align(2); > > > > or so instead. > > > >> --- a/net/wireless/scan.c > >> +++ b/net/wireless/scan.c > >> @@ -134,7 +134,7 @@ static bool is_bss(struct cfg80211_bss *a, > >> { > >> const u8 *ssidie; > >> > >> - if (bssid && compare_ether_addr(a->bssid, bssid)) > >> + if (bssid && memcmp(a->bssid, bssid, ETH_ALEN)) > > > > Since a->bssid is after a pointer I can't see how it would be unaligned, > > and bssid should be unaligned only if the call trace shows it's coming > > from the above u.ibss.bssid. > > > > johannes > > [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wireless: Compare ethernet addresses by unaligned safe way 2009-06-29 7:58 ` Johannes Berg @ 2009-07-23 17:56 ` Ivan Kuten 2009-07-23 17:59 ` Johannes Berg 0 siblings, 1 reply; 8+ messages in thread From: Ivan Kuten @ 2009-07-23 17:56 UTC (permalink / raw) To: linux-wireless; +Cc: Johannes Berg, Yauhen Kharuzhy > On Sun, 2009-06-28 at 15:18 +0300, Ivan Kuten wrote: >> Hello, >> >> In net/wireless/scan.c : cfg80211_wext_siwscan there seems also unaligned allocations >> for creq->ssids and creq->channels. Should it be something like that? > > Seems alright, but there is more than one instance of this, maybe you > can make a function to allocate a scan request properly and have it be > called from all the places it's needed. > > johannes > Hello Johannes, Can you point to that multiple scan allocations? I see only one kzalloc with followed possible alignment violation - it's in cfg80211_wext_siwscan in scan.c Regards, Ivan >> Modified: trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c >> ============================================================================== >> --- trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c (original) >> +++ trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c Fri Jun 26 14:00:52 2009 >> @@ -619,7 +619,7 @@ >> if (wiphy->bands[band]) >> n_channels += wiphy->bands[band]->n_channels; >> >> - creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) + >> + creq = kzalloc(roundup(sizeof(*creq), 4) + roundup(sizeof(struct cfg80211_ssid), 4) + >> n_channels * sizeof(void *), >> GFP_ATOMIC); >> if (!creq) { >> @@ -629,8 +629,8 @@ >> >> creq->wiphy = wiphy; >> creq->ifidx = dev->ifindex; >> - creq->ssids = (void *)(creq + 1); >> - creq->channels = (void *)(creq->ssids + 1); >> + creq->ssids = (void *)creq + roundup(sizeof(*creq), 4); >> + creq->channels = (void *)creq->ssids + roundup(sizeof(*creq->ssids), 4); >> creq->n_channels = n_channels; >> creq->n_ssids = 1; >> >> Regards, >> Ivan >> >>> On Tue, 2009-06-16 at 13:54 +0300, Yauhen Kharuzhy wrote: >>>> When we try to run RTL8187 driver on AD BlackFin platform, we got >>>> messages from kernel about unaligned memory access at >>>> compare_ether_addr() calls. >>>> >>>> Replacing of compare_ether_addr() by memcmp() fixes this problem. >>> This shouldn't be necessary. Which operand is unaligned? >>> >>>> --- a/net/mac80211/ibss.c >>>> +++ b/net/mac80211/ibss.c >>>> @@ -395,7 +395,7 @@ struct sta_info *ieee80211_ibss_add_sta(struct ieee80211_sub_if_data *sdata, >>>> return NULL; >>>> } >>>> >>>> - if (compare_ether_addr(bssid, sdata->u.ibss.bssid)) >>>> + if (memcmp(bssid, sdata->u.ibss.bssid, ETH_ALEN)) >>>> return NULL; >>> So in this case it seems that it is possible that u.ibss.bssid is not >>> aligned, consider fixing by doing >>> >>> --- ieee80211_i.h >>> +++ ieee80211_i.h >>> - u8 bssid[ETH_ALEN]; >>> + u8 bssid[ETH_ALEN] __align(2); >>> >>> or so instead. >>> >>>> --- a/net/wireless/scan.c >>>> +++ b/net/wireless/scan.c >>>> @@ -134,7 +134,7 @@ static bool is_bss(struct cfg80211_bss *a, >>>> { >>>> const u8 *ssidie; >>>> >>>> - if (bssid && compare_ether_addr(a->bssid, bssid)) >>>> + if (bssid && memcmp(a->bssid, bssid, ETH_ALEN)) >>> Since a->bssid is after a pointer I can't see how it would be unaligned, >>> and bssid should be unaligned only if the call trace shows it's coming >>> from the above u.ibss.bssid. >>> >>> johannes >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wireless: Compare ethernet addresses by unaligned safe way 2009-07-23 17:56 ` Ivan Kuten @ 2009-07-23 17:59 ` Johannes Berg 2009-08-07 15:34 ` John W. Linville 0 siblings, 1 reply; 8+ messages in thread From: Johannes Berg @ 2009-07-23 17:59 UTC (permalink / raw) To: ivan.kuten; +Cc: linux-wireless, Yauhen Kharuzhy [-- Attachment #1: Type: text/plain, Size: 766 bytes --] On Thu, 2009-07-23 at 20:56 +0300, Ivan Kuten wrote: > > On Sun, 2009-06-28 at 15:18 +0300, Ivan Kuten wrote: > >> Hello, > >> > >> In net/wireless/scan.c : cfg80211_wext_siwscan there seems also unaligned allocations > >> for creq->ssids and creq->channels. Should it be something like that? > > > > Seems alright, but there is more than one instance of this, maybe you > > can make a function to allocate a scan request properly and have it be > > called from all the places it's needed. > > > > johannes > > > > Hello Johannes, > Can you point to that multiple scan allocations? I see only one kzalloc > with followed possible alignment violation - it's in cfg80211_wext_siwscan in scan.c I'm sure there's one in nl80211.c too johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wireless: Compare ethernet addresses by unaligned safe way 2009-07-23 17:59 ` Johannes Berg @ 2009-08-07 15:34 ` John W. Linville 0 siblings, 0 replies; 8+ messages in thread From: John W. Linville @ 2009-08-07 15:34 UTC (permalink / raw) To: Johannes Berg; +Cc: ivan.kuten, linux-wireless, Yauhen Kharuzhy On Thu, Jul 23, 2009 at 07:59:31PM +0200, Johannes Berg wrote: > On Thu, 2009-07-23 at 20:56 +0300, Ivan Kuten wrote: > > > On Sun, 2009-06-28 at 15:18 +0300, Ivan Kuten wrote: > > >> Hello, > > >> > > >> In net/wireless/scan.c : cfg80211_wext_siwscan there seems also unaligned allocations > > >> for creq->ssids and creq->channels. Should it be something like that? > > > > > > Seems alright, but there is more than one instance of this, maybe you > > > can make a function to allocate a scan request properly and have it be > > > called from all the places it's needed. > > > > > > johannes > > > > > > > Hello Johannes, > > Can you point to that multiple scan allocations? I see only one kzalloc > > with followed possible alignment violation - it's in cfg80211_wext_siwscan in scan.c > > I'm sure there's one in nl80211.c too Ivan/Yauhen, Will you be posted an updated patch? Time is short before the next merge window... John -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] wireless: Compare ethernet addresses by unaligned safe way 2009-06-28 12:18 ` Ivan Kuten 2009-06-29 7:58 ` Johannes Berg @ 2009-06-29 8:23 ` Johannes Berg 1 sibling, 0 replies; 8+ messages in thread From: Johannes Berg @ 2009-06-29 8:23 UTC (permalink / raw) To: ivan.kuten; +Cc: linux-wireless, Yauhen Kharuzhy [-- Attachment #1: Type: text/plain, Size: 1013 bytes --] On Sun, 2009-06-28 at 15:18 +0300, Ivan Kuten wrote: > Hello, > > In net/wireless/scan.c : cfg80211_wext_siwscan there seems also unaligned allocations > for creq->ssids and creq->channels. Should it be something like that? > > Modified: trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c > ============================================================================== > --- trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c (original) > +++ trunk/uClinux-dist-2008R1-RC8/compat-wireless-2009-06-11/net/wireless/scan.c Fri Jun 26 14:00:52 2009 > @@ -619,7 +619,7 @@ > if (wiphy->bands[band]) > n_channels += wiphy->bands[band]->n_channels; > > - creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) + > + creq = kzalloc(roundup(sizeof(*creq), 4) + roundup(sizeof(struct cfg80211_ssid), 4) + > n_channels * sizeof(void *), > GFP_ATOMIC); Also, that should be sizeof(void *) instead of "4". johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-08-07 15:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-06-16 10:54 [PATCH] wireless: Compare ethernet addresses by unaligned safe way Yauhen Kharuzhy 2009-06-16 11:14 ` Johannes Berg 2009-06-28 12:18 ` Ivan Kuten 2009-06-29 7:58 ` Johannes Berg 2009-07-23 17:56 ` Ivan Kuten 2009-07-23 17:59 ` Johannes Berg 2009-08-07 15:34 ` John W. Linville 2009-06-29 8:23 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).