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

On Wed, 12 Jul 2017 16:34:50 +0200 Maxime Ripard maxime.ripard@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@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];