[PATCH linux-next] net: batman-adv: use "__packed __aligned(2)" for each structure instead of "__packed(2)" region

[PATCH linux-next] net: batman-adv: use "__packed __aligned(2)" for each structure instead of "__packed(2)" region

[Chen Gang]

Unfortunately, not all compilers assumes the structures within a pack
region also need be packed (e.g. metag), so need add a pack explicitly
to satisfy all compilers.

The related error (under metag with allmodconfig):

    MODPOST 2952 modules
  ERROR: "__compiletime_assert_431" [net/batman-adv/batman-adv.ko] undefined!
  ERROR: "__compiletime_assert_432" [net/batman-adv/batman-adv.ko] undefined!
  ERROR: "__compiletime_assert_429" [net/batman-adv/batman-adv.ko] undefined!
  ERROR: "__compiletime_assert_428" [net/batman-adv/batman-adv.ko] undefined!
  ERROR: "__compiletime_assert_423" [net/batman-adv/batman-adv.ko] undefined!


Signed-off-by: Chen Gang <gang.chen.5i5j-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 net/batman-adv/packet.h | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/net/batman-adv/packet.h b/net/batman-adv/packet.h
index 0a381d1..9206b48 100644
--- a/net/batman-adv/packet.h
+++ b/net/batman-adv/packet.h
@@ -154,7 +154,6 @@ enum batadv_tvlv_type {
 	BATADV_TVLV_ROAM	= 0x05,
 };
 
-#pragma pack(2)
 /* the destination hardware field in the ARP frame is used to
  * transport the claim type and the group id
  */
@@ -162,8 +161,7 @@ struct batadv_bla_claim_dst {
 	uint8_t magic[3];	/* FF:43:05 */
 	uint8_t type;		/* bla_claimframe */
 	__be16 group;		/* group id */
-};
-#pragma pack()
+} __packed __aligned(2);
 
 /**
  * struct batadv_ogm_packet - ogm (routing protocol) packet
@@ -281,7 +279,6 @@ struct batadv_icmp_packet_rr {
  * misalignment of the payload after the ethernet header. It may also lead to
  * leakage of information when the padding it not initialized before sending.
  */
-#pragma pack(2)
 
 /**
  * struct batadv_unicast_packet - unicast packet for network payload
@@ -300,7 +297,7 @@ struct batadv_unicast_packet {
 	/* "4 bytes boundary + 2 bytes" long to make the payload after the
 	 * following ethernet header again 4 bytes boundary aligned
 	 */
-};
+}  __packed __aligned(2);
 
 /**
  * struct batadv_unicast_4addr_packet - extended unicast packet
@@ -316,7 +313,7 @@ struct batadv_unicast_4addr_packet {
 	/* "4 bytes boundary + 2 bytes" long to make the payload after the
 	 * following ethernet header again 4 bytes boundary aligned
 	 */
-};
+}  __packed __aligned(2);
 
 /**
  * struct batadv_frag_packet - fragmented packet
@@ -347,7 +344,7 @@ struct batadv_frag_packet {
 	uint8_t orig[ETH_ALEN];
 	__be16  seqno;
 	__be16  total_size;
-};
+}  __packed __aligned(2);
 
 /**
  * struct batadv_bcast_packet - broadcast packet for network payload
@@ -368,7 +365,7 @@ struct batadv_bcast_packet {
 	/* "4 bytes boundary + 2 bytes" long to make the payload after the
 	 * following ethernet header again 4 bytes boundary aligned
 	 */
