Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1537124 > unrolled thread
| Started by | Abel Vesa <abelvesa@linux.com> |
|---|---|
| First post | 2016-12-06 18:10 +0100 |
| Last post | 2016-12-07 13:20 +0100 |
| Articles | 5 — 5 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 4/7] arm: Add ftrace with regs support Abel Vesa <abelvesa@linux.com> - 2016-12-06 18:10 +0100
Re: [PATCH 4/7] arm: Add ftrace with regs support Steven Rostedt <rostedt@goodmis.org> - 2016-12-07 03:50 +0100
Re: [PATCH 4/7] arm: Add ftrace with regs support Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-12-07 12:00 +0100
Re: [PATCH 4/7] arm: Add ftrace with regs support Robin Murphy <robin.murphy@arm.com> - 2016-12-07 13:00 +0100
Re: [PATCH 4/7] arm: Add ftrace with regs support Abel Vesa <abelvesa@gmail.com> - 2016-12-07 13:20 +0100
| From | Abel Vesa <abelvesa@linux.com> |
|---|---|
| Date | 2016-12-06 18:10 +0100 |
| Subject | [PATCH 4/7] arm: Add ftrace with regs support |
| Message-ID | <sLrpE-4T9-17@gated-at.bofh.it> |
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}
+#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
--
2.7.4
[toc] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-12-07 03:50 +0100 |
| Message-ID | <sLAsV-22T-13@gated-at.bofh.it> |
| In reply to | #1537124 |
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
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@armlinux.org.uk> |
|---|---|
| Date | 2016-12-07 12:00 +0100 |
| Message-ID | <sLI79-7uU-61@gated-at.bofh.it> |
| In reply to | #1537124 |
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.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2016-12-07 13:00 +0100 |
| Message-ID | <sLJ3b-83N-3@gated-at.bofh.it> |
| In reply to | #1537124 |
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
>
[toc] | [prev] | [next] | [standalone]
| From | Abel Vesa <abelvesa@gmail.com> |
|---|---|
| Date | 2016-12-07 13:20 +0100 |
| Message-ID | <sLJmx-8sl-1@gated-at.bofh.it> |
| In reply to | #1537698 |
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
> >
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web