diff mbox

[U-Boot] Use packed structures for networking

Message ID 20170721162842.22317-1-denis.pynkin@collabora.com
State Accepted
Commit 704f3ac
Delegated to: Joe Hershberger
Headers show

Commit Message

Denis Pynkin July 21, 2017, 4:28 p.m. UTC 
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
by default for '-O2':

BOOTP broadcast 1
data abort
pc : [<8ff8bb30>]          lr : [<00004f1f>]
reloc pc : [<17832b30>]    lr : [<878abf1f>]
sp : 8f558bc0  ip : 00000000     fp : 8ffef5a4
r10: 8ffed248  r9 : 8f558ee0     r8 : 8ffef594
r7 : 0000000e  r6 : 8ffed700     r5 : 00000000  r4 : 8ffed74e
r3 : 00060101  r2 : 8ffed230     r1 : 8ffed706  r0 : 00000ddd
Flags: nzcv  IRQs off  FIQs off  Mode SVC_32
Resetting CPU ...

Core reason is usage of structures for network headers without packed
attribute.

Reviewed-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
---
 include/net.h | 14 +++++++-------
 net/bootp.h   |  2 +-
 net/dns.h     |  2 +-
 net/nfs.h     |  2 +-
 net/sntp.h    |  2 +-
 5 files changed, 11 insertions(+), 11 deletions(-)

Comments

Fabio Estevam July 21, 2017, 6:05 p.m. UTC | #1 
Adding Tom and Joe.

