[RFC][PATCH 0/2] ftrace: Add access to function arguments for all callbacks

[RFC][PATCH 0/2] ftrace: Add access to function arguments for all callbacks

[Steven Rostedt]

This is something I wanted to implement a long time ago, but held off until
there was a good reason to do so. Now it appears that having access to the
arguments of the function by default is very useful. As a bonus, because
arguments must be saved regardless before calling a callback, because they
need to be restored before returning back to the start of the traced
function, there's not much work to do to have them always be there for
normal function callbacks.

The basic idea is that if CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS is set, then
all callbacks registered to ftrace can use the regs parameter for the stack
and arguments (kernel_stack_pointer(regs), regs_get_kernel_argument(regs, n)),
without the need to set REGS that causes overhead by saving all registers as
REGS simulates a breakpoint.

Only the first patch is to be applied. The second patch is just to show how
this could work.

Steven Rostedt (VMware) (2):
      ftrace/x86: Allow for arguments to be passed in to REGS by default
      ftrace: Test arguments by adding trace_printk in function tracer

----
 arch/x86/Kconfig               |  1 +
 arch/x86/kernel/ftrace_64.S    | 12 +++++++-----
 kernel/trace/Kconfig           |  9 +++++++++
 kernel/trace/trace_functions.c | 13 ++++++++++++-
 4 files changed, 29 insertions(+), 6 deletions(-)

[RFC][PATCH 1/2] ftrace/x86: Allow for arguments to be passed in to REGS by default

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Currently, the only way to get access to the registers of a function via a
ftrace callback is to set the "FL_FLAGS" bit in the ftrace_ops. But as this
saves all regs as if a breakpoint were to trigger (for use with kprobes), it
is expensive.

The regs are already saved on the stack for the default ftrace callbacks, as
that is required otherwise a function being traced will get the wrong
arguments and possibly crash. And on x86, the argumentes are already stored
where they would be on a pt_regs structure to use that code for both the
regs version of a callback, it makes sense to pass that information always
to all functions.

If an architecture does this (as x86 now does), it is to set
HAVE_DYNAMIC_FTRACE_WITH_ARGS, and this will let the generic code that it
could use the regs without having to set the flags if it wants to access the
arguments from the regs pointer.

The stack pointer is also saved in the regs, and this could be useful for
having the function graph tracer not require its own trampoline!

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/Kconfig            |  1 +
 arch/x86/kernel/ftrace_64.S | 12 +++++++-----
 kernel/trace/Kconfig        |  9 +++++++++
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 7101ac64bb20..b4d2b1fcfd09 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -167,6 +167,7 @@ config X86
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if X86_64
 	select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	select HAVE_EBPF_JIT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index ac3d5f22fe64..77c512334b4c 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -86,6 +86,11 @@
 	movq %r8, R8(%rsp)
 	movq %r9, R9(%rsp)
 	movq $0, ORIG_RAX(%rsp)
+
+	/* Stack - skipping return address and flags */
+	leaq MCOUNT_REG_SIZE+\added+8(%rsp), %rcx
+	movq %rcx, RSP(%rsp)
+
 	/*
 	 * Save the original RBP. Even though the mcount ABI does not
 	 * require this, it helps out callers.
@@ -144,8 +149,8 @@ SYM_INNER_LABEL(ftrace_caller_op_ptr, SYM_L_GLOBAL)
 	/* Load the ftrace_ops into the 3rd parameter */
 	movq function_trace_op(%rip), %rdx
 
-	/* regs go into 4th parameter (but make it NULL) */
-	movq $0, %rcx
+	/* regs go into 4th parameter */
+	leaq (%rsp), %rcx
 
 SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	call ftrace_stub
@@ -204,9 +209,6 @@ SYM_INNER_LABEL(ftrace_regs_caller_op_ptr, SYM_L_GLOBAL)
 	movq %rcx, SS(%rsp)
 	movq $__KERNEL_CS, %rcx
 	movq %rcx, CS(%rsp)
