[U-Boot] [PATCH] net: Mark the ip_udp_hdr struct as packed

[Siarhei Siamashka]

On Fri, 21 Jul 2017 22:15:37 +0300
Siarhei Siamashka <siarhei.siamashka at gmail.com> wrote:

> On Wed, 12 Jul 2017 16:34:50 +0200
> Maxime Ripard <maxime.ripard at free-electrons.com> wrote:
> 
> > The -mno-unaligned-access flag used on ARM to prevent GCC from generating
> > unaligned accesses (obviously) will only do so on packed structures.  
> 
> This statement seems to be poorly worded.
> 
> > It seems like gcc 7.1 is a bit stricter than previous gcc versions on this,
> > and using it lead to data abort for unaligned accesses when generating
> > network traffic.  
> 
> Why don't we just clearly say that this patch fixes undefined behaviour
> in a buggy C code, caused by U-Boot failing to meet the 32-bit alignment
> expectations of GCC for this particular structure? 
> 
> > Fix this by adding the packed attribute to the ip_udp_hdr structure in
> > order to let GCC do its job.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard at free-electrons.com>
> > ---
> >  include/net.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/net.h b/include/net.h
> > index 997db9210a8f..7b815afffafa 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -390,7 +390,7 @@ struct ip_udp_hdr {
> >  	u16		udp_dst;	/* UDP destination port		*/
> >  	u16		udp_len;	/* Length of UDP packet		*/
> >  	u16		udp_xsum;	/* Checksum			*/
> > -};
> > +} __attribute__ ((packed));  
> 
> 
> Alternatively we could try to only mark the 32-bit structure fields as
> "packed" rather than marking the whole structure. Here is a test code:
> 
> /***********************************/
> #include <stdio.h>
> #include <stdint.h>
> 
> struct a
> {
>     uint32_t x;
>     uint16_t y;
> } a;
> 
> struct b
> {
>     uint32_t x __attribute((packed));
>     uint16_t y;
> };
> 
> int main(void)
> {
>     printf("sizeof(struct a) = %d\n", (int)sizeof(struct a));
>     printf("sizeof(struct b) = %d\n", (int)sizeof(struct b));
> 
>     return 0;
> }
> /***********************************/
> 
> Running it produces the following output:
> 
> sizeof(struct a) = 8
> sizeof(struct b) = 6
> __alignof__(struct a) = 4
> __alignof__(struct b) = 2
> 
> 
> 
> Also as an additional safety measure, we can add something like this
> to U-Boot:
> 
>   assert(__alignof__(struct ip_udp_hdr) == 2);
> 
> 
> Maybe it can be also done as a compile-time test rather than a
> runtime test. In the example above, I can add the following code:
> 
>   int dummy_b[3 - __alignof__(struct b)];
>   int dummy_a[3 - __alignof__(struct a)];
> 
> And then GCC complains at compile time, even though the error
> message is not exactly intuitive:
> 
> test.c:17:5: error: size of array ‘dummy_a’ is too large
>  int dummy_a[3 - __alignof__(struct a)];
>      ^

And if we do it this way, then the compile-time test can look a bit
cleaner:

test.c:17:5: error: size of array ‘compile_test_for_struct_a_alignment’ is negative
 int compile_test_for_struct_a_alignment[(__alignof__(struct a) == 2) ? 1 : -1];

-- 
Best regards,
Siarhei Siamashka