On Fri, Jul 21, 2017 at 1:28 PM, Denis Pynkin
<denis.pynkin@collabora.com> wrote:
> PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
> by default for '-O2':
>
> BOOTP broadcast 1
> data abort
> pc : [<8ff8bb30>]          lr : [<00004f1f>]
> reloc pc : [<17832b30>]    lr : [<878abf1f>]
> sp : 8f558bc0  ip : 00000000     fp : 8ffef5a4
> r10: 8ffed248  r9 : 8f558ee0     r8 : 8ffef594
> r7 : 0000000e  r6 : 8ffed700     r5 : 00000000  r4 : 8ffed74e
> r3 : 00060101  r2 : 8ffed230     r1 : 8ffed706  r0 : 00000ddd
> Flags: nzcv  IRQs off  FIQs off  Mode SVC_32
> Resetting CPU ...
>
> Core reason is usage of structures for network headers without packed
> attribute.
>
> Reviewed-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> ---
>  include/net.h | 14 +++++++-------
>  net/bootp.h   |  2 +-
>  net/dns.h     |  2 +-
>  net/nfs.h     |  2 +-
>  net/sntp.h    |  2 +-
>  5 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index 2eaa882..e126948 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -308,7 +308,7 @@ struct ethernet_hdr {
>         u8              et_dest[ARP_HLEN];      /* Destination node     */
>         u8              et_src[ARP_HLEN];       /* Source node          */
>         u16             et_protlen;             /* Protocol or length   */
> -};
> +} __attribute__((packed));
>
>  /* Ethernet header size */
>  #define ETHER_HDR_SIZE (sizeof(struct ethernet_hdr))
> @@ -326,7 +326,7 @@ struct e802_hdr {
>         u8              et_snap2;
>         u8              et_snap3;
>         u16             et_prot;                /* 802 protocol         */
> -};
> +} __attribute__((packed));
>
>  /* 802 + SNAP + ethernet header size */
>  #define E802_HDR_SIZE  (sizeof(struct e802_hdr))
> @@ -340,7 +340,7 @@ struct vlan_ethernet_hdr {
>         u16             vet_vlan_type;          /* PROT_VLAN            */
>         u16             vet_tag;                /* TAG of VLAN          */
>         u16             vet_type;               /* protocol type        */
> -};
> +} __attribute__((packed));
>
>  /* VLAN Ethernet header size */
>  #define VLAN_ETHER_HDR_SIZE    (sizeof(struct vlan_ethernet_hdr))
> @@ -369,7 +369,7 @@ struct ip_hdr {
>         u16             ip_sum;         /* checksum                     */
>         struct in_addr  ip_src;         /* Source IP address            */
>         struct in_addr  ip_dst;         /* Destination IP address       */
> -};
> +} __attribute__((packed));
>
>  #define IP_OFFS                0x1fff /* ip offset *= 8 */
>  #define IP_FLAGS       0xe000 /* first 3 bits */
> @@ -397,7 +397,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)
> @@ -435,7 +435,7 @@ struct arp_hdr {
>         u8              ar_tha[];       /* Target hardware address      */
>         u8              ar_tpa[];       /* Target protocol address      */
>  #endif /* 0 */
> -};
> +} __attribute__((packed));
>
>  #define ARP_HDR_SIZE   (8+20)          /* Size assuming ethernet       */
>
> @@ -470,7 +470,7 @@ struct icmp_hdr {
>                 } frag;
>                 u8 data[0];
>         } un;
> -};
> +} __attribute__((packed));
>
>  #define ICMP_HDR_SIZE          (sizeof(struct icmp_hdr))
>  #define IP_ICMP_HDR_SIZE       (IP_HDR_SIZE + ICMP_HDR_SIZE)
> diff --git a/net/bootp.h b/net/bootp.h
> index fcb0a64..a1291be 100644
> --- a/net/bootp.h
> +++ b/net/bootp.h
> @@ -49,7 +49,7 @@ struct bootp_hdr {
>         char            bp_sname[64];   /* Server host name             */
>         char            bp_file[128];   /* Boot file name               */
>         char            bp_vend[OPT_FIELD_SIZE]; /* Vendor information  */
> -};
> +}__attribute__((packed));
>
>  #define BOOTP_HDR_SIZE sizeof(struct bootp_hdr)
>
> diff --git a/net/dns.h b/net/dns.h
> index c4e96af..c55a5c1 100644
> --- a/net/dns.h
> +++ b/net/dns.h
> @@ -29,7 +29,7 @@ struct header {
>         uint16_t        nauth;          /* Authority PRs */
>         uint16_t        nother;         /* Other PRs */
>         unsigned char   data[1];        /* Data, variable length */
> -};
> +} __attribute__((packed));
>
>  void dns_start(void);          /* Begin DNS */
>
> diff --git a/net/nfs.h b/net/nfs.h
> index 45da246..70a1a6d 100644
> --- a/net/nfs.h
> +++ b/net/nfs.h
> @@ -79,7 +79,7 @@ struct rpc_t {
>                         uint32_t data[NFS_READ_SIZE];
>                 } reply;
>         } u;
> -};
> +} __attribute__((packed));
>  void nfs_start(void);  /* Begin NFS */
>
>
> diff --git a/net/sntp.h b/net/sntp.h
> index 6a9c6bb..c38bcee 100644
> --- a/net/sntp.h
> +++ b/net/sntp.h
> @@ -51,7 +51,7 @@ struct sntp_pkt_t {
>         unsigned long long originate_timestamp;
>         unsigned long long receive_timestamp;
>         unsigned long long transmit_timestamp;
> -};
> +} __attribute__((packed));
>
>  void sntp_start(void); /* Begin SNTP */
>
> --
> 2.10.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Tom Rini July 21, 2017, 7 p.m. UTC | #2 
On Fri, Jul 21, 2017 at 07:28:42PM +0300, Denis Pynkin wrote:

> PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
> by default for '-O2':
> 
> BOOTP broadcast 1
> data abort
> pc : [<8ff8bb30>]          lr : [<00004f1f>]
> reloc pc : [<17832b30>]    lr : [<878abf1f>]
> sp : 8f558bc0  ip : 00000000     fp : 8ffef5a4
> r10: 8ffed248  r9 : 8f558ee0     r8 : 8ffef594
> r7 : 0000000e  r6 : 8ffed700     r5 : 00000000  r4 : 8ffed74e
> r3 : 00060101  r2 : 8ffed230     r1 : 8ffed706  r0 : 00000ddd
> Flags: nzcv  IRQs off  FIQs off  Mode SVC_32
> Resetting CPU ...
> 
> Core reason is usage of structures for network headers without packed
> attribute.
> 
> Reviewed-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> ---
>  include/net.h | 14 +++++++-------
>  net/bootp.h   |  2 +-
>  net/dns.h     |  2 +-
>  net/nfs.h     |  2 +-
>  net/sntp.h    |  2 +-
>  5 files changed, 11 insertions(+), 11 deletions(-)

