kprobes/ftrace: Have kprobes use ftrace on ftrace nops

From: Steven Rostedt <[email protected]>

Return as the 4th paramater to the function tracer callback the pt_regs.

So far this is only supported by x86_64. The ftrace_ops flag FTRACE_OPS_FL_SAVE_REGS is added to tell the arch to save all regs to the pt_regs, otherwise a minimum is just passed back (the same regs that is saved by mcount itself).

arch/x86/kernel/entry_64.S +10 -1

 	MCOUNT_SAVE_FRAME
 
 	leaq function_trace_op, %rdx
-	movq 0x38(%rsp), %rdi
+
+	cmpl $0, ftrace_save_regs
+	jne  save_all_regs
+
+call_func:
+
+	/* regs go into 4th parameter */
+	leaq (%rsp), %rcx
+
+	movq ORIG_RAX(%rsp), %rdi
 	movq 8(%rbp), %rsi
 	subq $MCOUNT_INSN_SIZE, %rdi
 

@@ -96,6 +105,18 @@ GLOBAL(ftrace_stub)
 	retq
 END(ftrace_caller)
 

Quoted 8 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Return as the 4th paramater to the function tracer callback the pt_regs.

So far this is only supported by x86_64. The ftrace_ops flag FTRACE_OPS_FL_SAVE_REGS is added to tell the arch to save all regs to the pt_regs, otherwise a minimum is just passed back (the same regs that is saved by mcount itself).

I guess it will be a bit hard to port this on x86-32, because on x86-32, the top of stack address in pt_regs is the address of sp member (e.g. &(pt_regs->sp)). I mean that when mcount-entry calls ftrace_caller, it pushes an address of the next instruction of mcount-entry on the top of stack.
In that case, &(pt_regs->sp) points the entry which stores the address, instead of the return address of probed function.

e.g. with kprobes (on x86-32):

[  <bx>  ] <- pt_regs
[  ...   ]
[  <cs>  ]
[<flags> ]
[ret-addr] <- &(pt_regs.sp)
[  arg1  ]
[  arg2  ]

with this method:

[  <bx>  ] <- pt_regs
[  ...   ]
[  <cs>  ]
[<flags> ]
[mcount-ret] <- &(pt_regs.sp)
[ret-addr]
[  arg1  ]
[  arg2  ]

I think this is hard to solve without a tricky hack.

For example, on x86-32, MCOUNT_FRAME_SAVE saves flags on the entry which will be <cs> and it saves mcount-ret to local stack and moves flags to next entry.

<save-frame>
pushf # save flags on CS(%esp)
subl $12, %esp # skip ip, orig_ax and gs
pushl %fs
pushl %es
...
pushl %ebx
movl 56(%esp), %ebx # load mcount-ret address
movl 52(%esp), %ebp # load flags
movl %ebp, 56(%esp) # store flags

call function (ebx is callee save)

<restore-frame>
movl 56(%esp), %ebp # load flags
movl %ebp, 52(%esp) # store flags
movl %ebx, 56(%esp) # load mcount-ret address
...
popf
ret

Hmm?

arch/x86/kernel/entry_64.S ENTRY(ftrace_caller)

Quoted 78 lines

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/include/asm/ftrace.h     |   38 ++++++++++++++++++++----------------
 arch/x86/kernel/entry_64.S        |   23 +++++++++++++++++++++-
 include/linux/ftrace.h            |   15 ++++++++++++-
 kernel/trace/ftrace.c             |   29 ++++++++++++++++++---------
 kernel/trace/trace_events.c       |    2 +-
 kernel/trace/trace_functions.c    |    7 +++--
 kernel/trace/trace_irqsoff.c      |    2 +-
 kernel/trace/trace_sched_wakeup.c |    3 +-
 kernel/trace/trace_selftest.c     |   15 +++++++++----
 kernel/trace/trace_stack.c        |    3 +-
 10 files changed, 95 insertions(+), 42 deletions(-)

arch/x86/include/asm/ftrace.h +21 -17

index b3fcf16..0750c2a 100644
@@ -4,26 +4,29 @@
 #ifdef __ASSEMBLY__
 
 	.macro MCOUNT_SAVE_FRAME
-	/* taken from glibc */
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
+	 /*
+	  * We add enough stack to save all regs,
+	  * and we what we need in the location of pt_regs.
+	  */
+	subq $ORIG_RAX, %rsp
+	movq %rax, RAX(%rsp)
+	movq %rcx, RCX(%rsp)
+	movq %rdx, RDX(%rsp)
+	movq %rsi, RSI(%rsp)
+	movq %rdi, RDI(%rsp)
+	movq %r8, R8(%rsp)
+	movq %r9, R9(%rsp)
 	.endm
 
 	.macro MCOUNT_RESTORE_FRAME
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
+	movq R9(%rsp), %r9
+	movq R8(%rsp), %r8
+	movq RDI(%rsp), %rdi
+	movq RSI(%rsp), %rsi
+	movq RDX(%rsp), %rdx
+	movq RCX(%rsp), %rcx
+	movq RAX(%rsp), %rax
+	addq $ORIG_RAX, %rsp
 	.endm
 
 #endif

@@ -34,6 +37,7 @@
 
 #if defined(CONFIG_DYNAMIC_FTRACE) && defined(CONFIG_X86_64)
 #define ARCH_SUPPORTS_FTRACE_OPS 1
+#define ARCH_SUPPORTS_FTRACE_SAVE_REGS 1
 #endif
 
 #ifndef __ASSEMBLY__

arch/x86/kernel/entry_64.S +0 -0

index 27adc2b..b77f297 100644
@@ -78,7 +78,16 @@ ENTRY(ftrace_caller)

I can see below code before save frame.

cmpl $0, function_trace_stop
jne ftrace_stub

Please pushf before comparing it. :)
Sometimes, the value of eflags is worth to watch.
I know that SF/ZF will be never used between
function call, so it is OK if the eflags is saved
in MCOUNT_SAVE_FRAME.

Quoted 21 lines

arch/x86/kernel/entry_64.S +10 -1

 	MCOUNT_SAVE_FRAME
 
 	leaq function_trace_op, %rdx
-	movq 0x38(%rsp), %rdi
+
+	cmpl $0, ftrace_save_regs
+	jne  save_all_regs
+
+call_func:
+
+	/* regs go into 4th parameter */
+	leaq (%rsp), %rcx
+
+	movq ORIG_RAX(%rsp), %rdi
 	movq 8(%rbp), %rsi
 	subq $MCOUNT_INSN_SIZE, %rdi
 

@@ -96,6 +105,18 @@ GLOBAL(ftrace_stub)
 	retq
 END(ftrace_caller)
 

arch/x86/kernel/entry_64.S GLOBAL(ftrace_stub) · line 117

Quoted 10 lines

arch/x86/kernel/entry_64.S +10 -0

+save_all_regs:
+	/* Save the rest of pt_regs */
+	movq %r15, R15(%rsp)
+	movq %r14, R14(%rsp)
+	movq %r13, R13(%rsp)
+	movq %r12, R12(%rsp)
+	movq %r10, R10(%rsp)
+	movq %rbp, RBP(%rsp)
+	movq %rbx, RBX(%rsp)
+	jmp call_func

At least, pt_regs.sp must be saved for accessing
vars on stack.

arch/x86/kernel/entry_64.S GLOBAL(ftrace_stub) · line 122

Quoted 5 lines

arch/x86/kernel/entry_64.S +2 -0

+
+
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 ENTRY(mcount)
 	cmpl $0, function_trace_stop

You also need to restore the rest of pt_regs if
ftrace_save_regs is true.

Thank you,

Quoted 10 lines · Masami Hiramatsu

Quoted 8 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Return as the 4th paramater to the function tracer callback the pt_regs.

So far this is only supported by x86_64. The ftrace_ops flag FTRACE_OPS_FL_SAVE_REGS is added to tell the arch to save all regs to the pt_regs, otherwise a minimum is just passed back (the same regs that is saved by mcount itself).

Quoted 7 lines

I guess it will be a bit hard to port this on x86-32, because on x86-32, the top of stack address in pt_regs is the address of sp member (e.g. &(pt_regs->sp)). I mean that when mcount-entry calls ftrace_caller, it pushes an address of the next instruction of mcount-entry on the top of stack.
In that case, &(pt_regs->sp) points the entry which stores the address, instead of the return address of probed function.

