[mc1322x] Non-aligned structure copies

[Ian Martin]

I thought about this issue further and I suspect we're not as bad off
as we thought.  First, I think it's the compiler's (and linker's) job
to avoid unaligned words in statically & dynamically allocated data.
That is why the compiler typically inserts gaps in structs,
necessitating the use of the __packed__ attribute in some cases (e.g.
packet headers that won't tolerate gaps).  But I don't think
__packed__ will help us here.

You run into trouble is when the code declares (via typecast) that it
has a struct somewhere, like:

struct packet_header *ph = buffer+offset;

If the address buffer+offset doesn't follow the strict alignment
requirements of the struct, then we've created a runtime problem, like
the one I discovered in the DHCP client.  The solution is to allocate
the struct separately and do a memcpy.

struct packet_header ph;
memcpy(ph, buffer + offset, sizeof(ph));

So I think our job is to track down such risky typecasts, which
unfortunately can only be detected at runtime under the right
condition.

Jon, I haven't reviewed the compiler switches you suggested, but they
may very well detect further issues.

-Ian

On Tue, Nov 3, 2009 at 11:30 PM, Jon Smirl <jonsmirl at gmail.com> wrote:
> On Tue, Nov 3, 2009 at 11:15 PM, Ian Martin <martini at alum.mit.edu> wrote:
>> Hi Jon,
>>
>> I had a similar issue in either Contiki or uIP and inevitably had to
>> redefine the uip_ipaddr_copy macro with a call to the ANSI memcpy
>> function. It was something along the lines of:
>>
>> #define uip_ipaddr_copy(dest, src) memcpy((void*)(dest), (void*)(src),
>> sizeof(uip_ipaddr_t))
>>
>> ...which shouldn't have any trouble with unaligned addressing.  I see
>> that you have created the equivalent using inline code.  I think uIP
>> even defines the macro differently for IPv4 vs IPv6 (besides for the
>> obvious reason).  IIRC, the only other occasion where I ran into
>> trouble with unaligned access was in the DHCP client code.
>
> Now I'm afraid of how many other places this problem may be hidden.
>
> Maybe Mar can get Qemu fired up and convince it to generate exceptions
> on unaligned access. that would help locate the problems.
> My familiarity with Qemu is zero.
>
>>
>> I don't know of a compiler switch offhand.
>
> I don't think it is possible to build a switch. I'm sprinkling
> _attribute__ ((__packed__)) everywhere and looking for more struct
> copies.
>
> Some ARM7 core have hardware that does machine checks on unaligned
> access. mc13224 manual doesn't mention supporting it.
>
>
>>
>> -Ian
>>
>> On Tue, Nov 3, 2009 at 10:48 PM, Jon Smirl <jonsmirl at gmail.com> wrote:
>>> My TTL field was getting stomped in the packets so I added some debug
>>>
>>> uip_nd6_io_rs_output(void) {
>>>  UIP_IP_BUF->vtc = 0x60;
>>>  UIP_IP_BUF->tcflow = 0;
>>>  UIP_IP_BUF->flow = 0;
>>>  UIP_IP_BUF->proto = UIP_PROTO_ICMP6;
>>>  UIP_IP_BUF->ttl = UIP_ND6_HOP_LIMIT;
>>> printf("ttl 1 %d\n", UIP_IP_BUF->ttl);
>>>  uip_create_linklocal_allrouters_mcast(&UIP_IP_BUF->destipaddr);
>>> printf("ttl 2 %d %p %p\n", UIP_IP_BUF->ttl, &UIP_IP_BUF->ttl,
>>> &UIP_IP_BUF->srcipaddr);
>>>  uip_netif_select_src(&UIP_IP_BUF->srcipaddr, &UIP_IP_BUF->destipaddr);
>>> printf("ttl 3 %d\n", UIP_IP_BUF->ttl);
>>>
>>> The select_src(... was stomping TTL
>>>
>>> More debug and I track it down to ipaddr_copy...
>>>
>>> printf("uip_netif_select_src %d %d\n", best, index);
>>>
>>>  uip_ipaddr_copy(src, &(uip_netif_physical_if.addresses[index].ipaddr));
>>>  return;
>>>
>>> --- It's doing a struct copy. I'm suspicious about non-aligned access on ARM
>>>
>>> #ifndef uip_ipaddr_copy
>>> #define uip_ipaddr_copy(dest, src) (*(dest) = *(src))
>>> #endif
>>>
>>> I change it to:
>>>
>>> #ifndef uip_ipaddr_copy
>>> //#define uip_ipaddr_copy(dest, src) (*(dest) = *(src))
>>> #define uip_ipaddr_copy(dest, src) \
>>> { \
>>> int i; \
>>> for (i = 0; i < sizeof *(dest); i++) { \
>>>        *(((char *)(dest)) + i) = *(( (char *)(src)) + i); \
>>> } \
>>> }
>>> #endif
>>>
>>> --- TTL doesn't get stomped any more
>>>
>>> Sending NS to ff00:0000:0000:0000:0000:0001:ffff:ee00 from
>>> 0000:0000:0000:0000:0000:0000:0000:0000 with target address
>>> fe80:0000:0000:0000:dfcc:bbaa:aa99:ffee
>>> output;
>>> Sending RS 0
>>> ttl 1 255
>>> ttl 2 255 40e59a 40e59b
>>> uip_netif_select_src
>>> uip_netif_select_src 0 0
>>> ttl 3 255
>>> ttl 4 255
>>>
>>> The code still doesn't work because there are more of these floating around.
>>>
>>> Is there some compiler switch to tell it to do the right thing with an
>>> unaligned struct copy?
>>>
>>> --
>>> Jon Smirl
>>> jonsmirl at gmail.com
>>>
>>> _______________________________________________
>>> mc1322x mailing list
>>> mc1322x at devl.org
>>> http://devl.org/cgi-bin/mailman/listinfo/mc1322x
>>>
>>
>
>
>
> --
> Jon Smirl
> jonsmirl at gmail.com
>