So, what I've been wondering, and others have poked me about (who can
chime in if they like), how is the kernel not also tripping over this?
Joe Hershberger July 27, 2017, 9:26 p.m. UTC | #3 
On Fri, Jul 21, 2017 at 2:00 PM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Jul 21, 2017 at 07:28:42PM +0300, Denis Pynkin wrote:
>
>> PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
>> by default for '-O2':
>>
>> BOOTP broadcast 1
>> data abort
>> pc : [<8ff8bb30>]          lr : [<00004f1f>]
>> reloc pc : [<17832b30>]    lr : [<878abf1f>]
>> sp : 8f558bc0  ip : 00000000     fp : 8ffef5a4
>> r10: 8ffed248  r9 : 8f558ee0     r8 : 8ffef594
>> r7 : 0000000e  r6 : 8ffed700     r5 : 00000000  r4 : 8ffed74e
>> r3 : 00060101  r2 : 8ffed230     r1 : 8ffed706  r0 : 00000ddd
>> Flags: nzcv  IRQs off  FIQs off  Mode SVC_32
>> Resetting CPU ...
>>
>> Core reason is usage of structures for network headers without packed
>> attribute.
>>
>> Reviewed-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
>> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
>> ---
>>  include/net.h | 14 +++++++-------
>>  net/bootp.h   |  2 +-
>>  net/dns.h     |  2 +-
>>  net/nfs.h     |  2 +-
>>  net/sntp.h    |  2 +-
>>  5 files changed, 11 insertions(+), 11 deletions(-)
>
> So, what I've been wondering, and others have poked me about (who can
> chime in if they like), how is the kernel not also tripping over this?

Great question.
Denis Pynkin July 29, 2017, 9:53 a.m. UTC | #4 
28.07.2017 00:26, Joe Hershberger wrote:

>>> PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
>>> by default for '-O2':

>> So, what I've been wondering, and others have poked me about (who can
>> chime in if they like), how is the kernel not also tripping over this?
> 
> Great question.

Believe kernel do not work in single static buffer for the whole packet 
as u-boot do.

Problem is in ethernet header structure with size ETHER_HDR_SIZE=14, so 
IP and BOOTP headers are not aligned to 4 bytes.

Let's take bootp.c code as example:

755     bp = (struct bootp_hdr *)pkt;
756     bp->bp_op = OP_BOOTREQUEST;
757     bp->bp_htype = HWT_ETHER;
758     bp->bp_hlen = HWL_ETHER;
759     bp->bp_hops = 0;