I'm a little confused here. What is needed by kprobes handlers?

It should be no problem saving the sp member onto pt_regs of the time mcount was called, and simulate the probe as if it was put on the nop.

Quoted 23 lines

e.g. with kprobes (on x86-32):

[  <bx>  ] <- pt_regs
[  ...   ]
[  <cs>  ]
[<flags> ]
[ret-addr] <- &(pt_regs.sp)
[  arg1  ]
[  arg2  ]

with this method:

[  <bx>  ] <- pt_regs
[  ...   ]
[  <cs>  ]
[<flags> ]
[mcount-ret] <- &(pt_regs.sp)
[ret-addr]
[  arg1  ]
[  arg2  ]

Quoted 1 line

I think this is hard to solve without a tricky hack.

Is this for handling kretprobes? With mcount, we shouldn't be doing that because mcount is not the place that kretprobes should be doing this.

When we add __fentry__, the arch code will be different to handle that and it shouldn't be too hard to make that work with kretprobes.

Quoted 5 lines

For example, on x86-32, MCOUNT_FRAME_SAVE saves flags on the entry which will be <cs> and it saves mcount-ret to local stack and moves flags to next entry.

Quoted 10 lines

<save-frame>
pushf # save flags on CS(%esp)
subl $12, %esp # skip ip, orig_ax and gs
pushl %fs
pushl %es
...
pushl %ebx
movl 56(%esp), %ebx # load mcount-ret address
movl 52(%esp), %ebp # load flags
movl %ebp, 56(%esp) # store flags

Nope we don't do anything like this. I've already got x86_32 working (but maybe it needs more for pt_regs->sp) but why is pushf needed?

-- Steve

Quoted 149 lines

call function (ebx is callee save)

<restore-frame>
movl 56(%esp), %ebp # load flags
movl %ebp, 52(%esp) # store flags
movl %ebx, 56(%esp) # load mcount-ret address
...
popf
ret

Hmm?

arch/x86/kernel/entry_64.S ENTRY(ftrace_caller)

Quoted 78 lines

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/include/asm/ftrace.h     |   38 ++++++++++++++++++++----------------
 arch/x86/kernel/entry_64.S        |   23 +++++++++++++++++++++-
 include/linux/ftrace.h            |   15 ++++++++++++-
 kernel/trace/ftrace.c             |   29 ++++++++++++++++++---------
 kernel/trace/trace_events.c       |    2 +-
 kernel/trace/trace_functions.c    |    7 +++--
 kernel/trace/trace_irqsoff.c      |    2 +-
 kernel/trace/trace_sched_wakeup.c |    3 +-
 kernel/trace/trace_selftest.c     |   15 +++++++++----
 kernel/trace/trace_stack.c        |    3 +-
 10 files changed, 95 insertions(+), 42 deletions(-)

arch/x86/include/asm/ftrace.h +21 -17

index b3fcf16..0750c2a 100644
@@ -4,26 +4,29 @@
 #ifdef __ASSEMBLY__
 
 	.macro MCOUNT_SAVE_FRAME
-	/* taken from glibc */
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
+	 /*
+	  * We add enough stack to save all regs,
+	  * and we what we need in the location of pt_regs.
+	  */
+	subq $ORIG_RAX, %rsp
+	movq %rax, RAX(%rsp)
+	movq %rcx, RCX(%rsp)
+	movq %rdx, RDX(%rsp)
+	movq %rsi, RSI(%rsp)
+	movq %rdi, RDI(%rsp)
+	movq %r8, R8(%rsp)
+	movq %r9, R9(%rsp)
 	.endm
 
 	.macro MCOUNT_RESTORE_FRAME
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
+	movq R9(%rsp), %r9
+	movq R8(%rsp), %r8
+	movq RDI(%rsp), %rdi
+	movq RSI(%rsp), %rsi
+	movq RDX(%rsp), %rdx
+	movq RCX(%rsp), %rcx
+	movq RAX(%rsp), %rax
+	addq $ORIG_RAX, %rsp
 	.endm
 
 #endif

@@ -34,6 +37,7 @@
 
 #if defined(CONFIG_DYNAMIC_FTRACE) && defined(CONFIG_X86_64)
 #define ARCH_SUPPORTS_FTRACE_OPS 1
+#define ARCH_SUPPORTS_FTRACE_SAVE_REGS 1
 #endif
 
 #ifndef __ASSEMBLY__

arch/x86/kernel/entry_64.S +0 -0

index 27adc2b..b77f297 100644
@@ -78,7 +78,16 @@ ENTRY(ftrace_caller)

I can see below code before save frame.

cmpl $0, function_trace_stop
jne ftrace_stub

Please pushf before comparing it. :)
Sometimes, the value of eflags is worth to watch.
I know that SF/ZF will be never used between
function call, so it is OK if the eflags is saved
in MCOUNT_SAVE_FRAME.

Quoted 21 lines

arch/x86/kernel/entry_64.S +10 -1

 	MCOUNT_SAVE_FRAME
 
 	leaq function_trace_op, %rdx
-	movq 0x38(%rsp), %rdi
+
+	cmpl $0, ftrace_save_regs
+	jne  save_all_regs
+
+call_func:
+
+	/* regs go into 4th parameter */
+	leaq (%rsp), %rcx
+
+	movq ORIG_RAX(%rsp), %rdi
 	movq 8(%rbp), %rsi
 	subq $MCOUNT_INSN_SIZE, %rdi
 

@@ -96,6 +105,18 @@ GLOBAL(ftrace_stub)
 	retq
 END(ftrace_caller)
 

arch/x86/kernel/entry_64.S GLOBAL(ftrace_stub) · line 117

Quoted 10 lines

arch/x86/kernel/entry_64.S +10 -0

+save_all_regs:
+	/* Save the rest of pt_regs */
+	movq %r15, R15(%rsp)
+	movq %r14, R14(%rsp)
+	movq %r13, R13(%rsp)
+	movq %r12, R12(%rsp)
+	movq %r10, R10(%rsp)
+	movq %rbp, RBP(%rsp)
+	movq %rbx, RBX(%rsp)
+	jmp call_func

At least, pt_regs.sp must be saved for accessing
vars on stack.

arch/x86/kernel/entry_64.S GLOBAL(ftrace_stub) · line 122

Quoted 5 lines

arch/x86/kernel/entry_64.S +2 -0

+
+
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 ENTRY(mcount)
 	cmpl $0, function_trace_stop

You also need to restore the rest of pt_regs if
ftrace_save_regs is true.

Thank you,

Please note, kprobes is the function which basically provides a breakpoint handler in the kernel. Users expect that. So all registers should be provided as breakpoint interrupt does. That's what I said "transparently".

From: Steven Rostedt <[email protected]>

Currently, if a probe is set at an ftrace nop, the probe will fail.

When -mfentry is used by ftrace (in the near future), the nop will be at the beginning of the function. This will prevent kprobes from hooking to the most common place that kprobes are attached.

Now that ftrace supports individual users as well as pt_regs passing, kprobes can use the ftrace infrastructure when a probe is placed on a ftrace nop.

kernel/kprobes.c +4 -0

 #else /* !CONFIG_OPTPROBES */
 
+static void __kprobes __arm_kprobe(struct kprobe *p)
+{
+	/* Only arm non-ftrace probes */
+	if (!ftrace_kprobe(p))

kernel/kprobes.c +12 -0

 /* Arm a kprobe with text_mutex */
 static void __kprobes arm_kprobe(struct kprobe *kp)
 {
+	/* ftrace probes can skip arch calls */
+	if (ftrace_kprobe(kp)) {
+		register_ftrace_function(&kp->fops);
+		return;
+	}
+
 	/*
 	 * Here, since __arm_kprobe() doesn't use stop_machine(),
 	 * this doesn't cause deadlock on text_mutex. So, we don't

@@ -924,6 +984,12 @@ static void __kprobes arm_kprobe(struct kprobe *kp)
 static void __kprobes disarm_kprobe(struct kprobe *kp)
 {
 	/* Ditto */
+
+	if (ftrace_kprobe(kp)) {
+		unregister_ftrace_function(&kp->fops);
+		return;
+	}
+
 	mutex_lock(&text_mutex);
 	__disarm_kprobe(kp, true);
 	mutex_unlock(&text_mutex);

kernel/kprobes.c +5 -0

+
+static int use_ftrace_hook(struct kprobe *p)
+{
+	char str[KSYM_SYMBOL_LEN];
+

kernel/kprobes.c +20 -5

+
+	return 0;
+}
+#else
+static int use_ftrace_hook(struct kprobe *p)
+{
+	return 0;
+}
+#endif
+
 int __kprobes register_kprobe(struct kprobe *p)
 {
 	int ret = 0;

@@ -1329,11 +1445,14 @@ int __kprobes register_kprobe(struct kprobe *p)
 	if (ret)
 		return ret;
 
+	ret = use_ftrace_hook(p);
+	if (ret)
+		return ret;
+
 	jump_label_lock();
 	preempt_disable();
 	if (!kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr) ||
-	    ftrace_text_reserved(p->addr, p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr))
 		goto fail_with_jump_label;
 

@@ -1384,15 +1503,17 @@ int __kprobes register_kprobe(struct kprobe *p)
 		goto out;
 	}
 
-	ret = arch_prepare_kprobe(p);
-	if (ret)
-		goto out;
+	if (!ftrace_kprobe(p)) {
+		ret = arch_prepare_kprobe(p);
+		if (ret)
+			goto out;
+	}
 
 	INIT_HLIST_NODE(&p->hlist);
 	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
-	if (!kprobes_all_disarmed && !kprobe_disabled(p))
+	if (!ftrace_kprobe(p) && !kprobes_all_disarmed && !kprobe_disabled(p))
 		__arm_kprobe(p);
 
 	/* Try to optimize kprobe */

@@ -1400,6 +1521,12 @@ int __kprobes register_kprobe(struct kprobe *p)
 
 out:

kernel/kprobes.c +12 -1

@@ -2134,6 +2261,14 @@ static void __kprobes arm_all_kprobes(void)
 	}
 	mutex_unlock(&text_mutex);
 
+	/* ftrace kprobes are enabled outside of text_mutex */
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+		head = &kprobe_table[i];
+		hlist_for_each_entry_rcu(p, node, head, hlist)
+			if (ftrace_kprobe(p) && !kprobe_disabled(p))
+				arm_kprobe(p);
+	}
+
 	kprobes_all_disarmed = false;
 	printk(KERN_INFO "Kprobes globally enabled\n");
 

@@ -2164,11 +2299,21 @@ static void __kprobes disarm_all_kprobes(void)
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
 		hlist_for_each_entry_rcu(p, node, head, hlist) {
-			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
+			if (!ftrace_kprobe(p) &&
+			    !arch_trampoline_kprobe(p) && !kprobe_disabled(p))
 				__disarm_kprobe(p, false);
 		}
 	}
 	mutex_unlock(&text_mutex);
+
+	/* ftrace kprobes are disabled outside of text_mutex */

Hi Steven,

As I suggested in another reply, I'm now attracted by the idea of using ftrace in kprobe-tracer, because of simplicity. Anyway, here is my review.

Quoted 11 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Currently, if a probe is set at an ftrace nop, the probe will fail.

When -mfentry is used by ftrace (in the near future), the nop will be at the beginning of the function. This will prevent kprobes from hooking to the most common place that kprobes are attached.

Now that ftrace supports individual users as well as pt_regs passing, kprobes can use the ftrace infrastructure when a probe is placed on a ftrace nop.

My one general concern is the timing of enabling ftrace-based probe. Breakpoint-based kprobe (including optimized kprobe) is enabled right after registering. Users might expect that.
And AFAIK, dynamic ftraces handler will be enabled (activated) after a while, because it has to wait for an NMI, doesn't it?

And theoretically, this ftrace-based probe can't support jprobe, because it changes IP address. Nowadays, this may becomes minor problem (because someone who wants to trace function args can use kprobe-tracer), but still exist.

kernel/kprobes.c static void __kprobes __disarm_kprobe(struct kprobe *p,

Quoted 100 lines

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/kprobes.h |    6 ++
 kernel/kprobes.c        |  163 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 160 insertions(+), 9 deletions(-)

include/linux/kprobes.h +6 -0

index dd7c12e..5742071 100644
@@ -37,6 +37,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
+#include <linux/ftrace.h>
 
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>

@@ -112,6 +113,11 @@ struct kprobe {
 	/* copy of the original instruction */
 	struct arch_specific_insn ainsn;
 
+#ifdef CONFIG_FUNCTION_TRACER
+	/* If it is possible to use ftrace to probe */
+	struct ftrace_ops fops;
+#endif
+
 	/*
 	 * Indicates various status flags.
 	 * Protected by kprobe_mutex after this kprobe is registered.

kernel/kprobes.c +35 -0

index e6c25eb..2160768 100644
@@ -102,6 +102,31 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
 	{NULL}    /* Terminator */
 };
 
+#ifdef CONFIG_FUNCTION_TRACER
+/* kprobe stub function for use when probe is not connected to ftrace */
+static void
+kprobe_ftrace_stub(unsigned long ip, unsigned long pip,
+		   struct ftrace_ops *op, struct pt_regs *pt_regs)
+{
+}
+
+static bool ftrace_kprobe(struct kprobe *p)
+{
+	return p->fops.func && p->fops.func != kprobe_ftrace_stub;
+}
+
+static void init_non_ftrace_kprobe(struct kprobe *p)
+{
+	p->fops.func = kprobe_ftrace_stub;
+}
+
+#else
+static bool ftrace_kprobe(struct kprobe *p)
+{
+	return false;
+}
+#endif
+
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be

@@ -396,6 +421,9 @@ static __kprobes void free_aggr_kprobe(struct kprobe *p)
 {
 	struct optimized_kprobe *op;
 
+	if (ftrace_kprobe(p))
+		return;
+
 	op = container_of(p, struct optimized_kprobe, kp);
 	arch_remove_optimized_kprobe(op);
 	arch_remove_kprobe(p);

@@ -759,6 +787,9 @@ static __kprobes void try_to_optimize_kprobe(struct kprobe *p)
 	struct kprobe *ap;
 	struct optimized_kprobe *op;
 
+	if (ftrace_kprobe(p))
+		return;
+
 	ap = alloc_aggr_kprobe(p);
 	if (!ap)
 		return;

@@ -849,6 +880,10 @@ static void __kprobes __arm_kprobe(struct kprobe *p)
 {
 	struct kprobe *_p;
 
+	/* Only arm non-ftrace probes */
+	if (ftrace_kprobe(p))
+		return;
+
 	/* Check collision with other optimized kprobes */
 	_p = get_optimized_kprobe((unsigned long)p->addr);
 	if (unlikely(_p))

@@ -864,6 +899,10 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,

bool reopt)

kernel/kprobes.c static void __kprobes __disarm_kprobe(struct kprobe *p,

Quoted 11 lines

kernel/kprobes.c +4 -0

 {
 	struct kprobe *_p;
 
+	/* Only disarm non-ftrace probes */
+	if (ftrace_kprobe(p))
+		return;
+
 	unoptimize_kprobe(p, false);	/* Try to unoptimize */
 
 	if (!kprobe_queued(p)) {

@@ -878,13 +917,26 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,

bool reopt)

Quoted 12 lines

kernel/kprobes.c +9 -0

 #else /* !CONFIG_OPTPROBES */
 
+static void __kprobes __arm_kprobe(struct kprobe *p)
+{
+	/* Only arm non-ftrace probes */
+	if (!ftrace_kprobe(p))
+		arch_arm_kprobe(p);
+}
+
+/* Remove the breakpoint of a probe. Must be called with text_mutex locked */
+static void __kprobes __disarm_kprobe(struct kprobe *p, bool reopt)

kernel/kprobes.c static void __kprobes __disarm_kprobe(struct kprobe *p, bool reopt) · line 933

Quoted 5 lines

kernel/kprobes.c +5 -0

+{
+	/* Only disarm non-ftrace probes */
+	if (!ftrace_kprobe(p))
+		arch_disarm_kprobe(p);
+}

If it ignores disabling/enabling, kprobe_ftrace_callback must check kprobe_disabled(p) and skip it.

kernel/kprobes.c static __kprobes struct kprobe *alloc_aggr_kprobe(struct

Quoted 24 lines

kernel/kprobes.c +4 -3

+
 #define optimize_kprobe(p)			do {} while (0)
 #define unoptimize_kprobe(p, f)			do {} while (0)
 #define kill_optimized_kprobe(p)		do {} while (0)
 #define prepare_optimized_kprobe(p)		do {} while (0)
 #define try_to_optimize_kprobe(p)		do {} while (0)
-#define __arm_kprobe(p)				arch_arm_kprobe(p)
-#define __disarm_kprobe(p, o)			arch_disarm_kprobe(p)
 #define kprobe_disarmed(p)			kprobe_disabled(p)
 #define wait_for_kprobe_optimizer()		do {} while (0)
 

@@ -897,7 +949,9 @@ static void reuse_unused_kprobe(struct kprobe *ap)
 
 static __kprobes void free_aggr_kprobe(struct kprobe *p)
 {
-	arch_remove_kprobe(p);
+
+	if (!ftrace_kprobe(p))
+		arch_remove_kprobe(p);
 	kfree(p);
 }
 

@@ -910,6 +964,12 @@ static __kprobes struct kprobe *alloc_aggr_kprobe(struct

kprobe *p)

Quoted 29 lines

kernel/kprobes.c +12 -0

 /* Arm a kprobe with text_mutex */
 static void __kprobes arm_kprobe(struct kprobe *kp)
 {
+	/* ftrace probes can skip arch calls */
+	if (ftrace_kprobe(kp)) {
+		register_ftrace_function(&kp->fops);
+		return;
+	}
+
 	/*
 	 * Here, since __arm_kprobe() doesn't use stop_machine(),
 	 * this doesn't cause deadlock on text_mutex. So, we don't

@@ -924,6 +984,12 @@ static void __kprobes arm_kprobe(struct kprobe *kp)
 static void __kprobes disarm_kprobe(struct kprobe *kp)
 {
 	/* Ditto */
+
+	if (ftrace_kprobe(kp)) {
+		unregister_ftrace_function(&kp->fops);
+		return;
+	}
+
 	mutex_lock(&text_mutex);
 	__disarm_kprobe(kp, true);
 	mutex_unlock(&text_mutex);

@@ -1313,6 +1379,56 @@ static inline int check_kprobe_rereg(struct kprobe *p)
 	return ret;
 }
 

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1388

Quoted 7 lines

kernel/kprobes.c +7 -0

+#ifdef CONFIG_FUNCTION_TRACER
+static notrace void
+kprobe_ftrace_callback(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct pt_regs *pt_regs)
+{
+	struct kprobe *p = container_of(op, struct kprobe, fops);
+

Here, we need to set up kprobe_ctlblk and some of pt_regs members, ip, cs and orig_ax as optimized_callback()@arch/x86/kernel/kprobes.c does.

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1391

Quoted 4 lines

kernel/kprobes.c +3 -0

+	/* A nop has been trapped, just run both handlers back to back */
+	if (p->pre_handler)
+		p->pre_handler(p, pt_regs);

And increment regs->ip here for NOP.

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1394

Quoted 4 lines

kernel/kprobes.c +3 -0

+	if (p->post_handler)
+		p->post_handler(p, pt_regs, 0);
+}

Anyway, above operations strongly depends on arch, so kprobe_ftrace_callback should be moved to arch/*/kprobes.c.

And I think most of the code can be shared with optimized code.

Quoted 10 lines

kernel/kprobes.c +9 -0

+
+static int use_ftrace_hook(struct kprobe *p)
+{
+	char str[KSYM_SYMBOL_LEN];
+
+	/* see if this is an ftrace function */
+	if (!ftrace_text_reserved(p->addr, p->addr)) {
+		/* make sure fops->func is nop */
+		init_non_ftrace_kprobe(p);

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1409

Quoted 6 lines

kernel/kprobes.c +6 -0

+		return 0;
+	}
+
+	/* If arch does not support pt_regs passing, bail */
+	if (!ARCH_SUPPORTS_FTRACE_SAVE_REGS)
+		return -EINVAL;

Hmm, I think this should be checked at build time...

Quoted 11 lines

kernel/kprobes.c +10 -0

+
+	/* Use ftrace hook instead */
+
+	memset(&p->fops, 0, sizeof(p->fops));
+
+	/* Find the function this is connected with this addr */
+	kallsyms_lookup((unsigned long)p->addr, NULL, NULL, NULL, str);
+
+	p->fops.flags = FTRACE_OPS_FL_SAVE_REGS;
+	p->fops.func = kprobe_ftrace_callback;

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1421

Quoted 2 lines

kernel/kprobes.c +2 -0

+
+	ftrace_set_filter(&p->fops, str, strlen(str), 1);

Hmm, IMHO, ftrace_set_filter should not be called here, because there can be other kprobes are already registered on the same address. In that case, it is natural that we use an aggr_kprobe for handling several kprobes on same address. Or, kprobe hash table will have several different probes on same address.

Quoted 54 lines

kernel/kprobes.c +20 -5

+
+	return 0;
+}
+#else
+static int use_ftrace_hook(struct kprobe *p)
+{
+	return 0;
+}
+#endif
+
 int __kprobes register_kprobe(struct kprobe *p)
 {
 	int ret = 0;

@@ -1329,11 +1445,14 @@ int __kprobes register_kprobe(struct kprobe *p)
 	if (ret)
 		return ret;
 
+	ret = use_ftrace_hook(p);
+	if (ret)
+		return ret;
+
 	jump_label_lock();
 	preempt_disable();
 	if (!kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr) ||
-	    ftrace_text_reserved(p->addr, p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr))
 		goto fail_with_jump_label;
 

@@ -1384,15 +1503,17 @@ int __kprobes register_kprobe(struct kprobe *p)
 		goto out;
 	}
 
-	ret = arch_prepare_kprobe(p);
-	if (ret)
-		goto out;
+	if (!ftrace_kprobe(p)) {
+		ret = arch_prepare_kprobe(p);
+		if (ret)
+			goto out;
+	}
 
 	INIT_HLIST_NODE(&p->hlist);
 	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
-	if (!kprobes_all_disarmed && !kprobe_disabled(p))
+	if (!ftrace_kprobe(p) && !kprobes_all_disarmed && !kprobe_disabled(p))
 		__arm_kprobe(p);
 
 	/* Try to optimize kprobe */

@@ -1400,6 +1521,12 @@ int __kprobes register_kprobe(struct kprobe *p)
 

kernel/kprobes.c int __kprobes register_kprobe(struct kprobe *p) · line 1532

Quoted 11 lines

kernel/kprobes.c +6 -0

 out:
 	mutex_unlock(&text_mutex);
+
+	/* ftrace kprobes must be set outside of text_mutex */
+	if (!ret && ftrace_kprobe(p) &&
+	    !kprobes_all_disarmed && !kprobe_disabled(p))
+		arm_kprobe(p);
+
 	put_online_cpus();
 	jump_label_unlock();
 	mutex_unlock(&kprobe_mutex);

After this, we must handle some fails which can happen when probing on a module.

Quoted 39 lines

kernel/kprobes.c +19 -1

@@ -2134,6 +2261,14 @@ static void __kprobes arm_all_kprobes(void)
 	}
 	mutex_unlock(&text_mutex);
 
+	/* ftrace kprobes are enabled outside of text_mutex */
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+		head = &kprobe_table[i];
+		hlist_for_each_entry_rcu(p, node, head, hlist)
+			if (ftrace_kprobe(p) && !kprobe_disabled(p))
+				arm_kprobe(p);
+	}
+
 	kprobes_all_disarmed = false;
 	printk(KERN_INFO "Kprobes globally enabled\n");
 

@@ -2164,11 +2299,21 @@ static void __kprobes disarm_all_kprobes(void)
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
 		hlist_for_each_entry_rcu(p, node, head, hlist) {
-			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
+			if (!ftrace_kprobe(p) &&
+			    !arch_trampoline_kprobe(p) && !kprobe_disabled(p))
 				__disarm_kprobe(p, false);
 		}
 	}
 	mutex_unlock(&text_mutex);
+
+	/* ftrace kprobes are disabled outside of text_mutex */
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+		head = &kprobe_table[i];
+		hlist_for_each_entry_rcu(p, node, head, hlist) {
+			if (ftrace_kprobe(p) && !kprobe_disabled(p))
+				disarm_kprobe(p);
+		}
+	}
 	mutex_unlock(&kprobe_mutex);
 

kernel/kprobes.c static void __kprobes disarm_all_kprobes(void) · line 2319

Quoted 1 line

kernel/kprobes.c +0 -0

 	/* Wait for disarming all kprobes by optimizer */

Thank you,

Quoted 5 lines · Masami Hiramatsu

Hi Steven,

As I suggested in another reply, I'm now attracted by the idea of using ftrace in kprobe-tracer, because of simplicity. Anyway, here is my review.

It may be a little simpler, but it defeats the purpose of what I'm trying to accomplish. I do want the ability to expand this to do other things than just trace_kprobe.

Quoted 13 lines

Quoted 11 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Currently, if a probe is set at an ftrace nop, the probe will fail.

When -mfentry is used by ftrace (in the near future), the nop will be at the beginning of the function. This will prevent kprobes from hooking to the most common place that kprobes are attached.

Now that ftrace supports individual users as well as pt_regs passing, kprobes can use the ftrace infrastructure when a probe is placed on a ftrace nop.

Quoted 5 lines

My one general concern is the timing of enabling ftrace-based probe. Breakpoint-based kprobe (including optimized kprobe) is enabled right after registering. Users might expect that.
And AFAIK, dynamic ftraces handler will be enabled (activated) after a while, because it has to wait for an NMI, doesn't it?

There's no NMI needed. Not sure where you got that idea. When register_ftrace_function() is called, it will start tracing immediately. Note, stop_machine() is called, but we could fix that in the future too.

Quoted 4 lines

And theoretically, this ftrace-based probe can't support jprobe, because it changes IP address. Nowadays, this may becomes minor problem (because someone who wants to trace function args can use kprobe-tracer), but still exist.

I want to fix that too. :)

We don't need to worry about that until -mfentry exists. But once that does, then, yes, I want the ftrace version working for jprobe too.

Quoted 70 lines

Quoted 70 lines

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/kprobes.h |    6 ++
 kernel/kprobes.c        |  163 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 160 insertions(+), 9 deletions(-)

include/linux/kprobes.h +6 -0

index dd7c12e..5742071 100644
@@ -37,6 +37,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
+#include <linux/ftrace.h>
 
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>

@@ -112,6 +113,11 @@ struct kprobe {
 	/* copy of the original instruction */
 	struct arch_specific_insn ainsn;
 
+#ifdef CONFIG_FUNCTION_TRACER
+	/* If it is possible to use ftrace to probe */
+	struct ftrace_ops fops;
+#endif
+
 	/*
 	 * Indicates various status flags.
 	 * Protected by kprobe_mutex after this kprobe is registered.

kernel/kprobes.c +25 -0

index e6c25eb..2160768 100644
@@ -102,6 +102,31 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
 	{NULL}    /* Terminator */
 };
 
+#ifdef CONFIG_FUNCTION_TRACER
+/* kprobe stub function for use when probe is not connected to ftrace */
+static void
+kprobe_ftrace_stub(unsigned long ip, unsigned long pip,
+		   struct ftrace_ops *op, struct pt_regs *pt_regs)
+{
+}
+
+static bool ftrace_kprobe(struct kprobe *p)
+{
+	return p->fops.func && p->fops.func != kprobe_ftrace_stub;
+}
+
+static void init_non_ftrace_kprobe(struct kprobe *p)
+{
+	p->fops.func = kprobe_ftrace_stub;
+}
+
+#else
+static bool ftrace_kprobe(struct kprobe *p)
+{
+	return false;
+}
+#endif
+
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be

@@ -396,6 +421,9 @@ static __kprobes void free_aggr_kprobe(struct kprobe *p)
 {
 	struct optimized_kprobe *op;
 

Quoted 7 lines

kernel/kprobes.c static __kprobes void try_to_optimize_kprobe(struct

Quoted 7 lines

kernel/kprobes.c +3 -0

+	if (ftrace_kprobe(p))
+		return;
+
 	op = container_of(p, struct optimized_kprobe, kp);
 	arch_remove_optimized_kprobe(op);
 	arch_remove_kprobe(p);

@@ -759,6 +787,9 @@ static __kprobes void try_to_optimize_kprobe(struct

Quoted 46 lines

kernel/kprobes.c static void __kprobes __disarm_kprobe(struct kprobe *p,

Quoted 21 lines

kernel/kprobes.c +7 -0

 	struct kprobe *ap;
 	struct optimized_kprobe *op;
 
+	if (ftrace_kprobe(p))
+		return;
+
 	ap = alloc_aggr_kprobe(p);
 	if (!ap)
 		return;

@@ -849,6 +880,10 @@ static void __kprobes __arm_kprobe(struct kprobe *p)
 {
 	struct kprobe *_p;
 
+	/* Only arm non-ftrace probes */
+	if (ftrace_kprobe(p))
+		return;
+
 	/* Check collision with other optimized kprobes */
 	_p = get_optimized_kprobe((unsigned long)p->addr);
 	if (unlikely(_p))

@@ -864,6 +899,10 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,

bool reopt)

kernel/kprobes.c static void __kprobes __disarm_kprobe(struct kprobe *p,

Quoted 11 lines

kernel/kprobes.c +4 -0

 {
 	struct kprobe *_p;
 
+	/* Only disarm non-ftrace probes */
+	if (ftrace_kprobe(p))
+		return;
+
 	unoptimize_kprobe(p, false);	/* Try to unoptimize */
 
 	if (!kprobe_queued(p)) {

@@ -878,13 +917,26 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,

bool reopt)

Quoted 12 lines

kernel/kprobes.c +9 -0

 #else /* !CONFIG_OPTPROBES */
 
+static void __kprobes __arm_kprobe(struct kprobe *p)
+{
+	/* Only arm non-ftrace probes */
+	if (!ftrace_kprobe(p))
+		arch_arm_kprobe(p);
+}
+
+/* Remove the breakpoint of a probe. Must be called with text_mutex locked */
+static void __kprobes __disarm_kprobe(struct kprobe *p, bool reopt)

Quoted 8 lines

kernel/kprobes.c static void __kprobes __disarm_kprobe(struct kprobe *p, bool reopt) · line 933

Quoted 5 lines

kernel/kprobes.c +5 -0

+{
+	/* Only disarm non-ftrace probes */
+	if (!ftrace_kprobe(p))
+		arch_disarm_kprobe(p);
+}

If it ignores disabling/enabling, kprobe_ftrace_callback must check kprobe_disabled(p) and skip it.

No, the callers of __(dis)arm_kprobe() also call the (dis)arm_kprobe() later, which calls (un)register_ftrace_function(). The reason we can't call the ftrace register functions here is because they call stop_machine() and then we get into a deadlock with the text_mutex. The places that call __(dis)arm_kprobe() later call (dis)arm_kprobe() after releasing the text_mutex.

Quoted 53 lines

kernel/kprobes.c static __kprobes struct kprobe *alloc_aggr_kprobe(struct

Quoted 23 lines

kernel/kprobes.c +4 -3

+
 #define optimize_kprobe(p)			do {} while (0)
 #define unoptimize_kprobe(p, f)			do {} while (0)
 #define kill_optimized_kprobe(p)		do {} while (0)
 #define prepare_optimized_kprobe(p)		do {} while (0)
 #define try_to_optimize_kprobe(p)		do {} while (0)
-#define __arm_kprobe(p)				arch_arm_kprobe(p)
-#define __disarm_kprobe(p, o)			arch_disarm_kprobe(p)
 #define kprobe_disarmed(p)			kprobe_disabled(p)
 #define wait_for_kprobe_optimizer()		do {} while (0)
 

@@ -897,7 +949,9 @@ static void reuse_unused_kprobe(struct kprobe *ap)
 
 static __kprobes void free_aggr_kprobe(struct kprobe *p)
 {
-	arch_remove_kprobe(p);
+
+	if (!ftrace_kprobe(p))
+		arch_remove_kprobe(p);
 	kfree(p);
 }
 

@@ -910,6 +964,12 @@ static __kprobes struct kprobe *alloc_aggr_kprobe(struct

kprobe *p)

Quoted 29 lines

kernel/kprobes.c +12 -0

 /* Arm a kprobe with text_mutex */
 static void __kprobes arm_kprobe(struct kprobe *kp)
 {
+	/* ftrace probes can skip arch calls */
+	if (ftrace_kprobe(kp)) {
+		register_ftrace_function(&kp->fops);
+		return;
+	}
+
 	/*
 	 * Here, since __arm_kprobe() doesn't use stop_machine(),
 	 * this doesn't cause deadlock on text_mutex. So, we don't

@@ -924,6 +984,12 @@ static void __kprobes arm_kprobe(struct kprobe *kp)
 static void __kprobes disarm_kprobe(struct kprobe *kp)
 {
 	/* Ditto */
+
+	if (ftrace_kprobe(kp)) {
+		unregister_ftrace_function(&kp->fops);
+		return;
+	}
+
 	mutex_lock(&text_mutex);
 	__disarm_kprobe(kp, true);
 	mutex_unlock(&text_mutex);

@@ -1313,6 +1379,56 @@ static inline int check_kprobe_rereg(struct kprobe *p)
 	return ret;
 }
 

Quoted 11 lines

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1388

Quoted 7 lines

kernel/kprobes.c +7 -0

+#ifdef CONFIG_FUNCTION_TRACER
+static notrace void
+kprobe_ftrace_callback(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct pt_regs *pt_regs)
+{
+	struct kprobe *p = container_of(op, struct kprobe, fops);
+

Here, we need to set up kprobe_ctlblk and some of pt_regs members, ip, cs and orig_ax as optimized_callback()@arch/x86/kernel/kprobes.c does.

I'm curious to what this is used for? It doesn't seem to be needed for the generic kprobes. Because we know the probe was on a nop, there's no need to simulate the operation. IOW, there's no need for singlestep() or other gdb like operations.

Quoted 6 lines

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1391

Quoted 4 lines

kernel/kprobes.c +3 -0

+	/* A nop has been trapped, just run both handlers back to back */
+	if (p->pre_handler)
+		p->pre_handler(p, pt_regs);

And increment regs->ip here for NOP.

Does the post_handler() expect the ip to be after the call? Thus a post_handle() is the same as pre_handle() on rip+next_ins?

Quoted 7 lines

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1394

Quoted 3 lines

kernel/kprobes.c +3 -0

+	if (p->post_handler)
+		p->post_handler(p, pt_regs, 0);
+}

Anyway, above operations strongly depends on arch, so kprobe_ftrace_callback should be moved to arch/*/kprobes.c.

Quoted 1 line

And I think most of the code can be shared with optimized code.

Not sure why. Except if regs->ip needs to be incremented. And that can be a per arch header file. Remember, the ftrace version has no need for single stepping like the optimized version does. ftrace replaces a nop, where as other kprobes replace actual instructions. This is what make the ftrace version so much less complex.

Quoted 9 lines

Quoted 9 lines

kernel/kprobes.c +9 -0

+
+static int use_ftrace_hook(struct kprobe *p)
+{
+	char str[KSYM_SYMBOL_LEN];
+
+	/* see if this is an ftrace function */
+	if (!ftrace_text_reserved(p->addr, p->addr)) {
+		/* make sure fops->func is nop */
+		init_non_ftrace_kprobe(p);

Quoted 8 lines

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1409

Quoted 6 lines

kernel/kprobes.c +6 -0

+		return 0;
+	}
+
+	/* If arch does not support pt_regs passing, bail */
+	if (!ARCH_SUPPORTS_FTRACE_SAVE_REGS)
+		return -EINVAL;

Hmm, I think this should be checked at build time...

This is actually keeping the same logic that exists now. See below, we removed the check of ftrace_text_reserved() from the conditions that make register_kprobe() return -EINVAL. Now if the arch supports FTRACE_SAVE_REGS, it can handle the ftrace_text_reserved(), if not, then it goes back to its original behavior.

The only way to remove the later code is with #ifdef ugliness, and I didn't want to add that.

Quoted 12 lines

Quoted 12 lines

kernel/kprobes.c +10 -0

+
+	/* Use ftrace hook instead */
+
+	memset(&p->fops, 0, sizeof(p->fops));
+
+	/* Find the function this is connected with this addr */
+	kallsyms_lookup((unsigned long)p->addr, NULL, NULL, NULL, str);
+
+	p->fops.flags = FTRACE_OPS_FL_SAVE_REGS;
+	p->fops.func = kprobe_ftrace_callback;

Quoted 8 lines

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1421

Quoted 2 lines

kernel/kprobes.c +2 -0

+
+	ftrace_set_filter(&p->fops, str, strlen(str), 1);

Hmm, IMHO, ftrace_set_filter should not be called here, because there can be other kprobes are already registered on the same address. In that case, it is natural that we use an aggr_kprobe for handling several kprobes on same address. Or, kprobe hash table will have several different probes on same address.

The ftrace_set_filter() only updates the p->fops to what it will trace. It's the register_ftrace_function() that enables the ftrace tracing, and that is done with the arm_kprobe() call. If that's where the aggr_kprobe enables its call, then that will be the only probe that is called.

Now I need to re-examine how the aggr_kprobes work. Does it have its own handler to call multiple probe->handlers?

Quoted 53 lines

Quoted 53 lines

kernel/kprobes.c +20 -5

+
+	return 0;
+}
+#else
+static int use_ftrace_hook(struct kprobe *p)
+{
+	return 0;
+}
+#endif
+
 int __kprobes register_kprobe(struct kprobe *p)
 {
 	int ret = 0;

@@ -1329,11 +1445,14 @@ int __kprobes register_kprobe(struct kprobe *p)
 	if (ret)
 		return ret;
 
+	ret = use_ftrace_hook(p);
+	if (ret)
+		return ret;
+
 	jump_label_lock();
 	preempt_disable();
 	if (!kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr) ||
-	    ftrace_text_reserved(p->addr, p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr))
 		goto fail_with_jump_label;
 

@@ -1384,15 +1503,17 @@ int __kprobes register_kprobe(struct kprobe *p)
 		goto out;
 	}
 
-	ret = arch_prepare_kprobe(p);
-	if (ret)
-		goto out;
+	if (!ftrace_kprobe(p)) {
+		ret = arch_prepare_kprobe(p);
+		if (ret)
+			goto out;
+	}
 
 	INIT_HLIST_NODE(&p->hlist);
 	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
-	if (!kprobes_all_disarmed && !kprobe_disabled(p))
+	if (!ftrace_kprobe(p) && !kprobes_all_disarmed && !kprobe_disabled(p))
 		__arm_kprobe(p);
 
 	/* Try to optimize kprobe */

@@ -1400,6 +1521,12 @@ int __kprobes register_kprobe(struct kprobe *p)
 

Quoted 14 lines

kernel/kprobes.c int __kprobes register_kprobe(struct kprobe *p) · line 1532

Quoted 11 lines

kernel/kprobes.c +6 -0

 out:
 	mutex_unlock(&text_mutex);
+
+	/* ftrace kprobes must be set outside of text_mutex */
+	if (!ret && ftrace_kprobe(p) &&
+	    !kprobes_all_disarmed && !kprobe_disabled(p))
+		arm_kprobe(p);
+
 	put_online_cpus();
 	jump_label_unlock();
 	mutex_unlock(&kprobe_mutex);

After this, we must handle some fails which can happen when probing on a module.

What problems that were added by ftrace that isn't a problem with normal probes?

Quoted 40 lines

Quoted 40 lines

kernel/kprobes.c +19 -1

@@ -2134,6 +2261,14 @@ static void __kprobes arm_all_kprobes(void)
 	}
 	mutex_unlock(&text_mutex);
 
+	/* ftrace kprobes are enabled outside of text_mutex */
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+		head = &kprobe_table[i];
+		hlist_for_each_entry_rcu(p, node, head, hlist)
+			if (ftrace_kprobe(p) && !kprobe_disabled(p))
+				arm_kprobe(p);
+	}
+
 	kprobes_all_disarmed = false;
 	printk(KERN_INFO "Kprobes globally enabled\n");
 

@@ -2164,11 +2299,21 @@ static void __kprobes disarm_all_kprobes(void)
 	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
 		head = &kprobe_table[i];
 		hlist_for_each_entry_rcu(p, node, head, hlist) {
-			if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
+			if (!ftrace_kprobe(p) &&
+			    !arch_trampoline_kprobe(p) && !kprobe_disabled(p))
 				__disarm_kprobe(p, false);
 		}
 	}
 	mutex_unlock(&text_mutex);
+
+	/* ftrace kprobes are disabled outside of text_mutex */
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+		head = &kprobe_table[i];
+		hlist_for_each_entry_rcu(p, node, head, hlist) {
+			if (ftrace_kprobe(p) && !kprobe_disabled(p))
+				disarm_kprobe(p);
+		}
+	}
 	mutex_unlock(&kprobe_mutex);
 

Quoted 4 lines

kernel/kprobes.c static void __kprobes disarm_all_kprobes(void) · line 2319

Quoted 1 line

kernel/kprobes.c +0 -0

 	/* Wait for disarming all kprobes by optimizer */

Thank you,

Thanks for taking the time for your review!

-- Steve

On Thu, 2011-08-11 at 16:41 +0900, Masami Hiramatsu wrote:

Quoted 11 lines

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1388

Quoted 7 lines

kernel/kprobes.c +7 -0

+#ifdef CONFIG_FUNCTION_TRACER
+static notrace void
+kprobe_ftrace_callback(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct pt_regs *pt_regs)
+{
+	struct kprobe *p = container_of(op, struct kprobe, fops);
+

Here, we need to set up kprobe_ctlblk and some of pt_regs members, ip, cs and orig_ax as optimized_callback()@arch/x86/kernel/kprobes.c does.

I'm curious to what this is used for? It doesn't seem to be needed for the generic kprobes. Because we know the probe was on a nop, there's no need to simulate the operation. IOW, there's no need for singlestep() or other gdb like operations.

Quoted 1 line · Steven Rostedt

On Thu, 2011-08-11 at 16:41 +0900, Masami Hiramatsu wrote:

...

Quoted 12 lines

Quoted 11 lines

kernel/kprobes.c static inline int check_kprobe_rereg(struct kprobe *p) · line 1388

Quoted 7 lines

kernel/kprobes.c +7 -0

+#ifdef CONFIG_FUNCTION_TRACER
+static notrace void
+kprobe_ftrace_callback(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct pt_regs *pt_regs)
+{
+	struct kprobe *p = container_of(op, struct kprobe, fops);
+

Here, we need to set up kprobe_ctlblk and some of pt_regs members, ip, cs and orig_ax as optimized_callback()@arch/x86/kernel/kprobes.c does.

Quoted 4 lines

I'm curious to what this is used for? It doesn't seem to be needed for the generic kprobes. Because we know the probe was on a nop, there's no need to simulate the operation. IOW, there's no need for singlestep() or other gdb like operations.

It is needed to handle recursion, for instance. If in the rare case a kprobe handler calls another routine which also has a kprobe installed (and so on), we just bypass calling the 2nd kprobe's handlers. To do this and to set back the original kprobe context, we track the kprobe state in the per_cpu kprobe_ctlblk.

Hi All,

I started working on adding the -mfentry switch to ftrace, which allows us to remove the frame pointers requirement from function tracing as well as makes mcount (fentry) work just better.

But when I did this in another branch, I noticed that I broke kprobes in its most common usage. The attaching a probe at the beginning of a function to use get to its parameters.

So I started this branch. This branch is to have kprobes use ftrace directly when a probe is attached to a ftrace nop. Currently, kprobes will just error when that happens. With this patch set, it will hook into the ftrace infrastructure and use ftrace instead. This is more like an optimized probe as no breakpoints need to be set. A call to the function is done directly via the mcount trampoline. If ftrace pt_regs is implemented for an arch, kprobes gets this feature for free.

The first patch is just a clean up that I need to push out to get rid of the annoying compile warning about initialized variables that gcc can't tell have been initialized.

The next two patches have ftrace pass both the ftrace_ops structure and the pt_regs to the callback function that is registered with ftrace.

The last two patches have kprobes interact with ftrace and use the ftrace infrastructure instead.

I only did this for x86_64, and will do it for x86_32 and PPC64 if everyone agrees with this approach. Then I could find people to do it for other archs :)

Hi Steven,

Thanks for this nice feature!

Quoted 10 lines · Steven Rostedt

Hi All,

I started working on adding the -mfentry switch to ftrace, which allows us to remove the frame pointers requirement from function tracing as well as makes mcount (fentry) work just better.

But when I did this in another branch, I noticed that I broke kprobes in its most common usage. The attaching a probe at the beginning of a function to use get to its parameters.

Quoted 7 lines

So I started this branch. This branch is to have kprobes use ftrace directly when a probe is attached to a ftrace nop. Currently, kprobes will just error when that happens. With this patch set, it will hook into the ftrace infrastructure and use ftrace instead. This is more like an optimized probe as no breakpoints need to be set. A call to the function is done directly via the mcount trampoline. If ftrace pt_regs is implemented for an arch, kprobes gets this feature for free.

I agreed this idea, this looks good to me too :) With -fentry, this can improve dynamic trace events very much.

BTW (OT), it seems that current kprobe data structure becomes a bit fat. Maybe what we need is just a "holder of hooking handler" as what ftrace provides, not a full storage data structure of copied instrucutions. Perhaps, we'd better diet the kprobe structure for transparency of hooking infrastructure.

Quoted 8 lines

The first patch is just a clean up that I need to push out to get rid of the annoying compile warning about initialized variables that gcc can't tell have been initialized.

The next two patches have ftrace pass both the ftrace_ops structure and the pt_regs to the callback function that is registered with ftrace.

Quoted 6 lines

The last two patches have kprobes interact with ftrace and use the ftrace infrastructure instead.

I only did this for x86_64, and will do it for x86_32 and PPC64 if everyone agrees with this approach. Then I could find people to do it for other archs :)

