Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1587994 > unrolled thread

[PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

Back to article view | Back to linux.kernel

Contents

  [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS Abel Vesa <abelvesa@linux.com> - 2017-02-25 00:00 +0100
    Re: [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS Nicolai Stange <nicstange@gmail.com> - 2017-02-27 17:00 +0100
      Re: [PATCHv4] arm: ftrace: Adds support for  CONFIG_DYNAMIC_FTRACE_WITH_REGS Abel Vesa <abelvesa@gmail.com> - 2017-02-28 02:00 +0100
        Re: [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS Nicolai Stange <nicstange@gmail.com> - 2017-02-28 12:10 +0100
          Re: [PATCHv4] arm: ftrace: Adds support for  CONFIG_DYNAMIC_FTRACE_WITH_REGS Abel Vesa <abelvesa@gmail.com> - 2017-02-28 12:30 +0100
            Re: [PATCHv4] arm: ftrace: Adds support for  CONFIG_DYNAMIC_FTRACE_WITH_REGS Abel Vesa <abelvesa@gmail.com> - 2017-02-28 13:00 +0100
            Re: [PATCHv4] arm: ftrace: Adds support for  CONFIG_DYNAMIC_FTRACE_WITH_REGS Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-28 13:10 +0100

#1587994 — [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

The DYNAMIC_FTRACE_WITH_REGS configuration makes it possible for a ftrace
operation to specify if registers need to saved/restored by the ftrace handler.
This is needed by kgraft and possibly other ftrace-based tools, and the ARM
architecture is currently lacking this feature. It would also be the first step
to support the "Kprobes-on-ftrace" optimization on ARM.

This patch introduces a new ftrace handler that stores the registers on the
stack before calling the next stage. The registers are restored from the stack
before going back to the instrumented function.

A side-effect of this patch is to activate the support for ftrace_modify_call()
as it defines ARCH_SUPPORTS_FTRACE_OPS for the ARM architecture.

Signed-off-by: Abel Vesa <abelvesa@linux.com>
---
 arch/arm/Kconfig               |  1 +
 arch/arm/include/asm/ftrace.h  |  4 ++
 arch/arm/kernel/entry-ftrace.S | 83 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/ftrace.c       | 37 +++++++++++++++++++
 4 files changed, 125 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index fda6a46..877df5b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -55,6 +55,7 @@ config ARM
 	select HAVE_DMA_API_DEBUG
 	select HAVE_DMA_CONTIGUOUS if MMU
 	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
 	select HAVE_EXIT_THREAD
 	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index 22b7311..f379881 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -1,6 +1,10 @@
 #ifndef _ASM_ARM_FTRACE
 #define _ASM_ARM_FTRACE
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#endif
+
 #ifdef CONFIG_FUNCTION_TRACER
 #define MCOUNT_ADDR		((unsigned long)(__gnu_mcount_nc))
 #define MCOUNT_INSN_SIZE	4 /* sizeof mcount call */
diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
index c73c403..3916dd6 100644
--- a/arch/arm/kernel/entry-ftrace.S
+++ b/arch/arm/kernel/entry-ftrace.S
@@ -92,12 +92,78 @@
 2:	mcount_exit
 .endm
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
+
+	add 	ip, sp, #12	@ move in IP the value of SP as it was
+				@ before the push {lr} of the mcount mechanism
+	stmdb	sp!, {ip,lr,pc}
+	stmdb	sp!, {r0-r11,lr}
+
+	@ stack content at this point:
+	@ 0  4          48   52       56   60   64    68       72
+	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |
+
+	mov r3, sp				@ struct pt_regs*
+	ldr r2, =function_trace_op
+	ldr r2, [r2]				@ pointer to the current
+						@ function tracing op
+	ldr	r1, [sp, #PT_REGS_SIZE]		@ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+
+	.globl ftrace_regs_call
+ftrace_regs_call:
+	bl	ftrace_stub
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_graph_regs_call
+ftrace_graph_regs_call:
+	mov	r0, r0
+#endif
+	@ pop saved regs
+	@ first, get the previous LR value from stack
+	ldr	lr, [sp, #PT_REGS_SIZE]
+	@ now pop the rest of the saved registers INCLUDING stack pointer
+	ldmia   sp, {r0-r11, ip, sp, pc}
+.endm
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+.macro __ftrace_graph_regs_caller
+
+	sub	r0, fp, #4		@ lr of instrumented routine (parent)
+
+	@ called from __ftrace_regs_caller
+	ldr     r1, [sp, #S_LR]		@ instrumented routine (func)
+	mcount_adjust_addr	r1, r1
+
+	sub	r2, r0, #4		@ frame pointer
+	bl	prepare_ftrace_return
+
+	@ pop registers saved in ftrace_regs_caller
+	@ first, get the previous LR value from stack
+	ldr	lr, [sp, #PT_REGS_SIZE]
+	@ now pop the rest of the saved registers INCLUDING stack pointer
+	ldmia   sp, {r0-r11, ip, sp, pc}
+.endm
+#endif
+#endif
+
 .macro __ftrace_caller suffix
 	mcount_enter
 
 	mcount_get_lr	r1			@ lr of instrumented func
 	mcount_adjust_addr	r0, lr		@ instrumented function
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	ldr r2, =function_trace_op
+	ldr r2, [r2]				@ pointer to the current
+						@ function tracing op
+	mov r3, #0				@ regs is NULL
+#endif
+
 	.globl ftrace_call\suffix
 ftrace_call\suffix:
 	bl	ftrace_stub
@@ -212,6 +278,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
@@ -220,6 +295,14 @@ UNWIND(.fnstart)
 	__ftrace_graph_caller
 UNWIND(.fnend)
 ENDPROC(ftrace_graph_caller)
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ENTRY(ftrace_graph_regs_caller)
+UNWIND(.fnstart)
+	__ftrace_graph_regs_caller
+UNWIND(.fnend)
+ENDPROC(ftrace_graph_regs_caller)
+#endif
 #endif
 
 .purgem mcount_enter
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 3f17594..cb0358c 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -139,6 +139,15 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 
 	ret = ftrace_modify_code(pc, 0, new, false);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	if (!ret) {
+		pc = (unsigned long)&ftrace_regs_call;
+		new = ftrace_call_replace(pc, (unsigned long)func);
+
+		ret = ftrace_modify_code(pc, 0, new, false);
+	}
+#endif
+
 #ifdef CONFIG_OLD_MCOUNT
 	if (!ret) {
 		pc = (unsigned long)&ftrace_call_old;
@@ -157,11 +166,29 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	unsigned long ip = rec->ip;
 
 	old = ftrace_nop_replace(rec);
+
+	new = ftrace_call_replace(ip, adjust_address(rec, addr));
+
+	return ftrace_modify_code(rec->ip, old, new, true);
+}
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+				unsigned long addr)
+{
+	unsigned long new, old;
+	unsigned long ip = rec->ip;
+
+	old = ftrace_call_replace(ip, adjust_address(rec, old_addr));
+
 	new = ftrace_call_replace(ip, adjust_address(rec, addr));
 
 	return ftrace_modify_code(rec->ip, old, new, true);
 }
 
+#endif
+
 int ftrace_make_nop(struct module *mod,
 		    struct dyn_ftrace *rec, unsigned long addr)
 {
@@ -229,6 +256,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 extern unsigned long ftrace_graph_call;
 extern unsigned long ftrace_graph_call_old;
 extern void ftrace_graph_caller_old(void);
+extern unsigned long ftrace_graph_regs_call;
+extern void ftrace_graph_regs_caller(void);
 
 static int __ftrace_modify_caller(unsigned long *callsite,
 				  void (*func) (void), bool enable)
@@ -251,6 +280,14 @@ static int ftrace_modify_graph_caller(bool enable)
 				     ftrace_graph_caller,
 				     enable);
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	if (!ret)
+		ret = __ftrace_modify_caller(&ftrace_graph_regs_call,
+				     ftrace_graph_regs_caller,
+				     enable);
+#endif
+
+
 #ifdef CONFIG_OLD_MCOUNT
 	if (!ret)
 		ret = __ftrace_modify_caller(&ftrace_graph_call_old,
-- 
2.7.4

[toc] | [next] | [standalone]

#1588776

Hi Abel,

On Fri, Feb 24 2017, Abel Vesa wrote:

<snip>

> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index fda6a46..877df5b 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -55,6 +55,7 @@ config ARM
>  	select HAVE_DMA_API_DEBUG
>  	select HAVE_DMA_CONTIGUOUS if MMU
>  	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
> +	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT


AFAICS, your code depends on the __gnu_mcount_nc calling conventions,
i.e. on gcc pushing the original lr on the stack. In particular, there's
no implementation of a ftrace_regs_caller_old or so.

So shouldn't this read as !OLD_MCOUNT instead?

Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.


>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
>  	select HAVE_EXIT_THREAD
>  	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)

<snip>

> diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
> index c73c403..3916dd6 100644
> --- a/arch/arm/kernel/entry-ftrace.S
> +++ b/arch/arm/kernel/entry-ftrace.S
> @@ -92,12 +92,78 @@
>  2:	mcount_exit
>  .endm
>  
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +
> +.macro __ftrace_regs_caller
> +
> +	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
> +
> +	add 	ip, sp, #12	@ move in IP the value of SP as it was
> +				@ before the push {lr} of the mcount mechanism
> +	stmdb	sp!, {ip,lr,pc}
> +	stmdb	sp!, {r0-r11,lr}
> +
> +	@ stack content at this point:
> +	@ 0  4          48   52       56   60   64    68       72
> +	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |

Being a constant, the saved pc is not very useful, I think.

Wouldn't it be better (and more consistent with other archs) to have

  pt_regs->ARM_lr = original lr
  pt_refs->ARM_pc = current lr

instead?

A (hypothetical) klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
would do the more intuitive

  regs->ARM_pc = ip;

rather than a

  regs->ARM_lr = ip

then.

In addition, the original lr register would be made available to ftrace
handlers this way.


> +	mov r3, sp				@ struct pt_regs*
> +	ldr r2, =function_trace_op
> +	ldr r2, [r2]				@ pointer to the current
> +						@ function tracing op
> +	ldr	r1, [sp, #PT_REGS_SIZE]		@ lr of instrumented func
> +	mcount_adjust_addr	r0, lr		@ instrumented function
> +
> +	.globl ftrace_regs_call
> +ftrace_regs_call:
> +	bl	ftrace_stub
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	.globl ftrace_graph_regs_call
> +ftrace_graph_regs_call:
> +	mov	r0, r0
> +#endif
> +	@ pop saved regs
> +	@ first, get the previous LR value from stack
> +	ldr	lr, [sp, #PT_REGS_SIZE]
> +	@ now pop the rest of the saved registers INCLUDING stack pointer
> +	ldmia   sp, {r0-r11, ip, sp, pc}
> +.endm
> +


> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +.macro __ftrace_graph_regs_caller
> +
> +	sub	r0, fp, #4		@ lr of instrumented routine (parent)
> +
> +	@ called from __ftrace_regs_caller
> +	ldr     r1, [sp, #S_LR]		@ instrumented routine (func)
> +	mcount_adjust_addr	r1, r1
> +
> +	sub	r2, r0, #4		@ frame pointer

Given that r2 is prepare_ftrace_return()'s frame_pointer argument, is

  r2 = fp - 4 - 4 = fp - 8

really correct / what is wanted here?

> +	bl	prepare_ftrace_return
> +
> +	@ pop registers saved in ftrace_regs_caller
> +	@ first, get the previous LR value from stack
> +	ldr	lr, [sp, #PT_REGS_SIZE]
> +	@ now pop the rest of the saved registers INCLUDING stack pointer
> +	ldmia   sp, {r0-r11, ip, sp, pc}
> +.endm
> +#endif
> +#endif
> +

<snip>


Thanks,

Nicolai

[toc] | [prev] | [next] | [standalone]

#1589074 — Re: [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote:
> Hi Abel,
> 
> On Fri, Feb 24 2017, Abel Vesa wrote:
> 
> <snip>
> 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index fda6a46..877df5b 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -55,6 +55,7 @@ config ARM
> >  	select HAVE_DMA_API_DEBUG
> >  	select HAVE_DMA_CONTIGUOUS if MMU
> >  	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
> > +	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
> 
> 
> AFAICS, your code depends on the __gnu_mcount_nc calling conventions,
> i.e. on gcc pushing the original lr on the stack. In particular, there's
> no implementation of a ftrace_regs_caller_old or so.
> 
> So shouldn't this read as !OLD_MCOUNT instead?
The implementation of __ftrace_modify_code which sets the kernel text to rw
needs OLD_MCOUNT (that is, the arch specific one, not the generic one).
> 
> Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.
> 
> 
> >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
> >  	select HAVE_EXIT_THREAD
> >  	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
> 
> <snip>
> 
> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
> > index c73c403..3916dd6 100644
> > --- a/arch/arm/kernel/entry-ftrace.S
> > +++ b/arch/arm/kernel/entry-ftrace.S
> > @@ -92,12 +92,78 @@
> >  2:	mcount_exit
> >  .endm
> >  
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > +
> > +.macro __ftrace_regs_caller
> > +
> > +	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
> > +
> > +	add 	ip, sp, #12	@ move in IP the value of SP as it was
> > +				@ before the push {lr} of the mcount mechanism
> > +	stmdb	sp!, {ip,lr,pc}
> > +	stmdb	sp!, {r0-r11,lr}
> > +
> > +	@ stack content at this point:
> > +	@ 0  4          48   52       56   60   64    68       72
> > +	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |
> 
> Being a constant, the saved pc is not very useful, I think.
So you're saying skip it ? But you still need to leave space for it. So why not
just save it even if the value is not useful?
> 
> Wouldn't it be better (and more consistent with other archs) to have
> 
>   pt_regs->ARM_lr = original lr
>   pt_refs->ARM_pc = current lr
> 
> instead?
> 
> A (hypothetical) klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> would do the more intuitive
> 
>   regs->ARM_pc = ip;
> 
> rather than a
> 
>   regs->ARM_lr = ip
> 
> then.
You are right. There is a subsequent patch I'm currently working on that will 
enable livepatch and will bring an implementation for klp_arch_set_pc as you 
described it. I still don't get what is wrong with the code though?

> 
> In addition, the original lr register would be made available to ftrace
> handlers this way.
> 
> 
> > +	mov r3, sp				@ struct pt_regs*
> > +	ldr r2, =function_trace_op
> > +	ldr r2, [r2]				@ pointer to the current
> > +						@ function tracing op
> > +	ldr	r1, [sp, #PT_REGS_SIZE]		@ lr of instrumented func
> > +	mcount_adjust_addr	r0, lr		@ instrumented function
> > +
> > +	.globl ftrace_regs_call
> > +ftrace_regs_call:
> > +	bl	ftrace_stub
> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +	.globl ftrace_graph_regs_call
> > +ftrace_graph_regs_call:
> > +	mov	r0, r0
> > +#endif
> > +	@ pop saved regs
> > +	@ first, get the previous LR value from stack
> > +	ldr	lr, [sp, #PT_REGS_SIZE]
> > +	@ now pop the rest of the saved registers INCLUDING stack pointer
> > +	ldmia   sp, {r0-r11, ip, sp, pc}
> > +.endm
> > +
> 
> 
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +.macro __ftrace_graph_regs_caller
> > +
> > +	sub	r0, fp, #4		@ lr of instrumented routine (parent)
> > +
> > +	@ called from __ftrace_regs_caller
> > +	ldr     r1, [sp, #S_LR]		@ instrumented routine (func)
> > +	mcount_adjust_addr	r1, r1
> > +
> > +	sub	r2, r0, #4		@ frame pointer
> 
> Given that r2 is prepare_ftrace_return()'s frame_pointer argument, is
> 
>   r2 = fp - 4 - 4 = fp - 8
> 
> really correct / what is wanted here?
> 
You are right. 
-	sub     r2, r0, #4              @ frame pointer
+	mov     r2, fp                  @ frame pointer

> > +	bl	prepare_ftrace_return
> > +
> > +	@ pop registers saved in ftrace_regs_caller
> > +	@ first, get the previous LR value from stack
> > +	ldr	lr, [sp, #PT_REGS_SIZE]
> > +	@ now pop the rest of the saved registers INCLUDING stack pointer
> > +	ldmia   sp, {r0-r11, ip, sp, pc}
> > +.endm
> > +#endif
> > +#endif
> > +
> 
> <snip>
> 
> 
> Thanks,
> 
> Nicolai

Thanks

[toc] | [prev] | [next] | [standalone]

#1589363

Hi Abel,

On Tue, Feb 28 2017, Abel Vesa wrote:

> On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote:
>> On Fri, Feb 24 2017, Abel Vesa wrote:
>> 
>> <snip>
>> 
>> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> > index fda6a46..877df5b 100644
>> > --- a/arch/arm/Kconfig
>> > +++ b/arch/arm/Kconfig
>> > @@ -55,6 +55,7 @@ config ARM
>> >  	select HAVE_DMA_API_DEBUG
>> >  	select HAVE_DMA_CONTIGUOUS if MMU
>> >  	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
>> > +	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
>> 
>> 
>> AFAICS, your code depends on the __gnu_mcount_nc calling conventions,
>> i.e. on gcc pushing the original lr on the stack. In particular, there's
>> no implementation of a ftrace_regs_caller_old or so.
>> 
>> So shouldn't this read as !OLD_MCOUNT instead?
> The implementation of __ftrace_modify_code which sets the kernel text to rw
> needs OLD_MCOUNT (that is, the arch specific one, not the generic one).

You're right that ARM's implementation of __ftrace_modify_code() is hidden
within an #ifdef CONFIG_OLD_MCOUNT.

But,
 - its implementation doesn't "need" or depend on OLD_MCOUNT
 - and it's true in general that the kernel text must be made writable
   before ftrace_modify_all_code() attempts to write to it.

So, I bet that the set_kernel_text_rw()-calling ARM implementations of
arch_ftrace_update_code() and __ftrace_modify_code() resp. have been
inserted under that CONFIG_OLD_MCOUNT #ifdef by mistake with commit
80d6b0c2eed2 ("ARM: mm: allow text and rodata sections to be
read-only").

In conclusion, I claim that DYNAMIC_FTRACE w/o OLD_MCOUNT had been
broken before your patch already. I didn't explicitly test that though.

I think that should be fixed rather than your DYNAMIC_FTRACE_WITH_REGS
pulling in OLD_MCOUNT in order to repair DYNAMIC_FTRACE.

Especially since your implementation seems to require !OLD_MCOUNT...

>> Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.
>> 
>> 
>> >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
>> >  	select HAVE_EXIT_THREAD
>> >  	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
>> 
>> <snip>
>> 
>> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
>> > index c73c403..3916dd6 100644
>> > --- a/arch/arm/kernel/entry-ftrace.S
>> > +++ b/arch/arm/kernel/entry-ftrace.S
>> > @@ -92,12 +92,78 @@
>> >  2:	mcount_exit
>> >  .endm
>> >  
>> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>> > +
>> > +.macro __ftrace_regs_caller
>> > +
>> > +	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
>> > +
>> > +	add 	ip, sp, #12	@ move in IP the value of SP as it was
>> > +				@ before the push {lr} of the mcount mechanism
>> > +	stmdb	sp!, {ip,lr,pc}
>> > +	stmdb	sp!, {r0-r11,lr}
>> > +
>> > +	@ stack content at this point:
>> > +	@ 0  4          48   52       56   60   64    68       72
>> > +	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |
>> 
>> Being a constant, the saved pc is not very useful, I think.
> So you're saying skip it ? But you still need to leave space for it. So why not
> just save it even if the value is not useful?

No, no, I don't want to skip it. I'd just prefer to have the pt_regs'
->ARM_lr and ->ARM_pc slots filled with different, perhaps more useful
values:

>> 
>> Wouldn't it be better (and more consistent with other archs) to have
>> 
>>   pt_regs->ARM_lr = original lr
>>   pt_refs->ARM_pc = current lr
>> 
>> instead?

The stack would look like this then

@ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
@ 0  4          48       52       56            60            64    68       72
@ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |

I.e. the pt_regs would capture almost the full context of the
instrumented function (except for ip).

Thanks,

Nicolai

[toc] | [prev] | [next] | [standalone]

#1589371 — Re: [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote:
> Hi Abel,
> 
> On Tue, Feb 28 2017, Abel Vesa wrote:
> 
> > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote:
> >> On Fri, Feb 24 2017, Abel Vesa wrote:
> >> 
> >> <snip>
> >> 
> >> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >> > index fda6a46..877df5b 100644
> >> > --- a/arch/arm/Kconfig
> >> > +++ b/arch/arm/Kconfig
> >> > @@ -55,6 +55,7 @@ config ARM
> >> >  	select HAVE_DMA_API_DEBUG
> >> >  	select HAVE_DMA_CONTIGUOUS if MMU
> >> >  	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU
> >> > +	select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT
> >> 
> >> 
> >> AFAICS, your code depends on the __gnu_mcount_nc calling conventions,
> >> i.e. on gcc pushing the original lr on the stack. In particular, there's
> >> no implementation of a ftrace_regs_caller_old or so.
> >> 
> >> So shouldn't this read as !OLD_MCOUNT instead?
> > The implementation of __ftrace_modify_code which sets the kernel text to rw
> > needs OLD_MCOUNT (that is, the arch specific one, not the generic one).
> 
> You're right that ARM's implementation of __ftrace_modify_code() is hidden
> within an #ifdef CONFIG_OLD_MCOUNT.
> 
> But,
>  - its implementation doesn't "need" or depend on OLD_MCOUNT
>  - and it's true in general that the kernel text must be made writable
>    before ftrace_modify_all_code() attempts to write to it.
> 
> So, I bet that the set_kernel_text_rw()-calling ARM implementations of
> arch_ftrace_update_code() and __ftrace_modify_code() resp. have been
> inserted under that CONFIG_OLD_MCOUNT #ifdef by mistake with commit
> 80d6b0c2eed2 ("ARM: mm: allow text and rodata sections to be
> read-only").
> 
> In conclusion, I claim that DYNAMIC_FTRACE w/o OLD_MCOUNT had been
> broken before your patch already. I didn't explicitly test that though.
>
That is correct. The DYNAMIC_FTRACE w/o OLD_MCOUNT doesn't work. 
> I think that should be fixed rather than your DYNAMIC_FTRACE_WITH_REGS
> pulling in OLD_MCOUNT in order to repair DYNAMIC_FTRACE.
> 
> Especially since your implementation seems to require !OLD_MCOUNT...
> 
So making arch specific __ftrace_modify_code to be OLD_MCOUNT independent might fix 
DYNAMIC_FTRACE w/o OLD_MCOUNT and implicitly make DYNAMIC_FTRACE_WITH_REGS not dependent on
OLD_MCOUNT.

I will investigate this further today. Probably the whole dependancy between FUNCTION_TRACER
and OLD_MCOUNT will need to be changed/updated.
> >> Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL.
> >> 
> >> 
> >> >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU
> >> >  	select HAVE_EXIT_THREAD
> >> >  	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
> >> 
> >> <snip>
> >> 
> >> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
> >> > index c73c403..3916dd6 100644
> >> > --- a/arch/arm/kernel/entry-ftrace.S
> >> > +++ b/arch/arm/kernel/entry-ftrace.S
> >> > @@ -92,12 +92,78 @@
> >> >  2:	mcount_exit
> >> >  .endm
> >> >  
> >> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> >> > +
> >> > +.macro __ftrace_regs_caller
> >> > +
> >> > +	sub	sp, sp, #8	@ space for CPSR and OLD_R0 (not used)
> >> > +
> >> > +	add 	ip, sp, #12	@ move in IP the value of SP as it was
> >> > +				@ before the push {lr} of the mcount mechanism
> >> > +	stmdb	sp!, {ip,lr,pc}
> >> > +	stmdb	sp!, {r0-r11,lr}
> >> > +
> >> > +	@ stack content at this point:
> >> > +	@ 0  4          48   52       56   60   64    68       72
> >> > +	@ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR |
> >> 
> >> Being a constant, the saved pc is not very useful, I think.
> > So you're saying skip it ? But you still need to leave space for it. So why not
> > just save it even if the value is not useful?
> 
> No, no, I don't want to skip it. I'd just prefer to have the pt_regs'
> ->ARM_lr and ->ARM_pc slots filled with different, perhaps more useful
> values:
> 
> >> 
> >> Wouldn't it be better (and more consistent with other archs) to have
> >> 
> >>   pt_regs->ARM_lr = original lr
> >>   pt_refs->ARM_pc = current lr
> >> 
> >> instead?
> 
> The stack would look like this then
> 
> @ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
> @ 0  4          48       52       56            60            64    68       72
> @ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |
> 
> I.e. the pt_regs would capture almost the full context of the
> instrumented function (except for ip).
> 
So basicly what you are saying is:
- instead of current LR save original LR (previous one saved in instrumented function epilog)
- instead of current PC save original PC (previous one saved in instrumented function epilog)

I still don't see the point of saving the actual value of PC since nobody will ever
restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree,
it could be the original one in pt_regs.

I'll look into this sometime today or tomorrow and get back with updates.
> Thanks,
> 
> Nicolai

Thanks

[toc] | [prev] | [next] | [standalone]

#1589387 — Re: [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

On Tue, Feb 28, 2017 at 11:46:38AM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 28, 2017 at 11:22:27AM +0000, Abel Vesa wrote:
> > On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote:
> > > Hi Abel,
> > > 
> > > On Tue, Feb 28 2017, Abel Vesa wrote:
> > > 
> > > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote:
> > > >> On Fri, Feb 24 2017, Abel Vesa wrote:
> > > >> Wouldn't it be better (and more consistent with other archs) to have
> > > >> 
> > > >>   pt_regs->ARM_lr = original lr
> > > >>   pt_refs->ARM_pc = current lr
> > > >> 
> > > >> instead?
> > > 
> > > The stack would look like this then
> > > 
> > > @ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
> > > @ 0  4          48       52       56            60            64    68       72
> > > @ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |
> > > 
> > > I.e. the pt_regs would capture almost the full context of the
> > > instrumented function (except for ip).
> > > 
> > So basicly what you are saying is:
> > - instead of current LR save original LR (previous one saved in instrumented function epilog)
> > - instead of current PC save original PC (previous one saved in instrumented function epilog)
> > 
> > I still don't see the point of saving the actual value of PC since nobody will ever
> > restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree,
> > it could be the original one in pt_regs.
> > 
> > I'll look into this sometime today or tomorrow and get back with updates.
> 
> Which is exactly what I proposed, with code, on one of the previous
> iterations of this patch...
Fair enough. I probably missunderstood your comments then.

Thanks.
> 
> -- 
> 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]

#1589393 — Re: [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS

On Tue, Feb 28, 2017 at 11:22:27AM +0000, Abel Vesa wrote:
> On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote:
> > Hi Abel,
> > 
> > On Tue, Feb 28 2017, Abel Vesa wrote:
> > 
> > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote:
> > >> On Fri, Feb 24 2017, Abel Vesa wrote:
> > >> Wouldn't it be better (and more consistent with other archs) to have
> > >> 
> > >>   pt_regs->ARM_lr = original lr
> > >>   pt_refs->ARM_pc = current lr
> > >> 
> > >> instead?
> > 
> > The stack would look like this then
> > 
> > @ ...           | ARM_ip | ARM_sp | ARM_lr      | ARM_pc      | ...          |
> > @ 0  4          48       52       56            60            64    68       72
> > @ R0 | R1 | ... | LR     | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR |
> > 
> > I.e. the pt_regs would capture almost the full context of the
> > instrumented function (except for ip).
> > 
> So basicly what you are saying is:
> - instead of current LR save original LR (previous one saved in instrumented function epilog)
> - instead of current PC save original PC (previous one saved in instrumented function epilog)
> 
> I still don't see the point of saving the actual value of PC since nobody will ever
> restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree,
> it could be the original one in pt_regs.
> 
> I'll look into this sometime today or tomorrow and get back with updates.

Which is exactly what I proposed, with code, on one of the previous
iterations of this patch...

-- 
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web