| Message ID | 20180913232704.GL3174@bubble.grove.modra.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Correct PowerPC VDSO call frame info | expand |
| 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 |
Alan, Thanks for the patch... A few minor comments... > Re: [PATCH] Correct PowerPC VDSO call frame info Our convention is to add powerpc: to the start. ie [PATCH] powerpc: Correct VDSO call frame info or even: [PATCH] powerpc/vdso: Correct call frame info On Fri, 2018-09-14 at 08:57 +0930, Alan Modra wrote: > 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. Can you expand on CFI == Call Frame Information in the commit message. Not a common term for us mere kernel developers? > 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. Can you describe the problem this fixes at a high level? ie This fixes doing a stack unwind with gdb when in __kernel_clock_gettime. Mikey > Signed-off-by: Alan Modra <amodra@gmail.com> > > 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 >
On Fri, Sep 14, 2018 at 08:57:04AM +0930, Alan Modra wrote: >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. Thanks for this! It looks like v2 will just be a commit log change, so feel free to carry over my Tested-by: Reza Arbab <arbab@linux.ibm.com>
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
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>