Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote: The return from myid() isn't aligned correctly for ether_addr_copy(). Hey Dan. Actual evidence showing ether_addr_copy conversions may not always be wise. How did you find them? Is there a new alignment capability in smatch? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote: On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote: The return from myid() isn't aligned correctly for ether_addr_copy(). Hey Dan. Actual evidence showing ether_addr_copy conversions may not always be wise. How did you find them? I was just trying to see how common these kinds of bugs are. It didn't take long to find, but my impression is that they are rare and I got lucky. These kinds of bugs are tricky to find and we don't have any tools for it. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
On Wed, 8 Oct 2014, Dan Carpenter wrote: On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote: On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote: The return from myid() isn't aligned correctly for ether_addr_copy(). Hey Dan. Actual evidence showing ether_addr_copy conversions may not always be wise. How did you find them? I was just trying to see how common these kinds of bugs are. It didn't take long to find, but my impression is that they are rare and I got lucky. These kinds of bugs are tricky to find and we don't have any tools for it. Couldn't you just use your favorite matching tool, collect the file names, compile them, run pahole, and process the output in some way? It doesn't give a complete analysis (you don't find all problems), but if you find a problem it is a real problem. julia ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
On Wed, Oct 08, 2014 at 02:50:50PM +0200, Julia Lawall wrote: Couldn't you just use your favorite matching tool, collect the file names, compile them, run pahole, and process the output in some way? It doesn't give a complete analysis (you don't find all problems), but if you find a problem it is a real problem. Well... You would be better off using Smatch than pahole. And obviously I did try something like this, but it's fairly tricky. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
Dan Carpenter dan.carpen...@oracle.com writes:
The return from myid() isn't aligned correctly for ether_addr_copy().
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
Sorry, this makes no sense, just fix it properly!
drivers/staging/rtl8723au/include/rtw_eeprom.h:
struct eeprom_priv {
u8 bautoload_fail_flag;
u8 bloadfile_fail_flag;
u8 bloadmac_fail_flag;
/* u8 bempty; */
/* u8 sys_config; */
u8 mac_addr[6];/* PermanentAddress */
/* u8 config0; */
Move mac_addr[6] to the top of the struct and be done with it.
NACK
Jes
diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
index 3eb77de..c8f7890 100644
--- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
@@ -2402,7 +2402,7 @@ void issue_beacon23a(struct rtw_adapter *padapter, int
timeout_ms)
mgmt-seq_ctrl = 0;
ether_addr_copy(mgmt-da, bc_addr);
- ether_addr_copy(mgmt-sa, myid(padapter-eeprompriv));
+ memcpy(mgmt-sa, myid(padapter-eeprompriv), ETH_ALEN);
ether_addr_copy(mgmt-bssid, get_my_bssid23a(cur_network));
/* timestamp will be inserted by hardware */
@@ -2556,7 +2556,7 @@ static void issue_probersp(struct rtw_adapter
*padapter, unsigned char *da,
cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_PROBE_RESP);
ether_addr_copy(mgmt-da, da);
- ether_addr_copy(mgmt-sa, mac);
+ memcpy(mgmt-sa, mac, ETH_ALEN);
ether_addr_copy(mgmt-bssid, bssid);
mgmt-seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext-mgnt_seq));
@@ -2718,7 +2718,7 @@ static int _issue_probereq(struct rtw_adapter *padapter,
ether_addr_copy(pwlanhdr-addr3, bc_addr);
}
- ether_addr_copy(pwlanhdr-addr2, mac);
+ memcpy(pwlanhdr-addr2, mac, ETH_ALEN);
pwlanhdr-seq_ctrl =
cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext-mgnt_seq));
@@ -2863,8 +2863,8 @@ static void issue_auth(struct rtw_adapter *padapter,
struct sta_info *psta,
#ifdef CONFIG_8723AU_AP_MODE
unsigned short val16;
ether_addr_copy(mgmt-da, psta-hwaddr);
- ether_addr_copy(mgmt-sa, myid(padapter-eeprompriv));
- ether_addr_copy(mgmt-bssid, myid(padapter-eeprompriv));
+ memcpy(mgmt-sa, myid(padapter-eeprompriv), ETH_ALEN);
+ memcpy(mgmt-bssid, myid(padapter-eeprompriv), ETH_ALEN);
/* setting auth algo number */
val16 = (u16)psta-authalg;
@@ -2895,7 +2895,7 @@ static void issue_auth(struct rtw_adapter *padapter,
struct sta_info *psta,
struct ieee80211_mgmt *iv_mgmt;
ether_addr_copy(mgmt-da, get_my_bssid23a(pmlmeinfo-network));
- ether_addr_copy(mgmt-sa, myid(padapter-eeprompriv));
+ memcpy(mgmt-sa, myid(padapter-eeprompriv), ETH_ALEN);
ether_addr_copy(mgmt-bssid,
get_my_bssid23a(pmlmeinfo-network));
@@ -3006,7 +3006,7 @@ static void issue_assocrsp(struct rtw_adapter
*padapter, unsigned short status,
mgmt-frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT | pkt_type);
ether_addr_copy(mgmt-da, pstat-hwaddr);
- ether_addr_copy(mgmt-sa, myid(padapter-eeprompriv));
+ memcpy(mgmt-sa, myid(padapter-eeprompriv), ETH_ALEN);
ether_addr_copy(mgmt-bssid, get_my_bssid23a(pmlmeinfo-network));
mgmt-seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext-mgnt_seq));
@@ -3129,7 +3129,7 @@ static void issue_assocreq(struct rtw_adapter *padapter)
cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_ASSOC_REQ);
ether_addr_copy(mgmt-da, get_my_bssid23a(pmlmeinfo-network));
- ether_addr_copy(mgmt-sa, myid(padapter-eeprompriv));
+ memcpy(mgmt-sa, myid(padapter-eeprompriv), ETH_ALEN);
ether_addr_copy(mgmt-bssid, get_my_bssid23a(pmlmeinfo-network));
mgmt-seq_ctrl = cpu_to_le16(IEEE80211_SN_TO_SEQ(pmlmeext-mgnt_seq));
@@ -3400,7 +3400,7 @@ static int _issue_nulldata23a(struct rtw_adapter
*padapter, unsigned char *da,
pwlanhdr-frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
ether_addr_copy(pwlanhdr-addr1, da);
- ether_addr_copy(pwlanhdr-addr2, myid(padapter-eeprompriv));
+ memcpy(pwlanhdr-addr2, myid(padapter-eeprompriv), ETH_ALEN);
ether_addr_copy(pwlanhdr-addr3, get_my_bssid23a(pmlmeinfo-network));
pwlanhdr-seq_ctrl =
@@ -3528,7 +3528,7 @@ static int _issue_qos_nulldata23a(struct rtw_adapter
*padapter,
pwlanhdr-qos_ctrl |= cpu_to_le16(IEEE80211_QOS_CTL_EOSP);
ether_addr_copy(pwlanhdr-addr1, da);
- ether_addr_copy(pwlanhdr-addr2, myid(padapter-eeprompriv));
+ memcpy(pwlanhdr-addr2, myid(padapter-eeprompriv), ETH_ALEN);
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
On Wed, Oct 08, 2014 at 03:59:33PM +0200, Jes Sorensen wrote:
Dan Carpenter dan.carpen...@oracle.com writes:
The return from myid() isn't aligned correctly for ether_addr_copy().
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
Sorry, this makes no sense, just fix it properly!
drivers/staging/rtl8723au/include/rtw_eeprom.h:
struct eeprom_priv {
u8 bautoload_fail_flag;
u8 bloadfile_fail_flag;
u8 bloadmac_fail_flag;
/* u8 bempty; */
/* u8 sys_config; */
u8 mac_addr[6];/* PermanentAddress */
/* u8 config0; */
Move mac_addr[6] to the top of the struct and be done with it.
NACK
Oops. I thought it was something from the hardware. Actually can you
fix it and give me a reported-by tag?
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
The return from myid() isn't aligned correctly for ether_addr_copy().
Hey Dan.
Actual evidence showing ether_addr_copy conversions
may not always be wise.
How did you find them?
I was just trying to see how common these kinds of bugs are. It didn't
take long to find, but my impression is that they are rare and I got
lucky. These kinds of bugs are tricky to find and we don't have any
tools for it.
As far as I know, that's true too.
Jes, was the mac_addr field in this struct
ever __aligned(2)?
struct eeprom_priv {
u8 bautoload_fail_flag;
u8 bloadfile_fail_flag;
u8 bloadmac_fail_flag;
/* u8 bempty; */
/* u8 sys_config; */
u8 mac_addr[6];/* PermanentAddress */
...
}
As far as I can tell from git history, it was
that way at the first check-in.
Dan, did you also look for any other alignment
defects in uses of any is_foo_ether_addr calls?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
Dan Carpenter dan.carpen...@oracle.com writes:
On Wed, Oct 08, 2014 at 03:59:33PM +0200, Jes Sorensen wrote:
Dan Carpenter dan.carpen...@oracle.com writes:
The return from myid() isn't aligned correctly for ether_addr_copy().
Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
Sorry, this makes no sense, just fix it properly!
drivers/staging/rtl8723au/include/rtw_eeprom.h:
struct eeprom_priv {
u8 bautoload_fail_flag;
u8 bloadfile_fail_flag;
u8 bloadmac_fail_flag;
/* u8 bempty; */
/* u8 sys_config; */
u8 mac_addr[6];/* PermanentAddress */
/* u8 config0; */
Move mac_addr[6] to the top of the struct and be done with it.
NACK
Oops. I thought it was something from the hardware. Actually can you
fix it and give me a reported-by tag?
That stuff is just copied into memory from the eeprom, so we can pretty
much do with it as we like.
I'll put it on my list.
Jes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
Joe Perches j...@perches.com writes:
On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
The return from myid() isn't aligned correctly for ether_addr_copy().
Hey Dan.
Actual evidence showing ether_addr_copy conversions
may not always be wise.
How did you find them?
I was just trying to see how common these kinds of bugs are. It didn't
take long to find, but my impression is that they are rare and I got
lucky. These kinds of bugs are tricky to find and we don't have any
tools for it.
As far as I know, that's true too.
Jes, was the mac_addr field in this struct
ever __aligned(2)?
struct eeprom_priv {
u8 bautoload_fail_flag;
u8 bloadfile_fail_flag;
u8 bloadmac_fail_flag;
/* u8 bempty; */
/* u8 sys_config; */
u8 mac_addr[6];/* PermanentAddress */
...
}
As far as I can tell from git history, it was
that way at the first check-in.
I may have removed other entries that were unused, and that caused it to
become mis-aligned. I can't say for sure - the fix is straight forward
though.
Jes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
On Wed, 2014-10-08 at 16:33 +0200, Jes Sorensen wrote:
Joe Perches j...@perches.com writes:
On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
The return from myid() isn't aligned correctly for ether_addr_copy().
Hey Dan.
Actual evidence showing ether_addr_copy conversions
may not always be wise.
How did you find them?
I was just trying to see how common these kinds of bugs are. It didn't
take long to find, but my impression is that they are rare and I got
lucky. These kinds of bugs are tricky to find and we don't have any
tools for it.
As far as I know, that's true too.
Jes, was the mac_addr field in this struct
ever __aligned(2)?
struct eeprom_priv {
u8 bautoload_fail_flag;
u8 bloadfile_fail_flag;
u8 bloadmac_fail_flag;
/* u8 bempty; */
/* u8 sys_config; */
u8 mac_addr[6];/* PermanentAddress */
...
}
As far as I can tell from git history, it was
that way at the first check-in.
I may have removed other entries that were unused, and that caused it to
become mis-aligned. I can't say for sure - the fix is straight forward
though.
One option is to add __aligned(2) to the mac_addr field
and make no other change.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: rtl8723au: incorrect use of ether_addr_copy()
Joe Perches j...@perches.com writes:
On Wed, 2014-10-08 at 16:33 +0200, Jes Sorensen wrote:
Joe Perches j...@perches.com writes:
On Wed, 2014-10-08 at 15:46 +0300, Dan Carpenter wrote:
On Wed, Oct 08, 2014 at 05:26:11AM -0700, Joe Perches wrote:
On Wed, 2014-10-08 at 13:40 +0300, Dan Carpenter wrote:
The return from myid() isn't aligned correctly for ether_addr_copy().
Hey Dan.
Actual evidence showing ether_addr_copy conversions
may not always be wise.
How did you find them?
I was just trying to see how common these kinds of bugs are. It didn't
take long to find, but my impression is that they are rare and I got
lucky. These kinds of bugs are tricky to find and we don't have any
tools for it.
As far as I know, that's true too.
Jes, was the mac_addr field in this struct
ever __aligned(2)?
struct eeprom_priv {
u8 bautoload_fail_flag;
u8 bloadfile_fail_flag;
u8 bloadmac_fail_flag;
/* u8 bempty; */
/* u8 sys_config; */
u8 mac_addr[6];/* PermanentAddress */
...
}
As far as I can tell from git history, it was
that way at the first check-in.
I may have removed other entries that were unused, and that caused it to
become mis-aligned. I can't say for sure - the fix is straight forward
though.
One option is to add __aligned(2) to the mac_addr field
and make no other change.
As I said in another mail, just move it to the front of the struct and
be done with it. No point in wasting alignment bytes if we don't have
to.
Jes
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