-	/* Stack - skipping return address and flags */
-	leaq MCOUNT_REG_SIZE+8*2(%rsp), %rcx
-	movq %rcx, RSP(%rsp)
 
 	ENCODE_FRAME_POINTER
 
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index a4020c0b4508..6a5b7a818d7d 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -31,6 +31,15 @@ config HAVE_DYNAMIC_FTRACE_WITH_REGS
 config HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	bool
 
+config HAVE_DYNAMIC_FTRACE_WITH_ARGS
+	bool
+	help
+	 If this is set, then arguments and stack can be found from
+	 the pt_regs passed into the function callback regs parameter
+	 by default, even without setting the REGS flag in the ftrace_ops.
+	 This allows for use of regs_get_kernel_argument() and
+	 kernel_stack_pointer().
+
 config HAVE_FTRACE_MCOUNT_RECORD
 	bool
 	help
-- 
2.28.0

[RFC][PATCH 2/2] ftrace: Test arguments by adding trace_printk in function tracer

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_functions.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 89c414ce1388..4bd3194540f1 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -129,7 +129,7 @@ static void function_trace_start(struct trace_array *tr)
 
 static void
 function_trace_call(unsigned long ip, unsigned long parent_ip,
-		    struct ftrace_ops *op, struct pt_regs *pt_regs)
+		    struct ftrace_ops *op, struct pt_regs *regs)
 {
 	struct trace_array *tr = op->private;
 	struct trace_array_cpu *data;
@@ -153,6 +153,17 @@ function_trace_call(unsigned long ip, unsigned long parent_ip,
 	if (!atomic_read(&data->disabled)) {
 		local_save_flags(flags);
 		trace_function(tr, ip, parent_ip, flags, pc);
+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+		trace_printk("%pS %lx %lx %lx %lx %lx %lx %lx\n",
+			     (void *)ip,
+			     kernel_stack_pointer(regs),
+			     regs_get_kernel_argument(regs, 0),
+			     regs_get_kernel_argument(regs, 1),
+			     regs_get_kernel_argument(regs, 2),
+			     regs_get_kernel_argument(regs, 3),
+			     regs_get_kernel_argument(regs, 4),
+			     regs_get_kernel_argument(regs, 5));
+#endif
 	}
 	ftrace_test_recursion_unlock(bit);
 	preempt_enable_notrace();
-- 
2.28.0

[RFC][PATCH 1/2 v2] ftrace/x86: Allow for arguments to be passed in to REGS by default

[Steven Rostedt]

From b5f5cfc63f38415b4ca7eb4cfb8c78113bfa17e0 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Tue, 27 Oct 2020 10:55:55 -0400
Subject: [PATCH] ftrace/x86: Allow for arguments to be passed in to REGS by
 default

Currently, the only way to get access to the registers of a function via a
ftrace callback is to set the "FL_SAVE_REGS" bit in the ftrace_ops. But as this
saves all regs as if a breakpoint were to trigger (for use with kprobes), it
is expensive.

The regs are already saved on the stack for the default ftrace callbacks, as
that is required otherwise a function being traced will get the wrong
arguments and possibly crash. And on x86, the argumentes are already stored
where they would be on a pt_regs structure to use that code for both the
regs version of a callback, it makes sense to pass that information always
to all functions.

If an architecture does this (as x86 now does), it is to set
HAVE_DYNAMIC_FTRACE_WITH_ARGS, and this will let the generic code that it
could use the regs without having to set the flags if it wants to access the
arguments from the regs pointer.

The stack pointer is also saved in the regs, and this could be useful for
having the function graph tracer not require its own trampoline!

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
Changes since v1:

 - Fixed change log from "FL_FLAGS" to "FL_SAVE_REGS"
 - Add ftrace_valid_regs() macro to test if regs are safe to use
   for full regs operations.


 arch/x86/Kconfig              |  1 +
 arch/x86/include/asm/ftrace.h |  4 ++++
 arch/x86/kernel/ftrace_64.S   | 15 ++++++++++-----
 include/linux/ftrace.h        |  4 ++++
 kernel/trace/Kconfig          |  9 +++++++++
 kernel/trace/ftrace.c         |  2 +-
 6 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 7101ac64bb20..b4d2b1fcfd09 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -167,6 +167,7 @@ config X86
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if X86_64
 	select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	select HAVE_EBPF_JIT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 84b9449be080..d3b26d61a1f6 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -49,6 +49,10 @@ struct dyn_arch_ftrace {
 
 #define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
 
+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+#define ftrace_valid_regs(regs) (((struct pt_regs *)(regs))->ip != 0)
+#endif
+
 #endif /*  CONFIG_DYNAMIC_FTRACE */
 #endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_TRACER */
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index ac3d5f22fe64..f769881b1cdf 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -86,6 +86,11 @@
 	movq %r8, R8(%rsp)
 	movq %r9, R9(%rsp)
 	movq $0, ORIG_RAX(%rsp)
+
+	/* Stack - skipping return address and flags */
+	leaq MCOUNT_REG_SIZE+\added+8(%rsp), %rcx
+	movq %rcx, RSP(%rsp)
+
 	/*
 	 * Save the original RBP. Even though the mcount ABI does not
 	 * require this, it helps out callers.
@@ -144,8 +149,11 @@ SYM_INNER_LABEL(ftrace_caller_op_ptr, SYM_L_GLOBAL)
 	/* Load the ftrace_ops into the 3rd parameter */
 	movq function_trace_op(%rip), %rdx
 
-	/* regs go into 4th parameter (but make it NULL) */
-	movq $0, %rcx
+	/* regs go into 4th parameter */
+	leaq (%rsp), %rcx
+
+	/* Only ops with REGS flag set should have RIP set */
+	movq $0, RIP(%rcx)
 
 SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	call ftrace_stub
@@ -204,9 +212,6 @@ SYM_INNER_LABEL(ftrace_regs_caller_op_ptr, SYM_L_GLOBAL)
 	movq %rcx, SS(%rsp)
 	movq $__KERNEL_CS, %rcx
 	movq %rcx, CS(%rsp)
-	/* Stack - skipping return address and flags */
-	leaq MCOUNT_REG_SIZE+8*2(%rsp), %rcx
-	movq %rcx, RSP(%rsp)
 
 	ENCODE_FRAME_POINTER
 
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8dde9c17aaa5..fbc095393f9b 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -90,6 +90,10 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
 
 struct ftrace_ops;
 
+#ifndef ftrace_valid_regs
+#define ftrace_valid_regs(regs) ((regs) != NULL)
+#endif
+
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
 			      struct ftrace_ops *op, struct pt_regs *regs);
 
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index a4020c0b4508..6a5b7a818d7d 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -31,6 +31,15 @@ config HAVE_DYNAMIC_FTRACE_WITH_REGS
 config HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	bool
 
+config HAVE_DYNAMIC_FTRACE_WITH_ARGS
+	bool
+	help
+	 If this is set, then arguments and stack can be found from
+	 the pt_regs passed into the function callback regs parameter
+	 by default, even without setting the REGS flag in the ftrace_ops.
+	 This allows for use of regs_get_kernel_argument() and
+	 kernel_stack_pointer().
+
 config HAVE_FTRACE_MCOUNT_RECORD
 	bool
 	help
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 2dcae8251104..6b7e097a8c5e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1484,7 +1484,7 @@ ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs)
 	 * that wants regs, may be called without them. We can not
 	 * allow that handler to be called if regs is NULL.
 	 */
-	if (regs == NULL && (ops->flags & FTRACE_OPS_FL_SAVE_REGS))
+	if (!ftrace_valid_regs(regs) && (ops->flags & FTRACE_OPS_FL_SAVE_REGS))
 		return 0;
 #endif
 
-- 
2.25.4

Re: [RFC][PATCH 1/2 v2] ftrace/x86: Allow for arguments to be passed in to REGS by default

[Steven Rostedt]

Hi Masami,

Talking with Peter and Thomas on IRC, where they really don't like
passing a partial pt_regs around, got me thinking of redoing the REGS
parameter of ftrace. Kprobes is the only user that requires the full
registers being saved, and that's only because some kprobe user might
want them.

On Wed, 28 Oct 2020 10:25:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
>
>  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
>  			      struct ftrace_ops *op, struct pt_regs *regs);
>  


