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 <[email protected]>

---
 arch/arm/Kconfig               |  1 +
 arch/arm/include/asm/ftrace.h  |  4 +++
 arch/arm/kernel/entry-ftrace.S | 79 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/ftrace.c       | 37 ++++++++++++++++++++
 4 files changed, 121 insertions(+)

arch/arm/Kconfig +1 -0

index 186c4c2..df401f4 100644
@@ -50,6 +50,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)

arch/arm/include/asm/ftrace.h +4 -0

index 22b7311..f379881 100644
@@ -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 */

arch/arm/kernel/entry-ftrace.S +35 -0

index c73c403..b1fee6c 100644
@@ -92,12 +92,74 @@
 2:     mcount_exit
 .endm
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+       add     ip, sp, #4      @ 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          44    48   52       56   60   64
+       @ R0 | R1 | ... | R11 | LR | SP + 4 | LR | PC | 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, #64]                   @ 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
+       ldr     lr, [sp, #64]                   @ get the previous LR value from stack
+       ldmia   sp, {r0-r11, ip, sp, pc}        @ pop the saved registers INCLUDING
+                                               @ the stack pointer
+.endm
+

arch/arm/kernel/entry-ftrace.S +34 -0

+       bl      prepare_ftrace_return
+
+       @ pop regs saved in ftrace_regs_caller
+       ldr     lr, [sp, #64]                   @ restore lr from the stack
+       ldmia   sp, {r0-r11, ip, sp, pc}        @ restore r0 through 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 +274,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 +291,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

arch/arm/kernel/ftrace.c +37 -1

index 3f17594..cb0358c 100644
@@ -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

2017-02-07 23:57 GMT+01:00 Abel Vesa <[email protected]>:

arch/arm/kernel/entry-ftrace.S line 139

Quoted 10 lines

arch/arm/kernel/entry-ftrace.S +10 -0

+#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, #56]           @ instrumented routine (func)
+       mcount_adjust_addr      r1, r1
+
+       sub     r2, r0, #4              @ frame pointer

why not mov r2, fp ?

Quoted 9 lines · Abel Vesa

arch/arm/kernel/entry-ftrace.S +9 -0

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+	add 	ip, sp, #4	@ 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}
+

arch/arm/kernel/entry-ftrace.S line 106

Quoted 3 lines

arch/arm/kernel/entry-ftrace.S +3 -0

+	@ stack content at this point:
+	@ 0  4          44    48   52       56   60   64
+	@ R0 | R1 | ... | R11 | LR | SP + 4 | LR | PC | previous LR |  

How important is this to be close to "struct pt_regs" ? Do we care about r12 being "wrong" ? The other issue is that pt_regs is actually 72 bytes in size, not 68 bytes. So, does that mean we end up inappropriately leaking some of the kernel stack to userspace through ftrace?

It's possible to save all the registers like this if we need to provide a complete picture of the register set at function entry:

	str	ip, [sp, #-16]!
	add	ip, sp, #20
	stmia	sp, {ip, lr, pc}
	stmdb	sp!, {r0 - r11}

However, is that even correct - don't we want pt_regs' LR and PC to be related to the function call itself? The "previous LR" as you describe it is where the called function (the one that is being traced) will return to. The current LR at this point is the address within the traced function. So actually I think this is more strictly correct, if I'm understanding the intention here correctly:

str ip, [sp, #S_IP - PT_REGS_SIZE]! @ save current IP
ldr ip, [sp, #PT_REGS_SIZE - S_IP] @ get LR at traced function entry
str lr, [sp, #S_PC - S_IP] @ save current LR as PC
str ip, [sp, #S_LR - S_IP] @ save traced function return
add ip, sp, #PT_REGS_SIZE - S_IP + 4
str ip, [sp, #S_SP - SP_IP] @ save stack pointer at function entry
stmdb sp!, {r0 - r11}
@ clear CPSR and old_r0 words
mov r3, #0
str r3, [sp, #S_PSR]
str r3, [sp, #S_OLD_R0]

However, that has the side effect of misaligning the stack (the stack needs to be aligned to 8 bytes). So, if we decide we don't care about the saved LR value (except as a mechanism to preserve it across the call into the ftrace code):

	str	ip, [sp, #S_IP - PT_REGS_SIZE + 4]!
	str	lr, [sp, #S_PC - S_IP]
	ldr	lr, [sp, #PT_REGS_SIZE - 4 - S_IP]
	add	ip, sp, #PT_REGS_SIZE - S_IP
	stmib	sp, {ip, lr}
	stmdb	sp!, {r0 - r11}
	@ clear CPSR and old_r0 words
	mov	r3, #0
	str	r3, [sp, #S_PSR]
	str	r3, [sp, #S_OLD_R0]

and the return would be:

	ldmia	sp, {r0 - pc}

That all said - maybe someone from the ftrace community can comment on how much of pt_regs is actually necessary here?

Then came along live kernel patching, which I believe this series is trying to support. What is needed by pt_regs is a way to "hijack" the function being called to instead call the patched function. That is, ftrace is not being used for tracing, but in reality, being used to modify the running kernel. It is being used to change what function gets called. ftrace is just a hook for that mechanism.

Quoted 6 lines · Steven Rostedt

Then came along live kernel patching, which I believe this series is trying to support. What is needed by pt_regs is a way to "hijack" the function being called to instead call the patched function. That is, ftrace is not being used for tracing, but in reality, being used to modify the running kernel. It is being used to change what function gets called. ftrace is just a hook for that mechanism.

So, would I be correct to assume that the only parts of pt_regs that would be touched are those which contain arguments to the function, and the register which would contain the return value?

Quoted 8 lines · On Thu, 9 Feb 2017 18:06:44 +0000 Russell King - ARM Linux…

Quoted 6 lines · Steven Rostedt

Then came along live kernel patching, which I believe this series is trying to support. What is needed by pt_regs is a way to "hijack" the function being called to instead call the patched function. That is, ftrace is not being used for tracing, but in reality, being used to modify the running kernel. It is being used to change what function gets called. ftrace is just a hook for that mechanism.

Quoted 4 lines

So, would I be correct to assume that the only parts of pt_regs that would be touched are those which contain arguments to the function, and the register which would contain the return value?

For live kernel patching, perhaps.

But for kprobes, I think they can touch anything. Matters what the creater of the kprobe wanted to do.

-- Steve

[ sending again with Masami Cc'd ]

Quoted 21 lines · On Thu, 9 Feb 2017 13:14:14 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 8 lines · On Thu, 9 Feb 2017 18:06:44 +0000 Russell King - ARM Linux…

Quoted 6 lines · Steven Rostedt

Then came along live kernel patching, which I believe this series is trying to support. What is needed by pt_regs is a way to "hijack" the function being called to instead call the patched function. That is, ftrace is not being used for tracing, but in reality, being used to modify the running kernel. It is being used to change what function gets called. ftrace is just a hook for that mechanism.

Quoted 4 lines

So, would I be correct to assume that the only parts of pt_regs that would be touched are those which contain arguments to the function, and the register which would contain the return value?

For live kernel patching, perhaps.

But for kprobes, I think they can touch anything. Matters what the creater of the kprobe wanted to do.

Quoted 1 line

-- Steve

Quoted 27 lines · Steven Rostedt

[ sending again with Masami Cc'd ]

Quoted 21 lines · On Thu, 9 Feb 2017 13:14:14 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 8 lines · On Thu, 9 Feb 2017 18:06:44 +0000 Russell King - ARM Linux…

Quoted 6 lines · Steven Rostedt

Then came along live kernel patching, which I believe this series is trying to support. What is needed by pt_regs is a way to "hijack" the function being called to instead call the patched function. That is, ftrace is not being used for tracing, but in reality, being used to modify the running kernel. It is being used to change what function gets called. ftrace is just a hook for that mechanism.

Quoted 4 lines

So, would I be correct to assume that the only parts of pt_regs that would be touched are those which contain arguments to the function, and the register which would contain the return value?

For live kernel patching, perhaps.

But for kprobes, I think they can touch anything. Matters what the creater of the kprobe wanted to do.

Thing is, by saving all of them is the easiest way to ensure that the whole context is the same when the replacing function gets called, as I said before.

We can't be sure that while __ftrace_ops_list_func is executing, any of the regs will have the value they had when the function-to-be-replaced was called. That's the reason I say we need to save them all.

Quoted 2 lines

Quoted 1 line

-- Steve

Quoted 6 lines · Steven Rostedt

Then came along live kernel patching, which I believe this series is trying to support. What is needed by pt_regs is a way to "hijack" the function being called to instead call the patched function. That is, ftrace is not being used for tracing, but in reality, being used to modify the running kernel. It is being used to change what function gets called. ftrace is just a hook for that mechanism.

So, would I be correct to assume that the only parts of pt_regs that would be touched are those which contain arguments to the function, and the register which would contain the return value?

Signature · 4 lines

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

Quoted 9 lines · Abel Vesa

arch/arm/kernel/entry-ftrace.S +9 -0

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+	add 	ip, sp, #4	@ 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}
+

arch/arm/kernel/entry-ftrace.S line 106

Quoted 3 lines

arch/arm/kernel/entry-ftrace.S +3 -0

+	@ stack content at this point:
+	@ 0  4          44    48   52       56   60   64
+	@ R0 | R1 | ... | R11 | LR | SP + 4 | LR | PC | previous LR |

How important is this to be close to "struct pt_regs" ? Do we care about r12 being "wrong" ? The other issue is that pt_regs is actually 72 bytes in size, not 68 bytes. So, does that mean we end up inappropriately leaking some of the kernel stack to userspace through ftrace?

It's possible to save all the registers like this if we need to provide a complete picture of the register set at function entry:

	str	ip, [sp, #-16]!
	add	ip, sp, #20
	stmia	sp, {ip, lr, pc}
	stmdb	sp!, {r0 - r11}

However, is that even correct - don't we want pt_regs' LR and PC to be related to the function call itself? The "previous LR" as you describe it is where the called function (the one that is being traced) will return to. The current LR at this point is the address within the traced function. So actually I think this is more strictly correct, if I'm understanding the intention here correctly:

str ip, [sp, #S_IP - PT_REGS_SIZE]! @ save current IP
ldr ip, [sp, #PT_REGS_SIZE - S_IP] @ get LR at traced function entry
str lr, [sp, #S_PC - S_IP] @ save current LR as PC
str ip, [sp, #S_LR - S_IP] @ save traced function return
add ip, sp, #PT_REGS_SIZE - S_IP + 4
str ip, [sp, #S_SP - SP_IP] @ save stack pointer at function entry
stmdb sp!, {r0 - r11}
@ clear CPSR and old_r0 words
mov r3, #0
str r3, [sp, #S_PSR]
str r3, [sp, #S_OLD_R0]

However, that has the side effect of misaligning the stack (the stack needs to be aligned to 8 bytes). So, if we decide we don't care about the saved LR value (except as a mechanism to preserve it across the call into the ftrace code):

	str	ip, [sp, #S_IP - PT_REGS_SIZE + 4]!
	str	lr, [sp, #S_PC - S_IP]
	ldr	lr, [sp, #PT_REGS_SIZE - 4 - S_IP]
	add	ip, sp, #PT_REGS_SIZE - S_IP
	stmib	sp, {ip, lr}
	stmdb	sp!, {r0 - r11}
	@ clear CPSR and old_r0 words
	mov	r3, #0
	str	r3, [sp, #S_PSR]
	str	r3, [sp, #S_OLD_R0]

and the return would be:

	ldmia	sp, {r0 - pc}

That all said - maybe someone from the ftrace community can comment on how much of pt_regs is actually necessary here?

Signature · 3 lines

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.

arch/arm/kernel/entry-ftrace.S line 106

Quoted 12 lines

arch/arm/kernel/entry-ftrace.S +12 -0

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+     add     ip, sp, #4      @ 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          44    48   52       56   60   64
+     @ R0 | R1 | ... | R11 | LR | SP + 4 | LR | PC | previous LR |

How important is this to be close to "struct pt_regs" ? Do we care about r12 being "wrong" ? The other issue is that pt_regs is actually 72 bytes in size, not 68 bytes. So, does that mean we end up inappropriately leaking some of the kernel stack to userspace through ftrace?

It's possible to save all the registers like this if we need to provide a complete picture of the register set at function entry:

        str     ip, [sp, #-16]!
        add     ip, sp, #20
        stmia   sp, {ip, lr, pc}
        stmdb   sp!, {r0 - r11}

However, is that even correct - don't we want pt_regs' LR and PC to be related to the function call itself? The "previous LR" as you describe it is where the called function (the one that is being traced) will return to. The current LR at this point is the address within the traced function. So actually I think this is more strictly correct, if I'm understanding the intention here correctly:

        str     ip, [sp, #S_IP - PT_REGS_SIZE]! @ save current IP
        ldr     ip, [sp, #PT_REGS_SIZE - S_IP]  @ get LR at traced function entry
        str     lr, [sp, #S_PC - S_IP]          @ save current LR as PC
        str     ip, [sp, #S_LR - S_IP]          @ save traced function return
        add     ip, sp, #PT_REGS_SIZE - S_IP + 4
        str     ip, [sp, #S_SP - SP_IP]         @ save stack pointer at function entry
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

However, that has the side effect of misaligning the stack (the stack needs to be aligned to 8 bytes). So, if we decide we don't care about the saved LR value (except as a mechanism to preserve it across the call into the ftrace code):

        str     ip, [sp, #S_IP - PT_REGS_SIZE + 4]!
        str     lr, [sp, #S_PC - S_IP]
        ldr     lr, [sp, #PT_REGS_SIZE - 4 - S_IP]
        add     ip, sp, #PT_REGS_SIZE - S_IP
        stmib   sp, {ip, lr}
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

and the return would be:

        ldmia   sp, {r0 - pc}

That all said - maybe someone from the ftrace community can comment on how much of pt_regs is actually necessary here?

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

2017-02-09 17:29 GMT+01:00 Russell King - ARM Linux <[email protected]>:

Quoted 62 lines · Abel Vesa

arch/arm/kernel/entry-ftrace.S line 106

Quoted 12 lines

arch/arm/kernel/entry-ftrace.S +12 -0

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+     add     ip, sp, #4      @ 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          44    48   52       56   60   64
+     @ R0 | R1 | ... | R11 | LR | SP + 4 | LR | PC | previous LR |

How important is this to be close to "struct pt_regs" ? Do we care about r12 being "wrong" ? The other issue is that pt_regs is actually 72 bytes in size, not 68 bytes. So, does that mean we end up inappropriately leaking some of the kernel stack to userspace through ftrace?

It's possible to save all the registers like this if we need to provide a complete picture of the register set at function entry:

        str     ip, [sp, #-16]!
        add     ip, sp, #20
        stmia   sp, {ip, lr, pc}
        stmdb   sp!, {r0 - r11}

However, is that even correct - don't we want pt_regs' LR and PC to be related to the function call itself? The "previous LR" as you describe it is where the called function (the one that is being traced) will return to. The current LR at this point is the address within the traced function. So actually I think this is more strictly correct, if I'm understanding the intention here correctly:

        str     ip, [sp, #S_IP - PT_REGS_SIZE]! @ save current IP
        ldr     ip, [sp, #PT_REGS_SIZE - S_IP]  @ get LR at traced function entry
        str     lr, [sp, #S_PC - S_IP]          @ save current LR as PC
        str     ip, [sp, #S_LR - S_IP]          @ save traced function return
        add     ip, sp, #PT_REGS_SIZE - S_IP + 4
        str     ip, [sp, #S_SP - SP_IP]         @ save stack pointer at function entry
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

However, that has the side effect of misaligning the stack (the stack needs to be aligned to 8 bytes). So, if we decide we don't care about the saved LR value (except as a mechanism to preserve it across the call into the ftrace code):

        str     ip, [sp, #S_IP - PT_REGS_SIZE + 4]!
        str     lr, [sp, #S_PC - S_IP]
        ldr     lr, [sp, #PT_REGS_SIZE - 4 - S_IP]
        add     ip, sp, #PT_REGS_SIZE - S_IP
        stmib   sp, {ip, lr}
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

Quoted 6 lines

and the return would be:

        ldmia   sp, {r0 - pc}

That all said - maybe someone from the ftrace community can comment on how much of pt_regs is actually necessary here?

I would suggest the following:
r0-r11: filled with current values.
r12 : the value of r12 doesn't matter (Intra-procedure call scratch reg), we can either save it or not.
r13 - sp: the value as it was when the instrumented function was entered. in the mcount case, it's the current sp value - 4, otherwise it'f sp -4
r14 - lr: the value as it was when the instrumented function was entered. first element in stack or available in frame depending on GCC's version (mcount vs __gnu_mcount_nc)
r15 - pc : the address after the modified instruction (value of lr when the ftrace caller is entered)

I don't think we need CSPR and ORIG_r0.

Quoted 6 lines

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

Quoted 70 lines

2017-02-09 17:29 GMT+01:00 Russell King - ARM Linux <[email protected]>:

Quoted 62 lines · Abel Vesa

arch/arm/kernel/entry-ftrace.S line 106

Quoted 12 lines

arch/arm/kernel/entry-ftrace.S +12 -0

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+     add     ip, sp, #4      @ 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          44    48   52       56   60   64
+     @ R0 | R1 | ... | R11 | LR | SP + 4 | LR | PC | previous LR |

How important is this to be close to "struct pt_regs" ? Do we care about r12 being "wrong" ? The other issue is that pt_regs is actually 72 bytes in size, not 68 bytes. So, does that mean we end up inappropriately leaking some of the kernel stack to userspace through ftrace?

It's possible to save all the registers like this if we need to provide a complete picture of the register set at function entry:

        str     ip, [sp, #-16]!
        add     ip, sp, #20
        stmia   sp, {ip, lr, pc}
        stmdb   sp!, {r0 - r11}

However, is that even correct - don't we want pt_regs' LR and PC to be related to the function call itself? The "previous LR" as you describe it is where the called function (the one that is being traced) will return to. The current LR at this point is the address within the traced function. So actually I think this is more strictly correct, if I'm understanding the intention here correctly:

        str     ip, [sp, #S_IP - PT_REGS_SIZE]! @ save current IP
        ldr     ip, [sp, #PT_REGS_SIZE - S_IP]  @ get LR at traced function entry
        str     lr, [sp, #S_PC - S_IP]          @ save current LR as PC
        str     ip, [sp, #S_LR - S_IP]          @ save traced function return
        add     ip, sp, #PT_REGS_SIZE - S_IP + 4
        str     ip, [sp, #S_SP - SP_IP]         @ save stack pointer at function entry
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

However, that has the side effect of misaligning the stack (the stack needs to be aligned to 8 bytes). So, if we decide we don't care about the saved LR value (except as a mechanism to preserve it across the call into the ftrace code):

        str     ip, [sp, #S_IP - PT_REGS_SIZE + 4]!
        str     lr, [sp, #S_PC - S_IP]
        ldr     lr, [sp, #PT_REGS_SIZE - 4 - S_IP]
        add     ip, sp, #PT_REGS_SIZE - S_IP
        stmib   sp, {ip, lr}
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

Quoted 6 lines

and the return would be:

        ldmia   sp, {r0 - pc}

That all said - maybe someone from the ftrace community can comment on how much of pt_regs is actually necessary here?

Quoted 13 lines

I would suggest the following:
r0-r11: filled with current values.
r12 : the value of r12 doesn't matter (Intra-procedure call scratch reg), we can either save it or not.
r13 - sp: the value as it was when the instrumented function was entered. in the mcount case, it's the current sp value - 4, otherwise it'f sp -4
r14 - lr: the value as it was when the instrumented function was entered. first element in stack or available in frame depending on GCC's version (mcount vs __gnu_mcount_nc)
r15 - pc : the address after the modified instruction (value of lr when the ftrace caller is entered)

So basically you're saying: save all regs, r0 through r15, except r12. Based on that, I think it's easier to save all of them and then restore all of them except r12. Plus, you have to take into consideration all the possible users of ftrace with regs. At some point some implementation of ftrace_regs_call will probably need the value from r12.

Quoted 1 line

I don't think we need CSPR and ORIG_r0.

I think we do. As I said before, because PT_REGS is 72 and some function might (in the future) make use of CSPR or ORIG_r0, to ensure there is no stack corruption taking place, we have to provide whole pt_regs, that is 72 (including CSPR and ORIG_r0). Plus, the stack alignment thing Russell mentioned would be fixed.

The only problem I don't have a solution for at this point is OLD_LR (or previous LR as it is called in this patch). If we take the approach described earlier,

we need to add to pt_regs the OLD_LR which I really don't like because it is breaking the whole purpose of pt_regs (it should only contain one copy of each reg, though it already has the ORIG_r0 in it) and will also break the stack alignment.

Quoted 6 lines

Quoted 6 lines

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

2017-02-10 13:03 GMT+01:00 Abel Vesa <[email protected]>:

Quoted 90 lines · Jean-Jacques Hiblot

Quoted 70 lines

2017-02-09 17:29 GMT+01:00 Russell King - ARM Linux <[email protected]>:

Quoted 62 lines · Abel Vesa

arch/arm/kernel/entry-ftrace.S line 106

Quoted 12 lines

arch/arm/kernel/entry-ftrace.S +12 -0

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+
+.macro __ftrace_regs_caller
+
+     add     ip, sp, #4      @ 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          44    48   52       56   60   64
+     @ R0 | R1 | ... | R11 | LR | SP + 4 | LR | PC | previous LR |

How important is this to be close to "struct pt_regs" ? Do we care about r12 being "wrong" ? The other issue is that pt_regs is actually 72 bytes in size, not 68 bytes. So, does that mean we end up inappropriately leaking some of the kernel stack to userspace through ftrace?

It's possible to save all the registers like this if we need to provide a complete picture of the register set at function entry:

        str     ip, [sp, #-16]!
        add     ip, sp, #20
        stmia   sp, {ip, lr, pc}
        stmdb   sp!, {r0 - r11}

However, is that even correct - don't we want pt_regs' LR and PC to be related to the function call itself? The "previous LR" as you describe it is where the called function (the one that is being traced) will return to. The current LR at this point is the address within the traced function. So actually I think this is more strictly correct, if I'm understanding the intention here correctly:

        str     ip, [sp, #S_IP - PT_REGS_SIZE]! @ save current IP
        ldr     ip, [sp, #PT_REGS_SIZE - S_IP]  @ get LR at traced function entry
        str     lr, [sp, #S_PC - S_IP]          @ save current LR as PC
        str     ip, [sp, #S_LR - S_IP]          @ save traced function return
        add     ip, sp, #PT_REGS_SIZE - S_IP + 4
        str     ip, [sp, #S_SP - SP_IP]         @ save stack pointer at function entry
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

However, that has the side effect of misaligning the stack (the stack needs to be aligned to 8 bytes). So, if we decide we don't care about the saved LR value (except as a mechanism to preserve it across the call into the ftrace code):

        str     ip, [sp, #S_IP - PT_REGS_SIZE + 4]!
        str     lr, [sp, #S_PC - S_IP]
        ldr     lr, [sp, #PT_REGS_SIZE - 4 - S_IP]
        add     ip, sp, #PT_REGS_SIZE - S_IP
        stmib   sp, {ip, lr}
        stmdb   sp!, {r0 - r11}
        @ clear CPSR and old_r0 words
        mov     r3, #0
        str     r3, [sp, #S_PSR]
        str     r3, [sp, #S_OLD_R0]

Quoted 6 lines

and the return would be:

        ldmia   sp, {r0 - pc}

That all said - maybe someone from the ftrace community can comment on how much of pt_regs is actually necessary here?

Quoted 13 lines

I would suggest the following:
r0-r11: filled with current values.
r12 : the value of r12 doesn't matter (Intra-procedure call scratch reg), we can either save it or not.
r13 - sp: the value as it was when the instrumented function was entered. in the mcount case, it's the current sp value - 4, otherwise it'f sp -4
r14 - lr: the value as it was when the instrumented function was entered. first element in stack or available in frame depending on GCC's version (mcount vs __gnu_mcount_nc)
r15 - pc : the address after the modified instruction (value of lr when the ftrace caller is entered)

So basically you're saying: save all regs, r0 through r15, except r12. Based on that, I think it's easier to save all of them and then restore all of them except r12. Plus, you have to take into consideration all the possible users of ftrace with regs. At some point some implementation of ftrace_regs_call will probably need the value from r12.

Quoted 1 line

I don't think we need CSPR and ORIG_r0.

Quoted 5 lines

I think we do. As I said before, because PT_REGS is 72 and some function might (in the future) make use of CSPR or ORIG_r0, to ensure there is no stack corruption taking place, we have to provide whole pt_regs, that is 72 (including CSPR and ORIG_r0). Plus, the stack alignment thing Russell mentioned would be fixed.

You're right for the size of the structure. For the content, I don't think we need all of them but it won't hurt to save more than necessary.

Quoted 3 lines

The only problem I don't have a solution for at this point is OLD_LR (or previous LR as it is called in this patch). If we take the approach described earlier,

previous LR is the lr when the instrumented function is entered, it should be stored in pt_regs as r14

Quoted 9 lines

we need to add to pt_regs the OLD_LR which I really don't like because it is breaking the whole purpose of pt_regs (it should only contain one copy of each reg, though it already has the ORIG_r0 in it) and will also break the stack alignment.

Quoted 6 lines

Quoted 6 lines

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

The only problem I don't have a solution for at this point is OLD_LR (or previous LR as it is called in this patch).

Quoted 2 lines · Abel Vesa

The only problem I don't have a solution for at this point is OLD_LR (or previous LR as it is called in this patch).

If you want the context at function entry, then you need to save the registers as they were at that point.

The stacking of LR in the gnu_mcount thing is there to avoid this problem:

a:

	push	{lr}
	bl	__gnu_mcount_mc

That "bl" instruction can be thought of as being effectively this:

	adr	lr, 1f
	b	__gnu_mcount_mc

1:

and from that, you can plainly see that "lr" gets corrupted by the call. So, to save the register state as it was at point "a", you need to save (in order):

r0 through to sp
the saved lr on the stack (which was the value of lr at point a) the current lr (which is the value of the PC _after_ __gnu_mcount_mc
returns)
cpsr
write zero to old_r0

Stacking actual value of the PC at the point that you're stacking these registers is really senseless - it doesn't convey any useful information about the context being saved.

Does it make sense to leave the compiler's saving of lr on the stack? Probably not - which I think my last iteration overwrote with the old_r0

value. The only thing my last iteration did not do was save a real value for CPSR.

I didn't test it either...

Signature · 3 lines

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.