OK, I'll review this soon!

Quoted 26 lines · Masami Hiramatsu

Hi Steven,

Thanks for this nice feature!

Quoted 10 lines · Steven Rostedt

Hi All,

I started working on adding the -mfentry switch to ftrace, which allows us to remove the frame pointers requirement from function tracing as well as makes mcount (fentry) work just better.

But when I did this in another branch, I noticed that I broke kprobes in its most common usage. The attaching a probe at the beginning of a function to use get to its parameters.

Quoted 7 lines

So I started this branch. This branch is to have kprobes use ftrace directly when a probe is attached to a ftrace nop. Currently, kprobes will just error when that happens. With this patch set, it will hook into the ftrace infrastructure and use ftrace instead. This is more like an optimized probe as no breakpoints need to be set. A call to the function is done directly via the mcount trampoline. If ftrace pt_regs is implemented for an arch, kprobes gets this feature for free.

I agreed this idea, this looks good to me too :) With -fentry, this can improve dynamic trace events very much.

Quoted 5 lines

BTW (OT), it seems that current kprobe data structure becomes a bit fat. Maybe what we need is just a "holder of hooking handler" as what ftrace provides, not a full storage data structure of copied instrucutions. Perhaps, we'd better diet the kprobe structure for transparency of hooking infrastructure.

Sure, I can make the ftrace_ops field in kprobes dynamically allocated instead. That shouldn't be an issue.

Quoted 27 lines · Steven Rostedt

Quoted 26 lines · Masami Hiramatsu

Hi Steven,

Thanks for this nice feature!

Quoted 10 lines · Steven Rostedt

Hi All,

I started working on adding the -mfentry switch to ftrace, which allows us to remove the frame pointers requirement from function tracing as well as makes mcount (fentry) work just better.

But when I did this in another branch, I noticed that I broke kprobes in its most common usage. The attaching a probe at the beginning of a function to use get to its parameters.

Quoted 7 lines

So I started this branch. This branch is to have kprobes use ftrace directly when a probe is attached to a ftrace nop. Currently, kprobes will just error when that happens. With this patch set, it will hook into the ftrace infrastructure and use ftrace instead. This is more like an optimized probe as no breakpoints need to be set. A call to the function is done directly via the mcount trampoline. If ftrace pt_regs is implemented for an arch, kprobes gets this feature for free.

I agreed this idea, this looks good to me too :) With -fentry, this can improve dynamic trace events very much.

Quoted 8 lines

Quoted 5 lines

BTW (OT), it seems that current kprobe data structure becomes a bit fat. Maybe what we need is just a "holder of hooking handler" as what ftrace provides, not a full storage data structure of copied instrucutions. Perhaps, we'd better diet the kprobe structure for transparency of hooking infrastructure.

