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

[Siarhei Siamashka]

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

-- 
Best regards,
Siarhei Siamashka