diff mbox

[v2] arm: fix memset-related crashes caused by recent GCC (4.7.2) optimizations

Message ID alpine.LFD.2.03.1303110910540.28820@syhkavp.arg (mailing list archive)
State New, archived
Headers show

Commit Message

Nicolas Pitre March 11, 2013, 4:10 a.m. UTC
On Sun, 10 Mar 2013, Russell King - ARM Linux wrote:

> On Sun, Mar 10, 2013 at 06:06:11PM +0100, Alexander Holler wrote:
> > Am 07.03.2013 16:17, schrieb Russell King - ARM Linux:
> >> On Wed, Mar 06, 2013 at 08:15:17PM +0100, Dirk Behme wrote:
> >>> Am 11.02.2013 13:57, schrieb Ivan Djelic:
> >>>> Recent GCC versions (e.g. GCC-4.7.2) perform optimizations based on
> >>>> assumptions about the implementation of memset and similar functions.
> >>>> The current ARM optimized memset code does not return the value of
> >>>> its first argument, as is usually expected from standard implementations.
> >
> > I've just tried this patch with kernel 4.8.2 on an armv5-system where I  
> > use gcc 4.7.2 since several months and where most parts of the system  
> > are compiled with gcc 4.7.2 too.
> >
> > And I had at least one problem which manifested itself with
> 
> Yes, the patch _is_ wrong.  Reverted.  I was trusting Nicolas' review
> of it, but the patch is definitely wrong.

Worse: it is in v3.9-rc2 already.

Here's a fix.  Patch system?

---------- >8
Subject: fix the memset fix

