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

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

#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);
}

From da8a5909fed9bb801b7d9b175330f4e205b9dd61 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <[email protected]>
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.

---
 arch/sh/include/asm/cmpxchg-grb.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index 4676bf5..fc3deb6 100644
@@ -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

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.

Hi Paul,
Thanks for the suggestion.

Attached is the reworked patch for potential gUSA rollback users.

Thanks,
srini

From 49cab63ad11688c9b526dc8d14318cad5351cf47 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <[email protected]>
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.

---
 arch/sh/include/asm/cmpxchg-grb.h |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

index 4676bf5..509cd33 100644
@@ -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

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.