sh:fixed issue in xchg_u32 function when val==r15.

sh:fixed issue in xchg_u32 function when val==r15.

[Srinivas KANDAGATLA]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

Hi All,

Recently we have discovered a bug in xchg_u32 function of GUSA_RB feature.
This function breaks if one of the input parameter 'val' is r15.

Kernel version: Linux 2.6.39-rc2 and previous versions too.

Here is the part of disassembly code of exit_mm function in 
kernel/exit.c which calls xchg_u32 and looks
like: (generated by 'sh4-linux-objdump -d kernel/exit.o' )

     168:    02 c7           mova    174 <exit_mm+0x54>,r0
     16a:    09 00           nop   
     16c:    f3 61           mov    r15,r1
     16e:    fc ef           mov    #-4,r15
     170:    22 67           mov.l    @r2,r7
     172:    f2 22           mov.l    r15,@r2
     174:    13 6f           mov    r1,r15
     176:    01 e3           mov    #1,r3
     178:    7f 1b           mov.l    r7,@(60,r11)
     17a:    d3 62           mov    r13,r2
     17c:    02 c7           mova    188 <exit_mm+0x68>,r0

in '16e' we can see that r15 was changed to -4 and at '172' we can see 
it gets assigned to @r2.

Originally this bug was discovered as part of stlinux bugzilla triage.
This issue can be easily reproduced with a simple multi-threaded 
test-app attached, the kernel crashes while cleaning up its memory 
descriptor.

Fix is making val an input/output constraint so the compiler cannot pass 
r15 directly and must use a temporary register.

After applying the patch the code in exit_mm() looks like:

    168:    02 c7           mova    174 <exit_mm+0x54>,r0
     16a:    09 00           nop   
     16c:    f3 61           mov    r15,r1
     16e:    fc ef           mov    #-4,r15
     170:    22 67           mov.l    @r2,r7
     172:    32 22           mov.l    r3,@r2
     174:    13 6f           mov    r1,r15
     176:    01 e3           mov    #1,r3
     178:    7f 1b           mov.l    r7,@(60,r11)
     17a:    e3 62           mov    r14,r2
     17c:    02 c7           mova    188 <exit_mm+0x68>,r0

Patch is attached

Thanks,
srini