Sure, I can make the ftrace_ops field in kprobes dynamically allocated instead. That shouldn't be an issue.

By the way (again), perhaps, much simpler solution is using ftrace not in kprobe, but in the trace_kprobe. Of course, there are several pros and cons...

The pros:
- Arch independent solution (anyway, ftrace still needs passing pt_regs to their handler)
- Don't need to introduce more complexity into kprobes itself.
- Maybe systemtap also can catch up with this as using same method.

The cons:
- Native kprobes users will be disappointed... anyway, they just need to move their probes to the next instruction (usually addr+=5 is OK).

... are there any other cons? :)

Thank you,

Quoted 41 lines · Masami Hiramatsu

Quoted 27 lines · Steven Rostedt

Quoted 26 lines · Masami Hiramatsu

Hi Steven,

Thanks for this nice feature!

Quoted 10 lines · Steven Rostedt

Hi All,

I started working on adding the -mfentry switch to ftrace, which allows us to remove the frame pointers requirement from function tracing as well as makes mcount (fentry) work just better.

But when I did this in another branch, I noticed that I broke kprobes in its most common usage. The attaching a probe at the beginning of a function to use get to its parameters.

Quoted 7 lines

So I started this branch. This branch is to have kprobes use ftrace directly when a probe is attached to a ftrace nop. Currently, kprobes will just error when that happens. With this patch set, it will hook into the ftrace infrastructure and use ftrace instead. This is more like an optimized probe as no breakpoints need to be set. A call to the function is done directly via the mcount trampoline. If ftrace pt_regs is implemented for an arch, kprobes gets this feature for free.

I agreed this idea, this looks good to me too :) With -fentry, this can improve dynamic trace events very much.

Quoted 8 lines

Quoted 5 lines

BTW (OT), it seems that current kprobe data structure becomes a bit fat. Maybe what we need is just a "holder of hooking handler" as what ftrace provides, not a full storage data structure of copied instrucutions. Perhaps, we'd better diet the kprobe structure for transparency of hooking infrastructure.

Sure, I can make the ftrace_ops field in kprobes dynamically allocated instead. That shouldn't be an issue.

By the way (again), perhaps, much simpler solution is using ftrace not in kprobe, but in the trace_kprobe. Of course, there are several pros and cons...

Quoted 5 lines

The pros:
- Arch independent solution (anyway, ftrace still needs passing pt_regs to their handler)
- Don't need to introduce more complexity into kprobes itself.
- Maybe systemtap also can catch up with this as using same method.

Note that systemtap and others will be hooking into kprobes version, not the trace_kprobe one. If we do it in trace_kprobe, then everyone else needs to reimplement it too. I have bigger ideas for the future of this, and I really want to get this working. If it doesn't work for kprobes, then it won't work for anything else.

Quoted 3 lines

The cons:
- Native kprobes users will be disappointed... anyway, they just need to move their probes to the next instruction (usually addr+=5 is OK).

I've been told that doing the addr+=5 (which is also arch specific) can break things for other tools.

I don't think it won't work. It can work but on a long way. Could you tell me your "bigger ideas"? Perhaps, we are on the different way but aim to same goal.

Quoted 7 lines

Quoted 4 lines

The cons:
- Native kprobes users will be disappointed... anyway, they just need to move their probes to the next instruction (usually addr+=5 is OK).

I've been told that doing the addr+=5 (which is also arch specific) can break things for other tools.

As I told in previous mail, I think kprobes can do that transparently.

Quoted 3 lines · Masami Hiramatsu

I don't think it won't work. It can work but on a long way. Could you tell me your "bigger ideas"? Perhaps, we are on the different way but aim to same goal.

Part of the bigger ideas is to have things like function graph tracing use this, as it will simplify the entry.S code. There's other things that may come out of this too.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

The cons:
- Native kprobes users will be disappointed... anyway, they just need to move their probes to the next instruction (usually addr+=5 is OK).

I've been told that doing the addr+=5 (which is also arch specific) can break things for other tools.

Quoted 1 line

As I told in previous mail, I think kprobes can do that transparently.

Yeah, it could, but I'm afraid this may need to be done differently for every arch. If I'm working on getting this code to work for ftrace, which will require modifying ever arch as well, kprobes can get the changes for free.

Quoted 9 lines · Steven Rostedt

Quoted 3 lines · Masami Hiramatsu

I don't think it won't work. It can work but on a long way. Could you tell me your "bigger ideas"? Perhaps, we are on the different way but aim to same goal.

Part of the bigger ideas is to have things like function graph tracing use this, as it will simplify the entry.S code. There's other things that may come out of this too.

Hmm, I think that the current function graph tracing implementation is more scalable than kretprobes, because kretprobe requires spinlock on every hit. Moreover, you can't probe NMI handler with kprobe, and kprobes on irq-handler are also possible to fail because of recursive-call.
So I don't recommend using kretprobe for function-graph tracer :-(

Quoted 11 lines · Masami Hiramatsu

Quoted 9 lines · Steven Rostedt

Quoted 3 lines · Masami Hiramatsu

I don't think it won't work. It can work but on a long way. Could you tell me your "bigger ideas"? Perhaps, we are on the different way but aim to same goal.

Part of the bigger ideas is to have things like function graph tracing use this, as it will simplify the entry.S code. There's other things that may come out of this too.

Quoted 6 lines

Hmm, I think that the current function graph tracing implementation is more scalable than kretprobes, because kretprobe requires spinlock on every hit. Moreover, you can't probe NMI handler with kprobe, and kprobes on irq-handler are also possible to fail because of recursive-call.
So I don't recommend using kretprobe for function-graph tracer :-(

Sorry for the confusion. My idea is not to use kretprobe with function graph tracer, but to use the ftrace hooks with the pt_regs and friends for function graph tracer instead of what it does today, which is to add function graph code directly into entry.S.

The point I was making is, if I need to get ftrace function tracing being good enough for function graph tracer, then it should work with kprobes without any issues.

If I need to do the work anyway (for
function graph tracing) then why not use it directly with kprobes instead of doing more hooks just in the kprobe_trace?

From the kprobe-tracer point of view, I don't mind. I just care

Quoted 3 lines

The point I was making is, if I need to get ftrace function tracing being good enough for function graph tracer, then it should work with kprobes without any issues.

No, I don't think so, because kprobes user may trace a flags register or segment registers. However, function graph tracer only needs stack register etc. Thus, if ftrace function tracing is good enough for kprobes handlers, it is enough for function graph tracer too.

Quoted 6 lines

If I need to do the work anyway (for
function graph tracing) then why not use it directly with kprobes instead of doing more hooks just in the kprobe_trace?

From the kprobe-tracer point of view, I don't mind. I just care

about complexity, and compatibility of kprobe handlers.

Quoted 8 lines · Masami Hiramatsu

Quoted 3 lines

The point I was making is, if I need to get ftrace function tracing being good enough for function graph tracer, then it should work with kprobes without any issues.

No, I don't think so, because kprobes user may trace a flags register or segment registers. However, function graph tracer only needs stack register etc. Thus, if ftrace function tracing is good enough for kprobes handlers, it is enough for function graph tracer too.

The added code needed for ftrace is not much more. We just need to save all of pt_regs and that should work (I already save most of it). The flags can be saved, but things like compare flags will be useless, as those are used. But compare flags are undefined when calling a function anyway.

Quoted 4 lines

Quoted 3 lines

If I need to do the work anyway (for
function graph tracing) then why not use it directly with kprobes instead of doing more hooks just in the kprobe_trace?

Quoted 2 lines

Quoted 1 line

From the kprobe-tracer point of view, I don't mind. I just care

about complexity, and compatibility of kprobe handlers.

Right, so far there's not much changes to the kprobe side. I'll see if I can make ftrace do all the hard work to get the proper registers. I want to make as much as possible stay in the generic kprobe code, and not touch the arch code. But I may still need to do something like 'increment rip' or whatever.

Note, we will need to touch kprobes one way or another. Either we have kprobes hook to ftrace, or we need to do the nop skip trick, which will probably be as intrusive in the kprobe code.

I see. OK, I'm now looking forward to your work ;)

Quoted 1 line · Masami Hiramatsu

I see. OK, I'm now looking forward to your work ;)

Hi Masami,

BTW, do you have any test suits that I could use to test the changes I make. Something that I may be able to catch regressions with?