alignment faults in 3.6

[Scott Bambrough]

On 12-10-05 12:01 PM, Rob Herring wrote:
> On 10/05/2012 08:51 AM, Mikael Pettersson wrote:
>> Rob Herring writes:
>>   > On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
>>   > > On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
>>   > >> On 5 October 2012 08:12, Russell King - ARM Linux
>>   > >> <linux at arm.linux.org.uk> wrote:
>>   > >>> On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
>>   > >>>> On 5 October 2012 02:56, Rob Herring <robherring2 at gmail.com> wrote:
>>   > >>>>> This struct is the IP header, so a struct ptr is just set to the
>>   > >>>>> beginning of the received data. Since ethernet headers are 14 bytes,
>>   > >>>>> often the IP header is not aligned unless the NIC can place the frame at
>>   > >>>>> a 2 byte offset (which is something I need to investigate). So this
>>   > >>>>> function cannot make any assumptions about the alignment. Does the ABI
>>   > >>>>> define structs have some minimum alignment? Does the struct need to be
>>   > >>>>> declared as packed or something?
>>   > >>>>
>>   > >>>> The ABI defines the alignment of structs as the maximum alignment of its
>>   > >>>> members.  Since this struct contains 32-bit members, the alignment for the
>>   > >>>> whole struct becomes 32 bits as well.  Declaring it as packed tells gcc it
>>   > >>>> might be unaligned (in addition to removing any holes within).
>>   > >>>
>>   > >>> This has come up before in the past.
>>   > >>>
>>   > >>> The Linux network folk will _not_ allow - in any shape or form - for
>>   > >>> this struct to be marked packed (it's the struct which needs to be
>>   > >>> marked packed) because by doing so, it causes GCC to issue byte loads/
>>   > >>> stores on architectures where there isn't a problem, and that decreases
>>   > >>> the performance of the Linux IP stack unnecessarily.
>>   > >>
>>   > >> Which architectures?  I have never seen anything like that.
>>   > >
>>   > > Does it matter?  I'm just relaying the argument against adding __packed
>>   > > which was used before we were forced (by the networking folk) to implement
>>   > > the alignment fault handler.
>>   >
>>   > It doesn't really matter what will be accepted or not as adding __packed
>>   > to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm.
>>   > The only way I've found to eliminate the alignment fault is adding a
>>   > barrier between the 2 loads. That seems like a compiler issue to me if
>>   > there is not a better fix.
>>
>> If you suspect a GCC bug, please prepare a standalone user-space test case
>> and submit it to GCC's bugzilla (I can do the latter if you absolutely do not
>> want to).  It wouldn't be the first alignment-related GCC bug...
>>
>
> Here's a testcase. Compiled on ubuntu precise with
> "arm-linux-gnueabihf-gcc -O2 -marm -march=armv7-a test.c".
>
> typedef unsigned short u16;
> typedef unsigned short __sum16;
> typedef unsigned int __u32;
> typedef unsigned char __u8;
> typedef __u32 __be32;
> typedef u16 __be16;
>
> struct iphdr {
> 	__u8	ihl:4,
> 		version:4;
> 	__u8	tos;
> 	__be16	tot_len;
> 	__be16	id;
> 	__be16	frag_off;
> 	__u8	ttl;
> 	__u8	protocol;
> 	__sum16	check;
> 	__be32	saddr;
> 	__be32	daddr;
> 	/*The options start here. */
> };

I was reading this thread with some interest.  AFAIK, with the default 
alignment rules the above struct is packed; there will be no holes in it.

>
> #define ntohl(x) __swab32((__u32)(__be32)(x))
> #define IP_DF		0x4000		/* Flag: "Don't Fragment"	*/
>
> static inline __attribute__((const)) __u32 __swab32(__u32 x)
> {
> 	__asm__ ("rev %0, %1" : "=r" (x) : "r" (x));
> 	return x;
> }
>
> int main(void * buffer, unsigned int *p_id)
> {
> 	unsigned int id;
> 	int flush = 1;
> 	const struct iphdr *iph = buffer;
> 	__u32 len = *p_id;
> 	
> 	id = ntohl(*(__be32 *)&iph->id);

The above statement is the problem.  I think it is poorly written 
networking code.  It takes the address of a 16 bit quantity (aligned on 
a halfword address), attempts to do a type conversion using pointers, 
then dereference it.  I would have thought:

id = ntohs(iph->id);

would have been enough.

Scott

-- 
Scott Bambrough
Technical Director, Member Services
Linaro Ltd.
email: scott.bambrough at linaro.org
irc: scottb (freenode, irc.linaro.org)
web: http://www.linaro.org
Linaro: The future of Linux on ARM.