[U-Boot] Data Abort with gcc 7.1

[Måns Rullgård]

Maxime Ripard <maxime.ripard at free-electrons.com> writes:

> Hi,
>
> I recently got a gcc 7.1 based toolchain, and it seems like it
> generates unaligned code, specifically in the net_set_ip_header
> function in my case.
>
> Whenever some packet is sent, this data abort is triggered:
>
> => setenv ipaddr 10.42.0.1; ping 10.42.0.254
> using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> MAC de:ad:be:ef:00:01
> HOST MAC de:ad:be:af:00:00
> RNDIS ready
> musb-hdrc: peripheral reset irq lost!
> high speed config #2: 2 mA, Ethernet Gadget, using RNDIS
> USB RNDIS network up!
> Using usb_ether device
> data abort
> pc : [<7ff9db10>]	   lr : [<7ff9f00c>]
> reloc pc : [<4a043b10>]	   lr : [<4a04500c>]
> sp : 7bf37cc8  ip : 00000000	 fp : 7ff6236c
> r10: 7ffed2b8  r9 : 7bf39ee8	 r8 : 7ffed2b8
> r7 : 00000001  r6 : 00000000	 r5 : 0000002a  r4 : 7ffed30e
> r3 : 14000045  r2 : 01002a0a	 r1 : fe002a0a  r0 : 7ffed30e
> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
> Resetting CPU ...
>
> Running objdump on it gives us this:
>
> 4a043b04 <net_set_ip_header>:
>
> 	/*
> 	 *	Construct an IP header.
> 	 */
> 	/* IP_HDR_SIZE / 4 (not including UDP) */
> 	ip->ip_hl_v  = 0x45;
> 4a043b04:	e59f3074 	ldr	r3, [pc, #116]	; 4a043b80 <net_set_ip_header+0x7c>
> {
> 4a043b08:	e92d4013 	push	{r0, r1, r4, lr}
> 4a043b0c:	e1a04000 	mov	r4, r0
> 	ip->ip_hl_v  = 0x45;
> 4a043b10:	e5803000 	str	r3, [r0] <---- Abort
> 	ip->ip_tos   = 0;
> 	ip->ip_len   = htons(IP_HDR_SIZE);
> 	ip->ip_id    = htons(net_ip_id++);
> 4a043b14:	e59f3068 	ldr	r3, [pc, #104]	; 4a043b84 <net_set_ip_header+0x80>
>
> It seems like r0 is indeed set to an unaligned address (0x7ffed30e)
> for some reason.
>
> Using a Linaro 6.3 toolchain works on the same commit with the same
> config, so it really seems to be a compiler-related issue.
>
> It generates this code:
>
> 4a043ec4 <net_set_ip_header>:
>
> 	/*
> 	 *	Construct an IP header.
> 	 */
> 	/* IP_HDR_SIZE / 4 (not including UDP) */
> 	ip->ip_hl_v  = 0x45;
> 4a043ec4:	e3a03045 	mov	r3, #69	; 0x45
> {
> 4a043ec8:	e92d4013 	push	{r0, r1, r4, lr}
> 4a043ecc:	e1a04000 	mov	r4, r0
> 	ip->ip_hl_v  = 0x45;
> 4a043ed0:	e5c03000 	strb	r3, [r0]
> 	ip->ip_tos   = 0;
> 	ip->ip_len   = htons(IP_HDR_SIZE);
> 4a043ed4:	e3a03b05 	mov	r3, #5120	; 0x1400
> 	ip->ip_tos   = 0;
> 4a043ed8:	e3a00000 	mov	r0, #0
> 	ip->ip_len   = htons(IP_HDR_SIZE);
> 4a043edc:	e1c430b2 	strh	r3, [r4, #2]
> 	ip->ip_id    = htons(net_ip_id++);
> 4a043ee0:	e59f3064 	ldr	r3, [pc, #100]	; 4a043f4c <net_set_ip_header+0x88>
>
> And it seems like it's using an strb instruction to avoid the
> unaligned access.
>
> As far as I know, we are passing --wno-unaligned-access, so the broken
> situation should not arise, and yet it does, so I'm a bit confused,
> and not really sure what to do from there.

For reference, this is the relevant code:

struct ip_udp_hdr {
	u8		ip_hl_v;	/* header length and version	*/
	u8		ip_tos;		/* type of service		*/
	u16		ip_len;		/* total length			*/
	u16		ip_id;		/* identification		*/
	u16		ip_off;		/* fragment offset field	*/
	u8		ip_ttl;		/* time to live			*/
	u8		ip_p;		/* protocol			*/
	u16		ip_sum;		/* checksum			*/
	struct in_addr	ip_src;		/* Source IP address		*/
	struct in_addr	ip_dst;		/* Destination IP address	*/
	u16		udp_src;	/* UDP source port		*/
	u16		udp_dst;	/* UDP destination port		*/
	u16		udp_len;	/* Length of UDP packet		*/
	u16		udp_xsum;	/* Checksum			*/
};

void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr source)
{
	struct ip_udp_hdr *ip = (struct ip_udp_hdr *)pkt;

	/*
	 *	Construct an IP header.
	 */
	/* IP_HDR_SIZE / 4 (not including UDP) */
	ip->ip_hl_v  = 0x45;
	ip->ip_tos   = 0;
	ip->ip_len   = htons(IP_HDR_SIZE);
	ip->ip_id    = htons(net_ip_id++);
	ip->ip_off   = htons(IP_FLAGS_DFRAG);	/* Don't fragment */
	ip->ip_ttl   = 255;
	ip->ip_sum   = 0;
	/* already in network byte order */
	net_copy_ip((void *)&ip->ip_src, &source);
	/* already in network byte order */
	net_copy_ip((void *)&ip->ip_dst, &dest);
}

What's changed with gcc7 is that it now merges the writes to the first
three fields (occupying 4 bytes) into a single 32-bit store.  There is
nothing wrong with this.

Now struct ip_udp_hdr includes 32-bit fields, so it's natural alignment
is 4 bytes.  If the 'pkt' argument is not adequately aligned, the cast
in the first line of the function becomes invalid.  Judging by the
register dump and disassembly, the pointer is indeed not thusly
aligned, and this is an error.

The misaligned pointer should still not cause a hardware abort, however,
on ARMv6 or later which permits unaligned addresses with the STR
instruction.  That is unless some idiot has gone and enabled strict
alignment checking again despite this being against all common sense.
There was a lengthy argument about this a few years ago, ultimately
resulting in the proper settings being put in place.

What hardware did this happen on?  If it was on ARMv5, adding the packed
attribute is probably the correct fix.  If it was ARMv6 or later,
something else is broken as well.

-- 
Måns Rullgård