diff mbox series

PowerPC/VDSO: Correct call frame information

Message ID 20180914034004.GP3174@bubble.grove.modra.org (mailing list archive)
State Accepted
Commit 56d20861c027498b5a1112b4f9f05b56d906fdda
Headers show
Series PowerPC/VDSO: Correct call frame information | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied
snowpatch_ozlabs/checkpatch success Test checkpatch on branch next
snowpatch_ozlabs/build-ppc64le success Test build-ppc64le on branch next
snowpatch_ozlabs/build-ppc64be success Test build-ppc64be on branch next
snowpatch_ozlabs/build-ppc64e success Test build-ppc64e on branch next
snowpatch_ozlabs/build-ppc32 success Test build-ppc32 on branch next

Commit Message

Alan Modra Sept. 14, 2018, 3:40 a.m. UTC 
Call Frame Information is used by gdb for back-traces and inserting
breakpoints on function return for the "finish" command.  This failed
when inside __kernel_clock_gettime.  More concerning than difficulty
debugging is that CFI is also used by stack frame unwinding code to
implement exceptions.  If you have an app that needs to handle
asynchronous exceptions for some reason, and you are unlucky enough to
get one inside the VDSO time functions, your app will crash.

What's wrong:  There is control flow in __kernel_clock_gettime that
reaches label 99 without saving lr in r12.  CFI info however is
interpreted by the unwinder without reference to control flow: It's a
simple matter of "Execute all the CFI opcodes up to the current
address".  That means the unwinder thinks r12 contains the return
address at label 99.  Disabuse it of that notion by resetting CFI for
the return address at label 99.

Note that the ".cfi_restore lr" could have gone anywhere from the
"mtlr r12" a few instructions earlier to the instruction at label 99.
I put the CFI as late as possible, because in general that's best
practice (and if possible grouped with other CFI in order to reduce
the number of CFI opcodes executed when unwinding).  Using r12 as the
return address is perfectly fine after the "mtlr r12" since r12 on
that code path still contains the return address.

__get_datapage also has a CFI error.  That function temporarily saves
lr in r0, and reflects that fact with ".cfi_register lr,r0".  A later
use of r0 means the CFI at that point isn't correct, as r0 no longer
contains the return address.  Fix that too.

Signed-off-by: Alan Modra <amodra@gmail.com>
Tested-by: Reza Arbab <arbab@linux.ibm.com>

Comments

Michael Ellerman Sept. 20, 2018, 4:21 a.m. UTC | #1 
On Fri, 2018-09-14 at 03:40:04 UTC, Alan Modra wrote:
> Call Frame Information is used by gdb for back-traces and inserting
> breakpoints on function return for the "finish" command.  This failed
> when inside __kernel_clock_gettime.  More concerning than difficulty
> debugging is that CFI is also used by stack frame unwinding code to
> implement exceptions.  If you have an app that needs to handle
> asynchronous exceptions for some reason, and you are unlucky enough to
> get one inside the VDSO time functions, your app will crash.
> 
> What's wrong:  There is control flow in __kernel_clock_gettime that
> reaches label 99 without saving lr in r12.  CFI info however is
> interpreted by the unwinder without reference to control flow: It's a
> simple matter of "Execute all the CFI opcodes up to the current
> address".  That means the unwinder thinks r12 contains the return
> address at label 99.  Disabuse it of that notion by resetting CFI for
> the return address at label 99.
> 
> Note that the ".cfi_restore lr" could have gone anywhere from the
> "mtlr r12" a few instructions earlier to the instruction at label 99.
> I put the CFI as late as possible, because in general that's best
> practice (and if possible grouped with other CFI in order to reduce
> the number of CFI opcodes executed when unwinding).  Using r12 as the
> return address is perfectly fine after the "mtlr r12" since r12 on
> that code path still contains the return address.
> 
> __get_datapage also has a CFI error.  That function temporarily saves
> lr in r0, and reflects that fact with ".cfi_register lr,r0".  A later
> use of r0 means the CFI at that point isn't correct, as r0 no longer
> contains the return address.  Fix that too.
> 
> Signed-off-by: Alan Modra <amodra@gmail.com>
> Tested-by: Reza Arbab <arbab@linux.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/56d20861c027498b5a1112b4f9f05b

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/vdso32/datapage.S b/arch/powerpc/kernel/vdso32/datapage.S
index 3745113fcc65..2a7eb5452aba 100644
--- a/arch/powerpc/kernel/vdso32/datapage.S
+++ b/arch/powerpc/kernel/vdso32/datapage.S
@@ -37,6 +37,7 @@  data_page_branch:
 	mtlr	r0
 	addi	r3, r3, __kernel_datapage_offset-data_page_branch
 	lwz	r0,0(r3)
+  .cfi_restore lr
 	add	r3,r0,r3
 	blr
   .cfi_endproc
diff --git a/arch/powerpc/kernel/vdso32/gettimeofday.S b/arch/powerpc/kernel/vdso32/gettimeofday.S
index 769c2624e0a6..1e0bc5955a40 100644
--- a/arch/powerpc/kernel/vdso32/gettimeofday.S
+++ b/arch/powerpc/kernel/vdso32/gettimeofday.S
@@ -139,6 +139,7 @@  V_FUNCTION_BEGIN(__kernel_clock_gettime)
 	 */
 99:
 	li	r0,__NR_clock_gettime
+  .cfi_restore lr
 	sc
 	blr
   .cfi_endproc
diff --git a/arch/powerpc/kernel/vdso64/datapage.S b/arch/powerpc/kernel/vdso64/datapage.S
index abf17feffe40..bf9668691511 100644
--- a/arch/powerpc/kernel/vdso64/datapage.S
+++ b/arch/powerpc/kernel/vdso64/datapage.S
@@ -37,6 +37,7 @@  data_page_branch:
 	mtlr	r0
 	addi	r3, r3, __kernel_datapage_offset-data_page_branch
 	lwz	r0,0(r3)
+  .cfi_restore lr
 	add	r3,r0,r3
 	blr
   .cfi_endproc
diff --git a/arch/powerpc/kernel/vdso64/gettimeofday.S b/arch/powerpc/kernel/vdso64/gettimeofday.S
index c002adcc694c..a4ed9edfd5f0 100644
--- a/arch/powerpc/kernel/vdso64/gettimeofday.S
+++ b/arch/powerpc/kernel/vdso64/gettimeofday.S
@@ -169,6 +169,7 @@  V_FUNCTION_BEGIN(__kernel_clock_gettime)
 	 */
 99:
 	li	r0,__NR_clock_gettime
+  .cfi_restore lr
 	sc
 	blr
   .cfi_endproc