diff mbox series

asm/head: Fix comparison in opal_entry for switching to emergency

Message ID 20180427090007.19780-1-vaibhav@linux.vnet.ibm.com
State Accepted
Headers show
Series asm/head: Fix comparison in opal_entry for switching to emergency | expand

Commit Message

Vaibhav Jain April 27, 2018, 9 a.m. UTC
Commit 3fdd2629516d ("core/opal: Emergency stack for re-entry")
introduced an emergency stack for re-entrant OPAL calls. A branch was
added in opal_entry() that switches to emergency stack checks if
current thread is already in an active opal call. However the
conditional branch that checks the value cpu_thread->in_opal_call is
reverse forcing the use of emergency stack in even in non re-entrant
cases.

This causes a opal stack guard routine __mcount_stack_check() to
falsely assume that stack is overflown as stack pointer of
EMERGENCY_STACK is compared against the bounds of NORMAL_STACK,
forcing the function to call abort() with an error message of this
form:

 INIT: Starting kernel at 0x20010000, fdt at 0x3073f708 53664 bytes)
 CPU 0004 Stack overflow detected ! pc=3001d8ec sp=31c27c90 (gap=30488) token=70
 Aborting!
 CPU 0004 Backtrace:
 S: 0000000031c27b20 R: 000000003001ca2c E ._abort+0x60
 S: 0000000031c27bb0 R: 0000000030013e10 E .__mcount_stack_check+0x168
 S: 0000000031c27c90 R: 000000003001d8ec E .opal_entry_check+0x1c
 S: 0000000031c27d20 R: 00000000300051a4 E opal_entry+0xf4
 --- OPAL call token: 0x46 caller R1: 0xc0000000011e3e50 ---

So this patch update the 'bne' branch in opal_entry() to 'bgt' branch
so that switch to emergency stack only happens when current
cpu_thread->is_opal_call is greater than 1 indicating an re-entrant
opal call.

Fixes: 3fdd2629516d ("core/opal: Emergency stack for re-entry")
Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Signed-off-by: Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com>
---
 asm/head.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicholas Piggin April 29, 2018, 3:58 a.m. UTC | #1
On Fri, 27 Apr 2018 14:30:07 +0530
Vaibhav Jain <vaibhav@linux.vnet.ibm.com> wrote:

> Commit 3fdd2629516d ("core/opal: Emergency stack for re-entry")
> introduced an emergency stack for re-entrant OPAL calls. A branch was
> added in opal_entry() that switches to emergency stack checks if
> current thread is already in an active opal call. However the
> conditional branch that checks the value cpu_thread->in_opal_call is
> reverse forcing the use of emergency stack in even in non re-entrant
> cases.
> 
> This causes a opal stack guard routine __mcount_stack_check() to
> falsely assume that stack is overflown as stack pointer of
> EMERGENCY_STACK is compared against the bounds of NORMAL_STACK,
> forcing the function to call abort() with an error message of this
> form:
> 
>  INIT: Starting kernel at 0x20010000, fdt at 0x3073f708 53664 bytes)
>  CPU 0004 Stack overflow detected ! pc=3001d8ec sp=31c27c90 (gap=30488) token=70
>  Aborting!
>  CPU 0004 Backtrace:
>  S: 0000000031c27b20 R: 000000003001ca2c E ._abort+0x60
>  S: 0000000031c27bb0 R: 0000000030013e10 E .__mcount_stack_check+0x168
>  S: 0000000031c27c90 R: 000000003001d8ec E .opal_entry_check+0x1c
>  S: 0000000031c27d20 R: 00000000300051a4 E opal_entry+0xf4
>  --- OPAL call token: 0x46 caller R1: 0xc0000000011e3e50 ---
> 
> So this patch update the 'bne' branch in opal_entry() to 'bgt' branch
> so that switch to emergency stack only happens when current
> cpu_thread->is_opal_call is greater than 1 indicating an re-entrant
> opal call.
> 
> Fixes: 3fdd2629516d ("core/opal: Emergency stack for re-entry")
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> Signed-off-by: Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com>

Huh, yeah that looks good, thanks. I'm trying to think how this ever
worked, I think the error was introduced (by me) when this was rebased
onto the patch that moved the quiesce code up into asm.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  asm/head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/asm/head.S b/asm/head.S
> index 48d3acb0..2cf4a6b2 100644
> --- a/asm/head.S
> +++ b/asm/head.S
> @@ -1023,7 +1023,7 @@ opal_entry:
>  	cmpwi	%r11,1
>  
>  	mfspr	%r12,SPR_PIR
> -	beq	5f
> +	bgt	5f
>  	GET_STACK(%r12,%r12)
>  	b	6f
>  5:
Stewart Smith April 30, 2018, 7:37 a.m. UTC | #2
Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:
> Commit 3fdd2629516d ("core/opal: Emergency stack for re-entry")
> introduced an emergency stack for re-entrant OPAL calls. A branch was
> added in opal_entry() that switches to emergency stack checks if
> current thread is already in an active opal call. However the
> conditional branch that checks the value cpu_thread->in_opal_call is
> reverse forcing the use of emergency stack in even in non re-entrant
> cases.
>
> This causes a opal stack guard routine __mcount_stack_check() to
> falsely assume that stack is overflown as stack pointer of
> EMERGENCY_STACK is compared against the bounds of NORMAL_STACK,
> forcing the function to call abort() with an error message of this
> form:
>
>  INIT: Starting kernel at 0x20010000, fdt at 0x3073f708 53664 bytes)
>  CPU 0004 Stack overflow detected ! pc=3001d8ec sp=31c27c90 (gap=30488) token=70
>  Aborting!
>  CPU 0004 Backtrace:
>  S: 0000000031c27b20 R: 000000003001ca2c E ._abort+0x60
>  S: 0000000031c27bb0 R: 0000000030013e10 E .__mcount_stack_check+0x168
>  S: 0000000031c27c90 R: 000000003001d8ec E .opal_entry_check+0x1c
>  S: 0000000031c27d20 R: 00000000300051a4 E opal_entry+0xf4
>  --- OPAL call token: 0x46 caller R1: 0xc0000000011e3e50 ---
>
> So this patch update the 'bne' branch in opal_entry() to 'bgt' branch
> so that switch to emergency stack only happens when current
> cpu_thread->is_opal_call is greater than 1 indicating an re-entrant
> opal call.
>
> Fixes: 3fdd2629516d ("core/opal: Emergency stack for re-entry")
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> Signed-off-by: Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com>
> ---
>  asm/head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Ahhh, the old not-testing-with-STACK_CHECK=1-bites-again trick. Doh.

Thanks for the patch, merged to master as of 8ed37072c07ee0c8be2ed8d9e2ede19333856e86
diff mbox series

Patch

diff --git a/asm/head.S b/asm/head.S
index 48d3acb0..2cf4a6b2 100644
--- a/asm/head.S
+++ b/asm/head.S
@@ -1023,7 +1023,7 @@  opal_entry:
 	cmpwi	%r11,1
 
 	mfspr	%r12,SPR_PIR
-	beq	5f
+	bgt	5f
 	GET_STACK(%r12,%r12)
 	b	6f
 5: