[PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[Vitalii Demianets]

With some combinations of arch/compiler the sizeof operator on structure 
returns value greater than expected. In cases when the structure is used for 
mapping PDU fields it may lead to unexpected results (e.g. holes and 
alignment problems in skb data). Attribute "packed" prevents this undesired 
behavior.

Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
---
 drivers/net/bonding/bond_3ad.h |   10 +++++-----
 include/net/llc_pdu.h          |    8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
index 291dbd4..6d6327a 100644
--- a/drivers/net/bonding/bond_3ad.h
+++ b/drivers/net/bonding/bond_3ad.h
@@ -39,7 +39,7 @@
 
 typedef struct mac_addr {
 	u8 mac_addr_value[ETH_ALEN];
-} mac_addr_t;
+} __attribute__((packed)) mac_addr_t;
 
 enum {
 	BOND_AD_STABLE = 0,
@@ -134,12 +134,12 @@ typedef struct lacpdu {
 	u8 tlv_type_terminator;	     // = terminator
 	u8 terminator_length;	     // = 0
 	u8 reserved_50[50];	     // = 0
-} lacpdu_t;
+} __attribute__((packed)) lacpdu_t;
 
 typedef struct lacpdu_header {
 	struct ethhdr hdr;
 	struct lacpdu lacpdu;
-} lacpdu_header_t;
+} __attribute__((packed)) lacpdu_header_t;
 
 // Marker Protocol Data Unit(PDU) structure(43.5.3.2 in the 802.3ad standard)
 typedef struct bond_marker {
@@ -155,12 +155,12 @@ typedef struct bond_marker {
 	u8 tlv_type_terminator;	     //  = 0x00
 	u8 terminator_length;	     //  = 0x00
 	u8 reserved_90[90];	     //  = 0
-} bond_marker_t;
+} __attribute__((packed)) bond_marker_t;
 
 typedef struct bond_marker_header {
 	struct ethhdr hdr;
 	struct bond_marker marker;
-} bond_marker_header_t;
+} __attribute__((packed)) bond_marker_header_t;
 
 #pragma pack()
 
diff --git a/include/net/llc_pdu.h b/include/net/llc_pdu.h
index 75b8e29..a8eeb3f 100644
--- a/include/net/llc_pdu.h
+++ b/include/net/llc_pdu.h
@@ -199,7 +199,7 @@ struct llc_pdu_sn {
 	u8 ssap;
 	u8 ctrl_1;
 	u8 ctrl_2;
-};
+} __attribute__((packed));
 
 static inline struct llc_pdu_sn *llc_pdu_sn_hdr(struct sk_buff *skb)
 {
@@ -211,7 +211,7 @@ struct llc_pdu_un {
 	u8 dsap;
 	u8 ssap;
 	u8 ctrl_1;
-};
+} __attribute__((packed));
 
 static inline struct llc_pdu_un *llc_pdu_un_hdr(struct sk_buff *skb)
 {
@@ -359,7 +359,7 @@ struct llc_xid_info {
 	u8 fmt_id;	/* always 0x81 for LLC */
 	u8 type;	/* different if NULL/non-NULL LSAP */
 	u8 rw;		/* sender receive window */
-};
+} __attribute__((packed));
 
 /**
  *	llc_pdu_init_as_xid_cmd - sets bytes 3, 4 & 5 of LLC header as XID
@@ -415,7 +415,7 @@ struct llc_frmr_info {
 	u8  curr_ssv;		/* current send state variable val */
 	u8  curr_rsv;		/* current receive state variable */
 	u8  ind_bits;		/* indicator bits set with macro */
-};
+} __attribute__((packed));
 
 extern void llc_pdu_set_cmd_rsp(struct sk_buff *skb, u8 type);
 extern void llc_pdu_set_pf_bit(struct sk_buff *skb, u8 bit_value);
-- 
1.7.3.4

Re: [PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[Ben Hutchings]

On Thu, 2011-05-12 at 15:57 +0300, Vitalii Demianets wrote:
> With some combinations of arch/compiler the sizeof operator on structure 
> returns value greater than expected.

Yes, some architecture ABIs set a minimum alignment for structures so
they can have greater alignment than any of their members.

> In cases when the structure is used for 
> mapping PDU fields it may lead to unexpected results (e.g. holes and 
> alignment problems in skb data). Attribute "packed" prevents this undesired 
> behavior.
[...]

The '__packed' macro is preferred.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[Vitalii Demianets]

On Thursday 12 May 2011 17:33:36 Ben Hutchings wrote:
[...]
> The '__packed' macro is preferred.

Ok, it looks more compatible, i"ll resubmit the patch with this macro.

Also, I'm slightly in doubt if I should split the patch in 2 parts, one for 
bonding an one for LLC?

Re: [PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[Joe Perches]

On Thu, 2011-05-12 at 18:45 +0300, Vitalii Demianets wrote:
> On Thursday 12 May 2011 17:33:36 Ben Hutchings wrote:
> [...]
> > The '__packed' macro is preferred.
> Also, I'm slightly in doubt if I should split the patch in 2 parts, one for 
> bonding an one for LLC?

What arch needs __packed for

struct mac_addr {
	unsigned char addr[6];
};

?

Re: [PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[Eric Dumazet]

Le jeudi 12 mai 2011 à 08:52 -0700, Joe Perches a écrit :
> On Thu, 2011-05-12 at 18:45 +0300, Vitalii Demianets wrote:
> > On Thursday 12 May 2011 17:33:36 Ben Hutchings wrote:
> > [...]
> > > The '__packed' macro is preferred.
> > Also, I'm slightly in doubt if I should split the patch in 2 parts, one for 
> > bonding an one for LLC?
> 
> What arch needs __packed for
> 
> struct mac_addr {
> 	unsigned char addr[6];
> };
> 
> ?

Believe it or not, they do exist.  (ARM ...)

See "struct ethhdr" definition 

Check commit 7cd636fe9ce5de0051c112 for another argument.

Re: [PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[Vitalii Demianets]

On Thursday 12 May 2011 18:52:34 you wrote:
[...]
> What arch needs __packed for
>
> struct mac_addr {
> 	unsigned char addr[6];
> };
>
> ?

Currently I'm working with AT91SAM9260 which is an ARM CPU, and without 
__packed I get sizeof(struct mac_addr) == 8.

Re: [PATCH] bonding,llc: Fix structure sizeof incompatibility for some PDUs

[Joe Perches]

On Thu, 2011-05-12 at 19:10 +0300, Vitalii Demianets wrote:
> On Thursday 12 May 2011 18:52:34 you wrote:
> [...]
> > What arch needs __packed for
> > struct mac_addr {
> > 	unsigned char addr[6];
> > };
> > ?
> Currently I'm working with AT91SAM9260 which is an ARM CPU, and without 
> __packed I get sizeof(struct mac_addr) == 8.

Thanks.