net: Mark the ip_udp_hdr struct as packed

PATCH New Maxime Ripard 5 participants 8 messages Jul 12, 2017 – Jul 22, 2017

Share & tools

Maxime Ripard

author

2017-07-12 14:34 UTC #0 lore raw

PATCH 1/1 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]>

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

include/net.h +1 -1

index 997db9210a8f..7b815afffafa 100644
@@ -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)

Signature · 1 line

2.13.0

Joe Hershberger

2017-07-18 18:10 UTC #1 lore raw

PATCH 1/1 net: Mark the ip_udp_hdr struct as packed

Hi Maxime,

Quoted 20 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.
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));

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

Quoted 9 lines

include/net.h +0 -1
 #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

Maxime Ripard

author

2017-07-19 07:01 UTC #2 lore raw

PATCH 1/1 net: Mark the ip_udp_hdr struct as packed

Hi Joe,

Quoted 17 lines · Joe Hershberger

Quoted 15 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.
Signed-off-by Maxime Ripard <[email protected]>
---
 include/net.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

include/net.h struct ip_udp_hdr { · line 393

Quoted 10 lines

include/net.h +1 -1

index 997db9210a8f..7b815afffafa 100644
@@ -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.

Quoted 2 lines

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

That's true.

Quoted 4 lines

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

Signature · 10 lines

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>

Joe Hershberger

2017-07-19 18:13 UTC #3 lore raw

PATCH 1/1 net: Mark the ip_udp_hdr struct as packed

Quoted 46 lines · Maxime Ripard

Hi Joe,
Quoted 17 lines · Joe Hershberger
Quoted 15 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.
Signed-off-by Maxime Ripard <[email protected]>
---
 include/net.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 12 lines
include/net.h struct ip_udp_hdr { · line 393
Quoted 10 lines
include/net.h +1 -1
index 997db9210a8f..7b815afffafa 100644
@@ -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.
Quoted 2 lines
It seems that the structure itself is naturally packed (each element is aligned to its access size).
That's true.
Quoted 4 lines
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?

Quoted 7 lines

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.

Quoted 6 lines

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

Jeroen Hofstee

2017-07-19 18:26 UTC #4 lore raw

PATCH 1/1 net: Mark the ip_udp_hdr struct as packed

In reply to Joe Hershberger

Hi,

Quoted 19 lines · Joe Hershberger

Hi Maxime,
Quoted 15 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.
Signed-off-by Maxime Ripard <[email protected]>
---
  include/net.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

include/net.h struct ip_udp_hdr { · line 393
Quoted 10 lines
include/net.h +1 -1
index 997db9210a8f..7b815afffafa 100644
@@ -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 |

Siarhei Siamashka

2017-07-21 18:46 UTC #5 lore raw

PATCH 1/1 net: Mark the ip_udp_hdr struct as packed

Quoted 40 lines · On Wed, 19 Jul 2017 20:26:54 +0200 Jeroen Hofstee <[email protected]> wrote:

Hi,
Quoted 19 lines · Joe Hershberger
Hi Maxime,
Quoted 15 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.
Signed-off-by Maxime Ripard <[email protected]>
---
  include/net.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
include/net.h struct ip_udp_hdr { · line 393
Quoted 10 lines
include/net.h +1 -1
index 997db9210a8f..7b815afffafa 100644
@@ -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?

Quoted 2 lines

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

Signature · 2 lines

Best regards,
Siarhei Siamashka

Jeroen Hofstee

2017-07-22 13:32 UTC #6 lore raw

PATCH 1/1 net: Mark the ip_udp_hdr struct as packed

Hello Siarhei,

Quoted 42 lines · Siarhei Siamashka

Quoted 39 lines · On Wed, 19 Jul 2017 20:26:54 +0200 Jeroen Hofstee <[email protected]> wrote:
Hi,
Quoted 35 lines · Joe Hershberger
Hi Maxime,
Quoted 20 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.
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));
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?

Quoted 6 lines

Quoted 2 lines
|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

Joe Hershberger

2017-07-19 18:14 UTC #7 lore raw

PATCH 1/1 net: Mark the ip_udp_hdr struct as packed

In reply to Maxime Ripard

Quoted 11 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.
Signed-off-by Maxime Ripard <[email protected]>

Acked-by Joe Hershberger <[email protected]>