-};
+}  __packed __aligned(2);
 
 /**
  * struct batadv_coded_packet - network coded packet
@@ -404,9 +401,8 @@ struct batadv_coded_packet {
 	uint8_t  second_orig_dest[ETH_ALEN];
 	__be32   second_crc;
 	__be16   coded_len;
-};
+}  __packed __aligned(2);
 
-#pragma pack()
 
 /**
  * struct batadv_unicast_tvlv - generic unicast packet with tvlv payload
-- 
1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-metag" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH linux-next] net: batman-adv: use "__packed __aligned(2)" for each structure instead of "__packed(2)" region

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 3852 bytes --]

On 18/01/14 12:31, Chen Gang wrote:
> Unfortunately, not all compilers assumes the structures within a pack
> region also need be packed (e.g. metag), so need add a pack explicitly
> to satisfy all compilers.
> 
> The related error (under metag with allmodconfig):
> 
>     MODPOST 2952 modules
>   ERROR: "__compiletime_assert_431" [net/batman-adv/batman-adv.ko] undefined!
>   ERROR: "__compiletime_assert_432" [net/batman-adv/batman-adv.ko] undefined!
>   ERROR: "__compiletime_assert_429" [net/batman-adv/batman-adv.ko] undefined!
>   ERROR: "__compiletime_assert_428" [net/batman-adv/batman-adv.ko] undefined!
>   ERROR: "__compiletime_assert_423" [net/batman-adv/batman-adv.ko] undefined!
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

David, what do you think about this change?


Can "__packed __aligned(2)" generate a different structure padding than
"#pragma pack(2)" ?

I am not really sure about the difference between the two. But if we
have the possibility that the padding may change then this patch should
go into net, otherwise we will have a protocol compatibility problem
between 3.13 and 3.14.


Cheers,

> ---
>  net/batman-adv/packet.h | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/net/batman-adv/packet.h b/net/batman-adv/packet.h
> index 0a381d1..9206b48 100644
> --- a/net/batman-adv/packet.h
> +++ b/net/batman-adv/packet.h
> @@ -154,7 +154,6 @@ enum batadv_tvlv_type {
>  	BATADV_TVLV_ROAM	= 0x05,
>  };
>  
> -#pragma pack(2)
>  /* the destination hardware field in the ARP frame is used to
>   * transport the claim type and the group id
>   */
> @@ -162,8 +161,7 @@ struct batadv_bla_claim_dst {
>  	uint8_t magic[3];	/* FF:43:05 */
>  	uint8_t type;		/* bla_claimframe */
>  	__be16 group;		/* group id */
> -};
> -#pragma pack()
> +} __packed __aligned(2);
>  
>  /**
>   * struct batadv_ogm_packet - ogm (routing protocol) packet
> @@ -281,7 +279,6 @@ struct batadv_icmp_packet_rr {
>   * misalignment of the payload after the ethernet header. It may also lead to
>   * leakage of information when the padding it not initialized before sending.
>   */
> -#pragma pack(2)
>  
>  /**
>   * struct batadv_unicast_packet - unicast packet for network payload
> @@ -300,7 +297,7 @@ struct batadv_unicast_packet {
>  	/* "4 bytes boundary + 2 bytes" long to make the payload after the
>  	 * following ethernet header again 4 bytes boundary aligned
>  	 */
> -};
> +}  __packed __aligned(2);
>  
>  /**
>   * struct batadv_unicast_4addr_packet - extended unicast packet
> @@ -316,7 +313,7 @@ struct batadv_unicast_4addr_packet {
>  	/* "4 bytes boundary + 2 bytes" long to make the payload after the
>  	 * following ethernet header again 4 bytes boundary aligned
>  	 */
> -};
> +}  __packed __aligned(2);
>  
>  /**
>   * struct batadv_frag_packet - fragmented packet
> @@ -347,7 +344,7 @@ struct batadv_frag_packet {
>  	uint8_t orig[ETH_ALEN];
>  	__be16  seqno;
>  	__be16  total_size;
> -};
> +}  __packed __aligned(2);
>  
>  /**
>   * struct batadv_bcast_packet - broadcast packet for network payload
> @@ -368,7 +365,7 @@ struct batadv_bcast_packet {
>  	/* "4 bytes boundary + 2 bytes" long to make the payload after the
>  	 * following ethernet header again 4 bytes boundary aligned
>  	 */
> -};
> +}  __packed __aligned(2);
>  
>  /**
>   * struct batadv_coded_packet - network coded packet
> @@ -404,9 +401,8 @@ struct batadv_coded_packet {
>  	uint8_t  second_orig_dest[ETH_ALEN];
>  	__be32   second_crc;
>  	__be16   coded_len;
> -};
> +}  __packed __aligned(2);
>  
> -#pragma pack()
>  
>  /**
>   * struct batadv_unicast_tvlv - generic unicast packet with tvlv payload
> 


-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH linux-next] net: batman-adv: use "__packed __aligned(2)" for each structure instead of "__packed(2)" region

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]

On Saturday 18 January 2014 14:03:26 Antonio Quartulli wrote:
> On 18/01/14 12:31, Chen Gang wrote:
> > Unfortunately, not all compilers assumes the structures within a pack
> > region also need be packed (e.g. metag), so need add a pack explicitly
> > to satisfy all compilers.
> > 
> > The related error (under metag with allmodconfig):
> >     MODPOST 2952 modules
> >   
> >   ERROR: "__compiletime_assert_431" [net/batman-adv/batman-adv.ko]
> >   undefined!
> >   ERROR: "__compiletime_assert_432" [net/batman-adv/batman-adv.ko]
> >   undefined!
> >   ERROR: "__compiletime_assert_429" [net/batman-adv/batman-adv.ko]
> >   undefined!
> >   ERROR: "__compiletime_assert_428" [net/batman-adv/batman-adv.ko]
> >   undefined!
> >   ERROR: "__compiletime_assert_423" [net/batman-adv/batman-adv.ko]
> >   undefined!
> > 
> > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> David, what do you think about this change?
> 
> 
> Can "__packed __aligned(2)" generate a different structure padding than
> "#pragma pack(2)" ?
> 
> I am not really sure about the difference between the two. But if we
> have the possibility that the padding may change then this patch should
> go into net, otherwise we will have a protocol compatibility problem
> between 3.13 and 3.14.

(Chen: sorry I didn't twig what you were referring to before about #pragma 
pack not working)

It appears that the following gcc patch adds support for #pragma pack:
http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html

I gave it a quick spin on metag gcc (which is unfortunately stuck on an old 
version) and it seems to fix my simple test case so that #pragma pack(2) 
becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__).


However, the __packed and __aligned are linux specific macros to abstract 
compiler details, whereas #pragma pack appears to be a compiler-specific WIN32 
style equivalent to GCC's __attribute__((packed)) and 
__attribute__((aligned(2))) (these are what __packed and __aligned use in 
compiler-gcc.h).

Therefore I believe using the Linux abstractions is still more correct here.

Cheers
James

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH linux-next] net: batman-adv: use "__packed __aligned(2)" for each structure instead of "__packed(2)" region

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On 19/01/14 02:10, James Hogan wrote:
> 
> It appears that the following gcc patch adds support for #pragma pack:
> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html
> 
> I gave it a quick spin on metag gcc (which is unfortunately stuck on an old 
> version) and it seems to fix my simple test case so that #pragma pack(2) 
> becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__).
> 

Then I personally think that it is better to fix metag gcc instead of
changing the kernel.

Actually there are many different spots where "#pragma pack" is used.
batman-adv is just the only one having compile time checks for structure
sizes.

> 
> However, the __packed and __aligned are linux specific macros to abstract 
> compiler details, whereas #pragma pack appears to be a compiler-specific WIN32 
> style equivalent to GCC's __attribute__((packed)) and 
> __attribute__((aligned(2))) (these are what __packed and __aligned use in 
> compiler-gcc.h).
> 
> Therefore I believe using the Linux abstractions is still more correct here.

If you really think so, I'd suggest to grep in the kernel and catch all
the other occurrences of "#pragma pack" and change them all (assuming
that using __attribute__((aligned(2))) is the way to go).

Cheers,


-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH linux-next] net: batman-adv: use "__packed __aligned(2)" for each structure instead of "__packed(2)" region

[Chen Gang]

On 01/19/2014 05:30 PM, Antonio Quartulli wrote:
> On 19/01/14 02:10, James Hogan wrote:
>>
>> It appears that the following gcc patch adds support for #pragma pack:
>> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html
>>
>> I gave it a quick spin on metag gcc (which is unfortunately stuck on an old 
>> version) and it seems to fix my simple test case so that #pragma pack(2) 
>> becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__).
>>
> 
> Then I personally think that it is better to fix metag gcc instead of
> changing the kernel.
> 
> Actually there are many different spots where "#pragma pack" is used.
> batman-adv is just the only one having compile time checks for structure
> sizes.
> 

What Antonio said sounds reasonable to me.

>>
>> However, the __packed and __aligned are linux specific macros to abstract 
>> compiler details, whereas #pragma pack appears to be a compiler-specific WIN32 
>> style equivalent to GCC's __attribute__((packed)) and 
>> __attribute__((aligned(2))) (these are what __packed and __aligned use in 
>> compiler-gcc.h).
>>
>> Therefore I believe using the Linux abstractions is still more correct here.
> 
> If you really think so, I'd suggest to grep in the kernel and catch all
> the other occurrences of "#pragma pack" and change them all (assuming
> that using __attribute__((aligned(2))) is the way to go).
> 


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH linux-next] net: batman-adv: use "__packed __aligned(2)" for each structure instead of "__packed(2)" region

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

Hi Antonio,

On 19/01/14 09:30, Antonio Quartulli wrote:
> On 19/01/14 02:10, James Hogan wrote:
>> It appears that the following gcc patch adds support for #pragma pack:
>> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01115.html
>>
>> I gave it a quick spin on metag gcc (which is unfortunately stuck on an old 
>> version) and it seems to fix my simple test case so that #pragma pack(2) 
>> becomes equivalent to __packed __aligned(2) (for sizeof and __alignof__).
>>
> 
> Then I personally think that it is better to fix metag gcc instead of
> changing the kernel.

Indeed it makes sense to patch metag gcc to be safe in the presence of
unportable code like this.

> Actually there are many different spots where "#pragma pack" is used.
> batman-adv is just the only one having compile time checks for structure
> sizes.

Well the only vaguely interesting one I can find outside of drivers is
in fs/udf, and even that seems specific to CD-ROMs and DVDs.

If you care about portability then Chen's patch looks reasonable to me.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]