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 |
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:
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 --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: