| Message ID | 1481043967-15602-5-git-send-email-abelvesa@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 6 Dec 2016 17:06:04 +0000 Abel Vesa <abelvesa@linux.com> wrote: > This adds __ftrace_regs_caller which, unlike __ftrace_caller, > adds register saving/restoring and livepatch handling if > the pc register gets modified by klp_ftrace_handler. > > Signed-off-by: Abel Vesa <abelvesa@linux.com> > --- > arch/arm/kernel/entry-ftrace.S | 49 > ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 > insertions(+) > > diff --git a/arch/arm/kernel/entry-ftrace.S > b/arch/arm/kernel/entry-ftrace.S index c73c403..b6ada5c 100644 > --- a/arch/arm/kernel/entry-ftrace.S > +++ b/arch/arm/kernel/entry-ftrace.S > @@ -92,6 +92,46 @@ > 2: mcount_exit > .endm > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + > +.macro __ftrace_regs_caller suffix > + > + stmdb sp!, {r0-r15} > + mov r3, sp > + > + ldr r10, [sp, #60] > + > + mcount_get_lr r1 @ lr of instrumented > func > + mcount_adjust_addr r0, lr @ > instrumented function + > + .globl ftrace_regs_call\suffix > +ftrace_regs_call\suffix: > + bl ftrace_stub > + > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + .globl ftrace_regs_graph_call\suffix > +ftrace_regs_graph_call\suffix: > + mov r0, r0 > +#endif So basically what the below does is to undo the mcount prologue and recall the new function as the old function is called. > +#ifdef CONFIG_LIVEPATCH > + ldr r0, [sp, #60] > + cmp r0, r10 > + beq ftrace_regs_caller_end > + ldmia sp!, {r0-r12} > + add sp, sp, #8 > + ldmia sp!, {r11} > + sub sp, r12, #16 > + str r11, [sp, #12] > + ldmia sp!, {r11, r12, lr, pc} But the above really could do with a lot of comments to explain exactly what it is doing. I don't condemn or condone this code. I'm just happy I don't have to maintain it. -- Steve > +#endif > +ftrace_regs_caller_end\suffix: > + ldmia sp!, {r0-r14} > + add sp, sp, #4 > + mov pc, lr > +.endm > + > +#endif > + > .macro __ftrace_caller suffix > mcount_enter > > @@ -212,6 +252,15 @@ UNWIND(.fnstart) > __ftrace_caller > UNWIND(.fnend) > ENDPROC(ftrace_caller) > + > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > +ENTRY(ftrace_regs_caller) > +UNWIND(.fnstart) > + __ftrace_regs_caller > +UNWIND(.fnend) > +ENDPROC(ftrace_regs_caller) > +#endif > + > #endif > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER
On Tue, Dec 06, 2016 at 05:06:04PM +0000, Abel Vesa wrote: > This adds __ftrace_regs_caller which, unlike __ftrace_caller, > adds register saving/restoring and livepatch handling if > the pc register gets modified by klp_ftrace_handler. > > Signed-off-by: Abel Vesa <abelvesa@linux.com> > --- > arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > index c73c403..b6ada5c 100644 > --- a/arch/arm/kernel/entry-ftrace.S > +++ b/arch/arm/kernel/entry-ftrace.S > @@ -92,6 +92,46 @@ > 2: mcount_exit > .endm > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + > +.macro __ftrace_regs_caller suffix > + > + stmdb sp!, {r0-r15} > + mov r3, sp > + > + ldr r10, [sp, #60] > + > + mcount_get_lr r1 @ lr of instrumented func > + mcount_adjust_addr r0, lr @ instrumented function > + > + .globl ftrace_regs_call\suffix > +ftrace_regs_call\suffix: > + bl ftrace_stub > + > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + .globl ftrace_regs_graph_call\suffix > +ftrace_regs_graph_call\suffix: > + mov r0, r0 > +#endif > +#ifdef CONFIG_LIVEPATCH > + ldr r0, [sp, #60] > + cmp r0, r10 > + beq ftrace_regs_caller_end > + ldmia sp!, {r0-r12} > + add sp, sp, #8 > + ldmia sp!, {r11} > + sub sp, r12, #16 > + str r11, [sp, #12] > + ldmia sp!, {r11, r12, lr, pc} Some comments about the above stack manipulation (in the code) would be useful - remember, the contents of your cover letter is lost when the patches are applied. However, I'm not convinced yet that this doesn't unbalance the stack, unless the livepatching code pushes some extra registers onto it, particularly with the following unstacking code after the livepatch code.
Hi Abel, On 06/12/16 17:06, Abel Vesa wrote: > This adds __ftrace_regs_caller which, unlike __ftrace_caller, > adds register saving/restoring and livepatch handling if > the pc register gets modified by klp_ftrace_handler. > > Signed-off-by: Abel Vesa <abelvesa@linux.com> > --- > arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > index c73c403..b6ada5c 100644 > --- a/arch/arm/kernel/entry-ftrace.S > +++ b/arch/arm/kernel/entry-ftrace.S > @@ -92,6 +92,46 @@ > 2: mcount_exit > .endm > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > + > +.macro __ftrace_regs_caller suffix > + > + stmdb sp!, {r0-r15} As the kbuild robot reported, you can't do this. For ARM, it's unknown what the value stored for r13 will be, and for a Thumb-2 kernel the whole instruction is flat out unpredictable (i.e. it might just undef or anything). > + mov r3, sp > + > + ldr r10, [sp, #60] > + > + mcount_get_lr r1 @ lr of instrumented func > + mcount_adjust_addr r0, lr @ instrumented function > + > + .globl ftrace_regs_call\suffix > +ftrace_regs_call\suffix: > + bl ftrace_stub > + > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + .globl ftrace_regs_graph_call\suffix > +ftrace_regs_graph_call\suffix: > + mov r0, r0 > +#endif > +#ifdef CONFIG_LIVEPATCH > + ldr r0, [sp, #60] > + cmp r0, r10 > + beq ftrace_regs_caller_end > + ldmia sp!, {r0-r12} > + add sp, sp, #8 > + ldmia sp!, {r11} > + sub sp, r12, #16 > + str r11, [sp, #12] > + ldmia sp!, {r11, r12, lr, pc} > +#endif > +ftrace_regs_caller_end\suffix: > + ldmia sp!, {r0-r14} Again, the value of SP at this point is now unknown (regardless of whether what the value on memory was correct or not). Not good. Either use non-writeback addressing modes, or avoid saving/restoring SP at all (AFAICS it isn't necessary, since if the SP was changed in between, you then wouldn't know where to restore the saved registers from anyway). Robin. > + add sp, sp, #4 > + mov pc, lr > +.endm > + > +#endif > + > .macro __ftrace_caller suffix > mcount_enter > > @@ -212,6 +252,15 @@ UNWIND(.fnstart) > __ftrace_caller > UNWIND(.fnend) > ENDPROC(ftrace_caller) > + > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > +ENTRY(ftrace_regs_caller) > +UNWIND(.fnstart) > + __ftrace_regs_caller > +UNWIND(.fnend) > +ENDPROC(ftrace_regs_caller) > +#endif > + > #endif > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER >
On Wed, Dec 07, 2016 at 11:58:24AM +0000, Robin Murphy wrote: > Hi Abel, > > On 06/12/16 17:06, Abel Vesa wrote: > > This adds __ftrace_regs_caller which, unlike __ftrace_caller, > > adds register saving/restoring and livepatch handling if > > the pc register gets modified by klp_ftrace_handler. > > > > Signed-off-by: Abel Vesa <abelvesa@linux.com> > > --- > > arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > > index c73c403..b6ada5c 100644 > > --- a/arch/arm/kernel/entry-ftrace.S > > +++ b/arch/arm/kernel/entry-ftrace.S > > @@ -92,6 +92,46 @@ > > 2: mcount_exit > > .endm > > > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > > + > > +.macro __ftrace_regs_caller suffix > > + > > + stmdb sp!, {r0-r15} > > As the kbuild robot reported, you can't do this. For ARM, it's unknown > what the value stored for r13 will be, and for a Thumb-2 kernel the > whole instruction is flat out unpredictable (i.e. it might just undef or > anything). > > > + mov r3, sp > > + > > + ldr r10, [sp, #60] > > + > > + mcount_get_lr r1 @ lr of instrumented func > > + mcount_adjust_addr r0, lr @ instrumented function > > + > > + .globl ftrace_regs_call\suffix > > +ftrace_regs_call\suffix: > > + bl ftrace_stub > > + > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > > + .globl ftrace_regs_graph_call\suffix > > +ftrace_regs_graph_call\suffix: > > + mov r0, r0 > > +#endif > > +#ifdef CONFIG_LIVEPATCH > > + ldr r0, [sp, #60] > > + cmp r0, r10 > > + beq ftrace_regs_caller_end > > + ldmia sp!, {r0-r12} > > + add sp, sp, #8 > > + ldmia sp!, {r11} > > + sub sp, r12, #16 > > + str r11, [sp, #12] > > + ldmia sp!, {r11, r12, lr, pc} > > +#endif > > +ftrace_regs_caller_end\suffix: > > + ldmia sp!, {r0-r14} > > Again, the value of SP at this point is now unknown (regardless of > whether what the value on memory was correct or not). Not good. > > Either use non-writeback addressing modes, or avoid saving/restoring SP > at all (AFAICS it isn't necessary, since if the SP was changed in > between, you then wouldn't know where to restore the saved registers > from anyway). > > Robin. > Yep, as I said in the cover letter, I'll try to fix that in the next iteration of this patch. You're right, sp doesn't need to be pushed or popped. I'll send a another patch series which will include a fix for that tomorrow, latest. Thanks. > > + add sp, sp, #4 > > + mov pc, lr > > +.endm > > + > > +#endif > > + > > .macro __ftrace_caller suffix > > mcount_enter > > > > @@ -212,6 +252,15 @@ UNWIND(.fnstart) > > __ftrace_caller > > UNWIND(.fnend) > > ENDPROC(ftrace_caller) > > + > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > > +ENTRY(ftrace_regs_caller) > > +UNWIND(.fnstart) > > + __ftrace_regs_caller > > +UNWIND(.fnend) > > +ENDPROC(ftrace_regs_caller) > > +#endif > > + > > #endif > > > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > > >
diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S index c73c403..b6ada5c 100644 --- a/arch/arm/kernel/entry-ftrace.S +++ b/arch/arm/kernel/entry-ftrace.S @@ -92,6 +92,46 @@ 2: mcount_exit .endm +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS + +.macro __ftrace_regs_caller suffix + + stmdb sp!, {r0-r15} + mov r3, sp + + ldr r10, [sp, #60] + + mcount_get_lr r1 @ lr of instrumented func + mcount_adjust_addr r0, lr @ instrumented function + + .globl ftrace_regs_call\suffix +ftrace_regs_call\suffix: + bl ftrace_stub + +#ifdef CONFIG_FUNCTION_GRAPH_TRACER + .globl ftrace_regs_graph_call\suffix +ftrace_regs_graph_call\suffix: + mov r0, r0 +#endif +#ifdef CONFIG_LIVEPATCH + ldr r0, [sp, #60] + cmp r0, r10 + beq ftrace_regs_caller_end + ldmia sp!, {r0-r12} + add sp, sp, #8 + ldmia sp!, {r11} + sub sp, r12, #16 + str r11, [sp, #12] + ldmia sp!, {r11, r12, lr, pc} +#endif +ftrace_regs_caller_end\suffix: + ldmia sp!, {r0-r14} + add sp, sp, #4 + mov pc, lr +.endm + +#endif + .macro __ftrace_caller suffix mcount_enter @@ -212,6 +252,15 @@ UNWIND(.fnstart) __ftrace_caller UNWIND(.fnend) ENDPROC(ftrace_caller) + +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS +ENTRY(ftrace_regs_caller) +UNWIND(.fnstart) + __ftrace_regs_caller +UNWIND(.fnend) +ENDPROC(ftrace_regs_caller) +#endif + #endif #ifdef CONFIG_FUNCTION_GRAPH_TRACER
This adds __ftrace_regs_caller which, unlike __ftrace_caller, adds register saving/restoring and livepatch handling if the pc register gets modified by klp_ftrace_handler. Signed-off-by: Abel Vesa <abelvesa@linux.com> --- arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)