net: Mark the ip_udp_hdr struct as packed

The -mno-unaligned-access flag used on ARM to prevent GCC from generating unaligned accesses (obviously) will only do so on packed structures.

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.

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 <[email protected]>

—

Quoted 10 lines · Maxime Ripard

The -mno-unaligned-access flag used on ARM to prevent GCC from generating unaligned accesses (obviously) will only do so on packed structures.

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.

Quoted 5 lines

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 <[email protected]>

—

Reviewed-by Philipp Tomsich <[email protected]>

Quoted 13 lines · Dr. Philipp Tomsich

Quoted 13 lines · Maxime Ripard

The -mno-unaligned-access flag used on ARM to prevent GCC from generating unaligned accesses (obviously) will only do so on packed structures.

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.

Fix this by adding the packed attribute to the ip_udp_hdr structure in order to let GCC do its job.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Maxime Ripard <[email protected]>

—

Reviewed-by Philipp Tomsich <[email protected]>

I'm not exactly sure who is supposed to merge patches touching include/ ? Tom?

The -mno-unaligned-access flag used on ARM to prevent GCC from generating unaligned accesses (obviously) will only do so on packed structures.

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.

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 <[email protected]>

---
 include/net.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/net.h +0 -0

index 997db9210a8f..7b815afffafa 100644

Quoted 2 lines · On Wed, 12 Jul 2017 16:34:50 +0200 Maxime Ripard…

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.

Quoted 3 lines

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?

Quoted 13 lines

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 <[email protected]>

---
 include/net.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/net.h +0 -0

index 997db9210a8f..7b815afffafa 100644

include/net.h struct ip_udp_hdr { · line 393

Quoted 5 lines

include/net.h +1 -1

@@ -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)];
     ^