alignment faults in 3.6

[Mans Rullgard]

On 5 October 2012 02:56, Rob Herring <robherring2 at gmail.com> wrote:
> On 10/04/2012 08:26 PM, Mans Rullgard wrote:
>> On 5 October 2012 01:58, Michael Hope <michael.hope at linaro.org> wrote:
>>> On 5 October 2012 12:10, Rob Herring <robherring2 at gmail.com> wrote:
>>>> I've been scratching my head with a "scheduling while atomic" bug I
>>>> started seeing on 3.6. I can easily reproduce this problem when doing a
>>>> wget on my system. It ultimately seems to be a combination of factors.
>>>> The "scheduling while atomic" bug is triggered in do_alignment which
>>>> gets triggered by this code in net/ipv4/af_inet.c, line 1356:
>>>>
>>>> id = ntohl(*(__be32 *)&iph->id);
>>>> flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF));
>>>> id >>= 16;
>>>>
>>>> This code compiles into this using "gcc version 4.6.3 (Ubuntu/Linaro
>>>> 4.6.3-1ubuntu5)":
>>>>
>>>> c02ac020:       e8920840        ldm     r2, {r6, fp}
>>>> c02ac024:       e6bfbf3b        rev     fp, fp
>>>> c02ac028:       e6bf6f36        rev     r6, r6
>>>> c02ac02c:       e22bc901        eor     ip, fp, #16384  ; 0x4000
>>>> c02ac030:       e0266008        eor     r6, r6, r8
>>>> c02ac034:       e18c6006        orr     r6, ip, r6
>>>>
>>>> which generates alignment faults on the ldm. These are silent until this
>>>> commit is applied:
>>>
>>> Hi Rob.  I assume that iph is something like:
>>>
>>> struct foo {
>>>     u32 x;
>>>     char id[8];
>>> };
>>>
>>> struct foo *iph;
>>>
>>> GCC merged the two adjacent loads of x and id into one ldm.  This is
>>> an ARM specific optimisation done in load_multiple_sequence() and
>>> enabled with -fpeephole2.
>>>
>>> I think the assembly is correct - GCC knows that iph is aligned and
>>> knows the offsets of both x and id.  Happy to be corrected if I'm
>>> wrong, but I think the assembly is valid given the C code.
>>
>> The struct looks like this:
>>
>> struct iphdr {
>> #if defined(__LITTLE_ENDIAN_BITFIELD)
>>       __u8    ihl:4,
>>               version:4;
>> #elif defined (__BIG_ENDIAN_BITFIELD)
>>       __u8    version:4,
>>               ihl:4;
>> #else
>> #error        "Please fix <asm/byteorder.h>"
>> #endif
>>       __u8    tos;
>>       __be16  tot_len;
>>       __be16  id;
>>       __be16  frag_off;
>>       __u8    ttl;
>>       __u8    protocol;
>>       __sum16 check;
>>       __be32  saddr;
>>       __be32  daddr;
>>       /*The options start here. */
>> };
>>
>> In a normal build (there's some magic for special checkers) __be32 is a plain
>> __u32 so the struct should be at least 4-byte aligned.  If somehow it is not,
>> that is the real bug.
>
> 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).

-- 
Mans Rullgard / mru