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

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

[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.

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));
 
 #define IP_UDP_HDR_SIZE		(sizeof(struct ip_udp_hdr))
 #define UDP_HDR_SIZE		(IP_UDP_HDR_SIZE - IP_HDR_SIZE)
-- 
2.13.0

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

[Joe Hershberger]

Hi Maxime,

On Wed, Jul 12, 2017 at 9:34 AM, 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.
>
> 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 <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));

Do you have an example of why this is unaligned? It seems that the
structure itself is naturally packed (each element is aligned to its
access size). It seems the only time this would hit a dabort is if the
head of the buffer is not 32-bit aligned. Maybe we should address the
place where that is the case instead of forcing byte-wise accesses in
general for this structure?

Thanks,
-Joe

>  #define IP_UDP_HDR_SIZE                (sizeof(struct ip_udp_hdr))
>  #define UDP_HDR_SIZE           (IP_UDP_HDR_SIZE - IP_HDR_SIZE)
> --
> 2.13.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Maxime Ripard]

Hi Joe,

On Tue, Jul 18, 2017 at 01:10:59PM -0500, Joe Hershberger wrote:
> On Wed, Jul 12, 2017 at 9:34 AM, 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.
> >
> > 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 <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));
> 
> Do you have an example of why this is unaligned?

You can have the discussion that led to this patch in "Data abort with
gcc 7.1", started a week ago.

> It seems that the structure itself is naturally packed (each element
> is aligned to its access size).

That's true.

> It seems the only time this would hit a dabort is if the head of the
> buffer is not 32-bit aligned. Maybe we should address the place
> where that is the case instead of forcing byte-wise accesses in
> general for this structure?

That's exactly what happens, the pointer to the ip_up_hdr is not
aligned on 32 bits, and triggers an alignment error.

However, I'm not sure how feasible it would be to align the IP packets
on 32-bits, since the Ethernet header is only 14 bytes, right?  We
could use a bounce buffer for each packet, but that's not really
optimized either.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170719/c1115a4c/attachment.sig>

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

[Joe Hershberger]

On Wed, Jul 19, 2017 at 2:01 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Joe,
>
> On Tue, Jul 18, 2017 at 01:10:59PM -0500, Joe Hershberger wrote:
>> On Wed, Jul 12, 2017 at 9:34 AM, 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.
>> >
>> > 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 <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));
>>
>> Do you have an example of why this is unaligned?
>
> You can have the discussion that led to this patch in "Data abort with
> gcc 7.1", started a week ago.
>
>> It seems that the structure itself is naturally packed (each element
>> is aligned to its access size).
>
> That's true.
>
>> It seems the only time this would hit a dabort is if the head of the
>> buffer is not 32-bit aligned. Maybe we should address the place
>> where that is the case instead of forcing byte-wise accesses in
>> general for this structure?
>
> That's exactly what happens, the pointer to the ip_up_hdr is not
> aligned on 32 bits, and triggers an alignment error.
>
> However, I'm not sure how feasible it would be to align the IP packets
> on 32-bits, since the Ethernet header is only 14 bytes, right?  We
> could use a bounce buffer for each packet, but that's not really
> optimized either.

Yeah, good point.

> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

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

[Jeroen Hofstee]

Hi,


On 07/18/2017 08:10 PM, Joe Hershberger wrote:
> Hi Maxime,
>
> On Wed, Jul 12, 2017 at 9:34 AM, 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.
>>
>> 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 <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));
> Do you have an example of why this is unaligned? It seems that the
> structure itself is naturally packed (each element is aligned to its
> access size). It seems the only time this would hit a dabort is if the
> head of the buffer is not 32-bit aligned. Maybe we should address the
> place where that is the case instead of forcing byte-wise accesses in
> general for this structure?

|Perhaps __attribute__((aligned(2))) can prevent byte wise accesses? 
Regards, Jeroen |

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

[Siarhei Siamashka]

On Wed, 19 Jul 2017 20:26:54 +0200
Jeroen Hofstee <jeroen@myspectrum.nl> wrote:

> Hi,
> 
> 
> On 07/18/2017 08:10 PM, Joe Hershberger wrote:
> > Hi Maxime,
> >
> > On Wed, Jul 12, 2017 at 9:34 AM, 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.
> >>
> >> 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 <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));  
> > Do you have an example of why this is unaligned? It seems that the
> > structure itself is naturally packed (each element is aligned to its
> > access size). It seems the only time this would hit a dabort is if the
> > head of the buffer is not 32-bit aligned. Maybe we should address the
> > place where that is the case instead of forcing byte-wise accesses in
> > general for this structure?  
> 
> |Perhaps __attribute__((aligned(2))) can prevent byte wise accesses? 
> Regards, Jeroen |

https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html

It says that "The aligned attribute can only increase alignment".

-- 
Best regards,
Siarhei Siamashka

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

[Jeroen Hofstee]

Hello Siarhei,


On 07/21/2017 08:46 PM, Siarhei Siamashka wrote:
> On Wed, 19 Jul 2017 20:26:54 +0200
> Jeroen Hofstee <jeroen@myspectrum.nl> wrote:
>
>> Hi,
>>
>>
>> On 07/18/2017 08:10 PM, Joe Hershberger wrote:
>>> Hi Maxime,
>>>
>>> On Wed, Jul 12, 2017 at 9:34 AM, 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.
>>>>
>>>> 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 <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));
>>> Do you have an example of why this is unaligned? It seems that the
>>> structure itself is naturally packed (each element is aligned to its
>>> access size). It seems the only time this would hit a dabort is if the
>>> head of the buffer is not 32-bit aligned. Maybe we should address the
>>> place where that is the case instead of forcing byte-wise accesses in
>>> general for this structure?
>> |Perhaps __attribute__((aligned(2))) can prevent byte wise accesses?
>> Regards, Jeroen |
> https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html
>
> It says that "The aligned attribute can only increase alignment".
>

yes, __attribute__((packed, aligned(2))); would be needed it seems.
To first force alignment of 1 (which packed does) and thereafter increase
it to 2 again.

That _might_ prevent the 32 bit stores, and use 16 bit ones instead where
possible / not forcing byte accesses everywhere. Completely untested 
though ;)

Regards,
Jeroen

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

[Joe Hershberger]

On Wed, Jul 12, 2017 at 9:34 AM, 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.
>
> 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 <maxime.ripard@free-electrons.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

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

[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.

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));
 
 #define IP_UDP_HDR_SIZE		(sizeof(struct ip_udp_hdr))
 #define UDP_HDR_SIZE		(IP_UDP_HDR_SIZE - IP_HDR_SIZE)
-- 
2.13.0

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

[Dr. Philipp Tomsich]

> On 12 Jul 2017, at 16:34, 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.
> 
> 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 <maxime.ripard@free-electrons.com>
> —

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

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

[Maxime Ripard]

On Wed, Jul 12, 2017 at 04:37:43PM +0200, Dr. Philipp Tomsich wrote:
> 
> > On 12 Jul 2017, at 16:34, 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.
> > 
> > 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 <maxime.ripard@free-electrons.com>
> > —
> 
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

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

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170717/00fd6bd9/attachment.sig>

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

[Tom Rini]

On Mon, Jul 17, 2017 at 11:29:39AM +0200, Maxime Ripard wrote:
> On Wed, Jul 12, 2017 at 04:37:43PM +0200, Dr. Philipp Tomsich wrote:
> > 
> > > On 12 Jul 2017, at 16:34, 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.
> > > 
> > > 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 <maxime.ripard@free-electrons.com>
> > > —
> > 
> > Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> 
> I'm not exactly sure who is supposed to merge patches touching
> include/ ? Tom?

I'd like Joe to chime in, since it's net related.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170717/8e52121a/attachment.sig>

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

-- 
Best regards,
Siarhei Siamashka

[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@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];

-- 
Best regards,
Siarhei Siamashka