[-- Attachment #2: main.c --]
[-- Type: text/x-csrc, Size: 708 bytes --]

#include <pthread.h>
#include <stdio.h>
#define NTHREADS 40
 
void *dodump(void *threadid)
{
	char *ptr = 0x1212121;
	printf("DUmp... %s \n", ptr);
}
 
void *dowork(void *threadid)
{
	printf("\nThread %d started \n", (int )threadid);
	while(1);
}
 
int main(int argc, char *argv[])
{
   pthread_t threads[NTHREADS];
   int rc;
   long t;
 
   for(t=0; t<NTHREADS; t++){
	if(t==25)
	      	rc = pthread_create(&threads[t], NULL, dodump, (void *)t);
	else
	      rc = pthread_create(&threads[t], NULL, dowork, (void *)t);
	
      if (rc){
         printf("ERROR; return code from pthread_create() is %d\n", rc);
         exit(-1);
      }
   }
   printf("Created %ld threads.\n", t);
   pthread_exit(NULL);
}


[-- Attachment #3: 0001-sh-fixed-issue-in-xchg_u32-function.patch --]
[-- Type: text/x-patch, Size: 1632 bytes --]

From da8a5909fed9bb801b7d9b175330f4e205b9dd61 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Date: Thu, 7 Apr 2011 13:34:43 +0100
Subject: [PATCH sh-2.6.32.y] sh: fixed issue in xchg_u32 function.

This patch addresses a use case where input parameter (val) to xchg_u32
inline asm function is equal to r15. If val == r15 then xchg_u32 always
sets m to -4(0xfffffffc). In particular code in kernel/exit.c exit_mm()
will hit this bug.

This patch makes adds val to input/output constraint so that compiler
cannot pass r15 directly and must use a temporary register instead.

Without this patch a SEGFAULT in multithreaded program will crash the
kernel.

Originally this bug was discovered as part of stlinux bugzilla##11229
triage.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Reviewed-by: Stuart Menefy <stuart.menefy@st.com>
---
 arch/sh/include/asm/cmpxchg-grb.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/sh/include/asm/cmpxchg-grb.h b/arch/sh/include/asm/cmpxchg-grb.h
index 4676bf5..fc3deb6 100644
--- a/arch/sh/include/asm/cmpxchg-grb.h
+++ b/arch/sh/include/asm/cmpxchg-grb.h
@@ -15,8 +15,10 @@ static inline unsigned long xchg_u32(volatile u32 *m, unsigned long val)
 		"   mov.l   %2,   @%1     \n\t" /* store new value */
 		"1: mov     r1,   r15     \n\t" /* LOGOUT */
 		: "=&r" (retval),
-		  "+r"  (m)
-		: "r"   (val)
+		  "+r"  (m),
+		  "+r"  (val)	/* when val == r15 this function will not work as expected.
+				 * So val is added to output constriants */
+		:
 		: "memory", "r0", "r1");
 
 	return retval;
-- 
1.6.3.3

Re: sh:fixed issue in xchg_u32 function when val==r15.

[Paul Mundt]

On Thu, Apr 07, 2011 at 01:57:09PM +0100, Srinivas KANDAGATLA wrote:
> Recently we have discovered a bug in xchg_u32 function of GUSA_RB feature.
> This function breaks if one of the input parameter 'val' is r15.
> 
>     168:    02 c7           mova    174 <exit_mm+0x54>,r0
>     16a:    09 00           nop   
>     16c:    f3 61           mov    r15,r1
>     16e:    fc ef           mov    #-4,r15

The -4 here is part of the gUSA login sequence, so if you're seeing the
problem with gUSA based xchg_u32 it seems like you're going to have to
apply the same fix for all gUSA rollback users.

Re: sh:fixed issue in xchg_u32 function when val==r15.

[Srinivas KANDAGATLA]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

Hi Paul,
Thanks for the suggestion.

Attached is the reworked patch for potential gUSA rollback users.

Thanks,
srini


Paul Mundt wrote:
> On Thu, Apr 07, 2011 at 01:57:09PM +0100, Srinivas KANDAGATLA wrote:
>   
>> Recently we have discovered a bug in xchg_u32 function of GUSA_RB feature.
>> This function breaks if one of the input parameter 'val' is r15.
>>
>>     168:    02 c7           mova    174 <exit_mm+0x54>,r0
>>     16a:    09 00           nop   
>>     16c:    f3 61           mov    r15,r1
>>     16e:    fc ef           mov    #-4,r15
>>     
>
> The -4 here is part of the gUSA login sequence, so if you're seeing the
> problem with gUSA based xchg_u32 it seems like you're going to have to
> apply the same fix for all gUSA rollback users.
>   


[-- Attachment #2: 0001-sh-fixed-issue-in-xchg_u32-xchg_u8-and-__cmpxchg_u32.patch --]
[-- Type: text/x-patch, Size: 3205 bytes --]

From 49cab63ad11688c9b526dc8d14318cad5351cf47 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Date: Wed, 13 Apr 2011 16:22:03 +0100
Subject: [PATCH sh-2.6.32.y] sh: fixed issue in xchg_u32, xchg_u8 and __cmpxchg_u32 function.

This patch addresses a use-case when one of the input parameter to
xchg_u32 or xchg_u8 or __cmpxchg_u32 inline asm function is equal to
r15(stack-pointer).

For example:
Code in exit_mm() function passes address of structure(val) to xchg_u32
which is basically a stack pointer(r15). xchg_u32 always sets m to
-4(0xfffffffc). Which is incorrect(Actual Bug).

Considering there could be one or more instances of this kind in the
current or future code, this patch is must if gUSA is used.

This patch adds input parameters to input/output constraint so that
compiler cannot pass r15 directly and must use a temporary register
instead.
Also reorders the parameters in __cmpxchg_u32 to fit to the order they
are declared.

Originally this bug was discovered as part of stlinux bugzilla##11229
triage.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Reviewed-by: Stuart Menefy <stuart.menefy@st.com>
---
 arch/sh/include/asm/cmpxchg-grb.h |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/arch/sh/include/asm/cmpxchg-grb.h b/arch/sh/include/asm/cmpxchg-grb.h
index 4676bf5..509cd33 100644
--- a/arch/sh/include/asm/cmpxchg-grb.h
+++ b/arch/sh/include/asm/cmpxchg-grb.h
@@ -15,8 +15,10 @@ static inline unsigned long xchg_u32(volatile u32 *m, unsigned long val)
 		"   mov.l   %2,   @%1     \n\t" /* store new value */
 		"1: mov     r1,   r15     \n\t" /* LOGOUT */
 		: "=&r" (retval),
-		  "+r"  (m)
-		: "r"   (val)
+		  "+r"  (m),
+		  "+r"  (val) /* if val == r15 function doesn' work as expected
+				* So val is added to output constriants */
+		:
 		: "memory", "r0", "r1");
 
 	return retval;
@@ -36,8 +38,10 @@ static inline unsigned long xchg_u8(volatile u8 *m, unsigned long val)
 		"   mov.b   %2,   @%1     \n\t" /* store new value */
 		"1: mov     r1,   r15     \n\t" /* LOGOUT */
 		: "=&r" (retval),
-		  "+r"  (m)
-		: "r"   (val)
+		  "+r"  (m),
+		  "+r"  (val) /* if val == r15 function doesn' work as expected
+				* So val is added to output constriants */
+		:
 		: "memory" , "r0", "r1");
 
 	return retval;