Commit 455bd4c430b0 ("ARM: 7668/1: fix memset-related crashes caused by 
recent GCC (4.7.2) optimizations") attempted to fix a compliance issue 
with the memset return value.  However the memset itself was broken by 
that patch in the misaligned pointer case.

This fixes the above by branching over the entry code from the 
misaligned fixup code to avoid reloading the original pointer.

Also, because the function entry alignment is wrong in the Thumb mode 
compilation, that fixup code is moved to the end.

While at it, the entry instructions are slightly reworked to help dual 
issue pipelines.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---

Comments

Alexander Holler March 12, 2013, 10:06 a.m. UTC | #1
Am 11.03.2013 05:10, schrieb Nicolas Pitre:
> On Sun, 10 Mar 2013, Russell King - ARM Linux wrote:
>
>> On Sun, Mar 10, 2013 at 06:06:11PM +0100, Alexander Holler wrote:
>>> Am 07.03.2013 16:17, schrieb Russell King - ARM Linux:
>>>> On Wed, Mar 06, 2013 at 08:15:17PM +0100, Dirk Behme wrote:
>>>>> Am 11.02.2013 13:57, schrieb Ivan Djelic:
>>>>>> Recent GCC versions (e.g. GCC-4.7.2) perform optimizations based on
>>>>>> assumptions about the implementation of memset and similar functions.
>>>>>> The current ARM optimized memset code does not return the value of
>>>>>> its first argument, as is usually expected from standard implementations.
>>>
>>> I've just tried this patch with kernel 4.8.2 on an armv5-system where I
>>> use gcc 4.7.2 since several months and where most parts of the system
>>> are compiled with gcc 4.7.2 too.
>>>
>>> And I had at least one problem which manifested itself with
>>
>> Yes, the patch _is_ wrong.  Reverted.  I was trusting Nicolas' review
>> of it, but the patch is definitely wrong.
>
> Worse: it is in v3.9-rc2 already.
>
> Here's a fix.  Patch system?
>
> ---------- >8
> Subject: fix the memset fix
>
> Commit 455bd4c430b0 ("ARM: 7668/1: fix memset-related crashes caused by
> recent GCC (4.7.2) optimizations") attempted to fix a compliance issue
> with the memset return value.  However the memset itself was broken by
> that patch in the misaligned pointer case.
>
> This fixes the above by branching over the entry code from the
> misaligned fixup code to avoid reloading the original pointer.
>
> Also, because the function entry alignment is wrong in the Thumb mode
> compilation, that fixup code is moved to the end.
>
> While at it, the entry instructions are slightly reworked to help dual
> issue pipelines.
>
> Signed-off-by: Nicolas Pitre <nico@linaro.org>
> ---
>
> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
> index d912e7397e..94b0650ea9 100644
> --- a/arch/arm/lib/memset.S
> +++ b/arch/arm/lib/memset.S
> @@ -14,31 +14,15 @@
>
>   	.text
>   	.align	5
> -	.word	0
> -
> -1:	subs	r2, r2, #4		@ 1 do we have enough
> -	blt	5f			@ 1 bytes to align with?
> -	cmp	r3, #2			@ 1
> -	strltb	r1, [ip], #1		@ 1
> -	strleb	r1, [ip], #1		@ 1
> -	strb	r1, [ip], #1		@ 1
> -	add	r2, r2, r3		@ 1 (r2 = r2 - (4 - r3))
> -/*
> - * The pointer is now aligned and the length is adjusted.  Try doing the
> - * memset again.
> - */
>
>   ENTRY(memset)
> -/*
> - * Preserve the contents of r0 for the return value.
> - */
> -	mov	ip, r0
> -	ands	r3, ip, #3		@ 1 unaligned?
> -	bne	1b			@ 1
> +	ands	r3, r0, #3		@ 1 unaligned?
> +	mov	ip, r0			@ preserve r0 as return value
> +	bne	6f			@ 1
>   /*
>    * we know that the pointer in ip is aligned to a word boundary.
>    */
> -	orr	r1, r1, r1, lsl #8
> +1:	orr	r1, r1, r1, lsl #8
>   	orr	r1, r1, r1, lsl #16
>   	mov	r3, r1
>   	cmp	r2, #16
> @@ -127,4 +111,13 @@ ENTRY(memset)
>   	tst	r2, #1
>   	strneb	r1, [ip], #1
>   	mov	pc, lr
> +
> +6:	subs	r2, r2, #4		@ 1 do we have enough
> +	blt	5b			@ 1 bytes to align with?
> +	cmp	r3, #2			@ 1
> +	strltb	r1, [ip], #1		@ 1
> +	strleb	r1, [ip], #1		@ 1
> +	strb	r1, [ip], #1		@ 1
> +	add	r2, r2, r3		@ 1 (r2 = r2 - (4 - r3))
> +	b	1b
>   ENDPROC(memset)
>

Tested-by: Alexander Holler

It works since 2 days on my busy 24/7 armv5-thingy without me having 
seen an error. A quick test on an armv7-system hasn't revealed any 
problems too.

Thanks,

Alexander
Florian Fainelli Feb. 26, 2014, 12:43 a.m. UTC | #2
2013-03-12 3:06 GMT-07:00 Alexander Holler <holler@ahsoftware.de>:
>
> Tested-by: Alexander Holler
>
> It works since 2 days on my busy 24/7 armv5-thingy without me having seen an
> error. A quick test on an armv7-system hasn't revealed any problems too.

This is an old thread, but 3.8.13 is still affected, not quite sure if
there will be a 3.8.14 release, but if there is, we should
definitively get:

ARM: 7668/1: fix memset-related crashes caused by recent GCC (4.7.2)
optimizations
ARM: 7670/1: fix the memset fix

included in 3.8.14.

Thanks!
Dirk Behme Feb. 26, 2014, 5:13 p.m. UTC | #3
Am 26.02.2014 01:43, schrieb Florian Fainelli:
> 2013-03-12 3:06 GMT-07:00 Alexander Holler <holler@ahsoftware.de>:
>>
>> Tested-by: Alexander Holler
>>
>> It works since 2 days on my busy 24/7 armv5-thingy without me having seen an
>> error. A quick test on an armv7-system hasn't revealed any problems too.
>
> This is an old thread, but 3.8.13 is still affected, not quite sure if
> there will be a 3.8.14 release, but if there is, we should
> definitively get:
>
> ARM: 7668/1: fix memset-related crashes caused by recent GCC (4.7.2)
> optimizations
> ARM: 7670/1: fix the memset fix
>
> included in 3.8.14.

Ubuntu is continuing support for some kernels with -stable patches:

https://lwn.net/Articles/550581/

http://kernel.ubuntu.com/git?p=ubuntu/linux.git

Having a short check in v3.8.13.18, this patch is not included there, 
yet. Maybe you want to ask them if they want to include it.

Best regards

Dirk
diff mbox

Patch

diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
index d912e7397e..94b0650ea9 100644
--- a/arch/arm/lib/memset.S
+++ b/arch/arm/lib/memset.S
@@ -14,31 +14,15 @@ 
 
 	.text
 	.align	5
-	.word	0
-
-1:	subs	r2, r2, #4		@ 1 do we have enough
-	blt	5f			@ 1 bytes to align with?
-	cmp	r3, #2			@ 1
-	strltb	r1, [ip], #1		@ 1
-	strleb	r1, [ip], #1		@ 1
-	strb	r1, [ip], #1		@ 1
-	add	r2, r2, r3		@ 1 (r2 = r2 - (4 - r3))
-/*
- * The pointer is now aligned and the length is adjusted.  Try doing the
- * memset again.
- */
 
 ENTRY(memset)
-/*
- * Preserve the contents of r0 for the return value.
- */
-	mov	ip, r0
-	ands	r3, ip, #3		@ 1 unaligned?
-	bne	1b			@ 1
+	ands	r3, r0, #3		@ 1 unaligned?
+	mov	ip, r0			@ preserve r0 as return value
+	bne	6f			@ 1
 /*
  * we know that the pointer in ip is aligned to a word boundary.
  */
-	orr	r1, r1, r1, lsl #8
+1:	orr	r1, r1, r1, lsl #8
 	orr	r1, r1, r1, lsl #16
 	mov	r3, r1
 	cmp	r2, #16
@@ -127,4 +111,13 @@  ENTRY(memset)
 	tst	r2, #1
 	strneb	r1, [ip], #1
 	mov	pc, lr
+
+6:	subs	r2, r2, #4		@ 1 do we have enough
+	blt	5b			@ 1 bytes to align with?
+	cmp	r3, #2			@ 1
+	strltb	r1, [ip], #1		@ 1
+	strleb	r1, [ip], #1		@ 1
+	strb	r1, [ip], #1		@ 1
+	add	r2, r2, r3		@ 1 (r2 = r2 - (4 - r3))
+	b	1b
 ENDPROC(memset)