alignment faults in 3.6

[Mans Rullgard]

On 9 October 2012 15:05, Scott Bambrough <scott.bambrough at linaro.org> wrote:
> On 12-10-05 12:01 PM, Rob Herring wrote:
>> 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.

Correct.  The problem here is that something is passing around a pointer to
such a struct which is not 4-byte aligned as required by the ABI rules.

>> #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),

The 'id' member starts 4 bytes into the struct, so if the struct is properly
aligned, there will be no fault here.  The problem is that networking code does
not always align these structs correctly.

> attempts to do a type conversion using pointers, then dereference
> it.  I would have thought:
>
> id = ntohs(iph->id);
>
> would have been enough.

I'm assuming the is intentionally merging two 16-bit fields into one 32-bit
value.  What you suggest would do something rather different.

-- 
Mans Rullgard / mru