Without '-fstore-merging' or with enabled 'packed' structures assembler 
code looks like:
   6a:   77a3          strb    r3, [r4, #30]
   6c:   f884 b01c     strb.w  fp, [r4, #28]
   70:   f884 b01d     strb.w  fp, [r4, #29]
   74:   77e5          strb    r5, [r4, #31]

but with option '-fstore-merging' all these instructions merged into 
single instruction:
   62:   61e3          str     r3, [r4, #28]

and store address is not aligned to 32b boundary here, so it results to 
'data abort'.

I see some possible solutions here:
- add option '-fno-store-merging' -- works, I test it already
- use packed structures -- since there are some unsafe code in sources
- rewrite networking part to work with any protocol separately and merge 
headers and data only on send.

Even simple reordering of header flags initialization helps here, but it 
looks like a bit of black magic in code.

PS forgot to mention in patch description -- '-Os' includes 
'-fstore-merging' option as well for gcc 7.1.
Denis Pynkin July 29, 2017, 9:56 a.m. UTC | #5 
28.07.2017 00:26, Joe Hershberger wrote:

>>> PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
>>> by default for '-O2':

>> So, what I've been wondering, and others have poked me about (who can
>> chime in if they like), how is the kernel not also tripping over this?
> 
> Great question.
> 

Believe kernel do not work in single static buffer for the whole packet 
as u-boot do.

Problem is in ethernet header structure with size ETHER_HDR_SIZE=14, so 
IP and BOOTP headers are not aligned to 4 bytes.

Let's take bootp.c code as example:

755     bp = (struct bootp_hdr *)pkt;
756     bp->bp_op = OP_BOOTREQUEST;
757     bp->bp_htype = HWT_ETHER;
758     bp->bp_hlen = HWL_ETHER;
759     bp->bp_hops = 0;

Without '-fstore-merging' or with enabled 'packed' structures assembler 
code looks like:
   6a:   77a3          strb    r3, [r4, #30]
   6c:   f884 b01c     strb.w  fp, [r4, #28]
   70:   f884 b01d     strb.w  fp, [r4, #29]
   74:   77e5          strb    r5, [r4, #31]

but with option '-fstore-merging' all these instructions merged into 
single instruction:
   62:   61e3          str     r3, [r4, #28]

and store address is not aligned to 32b boundary here, so it results to 
'data abort'.

I see some possible solutions here:
- add option '-fno-store-merging' -- works, I test it already
- use packed structures -- since there are some unsafe code in sources
- rewrite networking part to work with any protocol separately and merge 
headers and data only on send.

Even simple reordering of header flags initialization helps here, but it 
looks like a bit of black magic in code.

PS forgot to mention in patch description -- '-Os' includes 
'-fstore-merging' option as well for gcc 7.1.
Tom Rini July 29, 2017, 12:04 p.m. UTC | #6 
On Sat, Jul 29, 2017 at 12:53:36PM +0300, Denis Pynkin wrote:
> 28.07.2017 00:26, Joe Hershberger wrote:
> 
> >>>PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
> >>>by default for '-O2':
> 
> >>So, what I've been wondering, and others have poked me about (who can
> >>chime in if they like), how is the kernel not also tripping over this?
> >
> >Great question.
> 
> Believe kernel do not work in single static buffer for the whole
> packet as u-boot do.
> 
> Problem is in ethernet header structure with size ETHER_HDR_SIZE=14,
> so IP and BOOTP headers are not aligned to 4 bytes.
> 
> Let's take bootp.c code as example:
> 
> 755     bp = (struct bootp_hdr *)pkt;
> 756     bp->bp_op = OP_BOOTREQUEST;
> 757     bp->bp_htype = HWT_ETHER;
> 758     bp->bp_hlen = HWL_ETHER;
> 759     bp->bp_hops = 0;
> 
> Without '-fstore-merging' or with enabled 'packed' structures
> assembler code looks like:
>   6a:   77a3          strb    r3, [r4, #30]
>   6c:   f884 b01c     strb.w  fp, [r4, #28]
>   70:   f884 b01d     strb.w  fp, [r4, #29]
>   74:   77e5          strb    r5, [r4, #31]
> 
> but with option '-fstore-merging' all these instructions merged into
> single instruction:
>   62:   61e3          str     r3, [r4, #28]
> 
> and store address is not aligned to 32b boundary here, so it results
> to 'data abort'.
> 
> I see some possible solutions here:
> - add option '-fno-store-merging' -- works, I test it already
> - use packed structures -- since there are some unsafe code in sources
> - rewrite networking part to work with any protocol separately and
> merge headers and data only on send.
> 
> Even simple reordering of header flags initialization helps here,
> but it looks like a bit of black magic in code.
> 
> PS forgot to mention in patch description -- '-Os' includes
> '-fstore-merging' option as well for gcc 7.1.

Thanks for explaining.  My inclination is that we should have a packed
change in for -rc1.  Philipp, have you had a chance to edit
doc/README.unaligned-memory-access.txt to your liking?  Thanks!
Joe Hershberger Aug. 1, 2017, 4:37 p.m. UTC | #7 
On Fri, Jul 21, 2017 at 11:28 AM, Denis Pynkin
<denis.pynkin@collabora.com> wrote:
> PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled
> by default for '-O2':
>
> BOOTP broadcast 1
> data abort
> pc : [<8ff8bb30>]          lr : [<00004f1f>]
> reloc pc : [<17832b30>]    lr : [<878abf1f>]
> sp : 8f558bc0  ip : 00000000     fp : 8ffef5a4
> r10: 8ffed248  r9 : 8f558ee0     r8 : 8ffef594
> r7 : 0000000e  r6 : 8ffed700     r5 : 00000000  r4 : 8ffed74e
> r3 : 00060101  r2 : 8ffed230     r1 : 8ffed706  r0 : 00000ddd
> Flags: nzcv  IRQs off  FIQs off  Mode SVC_32
> Resetting CPU ...
>
> Core reason is usage of structures for network headers without packed
> attribute.
>
> Reviewed-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Joe Hershberger Aug. 7, 2017, 8:33 p.m. UTC | #8 
Hi Denis,

https://patchwork.ozlabs.org/patch/792238/ was applied to u-boot-net.git.

Thanks!
-Joe
diff mbox

Patch

diff --git a/include/net.h b/include/net.h
index 2eaa882..e126948 100644
--- a/include/net.h
+++ b/include/net.h
@@ -308,7 +308,7 @@  struct ethernet_hdr {
 	u8		et_dest[ARP_HLEN];	/* Destination node	*/
 	u8		et_src[ARP_HLEN];	/* Source node		*/
 	u16		et_protlen;		/* Protocol or length	*/
-};
+} __attribute__((packed));
 
 /* Ethernet header size */
 #define ETHER_HDR_SIZE	(sizeof(struct ethernet_hdr))
@@ -326,7 +326,7 @@  struct e802_hdr {
 	u8		et_snap2;
 	u8		et_snap3;
 	u16		et_prot;		/* 802 protocol		*/
-};
+} __attribute__((packed));
 
 /* 802 + SNAP + ethernet header size */
 #define E802_HDR_SIZE	(sizeof(struct e802_hdr))
@@ -340,7 +340,7 @@  struct vlan_ethernet_hdr {
 	u16		vet_vlan_type;		/* PROT_VLAN		*/
 	u16		vet_tag;		/* TAG of VLAN		*/
 	u16		vet_type;		/* protocol type	*/
-};
+} __attribute__((packed));
 
 /* VLAN Ethernet header size */
 #define VLAN_ETHER_HDR_SIZE	(sizeof(struct vlan_ethernet_hdr))
@@ -369,7 +369,7 @@  struct ip_hdr {
 	u16		ip_sum;		/* checksum			*/
 	struct in_addr	ip_src;		/* Source IP address		*/
 	struct in_addr	ip_dst;		/* Destination IP address	*/
-};
+} __attribute__((packed));
 
 #define IP_OFFS		0x1fff /* ip offset *= 8 */
 #define IP_FLAGS	0xe000 /* first 3 bits */
@@ -397,7 +397,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)
@@ -435,7 +435,7 @@  struct arp_hdr {
 	u8		ar_tha[];	/* Target hardware address	*/
 	u8		ar_tpa[];	/* Target protocol address	*/
 #endif /* 0 */
-};
+} __attribute__((packed));
 
 #define ARP_HDR_SIZE	(8+20)		/* Size assuming ethernet	*/
 
@@ -470,7 +470,7 @@  struct icmp_hdr {
 		} frag;
 		u8 data[0];
 	} un;
-};
+} __attribute__((packed));
 
 #define ICMP_HDR_SIZE		(sizeof(struct icmp_hdr))
 #define IP_ICMP_HDR_SIZE	(IP_HDR_SIZE + ICMP_HDR_SIZE)
diff --git a/net/bootp.h b/net/bootp.h
index fcb0a64..a1291be 100644
--- a/net/bootp.h
+++ b/net/bootp.h
@@ -49,7 +49,7 @@  struct bootp_hdr {
 	char		bp_sname[64];	/* Server host name		*/
 	char		bp_file[128];	/* Boot file name		*/
 	char		bp_vend[OPT_FIELD_SIZE]; /* Vendor information	*/
-};
+}__attribute__((packed));
 
 #define BOOTP_HDR_SIZE	sizeof(struct bootp_hdr)
 
diff --git a/net/dns.h b/net/dns.h
index c4e96af..c55a5c1 100644
--- a/net/dns.h
+++ b/net/dns.h
@@ -29,7 +29,7 @@  struct header {
 	uint16_t	nauth;		/* Authority PRs */
 	uint16_t	nother;		/* Other PRs */
 	unsigned char	data[1];	/* Data, variable length */
-};
+} __attribute__((packed));
 
 void dns_start(void);		/* Begin DNS */
 
diff --git a/net/nfs.h b/net/nfs.h
index 45da246..70a1a6d 100644
--- a/net/nfs.h
+++ b/net/nfs.h
@@ -79,7 +79,7 @@  struct rpc_t {
 			uint32_t data[NFS_READ_SIZE];
 		} reply;
 	} u;
-};
+} __attribute__((packed));
 void nfs_start(void);	/* Begin NFS */
 
 
diff --git a/net/sntp.h b/net/sntp.h
index 6a9c6bb..c38bcee 100644
--- a/net/sntp.h
+++ b/net/sntp.h
@@ -51,7 +51,7 @@  struct sntp_pkt_t {
 	unsigned long long originate_timestamp;
 	unsigned long long receive_timestamp;
 	unsigned long long transmit_timestamp;
-};
+} __attribute__((packed));
 
 void sntp_start(void);	/* Begin SNTP */