@@ -54,13 +58,14 @@ static inline unsigned long __cmpxchg_u32(volatile int *m, unsigned long old,
 		"   nop                   \n\t"
 		"   mov    r15,   r1      \n\t" /* r1 = saved sp */
 		"   mov    #-8,   r15     \n\t" /* LOGIN */
-		"   mov.l  @%1,   %0      \n\t" /* load  old value */
-		"   cmp/eq  %0,   %2      \n\t"
+		"   mov.l  @%3,   %0      \n\t" /* load  old value */
+		"   cmp/eq  %0,   %1      \n\t"
 		"   bf            1f      \n\t" /* if not equal */
-		"   mov.l   %3,   @%1     \n\t" /* store new value */
+		"   mov.l   %2,   @%3     \n\t" /* store new value */
 		"1: mov     r1,   r15     \n\t" /* LOGOUT */
-		: "=&r" (retval)
-		:  "r"  (m), "r"  (old), "r"  (new)
+		: "=&r" (retval),
+		  "+r"  (old), "+r"  (new) /* old or new can be r15 */
+		:  "r"  (m)
 		: "memory" , "r0", "r1", "t");
 
 	return retval;
-- 
1.6.3.3

Re: sh:fixed issue in xchg_u32 function when val==r15.

[Paul Mundt]

On Tue, Apr 19, 2011 at 05:28:47PM +0100, Srinivas KANDAGATLA wrote:
> Attached is the reworked patch for potential gUSA rollback users.
> 
Sorry for taking so long to get back to this, it's been in my review
queue for awhile now but other things kept getting in the way.

This looks good for the cmpxchg bits, but I suspect we also have to make
the same fixes for atomic-grb.h and bitops-grb.h.

I've applied your patch now and will send it off for -rc3. I expect
you'll simply send incremental patches for the other two if necessary,
after which we can make sure that it also all finds its way back to
-stable.