Most registers of pt_regs at a start of a function is rather useless.
What if we got rid of FL_SAVE_REGS all together and had a "ftrace_regs"
structure passed in that would have only access to all the argument
registers, the stack pointer and the instruction pointer?

Then kprobes could just create its own pt_regs, fill in all the data
from ftrace_regs and then fill the rest with zeros or possibly whatever
the values currently are (does it really matter what those registers
are?), including flags.

Not only would this simplify the code, it would probably allow moving
more of the kprobe code from the arch specific to the generic code, and
remove a lot of duplication.

This would also help speed up the processing of live kernel patching.

And best of all, it would give everything access to the arguments of a
function and a stack pointer with out (ab)using pt_regs.

Do you think this would be feasible?

-- Steve

Re: [RFC][PATCH 1/2 v2] ftrace/x86: Allow for arguments to be passed in to REGS by default

[Steven Rostedt]

On Wed, 28 Oct 2020 11:29:16 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Then kprobes could just create its own pt_regs, fill in all the data
> from ftrace_regs and then fill the rest with zeros or possibly whatever
> the values currently are (does it really matter what those registers
> are?), including flags.

Here's a proof of concept patch. It passes all the kprobe self tests in
the ftracetest suite. This is just a proof of concept, and I already
know of a couple of subtle bugs that are easy to fix. But this shows
the general idea.

(this is based on my ftrace/core branch)

-- Steve

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 7101ac64bb20..b4d2b1fcfd09 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -167,6 +167,7 @@ config X86
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if X86_64
 	select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	select HAVE_EBPF_JIT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 84b9449be080..0b751f94c7ec 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -49,6 +49,10 @@ struct dyn_arch_ftrace {
 
 #define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
 
+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+#define ftrace_valid_regs(regs) (((struct pt_regs *)(regs))->cs != 0)
+#endif
+
 #endif /*  CONFIG_DYNAMIC_FTRACE */
 #endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_TRACER */
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index ac3d5f22fe64..f8833fe0ff44 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -140,16 +140,27 @@ SYM_FUNC_START(ftrace_caller)
 	/* save_mcount_regs fills in first two parameters */
 	save_mcount_regs
 
+	/* Stack - skipping return address and flags */
+	leaq MCOUNT_REG_SIZE+8(%rsp), %rcx
+	movq %rcx, RSP(%rsp)
+
 SYM_INNER_LABEL(ftrace_caller_op_ptr, SYM_L_GLOBAL)
 	/* Load the ftrace_ops into the 3rd parameter */
 	movq function_trace_op(%rip), %rdx
 
-	/* regs go into 4th parameter (but make it NULL) */
-	movq $0, %rcx
+	/* regs go into 4th parameter */
+	leaq (%rsp), %rcx
+
+	/* Only ops with REGS flag set should have CS set */
+	movq $0, CS(%rsp)
 
 SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	call ftrace_stub
 
+	/* Handlers can change the RIP */
+	movq RIP(%rsp), %rax
+	movq %rax, MCOUNT_REG_SIZE(%rsp)
+
 	restore_mcount_regs
 
 	/*
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index a40a6cdfcca3..c3a5b2675397 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -12,18 +12,42 @@
 
 #include "common.h"
 
+/*
+ * Note, this is just a proof of concept, ftrace_regs will
+ * be of its own type (struct ftrace_regs) and this function
+ * will be architecture dependent, to fill pt_regs from
+ * the ftrace_regs structure. But for now, we simply just copy it.
+ */
+static void arch_ftrace_fill_regs(struct pt_regs *regs,
+				  struct pt_regs *ftrace_regs)
+{
+	*regs = *ftrace_regs;
+	local_save_flags(regs->flags);
+	regs->cs = __KERNEL_CS;
+}
+
+static void arch_regs_fill_ftrace(struct pt_regs *ftrace_regs,
+				  struct pt_regs *regs)
+{
+	*ftrace_regs = *regs;
+}
+
 /* Ftrace callback handler for kprobes -- called under preepmt disabed */
 void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
-			   struct ftrace_ops *ops, struct pt_regs *regs)
+			   struct ftrace_ops *ops,
+			   struct pt_regs *ftrace_regs)
 {
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs regs;
 	int bit;
 
 	bit = ftrace_test_recursion_trylock();
 	if (bit < 0)
 		return;
 
+	arch_ftrace_fill_regs(&regs, ftrace_regs);
+
 	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
@@ -33,23 +57,23 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (kprobe_running()) {
 		kprobes_inc_nmissed_count(p);
 	} else {
-		unsigned long orig_ip = regs->ip;
-		/* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
-		regs->ip = ip + sizeof(kprobe_opcode_t);
+		unsigned long orig_ip = regs.ip;
+		/* Kprobe handler expects regs.ip = ip + 1 as breakpoint hit */
+		regs.ip = ip + sizeof(kprobe_opcode_t);
 
 		__this_cpu_write(current_kprobe, p);
 		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
-		if (!p->pre_handler || !p->pre_handler(p, regs)) {
+		if (!p->pre_handler || !p->pre_handler(p, &regs)) {
 			/*
-			 * Emulate singlestep (and also recover regs->ip)
+			 * Emulate singlestep (and also recover regs.ip)
 			 * as if there is a 5byte nop
 			 */
-			regs->ip = (unsigned long)p->addr + MCOUNT_INSN_SIZE;
+			regs.ip = (unsigned long)p->addr + MCOUNT_INSN_SIZE;
 			if (unlikely(p->post_handler)) {
 				kcb->kprobe_status = KPROBE_HIT_SSDONE;
-				p->post_handler(p, regs, 0);
+				p->post_handler(p, &regs, 0);
 			}
-			regs->ip = orig_ip;
+			regs.ip = orig_ip;
 		}
 		/*
 		 * If pre_handler returns !0, it changes regs->ip. We have to
@@ -57,6 +81,7 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		 */
 		__this_cpu_write(current_kprobe, NULL);
 	}
+	arch_regs_fill_ftrace(ftrace_regs, &regs);
 out:
 	preempt_enable_notrace();
 	ftrace_test_recursion_unlock(bit);
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8dde9c17aaa5..fbc095393f9b 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -90,6 +90,10 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
 
 struct ftrace_ops;
 
+#ifndef ftrace_valid_regs
+#define ftrace_valid_regs(regs) ((regs) != NULL)
+#endif
+
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
 			      struct ftrace_ops *op, struct pt_regs *regs);
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9303881aac84..9b552529c6a0 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1020,12 +1020,18 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 #ifdef CONFIG_KPROBES_ON_FTRACE
 static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
 	.flags = FTRACE_OPS_FL_SAVE_REGS,
+#endif
 };
 
 static struct ftrace_ops kprobe_ipmodify_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+	.flags = FTRACE_OPS_FL_IPMODIFY,
+#else
 	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
+#endif
 };
 
 static int kprobe_ipmodify_enabled;
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index a4020c0b4508..6a5b7a818d7d 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -31,6 +31,15 @@ config HAVE_DYNAMIC_FTRACE_WITH_REGS
 config HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	bool
 
+config HAVE_DYNAMIC_FTRACE_WITH_ARGS
+	bool
+	help
+	 If this is set, then arguments and stack can be found from
+	 the pt_regs passed into the function callback regs parameter
+	 by default, even without setting the REGS flag in the ftrace_ops.
+	 This allows for use of regs_get_kernel_argument() and
+	 kernel_stack_pointer().
+
 config HAVE_FTRACE_MCOUNT_RECORD
 	bool
 	help
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 2dcae8251104..d7875cbc17cd 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1484,7 +1484,8 @@ ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs)
 	 * that wants regs, may be called without them. We can not
 	 * allow that handler to be called if regs is NULL.
 	 */
-	if (regs == NULL && (ops->flags & FTRACE_OPS_FL_SAVE_REGS))
+	if ((ops->flags & FTRACE_OPS_FL_SAVE_REGS) &&
+	    (!regs || !ftrace_valid_regs(regs)))
 		return 0;
 #endif
 

Re: [RFC][PATCH 1/2 v2] ftrace/x86: Allow for arguments to be passed in to REGS by default

[Steven Rostedt]

On Wed, 28 Oct 2020 16:36:26 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Here's a proof of concept patch. It passes all the kprobe self tests in
> the ftracetest suite. This is just a proof of concept, and I already
> know of a couple of subtle bugs that are easy to fix. But this shows
> the general idea.

And this works for live patching as well (the below patch passed the
live patching test).

To make sure this really worked, I made the regular trampoline put RIP
into the R15 location, and live patching used that.

I'm going to start working on getting rid of FL_SAVE_REGS, by first
changing the regs parameter to struct ftrace_regs * type, and having
all regs users use ftrace_regs_get_regs(), that will return NULL if
it's not full regs (at least for x86), this way we can keep the current
regs (if kprobes *really* needs it). But then having other accesses for
this. And this way, live patching will no longer need a full regs to
work.

Then by default we can have all callbacks have access to the stack and
arguments!

-- Steve

diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index 1fde1ab6559e..de3bf7ce1c5d 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -14,7 +14,7 @@
 
 static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
 {
-	regs->ip = ip;
+	regs->r15 = ip;
 }
 
 #endif /* _ASM_X86_LIVEPATCH_H */
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index f8833fe0ff44..aad647500818 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -148,6 +148,11 @@ SYM_INNER_LABEL(ftrace_caller_op_ptr, SYM_L_GLOBAL)
 	/* Load the ftrace_ops into the 3rd parameter */
 	movq function_trace_op(%rip), %rdx
 
+	/* put RIP into R15 to test this */
+	movq RIP(%rsp), %rcx
+	movq %rcx, R15(%rsp)
+	movq $0, RIP(%rsp)
+
 	/* regs go into 4th parameter */
 	leaq (%rsp), %rcx
 
@@ -158,7 +163,7 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	call ftrace_stub
 
 	/* Handlers can change the RIP */
-	movq RIP(%rsp), %rax
+	movq R15(%rsp), %rax
 	movq %rax, MCOUNT_REG_SIZE(%rsp)
 
 	restore_mcount_regs
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index c3a5b2675397..3374d53a747a 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -22,6 +22,7 @@ static void arch_ftrace_fill_regs(struct pt_regs *regs,
 				  struct pt_regs *ftrace_regs)
 {
 	*regs = *ftrace_regs;
+	regs->ip = regs->r15;
 	local_save_flags(regs->flags);
 	regs->cs = __KERNEL_CS;
 }
@@ -30,6 +31,7 @@ static void arch_regs_fill_ftrace(struct pt_regs *ftrace_regs,
 				  struct pt_regs *regs)
 {
 	*ftrace_regs = *regs;
+	ftrace_regs->r15 = regs->ip;
 }
 
 /* Ftrace callback handler for kprobes -- called under preepmt disabed */
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 6c0164d24bbd..87ecdf486ca3 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -199,8 +199,10 @@ static int klp_patch_func(struct klp_func *func)
 			return -ENOMEM;
 
 		ops->fops.func = klp_ftrace_handler;
-		ops->fops.flags = FTRACE_OPS_FL_SAVE_REGS |
-				  FTRACE_OPS_FL_DYNAMIC |
+		ops->fops.flags = FTRACE_OPS_FL_DYNAMIC |
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+				  FTRACE_OPS_FL_SAVE_REGS |
+#endif
 				  FTRACE_OPS_FL_IPMODIFY |
 				  FTRACE_OPS_FL_PERMANENT;
 

Re: [RFC][PATCH 0/2] ftrace: Add access to function arguments for all callbacks

[Alexei Starovoitov]

On Wed, Oct 28, 2020 at 09:15:42AM -0400, Steven Rostedt wrote:
> 
> This is something I wanted to implement a long time ago, but held off until
> there was a good reason to do so. Now it appears that having access to the
> arguments of the function by default is very useful. As a bonus, because
> arguments must be saved regardless before calling a callback, because they
> need to be restored before returning back to the start of the traced
> function, there's not much work to do to have them always be there for
> normal function callbacks.
> 
> The basic idea is that if CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS is set, then
> all callbacks registered to ftrace can use the regs parameter for the stack
> and arguments (kernel_stack_pointer(regs), regs_get_kernel_argument(regs, n)),
> without the need to set REGS that causes overhead by saving all registers as
> REGS simulates a breakpoint.

I don't have strong opinion on this feature, but if you want to have it
please add a giant disclaimer that this is going to be x86-64 and, may be arm64,
feature _forever_. On x86-32 and other architectures there is no way
to provide sane regs_get_kernel_argument(regs, n) semantics by blindly
saving registers. The kernel needs to know types and calling convention
of the architecture. That's the reason bpf side has btf_func_model concept.
To make sure that bpf trampoline can support all architectures in the future.

Re: [RFC][PATCH 0/2] ftrace: Add access to function arguments for all callbacks

[Steven Rostedt]

On Wed, 28 Oct 2020 14:33:03 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> I don't have strong opinion on this feature, but if you want to have it
> please add a giant disclaimer that this is going to be x86-64 and, may be arm64,
> feature _forever_. On x86-32 and other architectures there is no way
> to provide sane regs_get_kernel_argument(regs, n) semantics by blindly
> saving registers. The kernel needs to know types and calling convention
> of the architecture. That's the reason bpf side has btf_func_model concept.
> To make sure that bpf trampoline can support all architectures in the future.

The trampoline is to only provide the information to get the arguments.
Yes, it is up to the callback that registered it that knows how to get them.

What does the bpf trampoline need to get an argument if it already
knows the type. If it has access to a subset of necessary registers and
the stack pointer of when the function was called, is it possible to
figure out the arguments?

This code is not about a "regs_get_kernel_argument(regs, n)", that was
only used to show that *that* could be done. Because currently, there's
*no* way to get those arguments without setting the SAVE_REGS flag,
which saves way more than necessary. I only used that to test the code
in a generic way. Ideally, ftrace could use btf to figure out the
actual arguments of the function if it had a mapping.

I believe the reason you gave up on using ftrace was because of the
overhead of the REGS code. If that wasn't the case, and you didn't see
that overhead, would you have used ftrace?

In fact, did you look at all to see if you could modify the current
infrastructure to make it work for you? Or did you just give up to
write your own from scratch?

-- Steve

Re: [RFC][PATCH 0/2] ftrace: Add access to function arguments for all callbacks

[Alexei Starovoitov]

On Wed, Oct 28, 2020 at 06:07:53PM -0400, Steven Rostedt wrote:
> On Wed, 28 Oct 2020 14:33:03 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> > I don't have strong opinion on this feature, but if you want to have it
> > please add a giant disclaimer that this is going to be x86-64 and, may be arm64,
> > feature _forever_. On x86-32 and other architectures there is no way
> > to provide sane regs_get_kernel_argument(regs, n) semantics by blindly
> > saving registers. The kernel needs to know types and calling convention
> > of the architecture. That's the reason bpf side has btf_func_model concept.
> > To make sure that bpf trampoline can support all architectures in the future.
> 
> The trampoline is to only provide the information to get the arguments.
> Yes, it is up to the callback that registered it that knows how to get them.
> 
> What does the bpf trampoline need to get an argument if it already
> knows the type. If it has access to a subset of necessary registers and
> the stack pointer of when the function was called, is it possible to
> figure out the arguments?
> 
> This code is not about a "regs_get_kernel_argument(regs, n)", that was
> only used to show that *that* could be done. Because currently, there's
> *no* way to get those arguments without setting the SAVE_REGS flag,
> which saves way more than necessary. I only used that to test the code
> in a generic way. Ideally, ftrace could use btf to figure out the
> actual arguments of the function if it had a mapping.
> 
> I believe the reason you gave up on using ftrace was because of the
> overhead of the REGS code. If that wasn't the case, and you didn't see
> that overhead, would you have used ftrace?
> 
> In fact, did you look at all to see if you could modify the current
> infrastructure to make it work for you? Or did you just give up to
> write your own from scratch?

I don't understand what you're trying to say.
Are we still discussing your patch or something else?
If your patch then, I said, I'm not against it. I just don't see
a value in it. Since you're agree that regs_get_kernel_argument()
is misleading then what's the point of the patch?
To make mini-SAVE_REGS version? For what kind of use case?

Re: [RFC][PATCH 0/2] ftrace: Add access to function arguments for all callbacks

[Steven Rostedt]

On Wed, 28 Oct 2020 16:03:05 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> I don't understand what you're trying to say.
> Are we still discussing your patch or something else?
> If your patch then, I said, I'm not against it. I just don't see
> a value in it. Since you're agree that regs_get_kernel_argument()
> is misleading then what's the point of the patch?

I agree that it can be meaningless, but it can also be useful for
debugging purposes. It works just fine on x86_32 (even when you access
4 or more arguments). One just needs to understand the arch (as a long
long will be broken up across "two" arguments). But even that is really
useful for finding out what's going on.

> To make mini-SAVE_REGS version? For what kind of use case?

If the types of the arguments are known, and you have a mini-SAVE_REGS,
can't you still get the true arguments?

-- Steve

Re: [RFC][PATCH 1/2 v2] ftrace/x86: Allow for arguments to be passed in to REGS by default

[Masami Hiramatsu]

On Wed, 28 Oct 2020 11:29:16 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi Masami,
> 
> Talking with Peter and Thomas on IRC, where they really don't like
> passing a partial pt_regs around, got me thinking of redoing the REGS
> parameter of ftrace. Kprobes is the only user that requires the full
> registers being saved, and that's only because some kprobe user might
> want them.

Yes, kprobes can be used for debugging use case. I think we can skip
coller-saved registers (because ftrace is embedded by compiler and it
may save such registers correctly), but we still need a pt_regs on memory
to access it.

> 
> On Wed, 28 Oct 2020 10:25:02 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
> >  			      struct ftrace_ops *op, struct pt_regs *regs);
> >  
> 
> 
> Most registers of pt_regs at a start of a function is rather useless.
> What if we got rid of FL_SAVE_REGS all together and had a "ftrace_regs"
> structure passed in that would have only access to all the argument
> registers, the stack pointer and the instruction pointer?

Yeah, that's OK to me. If someone wants to investigate suspicious compiler
bug, they should not put a kprobe on the fentry call site. (in most cases
symbol+5 will put a breakpoint right after fentry nop)

> Then kprobes could just create its own pt_regs, fill in all the data
> from ftrace_regs and then fill the rest with zeros or possibly whatever
> the values currently are (does it really matter what those registers
> are?), including flags.

That sounds good to me.

> 
> Not only would this simplify the code, it would probably allow moving
> more of the kprobe code from the arch specific to the generic code, and
> remove a lot of duplication.

Ah, right.

> 
> This would also help speed up the processing of live kernel patching.
> 
> And best of all, it would give everything access to the arguments of a
> function and a stack pointer with out (ab)using pt_regs.
> 
> Do you think this would be feasible?

Yes, I agreed.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>