Message ID | 1358090782.4514.92.camel@deadeye.wl.decadent.org.uk |
---|---|
State | New |
Headers | show |
On Sun, 2013-01-13 at 15:26 +0000, Ben Hutchings wrote: > On Sun, 2012-12-30 at 15:01 +0000, Malcolm Priestley wrote: > > On Sun, 2012-12-30 at 00:50 +0100, Ben Hutchings wrote: > > > On Sat, 2012-12-29 at 13:10 +0000, Malcolm Priestley wrote: > > > > shorted back-ported version of upstream commit > > > > 11d404cb56ecd53bb23499897fbe7be1a9ac4827 > > > > staging: vt6656: fix headers and add cfg80211. > > > > key.c only > > > > > > > > This patch fixes the deadlock of 64 bit systems > > > > on successful association. > > > > > > > > In key.h void pointer pvKeyTable in SKeyItem is out of alignment > > > > on 64 bit kernel. > > > > > > > > The upstream arrangement of headers fixes this. > > > [...] > > > > > > Please explain how. I don't see anything weird about key.h and mac.h > > > that would cause structure definitions to be interpreted differently > > > depending on inclusion order. > > > > > > The difference is that key.h is no longer defined in key.c. > > > > It is declared in path; > > mac.h > > ----->device.h > > -------------->key.h > > > > The structure is now as seen by rxtx.c and dpc.c. > > > > Before the patch, key.c has it's own localised version > > declared in key.h and for some reason(?) is not packed the same. > [...] > > Here are the key structure definitions and the layout they should have > depending on word size and native/packed alignment: > > 32n 32p 64n 64p > typedef struct tagSKeyItem > { > BOOL bKeyValid; 0 0 0 0 > u32 uKeyLength; 4 4 4 4 > BYTE abyKey[MAX_KEY_LEN]; 8 8 8 8 > QWORD KeyRSC; 40 40 40 40 > DWORD dwTSC47_16; 48 48 48 48 > WORD wTSC15_0; 52 52 52 52 > BYTE byCipherSuite; 54 54 54 54 > BYTE byReserved0; 55 55 55 55 > DWORD dwKeyIndex; 56 56 56 56 > void *pvKeyTable; 60 60 64 60 > } SKeyItem, *PSKeyItem; //64 64 64 72 68 > > typedef struct tagSKeyTable > { > BYTE abyBSSID[ETH_ALEN]; 0 0 0 0 > BYTE byReserved0[2]; 6 6 6 6 > SKeyItem PairwiseKey; 8 8 8 8 > SKeyItem GroupKey[MAX_GROUP_KEY]; 72 72 80 76 > DWORD dwGTKeyIndex; 328 328 368 348 > BOOL bInUse; 332 332 372 352 > WORD wKeyCtl; 336 336 376 356 > BOOL bSoftWEP; 340 338 380 358 > BYTE byReserved1[6]; 344 342 384 362 > } SKeyTable, *PSKeyTable; //352 352 348 392 368 > > This agrees with your observed offsets of bSoftWEP, if SKeyItem and > SKeyTable are defined packed or not depending on the order of inclusion. > > The only thing that should cause alignment to change in this way is the > abominable '#pragma pack', and sure enough several of the headers have > '#pragma pack(1)' but no '#pragma pack()' to reset this. > > Does the following work for you? (It's based on 3.7; the context is > slightly different for earlier versions.) > > --- > Subject: vt6656: Fix inconsistent structure packing > > vt6656 has several headers that use the #pragma pack(1) directive to > enable structure packing, but never disable it. The layout of > structures defined in other headers can then depend on which order the > various headers are included in, breaking the One Definition Rule. > > This removes those directives and adds __packed to structure > definitions where packing appears to have been intended. > > Reported-by: Malcolm Priestley <tvboxspy@gmail.com> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> Thanks Ben I have tested the patch on 3.7 and 3.8-rc2 and it has resolved the issue. Regards Malcolm
On Sun, Jan 13, 2013 at 08:50:43PM +0000, Malcolm Priestley wrote: > On Sun, 2013-01-13 at 15:26 +0000, Ben Hutchings wrote: > > On Sun, 2012-12-30 at 15:01 +0000, Malcolm Priestley wrote: > > > On Sun, 2012-12-30 at 00:50 +0100, Ben Hutchings wrote: > > > > On Sat, 2012-12-29 at 13:10 +0000, Malcolm Priestley wrote: > > > > > shorted back-ported version of upstream commit > > > > > 11d404cb56ecd53bb23499897fbe7be1a9ac4827 > > > > > staging: vt6656: fix headers and add cfg80211. > > > > > key.c only > > > > > > > > > > This patch fixes the deadlock of 64 bit systems > > > > > on successful association. > > > > > > > > > > In key.h void pointer pvKeyTable in SKeyItem is out of alignment > > > > > on 64 bit kernel. > > > > > > > > > > The upstream arrangement of headers fixes this. > > > > [...] > > > > > > > > Please explain how. I don't see anything weird about key.h and mac.h > > > > that would cause structure definitions to be interpreted differently > > > > depending on inclusion order. > > > > > > > > > The difference is that key.h is no longer defined in key.c. > > > > > > It is declared in path; > > > mac.h > > > ----->device.h > > > -------------->key.h > > > > > > The structure is now as seen by rxtx.c and dpc.c. > > > > > > Before the patch, key.c has it's own localised version > > > declared in key.h and for some reason(?) is not packed the same. > > [...] > > > > Here are the key structure definitions and the layout they should have > > depending on word size and native/packed alignment: > > > > 32n 32p 64n 64p > > typedef struct tagSKeyItem > > { > > BOOL bKeyValid; 0 0 0 0 > > u32 uKeyLength; 4 4 4 4 > > BYTE abyKey[MAX_KEY_LEN]; 8 8 8 8 > > QWORD KeyRSC; 40 40 40 40 > > DWORD dwTSC47_16; 48 48 48 48 > > WORD wTSC15_0; 52 52 52 52 > > BYTE byCipherSuite; 54 54 54 54 > > BYTE byReserved0; 55 55 55 55 > > DWORD dwKeyIndex; 56 56 56 56 > > void *pvKeyTable; 60 60 64 60 > > } SKeyItem, *PSKeyItem; //64 64 64 72 68 > > > > typedef struct tagSKeyTable > > { > > BYTE abyBSSID[ETH_ALEN]; 0 0 0 0 > > BYTE byReserved0[2]; 6 6 6 6 > > SKeyItem PairwiseKey; 8 8 8 8 > > SKeyItem GroupKey[MAX_GROUP_KEY]; 72 72 80 76 > > DWORD dwGTKeyIndex; 328 328 368 348 > > BOOL bInUse; 332 332 372 352 > > WORD wKeyCtl; 336 336 376 356 > > BOOL bSoftWEP; 340 338 380 358 > > BYTE byReserved1[6]; 344 342 384 362 > > } SKeyTable, *PSKeyTable; //352 352 348 392 368 > > > > This agrees with your observed offsets of bSoftWEP, if SKeyItem and > > SKeyTable are defined packed or not depending on the order of inclusion. > > > > The only thing that should cause alignment to change in this way is the > > abominable '#pragma pack', and sure enough several of the headers have > > '#pragma pack(1)' but no '#pragma pack()' to reset this. > > > > Does the following work for you? (It's based on 3.7; the context is > > slightly different for earlier versions.) > > > > --- > > Subject: vt6656: Fix inconsistent structure packing > > > > vt6656 has several headers that use the #pragma pack(1) directive to > > enable structure packing, but never disable it. The layout of > > structures defined in other headers can then depend on which order the > > various headers are included in, breaking the One Definition Rule. > > > > This removes those directives and adds __packed to structure > > definitions where packing appears to have been intended. > > > > Reported-by: Malcolm Priestley <tvboxspy@gmail.com> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > Thanks Ben > > I have tested the patch on 3.7 and 3.8-rc2 and it has resolved the > issue. Great, can someone send me the patch so that I can apply it upstream? thanks, greg k-h
On Thu, Jan 17, 2013 at 01:06:52PM -0800, Greg Kroah-Hartman wrote: > On Sun, Jan 13, 2013 at 08:50:43PM +0000, Malcolm Priestley wrote: > > On Sun, 2013-01-13 at 15:26 +0000, Ben Hutchings wrote: > > > On Sun, 2012-12-30 at 15:01 +0000, Malcolm Priestley wrote: > > > > On Sun, 2012-12-30 at 00:50 +0100, Ben Hutchings wrote: > > > > > On Sat, 2012-12-29 at 13:10 +0000, Malcolm Priestley wrote: > > > > > > shorted back-ported version of upstream commit > > > > > > 11d404cb56ecd53bb23499897fbe7be1a9ac4827 > > > > > > staging: vt6656: fix headers and add cfg80211. > > > > > > key.c only > > > > > > > > > > > > This patch fixes the deadlock of 64 bit systems > > > > > > on successful association. > > > > > > > > > > > > In key.h void pointer pvKeyTable in SKeyItem is out of alignment > > > > > > on 64 bit kernel. > > > > > > > > > > > > The upstream arrangement of headers fixes this. > > > > > [...] > > > > > > > > > > Please explain how. I don't see anything weird about key.h and mac.h > > > > > that would cause structure definitions to be interpreted differently > > > > > depending on inclusion order. > > > > > > > > > > > > The difference is that key.h is no longer defined in key.c. > > > > > > > > It is declared in path; > > > > mac.h > > > > ----->device.h > > > > -------------->key.h > > > > > > > > The structure is now as seen by rxtx.c and dpc.c. > > > > > > > > Before the patch, key.c has it's own localised version > > > > declared in key.h and for some reason(?) is not packed the same. > > > [...] > > > > > > Here are the key structure definitions and the layout they should have > > > depending on word size and native/packed alignment: > > > > > > 32n 32p 64n 64p > > > typedef struct tagSKeyItem > > > { > > > BOOL bKeyValid; 0 0 0 0 > > > u32 uKeyLength; 4 4 4 4 > > > BYTE abyKey[MAX_KEY_LEN]; 8 8 8 8 > > > QWORD KeyRSC; 40 40 40 40 > > > DWORD dwTSC47_16; 48 48 48 48 > > > WORD wTSC15_0; 52 52 52 52 > > > BYTE byCipherSuite; 54 54 54 54 > > > BYTE byReserved0; 55 55 55 55 > > > DWORD dwKeyIndex; 56 56 56 56 > > > void *pvKeyTable; 60 60 64 60 > > > } SKeyItem, *PSKeyItem; //64 64 64 72 68 > > > > > > typedef struct tagSKeyTable > > > { > > > BYTE abyBSSID[ETH_ALEN]; 0 0 0 0 > > > BYTE byReserved0[2]; 6 6 6 6 > > > SKeyItem PairwiseKey; 8 8 8 8 > > > SKeyItem GroupKey[MAX_GROUP_KEY]; 72 72 80 76 > > > DWORD dwGTKeyIndex; 328 328 368 348 > > > BOOL bInUse; 332 332 372 352 > > > WORD wKeyCtl; 336 336 376 356 > > > BOOL bSoftWEP; 340 338 380 358 > > > BYTE byReserved1[6]; 344 342 384 362 > > > } SKeyTable, *PSKeyTable; //352 352 348 392 368 > > > > > > This agrees with your observed offsets of bSoftWEP, if SKeyItem and > > > SKeyTable are defined packed or not depending on the order of inclusion. > > > > > > The only thing that should cause alignment to change in this way is the > > > abominable '#pragma pack', and sure enough several of the headers have > > > '#pragma pack(1)' but no '#pragma pack()' to reset this. > > > > > > Does the following work for you? (It's based on 3.7; the context is > > > slightly different for earlier versions.) > > > > > > --- > > > Subject: vt6656: Fix inconsistent structure packing > > > > > > vt6656 has several headers that use the #pragma pack(1) directive to > > > enable structure packing, but never disable it. The layout of > > > structures defined in other headers can then depend on which order the > > > various headers are included in, breaking the One Definition Rule. > > > > > > This removes those directives and adds __packed to structure > > > definitions where packing appears to have been intended. > > > > > > Reported-by: Malcolm Priestley <tvboxspy@gmail.com> > > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > > > Thanks Ben > > > > I have tested the patch on 3.7 and 3.8-rc2 and it has resolved the > > issue. > > Great, can someone send me the patch so that I can apply it upstream? Oh nevermind, you did on the 14th, I have it in my inbox... greg k-h
--- a/drivers/staging/vt6656/bssdb.h +++ b/drivers/staging/vt6656/bssdb.h @@ -90,7 +90,6 @@ typedef struct tagSRSNCapObject { } SRSNCapObject, *PSRSNCapObject; // BSS info(AP) -#pragma pack(1) typedef struct tagKnownBSS { // BSS info BOOL bActive; --- a/drivers/staging/vt6656/int.h +++ b/drivers/staging/vt6656/int.h @@ -34,7 +34,6 @@ #include "device.h" /*--------------------- Export Definitions -------------------------*/ -#pragma pack(1) typedef struct tagSINTData { BYTE byTSR0; BYTE byPkt0; --- a/drivers/staging/vt6656/iocmd.h +++ b/drivers/staging/vt6656/iocmd.h @@ -95,13 +95,12 @@ typedef enum tagWZONETYPE { // Ioctl interface structure // Command structure // -#pragma pack(1) typedef struct tagSCmdRequest { u8 name[16]; void *data; u16 wResult; u16 wCmdCode; -} SCmdRequest, *PSCmdRequest; +} __packed SCmdRequest, *PSCmdRequest; // // Scan @@ -111,7 +110,7 @@ typedef struct tagSCmdScan { u8 ssid[SSID_MAXLEN + 2]; -} SCmdScan, *PSCmdScan; +} __packed SCmdScan, *PSCmdScan; // // BSS Join @@ -126,7 +125,7 @@ typedef struct tagSCmdBSSJoin { BOOL bPSEnable; BOOL bShareKeyAuth; -} SCmdBSSJoin, *PSCmdBSSJoin; +} __packed SCmdBSSJoin, *PSCmdBSSJoin; // // Zonetype Setting @@ -137,7 +136,7 @@ typedef struct tagSCmdZoneTypeSet { BOOL bWrite; WZONETYPE ZoneType; -} SCmdZoneTypeSet, *PSCmdZoneTypeSet; +} __packed SCmdZoneTypeSet, *PSCmdZoneTypeSet; typedef struct tagSWPAResult { char ifname[100]; @@ -145,7 +144,7 @@ typedef struct tagSWPAResult { u8 key_mgmt; u8 eap_type; BOOL authenticated; -} SWPAResult, *PSWPAResult; +} __packed SWPAResult, *PSWPAResult; typedef struct tagSCmdStartAP { @@ -157,7 +156,7 @@ typedef struct tagSCmdStartAP { BOOL bShareKeyAuth; u8 byBasicRate; -} SCmdStartAP, *PSCmdStartAP; +} __packed SCmdStartAP, *PSCmdStartAP; typedef struct tagSCmdSetWEP { @@ -167,7 +166,7 @@ typedef struct tagSCmdSetWEP { BOOL bWepKeyAvailable[WEP_NKEYS]; u32 auWepKeyLength[WEP_NKEYS]; -} SCmdSetWEP, *PSCmdSetWEP; +} __packed SCmdSetWEP, *PSCmdSetWEP; typedef struct tagSBSSIDItem { @@ -180,14 +179,14 @@ typedef struct tagSBSSIDItem { BOOL bWEPOn; u32 uRSSI; -} SBSSIDItem; +} __packed SBSSIDItem; typedef struct tagSBSSIDList { u32 uItem; SBSSIDItem sBSSIDList[0]; -} SBSSIDList, *PSBSSIDList; +} __packed SBSSIDList, *PSBSSIDList; typedef struct tagSNodeItem { @@ -208,7 +207,7 @@ typedef struct tagSNodeItem { u32 uTxAttempts; u16 wFailureRatio; -} SNodeItem; +} __packed SNodeItem; typedef struct tagSNodeList { @@ -216,7 +215,7 @@ typedef struct tagSNodeList { u32 uItem; SNodeItem sNodeList[0]; -} SNodeList, *PSNodeList; +} __packed SNodeList, *PSNodeList; typedef struct tagSCmdLinkStatus { @@ -229,7 +228,7 @@ typedef struct tagSCmdLinkStatus { u32 uChannel; u32 uLinkRate; -} SCmdLinkStatus, *PSCmdLinkStatus; +} __packed SCmdLinkStatus, *PSCmdLinkStatus; // // 802.11 counter @@ -247,7 +246,7 @@ typedef struct tagSDot11MIBCount { u32 ReceivedFragmentCount; u32 MulticastReceivedFrameCount; u32 FCSErrorCount; -} SDot11MIBCount, *PSDot11MIBCount; +} __packed SDot11MIBCount, *PSDot11MIBCount; @@ -355,13 +354,13 @@ typedef struct tagSStatMIBCount { u32 ullTxBroadcastBytes[2]; u32 ullTxMulticastBytes[2]; u32 ullTxDirectedBytes[2]; -} SStatMIBCount, *PSStatMIBCount; +} __packed SStatMIBCount, *PSStatMIBCount; typedef struct tagSCmdValue { u32 dwValue; -} SCmdValue, *PSCmdValue; +} __packed SCmdValue, *PSCmdValue; // // hostapd & viawget ioctl related @@ -431,7 +430,7 @@ struct viawget_hostapd_param { u8 ssid[32]; } scan_req; } u; -}; +} __packed; /*--------------------- Export Classes ----------------------------*/ --- a/drivers/staging/vt6656/iowpa.h +++ b/drivers/staging/vt6656/iowpa.h @@ -67,12 +67,11 @@ enum { -#pragma pack(1) typedef struct viawget_wpa_header { u8 type; u16 req_ie_len; u16 resp_ie_len; -} viawget_wpa_header; +} __packed viawget_wpa_header; struct viawget_wpa_param { u32 cmd; @@ -113,9 +112,8 @@ struct viawget_wpa_param { u8 *buf; } scan_results; } u; -}; +} __packed; -#pragma pack(1) struct viawget_scan_result { u8 bssid[6]; u8 ssid[32]; @@ -130,7 +128,7 @@ struct viawget_scan_result { int noise; int level; int maxrate; -}; +} __packed; /*--------------------- Export Classes ----------------------------*/