ftrace: Add access to function arguments for all callbacks

PATCH In review Steven Rostedt (VMware) 2 participants 10 messages Nov 13, 2020 – Nov 19, 2020

Subsystems (11)

x86 PowerPC Tracing s390 +7 more subsystems

lore

Share & tools

lore

Steven Rostedt (VMware)

author maintainer

2020-11-13 17:20 UTC #0 lore raw

v7 cover ftrace: Add access to function arguments for all callbacks

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.

This could be extended to move the REGS portion to kprobes itself, and remove the SAVE_REGS flag completely, but for now, kprobes still uses the full SAVE_REGS support.

The last patch extends the WITH_ARGS to allow default function tracing to modify the instruction pointer, where livepatching for x86 no longer needs to save all registers.

The idea of this approach is to give enough information to a callback that it could retrieve all arguments, which includes the stack pointer and instruction pointer.

This can also be extended to modify the function graph tracer to use the function tracer instead of having a separate trampoline.

Changes since v6:
- Use the new name ftrace_instruction_pointer_set() in the live patching code.

Steven Rostedt (VMware) (3):
ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs ftrace/x86: Allow for arguments to be passed in to ftrace_regs by default
livepatch: Use the default ftrace_ops instead of REGS when ARGS is available

----

 arch/csky/kernel/probes/ftrace.c     |  4 +++-
 arch/nds32/kernel/ftrace.c           |  4 ++--
 arch/parisc/kernel/ftrace.c          |  8 +++++---
 arch/powerpc/include/asm/livepatch.h |  4 +++-
 arch/powerpc/kernel/kprobes-ftrace.c |  4 +++-
 arch/s390/include/asm/livepatch.h    |  5 ++++-
 arch/s390/kernel/ftrace.c            |  4 +++-
 arch/x86/Kconfig                     |  1 +
 arch/x86/include/asm/ftrace.h        | 18 ++++++++++++++++++
 arch/x86/include/asm/livepatch.h     |  4 ++--
 arch/x86/kernel/ftrace_64.S          | 15 +++++++++++++--
 arch/x86/kernel/kprobes/ftrace.c     |  3 ++-
 fs/pstore/ftrace.c                   |  2 +-
 include/linux/ftrace.h               | 28 ++++++++++++++++++++++++++--
 include/linux/kprobes.h              |  2 +-
 kernel/livepatch/Kconfig             |  2 +-
 kernel/livepatch/patch.c             | 10 ++++++----
 kernel/trace/Kconfig                 |  9 +++++++++
 kernel/trace/ftrace.c                | 27 +++++++++++++++------------
 kernel/trace/trace_event_perf.c      |  2 +-
 kernel/trace/trace_events.c          |  2 +-
 kernel/trace/trace_functions.c       |  9 ++++-----
 kernel/trace/trace_irqsoff.c         |  2 +-
 kernel/trace/trace_sched_wakeup.c    |  2 +-
 kernel/trace/trace_selftest.c        | 20 +++++++++++---------
 kernel/trace/trace_stack.c           |  2 +-
 26 files changed, 138 insertions(+), 55 deletions(-)

Steven Rostedt (VMware)

author maintainer

2020-11-13 17:20 UTC #1 lore raw

PATCH v7 1/3 ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs

From: "Steven Rostedt (VMware)" <[email protected]>

In preparation to have arguments of a function passed to callbacks attached to functions as default, change the default callback prototype to receive a struct ftrace_regs as the forth parameter instead of a pt_regs.

For callbacks that set the FL_SAVE_REGS flag in their ftrace_ops flags, they will now need to get the pt_regs via a ftrace_get_regs() helper call. If this is called by a callback that their ftrace_ops did not have a FL_SAVE_REGS flag set, it that helper function will return NULL.

This will allow the ftrace_regs to hold enough just to get the parameters and stack pointer, but without the worry that callbacks may have a pt_regs that is not completely filled.

Acked-by Peter Zijlstra (Intel) <[email protected]>

Reviewed-by Masami Hiramatsu <[email protected]>

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

---
 arch/csky/kernel/probes/ftrace.c     |  4 +++-
 arch/nds32/kernel/ftrace.c           |  4 ++--
 arch/parisc/kernel/ftrace.c          |  8 +++++---
 arch/powerpc/kernel/kprobes-ftrace.c |  4 +++-
 arch/s390/kernel/ftrace.c            |  4 +++-
 arch/x86/kernel/kprobes/ftrace.c     |  3 ++-
 fs/pstore/ftrace.c                   |  2 +-
 include/linux/ftrace.h               | 16 ++++++++++++++--
 include/linux/kprobes.h              |  2 +-
 kernel/livepatch/patch.c             |  3 ++-
 kernel/trace/ftrace.c                | 27 +++++++++++++++------------
 kernel/trace/trace_event_perf.c      |  2 +-
 kernel/trace/trace_events.c          |  2 +-
 kernel/trace/trace_functions.c       |  9 ++++-----
 kernel/trace/trace_irqsoff.c         |  2 +-
 kernel/trace/trace_sched_wakeup.c    |  2 +-
 kernel/trace/trace_selftest.c        | 20 +++++++++++---------
 kernel/trace/trace_stack.c           |  2 +-
 18 files changed, 71 insertions(+), 45 deletions(-)

arch/csky/kernel/probes/ftrace.c +3 -1

index f30b179924ef..ae2b1c7b3b5c 100644
@@ -11,17 +11,19 @@ int arch_check_ftrace_location(struct kprobe *p)
 
 /* 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 ftrace_regs *fregs)
 {
 	int bit;
 	bool lr_saver = false;
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs *regs;
 
 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (bit < 0)
 		return;
 
+	regs = ftrace_get_regs(fregs);
 	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (!p) {

arch/nds32/kernel/ftrace.c +2 -2

index 3763b3f8c3db..414f8a780cc3 100644
@@ -10,7 +10,7 @@ extern void (*ftrace_trace_function)(unsigned long, unsigned long,
 extern void ftrace_graph_caller(void);
 
 noinline void __naked ftrace_stub(unsigned long ip, unsigned long parent_ip,
-				  struct ftrace_ops *op, struct pt_regs *regs)
+				  struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	__asm__ ("");  /* avoid to optimize as pure function */
 }

@@ -38,7 +38,7 @@ EXPORT_SYMBOL(_mcount);
 #else /* CONFIG_DYNAMIC_FTRACE */
 
 noinline void __naked ftrace_stub(unsigned long ip, unsigned long parent_ip,
-				  struct ftrace_ops *op, struct pt_regs *regs)
+				  struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	__asm__ ("");  /* avoid to optimize as pure function */
 }

arch/parisc/kernel/ftrace.c +5 -3

index 1c5d3732bda2..0a1e75af5382 100644
@@ -51,7 +51,7 @@ static void __hot prepare_ftrace_return(unsigned long *parent,
 void notrace __hot ftrace_function_trampoline(unsigned long parent,
 				unsigned long self_addr,
 				unsigned long org_sp_gr3,
-				struct pt_regs *regs)
+				struct ftrace_regs *fregs)
 {
 #ifndef CONFIG_DYNAMIC_FTRACE
 	extern ftrace_func_t ftrace_trace_function;

@@ -61,7 +61,7 @@ void notrace __hot ftrace_function_trampoline(unsigned long parent,
 	if (function_trace_op->flags & FTRACE_OPS_FL_ENABLED &&
 	    ftrace_trace_function != ftrace_stub)
 		ftrace_trace_function(self_addr, parent,
-				function_trace_op, regs);
+				function_trace_op, fregs);
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	if (dereference_function_descriptor(ftrace_graph_return) !=

@@ -204,9 +204,10 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
 
 #ifdef CONFIG_KPROBES_ON_FTRACE
 void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
-			   struct ftrace_ops *ops, struct pt_regs *regs)
+			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs *regs;
 	struct kprobe *p;
 	int bit;

@@ -214,6 +215,7 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (bit < 0)
 		return;
 
+	regs = ftrace_get_regs(fregs);
 	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))

arch/powerpc/kernel/kprobes-ftrace.c +3 -1

index fdfee39938ea..660138f6c4b2 100644
@@ -14,16 +14,18 @@
 
 /* Ftrace callback handler for kprobes */
 void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip,
-			   struct ftrace_ops *ops, struct pt_regs *regs)
+			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs *regs;
 	int bit;
 
 	bit = ftrace_test_recursion_trylock(nip, parent_nip);
 	if (bit < 0)
 		return;
 
+	regs = ftrace_get_regs(fregs);
 	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)nip);
 	if (unlikely(!p) || kprobe_disabled(p))

arch/s390/kernel/ftrace.c +3 -1

index 657c1ab45408..67b80f4412f9 100644
@@ -198,9 +198,10 @@ int ftrace_disable_ftrace_graph_caller(void)
 
 #ifdef CONFIG_KPROBES_ON_FTRACE
 void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
-		struct ftrace_ops *ops, struct pt_regs *regs)
+		struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs *regs;
 	struct kprobe *p;
 	int bit;

@@ -208,6 +209,7 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (bit < 0)
 		return;
 
+	regs = ftrace_get_regs(fregs);
 	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))

arch/x86/kernel/kprobes/ftrace.c +2 -1

index 954d930a7127..373e5fa3ce1f 100644
@@ -14,8 +14,9 @@
 
 /* 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 ftrace_regs *fregs)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
 	int bit;

fs/pstore/ftrace.c +1 -1

index adb0935eb062..5939595f0115 100644
@@ -26,7 +26,7 @@ static u64 pstore_ftrace_stamp;
 static void notrace pstore_ftrace_call(unsigned long ip,
 				       unsigned long parent_ip,
 				       struct ftrace_ops *op,
-				       struct pt_regs *regs)
+				       struct ftrace_regs *fregs)
 {
 	int bit;
 	unsigned long flags;

include/linux/ftrace.h +14 -2

index 8dde9c17aaa5..24e1fa52337d 100644
@@ -90,8 +90,20 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
 
 struct ftrace_ops;
 
+struct ftrace_regs {
+	struct pt_regs		regs;
+};
+
+static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs)
+{
+	if (!fregs)
+		return NULL;
+
+	return &fregs->regs;
+}
+
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
-			      struct ftrace_ops *op, struct pt_regs *regs);
+			      struct ftrace_ops *op, struct ftrace_regs *fregs);
 
 ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);

@@ -259,7 +271,7 @@ int register_ftrace_function(struct ftrace_ops *ops);
 int unregister_ftrace_function(struct ftrace_ops *ops);
 
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
-			struct ftrace_ops *op, struct pt_regs *regs);
+			struct ftrace_ops *op, struct ftrace_regs *fregs);
 
 #else /* !CONFIG_FUNCTION_TRACER */
 /*

include/linux/kprobes.h +1 -1

index 629abaf25681..be73350955e4 100644
@@ -345,7 +345,7 @@ static inline void wait_for_kprobe_optimizer(void) { }
 #endif /* CONFIG_OPTPROBES */
 #ifdef CONFIG_KPROBES_ON_FTRACE
 extern void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
-				  struct ftrace_ops *ops, struct pt_regs *regs);
+				  struct ftrace_ops *ops, struct ftrace_regs *fregs);
 extern int arch_prepare_kprobe_ftrace(struct kprobe *p);
 #endif

kernel/livepatch/patch.c +2 -1

index 875c5dbbdd33..f89f9e7e9b07 100644
@@ -40,8 +40,9 @@ struct klp_ops *klp_find_ops(void *old_func)
 static void notrace klp_ftrace_handler(unsigned long ip,
 				       unsigned long parent_ip,
 				       struct ftrace_ops *fops,
-				       struct pt_regs *regs)
+				       struct ftrace_regs *fregs)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	struct klp_ops *ops;
 	struct klp_func *func;
 	int patch_state;

kernel/trace/ftrace.c +15 ~~-12~~

index 3db64fb0cce8..67888311784e 100644
@@ -121,7 +121,7 @@ struct ftrace_ops global_ops;
 
 #if ARCH_SUPPORTS_FTRACE_OPS
 static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
-				 struct ftrace_ops *op, struct pt_regs *regs);
+				 struct ftrace_ops *op, struct ftrace_regs *fregs);
 #else
 /* See comment below, where ftrace_ops_list_func is defined */
 static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip);

@@ -140,7 +140,7 @@ static inline void ftrace_ops_init(struct ftrace_ops *ops)
 }
 
 static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
-			    struct ftrace_ops *op, struct pt_regs *regs)
+			    struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	struct trace_array *tr = op->private;
 	int pid;

@@ -154,7 +154,7 @@ static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
 			return;
 	}
 
-	op->saved_func(ip, parent_ip, op, regs);
+	op->saved_func(ip, parent_ip, op, fregs);
 }
 
 static void ftrace_sync_ipi(void *data)

@@ -754,7 +754,7 @@ ftrace_profile_alloc(struct ftrace_profile_stat *stat, unsigned long ip)
 
 static void
 function_profile_call(unsigned long ip, unsigned long parent_ip,
-		      struct ftrace_ops *ops, struct pt_regs *regs)
+		      struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
 	struct ftrace_profile_stat *stat;
 	struct ftrace_profile *rec;

@@ -2143,6 +2143,7 @@ static int ftrace_check_record(struct dyn_ftrace *rec, bool enable, bool update)
 				else
 					rec->flags &= ~FTRACE_FL_TRAMP_EN;
 			}
+
 			if (flag & FTRACE_FL_DIRECT) {
 				/*
 				 * If there's only one user (direct_ops helper)

@@ -2368,8 +2369,9 @@ unsigned long ftrace_find_rec_direct(unsigned long ip)
 }
 
 static void call_direct_funcs(unsigned long ip, unsigned long pip,
-			      struct ftrace_ops *ops, struct pt_regs *regs)
+			      struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	unsigned long addr;
 
 	addr = ftrace_find_rec_direct(ip);

@@ -4292,7 +4294,7 @@ static int __init ftrace_mod_cmd_init(void)
 core_initcall(ftrace_mod_cmd_init);
 
 static void function_trace_probe_call(unsigned long ip, unsigned long parent_ip,
-				      struct ftrace_ops *op, struct pt_regs *pt_regs)
+				      struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	struct ftrace_probe_ops *probe_ops;
 	struct ftrace_func_probe *probe;

@@ -6911,8 +6913,9 @@ void ftrace_reset_array_ops(struct trace_array *tr)
 
 static nokprobe_inline void
 __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
-		       struct ftrace_ops *ignored, struct pt_regs *regs)
+		       struct ftrace_ops *ignored, struct ftrace_regs *fregs)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
 	struct ftrace_ops *op;
 	int bit;

@@ -6945,7 +6948,7 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 				pr_warn("op=%p %pS\n", op, op);
 				goto out;
 			}
-			op->func(ip, parent_ip, op, regs);
+			op->func(ip, parent_ip, op, fregs);
 		}
 	} while_for_each_ftrace_op(op);
 out:

@@ -6968,9 +6971,9 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
  */
 #if ARCH_SUPPORTS_FTRACE_OPS
 static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
-				 struct ftrace_ops *op, struct pt_regs *regs)
+				 struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
-	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
+	__ftrace_ops_list_func(ip, parent_ip, NULL, fregs);
 }
 NOKPROBE_SYMBOL(ftrace_ops_list_func);
 #else

@@ -6987,7 +6990,7 @@ NOKPROBE_SYMBOL(ftrace_ops_no_ops);
  * this function will be called by the mcount trampoline.
  */
 static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
-				   struct ftrace_ops *op, struct pt_regs *regs)
+				   struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	int bit;

@@ -6998,7 +7001,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
 	preempt_disable_notrace();
 
 	if (!(op->flags & FTRACE_OPS_FL_RCU) || rcu_is_watching())
-		op->func(ip, parent_ip, op, regs);
+		op->func(ip, parent_ip, op, fregs);
 
 	preempt_enable_notrace();
 	trace_clear_recursion(bit);

kernel/trace/trace_event_perf.c +1 -1

index 1b202e28dfaa..a71181655958 100644
@@ -432,7 +432,7 @@ NOKPROBE_SYMBOL(perf_trace_buf_update);
 #ifdef CONFIG_FUNCTION_TRACER
 static void
 perf_ftrace_function_call(unsigned long ip, unsigned long parent_ip,
-			  struct ftrace_ops *ops, struct pt_regs *pt_regs)
+			  struct ftrace_ops *ops,  struct ftrace_regs *fregs)
 {
 	struct ftrace_entry *entry;
 	struct perf_event *event;

kernel/trace/trace_events.c +1 -1

index f4b459bb6d33..98d194d8460e 100644
@@ -3673,7 +3673,7 @@ static struct trace_event_file event_trace_file __initdata;
 
 static void __init
 function_test_events_call(unsigned long ip, unsigned long parent_ip,
-			  struct ftrace_ops *op, struct pt_regs *pt_regs)
+			  struct ftrace_ops *op, struct ftrace_regs *regs)
 {
 	struct trace_buffer *buffer;
 	struct ring_buffer_event *event;

kernel/trace/trace_functions.c +4 -5

index 646eda6c44a5..c5095dd28e20 100644
@@ -23,10 +23,10 @@ static void tracing_start_function_trace(struct trace_array *tr);
 static void tracing_stop_function_trace(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 ftrace_regs *fregs);
 static void
 function_stack_trace_call(unsigned long ip, unsigned long parent_ip,
-			  struct ftrace_ops *op, struct pt_regs *pt_regs);
+			  struct ftrace_ops *op, struct ftrace_regs *fregs);
 static struct tracer_flags func_flags;
 
 /* Our option */

@@ -89,7 +89,6 @@ void ftrace_destroy_function_files(struct trace_array *tr)
 static int function_trace_init(struct trace_array *tr)
 {
 	ftrace_func_t func;
-
 	/*
 	 * Instance trace_arrays get their ops allocated
 	 * at instance creation. Unless it failed

@@ -129,7 +128,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 ftrace_regs *fregs)
 {
 	struct trace_array *tr = op->private;
 	struct trace_array_cpu *data;

@@ -178,7 +177,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip,
 
 static void
 function_stack_trace_call(unsigned long ip, unsigned long parent_ip,
-			  struct ftrace_ops *op, struct pt_regs *pt_regs)
+			  struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	struct trace_array *tr = op->private;
 	struct trace_array_cpu *data;

kernel/trace/trace_irqsoff.c +1 -1

index 10bbb0f381d5..d06aab4dcbb8 100644
@@ -138,7 +138,7 @@ static int func_prolog_dec(struct trace_array *tr,
  */
 static void
 irqsoff_tracer_call(unsigned long ip, unsigned long parent_ip,
-		    struct ftrace_ops *op, struct pt_regs *pt_regs)
+		    struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	struct trace_array *tr = irqsoff_trace;
 	struct trace_array_cpu *data;

kernel/trace/trace_sched_wakeup.c +1 -1

index 97b10bb31a1f..c0181066dbe9 100644
@@ -212,7 +212,7 @@ static void wakeup_print_header(struct seq_file *s)
  */
 static void
 wakeup_tracer_call(unsigned long ip, unsigned long parent_ip,
-		   struct ftrace_ops *op, struct pt_regs *pt_regs)
+		   struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	struct trace_array *tr = wakeup_trace;
 	struct trace_array_cpu *data;

kernel/trace/trace_selftest.c +11 -9

index 8ee3c0bb5d8a..5ed081c6471c 100644
@@ -107,7 +107,7 @@ static int trace_selftest_test_probe1_cnt;
 static void trace_selftest_test_probe1_func(unsigned long ip,
 					    unsigned long pip,
 					    struct ftrace_ops *op,
-					    struct pt_regs *pt_regs)
+					    struct ftrace_regs *fregs)
 {
 	trace_selftest_test_probe1_cnt++;
 }

@@ -116,7 +116,7 @@ static int trace_selftest_test_probe2_cnt;
 static void trace_selftest_test_probe2_func(unsigned long ip,
 					    unsigned long pip,
 					    struct ftrace_ops *op,
-					    struct pt_regs *pt_regs)
+					    struct ftrace_regs *fregs)
 {
 	trace_selftest_test_probe2_cnt++;
 }

@@ -125,7 +125,7 @@ static int trace_selftest_test_probe3_cnt;
 static void trace_selftest_test_probe3_func(unsigned long ip,
 					    unsigned long pip,
 					    struct ftrace_ops *op,
-					    struct pt_regs *pt_regs)
+					    struct ftrace_regs *fregs)
 {
 	trace_selftest_test_probe3_cnt++;
 }

@@ -134,7 +134,7 @@ static int trace_selftest_test_global_cnt;
 static void trace_selftest_test_global_func(unsigned long ip,
 					    unsigned long pip,
 					    struct ftrace_ops *op,
-					    struct pt_regs *pt_regs)
+					    struct ftrace_regs *fregs)
 {
 	trace_selftest_test_global_cnt++;
 }

@@ -143,7 +143,7 @@ static int trace_selftest_test_dyn_cnt;
 static void trace_selftest_test_dyn_func(unsigned long ip,
 					 unsigned long pip,
 					 struct ftrace_ops *op,
-					 struct pt_regs *pt_regs)
+					 struct ftrace_regs *fregs)
 {
 	trace_selftest_test_dyn_cnt++;
 }

@@ -414,7 +414,7 @@ static int trace_selftest_recursion_cnt;
 static void trace_selftest_test_recursion_func(unsigned long ip,
 					       unsigned long pip,
 					       struct ftrace_ops *op,
-					       struct pt_regs *pt_regs)
+					       struct ftrace_regs *fregs)
 {
 	/*
 	 * This function is registered without the recursion safe flag.

@@ -429,7 +429,7 @@ static void trace_selftest_test_recursion_func(unsigned long ip,
 static void trace_selftest_test_recursion_safe_func(unsigned long ip,
 						    unsigned long pip,
 						    struct ftrace_ops *op,
-						    struct pt_regs *pt_regs)
+						    struct ftrace_regs *fregs)
 {
 	/*
 	 * We said we would provide our own recursion. By calling

@@ -548,9 +548,11 @@ static enum {
 static void trace_selftest_test_regs_func(unsigned long ip,
 					  unsigned long pip,
 					  struct ftrace_ops *op,
-					  struct pt_regs *pt_regs)
+					  struct ftrace_regs *fregs)
 {
-	if (pt_regs)
+	struct pt_regs *regs = ftrace_get_regs(fregs);
+
+	if (regs)
 		trace_selftest_regs_stat = TRACE_SELFTEST_REGS_FOUND;
 	else
 		trace_selftest_regs_stat = TRACE_SELFTEST_REGS_NOT_FOUND;

kernel/trace/trace_stack.c +1 -1

index 969db526a563..63c285042051 100644
@@ -290,7 +290,7 @@ static void check_stack(unsigned long ip, unsigned long *stack)
 
 static void
 stack_trace_call(unsigned long ip, unsigned long parent_ip,
-		 struct ftrace_ops *op, struct pt_regs *pt_regs)
+		 struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	unsigned long stack;

Signature · 1 line

2.28.0

Petr Mladek

maintainer

2020-11-19 11:05 UTC #2 lore raw

Re: [PATCH 1/3 v7] ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs

Quoted 27 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>
In preparation to have arguments of a function passed to callbacks attached to functions as default, change the default callback prototype to receive a struct ftrace_regs as the forth parameter instead of a pt_regs.
For callbacks that set the FL_SAVE_REGS flag in their ftrace_ops flags, they will now need to get the pt_regs via a ftrace_get_regs() helper call. If this is called by a callback that their ftrace_ops did not have a FL_SAVE_REGS flag set, it that helper function will return NULL.
This will allow the ftrace_regs to hold enough just to get the parameters and stack pointer, but without the worry that callbacks may have a pt_regs that is not completely filled.
arch/csky/kernel/probes/ftrace.c +1 -1
index f30b179924ef..ae2b1c7b3b5c 100644
@@ -11,17 +11,19 @@ int arch_check_ftrace_location(struct kprobe *p)
 
 /* 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 ftrace_regs *fregs)
 {
 	int bit;

arch/csky/kernel/probes/ftrace.c int arch_check_ftrace_location(struct kprobe *p) · line 26

Quoted 10 lines

arch/csky/kernel/probes/ftrace.c +2 -0

 	bool lr_saver = false;
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs *regs;
 
 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (bit < 0)
 		return;
 
+	regs = ftrace_get_regs(fregs);

Should we check for NULL here?
Same in all callers?

arch/csky/kernel/probes/ftrace.c int arch_check_ftrace_location(struct kprobe *p) · line 29

Quoted 3 lines

arch/csky/kernel/probes/ftrace.c +0 -0

 	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (!p) {

Otherwise, the patch is pretty strightforard and looks good to me.

Best Regards,
Petr

Steven Rostedt

author maintainer

2020-11-19 14:08 UTC #3 lore raw

Re: [PATCH 1/3 v7] ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs

Quoted 10 lines · On Thu, 19 Nov 2020 12:05:44 +0100 Petr Mladek <[email protected]> wrote:

Quoted 10 lines

void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,

arch/csky/kernel/probes/ftrace.c +2 -1

-			   struct ftrace_ops *ops, struct pt_regs *regs)
+			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
 	int bit;
 	bool lr_saver = false;
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs *regs;

Quoted 8 lines

arch/csky/kernel/probes/ftrace.c int arch_check_ftrace_location(struct kprobe *p) · line 26
Quoted 5 lines
arch/csky/kernel/probes/ftrace.c +1 -0
 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (bit < 0)
 		return;
 
+	regs = ftrace_get_regs(fregs);  
Should we check for NULL here?
Same in all callers?

If regs is NULL that's a major bug.

It's no different than what we have today. If you set FL_SAVE_REGS, then the regs parameter will have regs. If you don't, it will be NULL. We don't check regs for NULL today, so we shouldn't need to check regs for NULL with this.

One of my bootup tests checks if this works. I work hard to make sure that regs is set for everything that wants it, otherwise bad things happen.

In other words, the functionality in this regard hasn't changed with this patch.

-- Steve

Quoted 6 lines

arch/csky/kernel/probes/ftrace.c int arch_check_ftrace_location(struct kprobe *p) · line 29
Quoted 4 lines
arch/csky/kernel/probes/ftrace.c +0 -0
 	preempt_disable_notrace();
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (!p) {  
Otherwise, the patch is pretty strightforard and looks good to me.

Petr Mladek

maintainer Acked-by

2020-11-19 16:04 UTC #4 lore raw

Re: [PATCH 1/3 v7] ftrace: Have the callbacks receive a struct ftrace_regs instead of pt_regs

Quoted 29 lines · Steven Rostedt

Quoted 10 lines · On Thu, 19 Nov 2020 12:05:44 +0100 Petr Mladek <[email protected]> wrote:
Quoted 10 lines
void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
arch/csky/kernel/probes/ftrace.c +2 -1
-			   struct ftrace_ops *ops, struct pt_regs *regs)
+			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
 	int bit;
 	bool lr_saver = false;
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
+	struct pt_regs *regs;
Quoted 8 lines
arch/csky/kernel/probes/ftrace.c int arch_check_ftrace_location(struct kprobe *p) · line 26
Quoted 5 lines
arch/csky/kernel/probes/ftrace.c +1 -0
 	bit = ftrace_test_recursion_trylock(ip, parent_ip);
 	if (bit < 0)
 		return;
 
+	regs = ftrace_get_regs(fregs);  
Should we check for NULL here?
Same in all callers?
If regs is NULL that's a major bug.
It's no different than what we have today. If you set FL_SAVE_REGS, then the regs parameter will have regs. If you don't, it will be NULL. We don't check regs for NULL today, so we shouldn't need to check regs for NULL with this.

Quoted 5 lines

One of my bootup tests checks if this works. I work hard to make sure that regs is set for everything that wants it, otherwise bad things happen.
In other words, the functionality in this regard hasn't changed with this patch.

Thanks for explanation. Feel free to use:

Acked-by Petr Mladek <[email protected]>

Best Regards,
Petr

Steven Rostedt (VMware)

author maintainer

2020-11-13 17:20 UTC #5 lore raw

PATCH v7 2/3 ftrace/x86: Allow for arguments to be passed in to ftrace_regs by default

In reply to Steven Rostedt (VMware)

From: "Steven Rostedt (VMware)" <[email protected]>

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 arguments 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_64 now does), it is to set HAVE_DYNAMIC_FTRACE_WITH_ARGS, and this will let the generic code that it could have access to arguments without having to set the flags.

This also includes having the stack pointer being saved, which could be used for accessing arguments on the stack, as well as having the function graph tracer not require its own trampoline!

Acked-by Peter Zijlstra (Intel) <[email protected]>

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

---
 arch/x86/Kconfig              |  1 +
 arch/x86/include/asm/ftrace.h | 15 +++++++++++++++
 arch/x86/kernel/ftrace_64.S   | 11 +++++++++--
 include/linux/ftrace.h        |  7 ++++++-
 kernel/trace/Kconfig          |  9 +++++++++
 5 files changed, 40 insertions(+), 3 deletions(-)

arch/x86/Kconfig +1 -0

index f6946b81f74a..478526aabe5d 100644
@@ -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

arch/x86/include/asm/ftrace.h +15 -0

index 84b9449be080..e00fe88146e0 100644
@@ -41,6 +41,21 @@ static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned
 	regs->orig_ax = addr;
 }
 
+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+struct ftrace_regs {
+	struct pt_regs		regs;
+};
+
+static __always_inline struct pt_regs *
+arch_ftrace_get_regs(struct ftrace_regs *fregs)
+{
+	/* Only when FL_SAVE_REGS is set, cs will be non zero */
+	if (!fregs->regs.cs)
+		return NULL;
+	return &fregs->regs;
+}
+#endif
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 struct dyn_arch_ftrace {

arch/x86/kernel/ftrace_64.S +9 -2

index ac3d5f22fe64..60e3b64f5ea6 100644
@@ -140,12 +140,19 @@ SYM_FUNC_START(ftrace_caller)
 	/* save_mcount_regs fills in first two parameters */
 	save_mcount_regs
 
+	/* Stack - skipping return address of ftrace_caller */
+	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 register set */
+	movq $0, CS(%rsp)
 
 SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	call ftrace_stub

include/linux/ftrace.h +6 -1

index 24e1fa52337d..588ea7023a7a 100644
@@ -90,16 +90,21 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
 
 struct ftrace_ops;
 
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+
 struct ftrace_regs {
 	struct pt_regs		regs;
 };
+#define arch_ftrace_get_regs(fregs) (&(fregs)->regs)
+
+#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
 
 static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs)
 {
 	if (!fregs)
 		return NULL;
 
-	return &fregs->regs;
+	return arch_ftrace_get_regs(fregs);
 }
 
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,

kernel/trace/Kconfig +9 -0

index 6aa36ec73ccb..c9b64dea1216 100644
@@ -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

Signature · 1 line

2.28.0

Steven Rostedt (VMware)

author maintainer

2020-11-13 17:19 UTC #6 lore raw

PATCH v7 3/3 livepatch: Use the default ftrace_ops instead of REGS when ARGS is available

In reply to Steven Rostedt (VMware)

From: "Steven Rostedt (VMware)" <[email protected]>

When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS is available, the ftrace call will be able to set the ip of the calling function. This will improve the performance of live kernel patching where it does not need all the regs to be stored just to change the instruction pointer.

If all archs that support live kernel patching also support HAVE_DYNAMIC_FTRACE_WITH_ARGS, then the architecture specific function klp_arch_set_pc() could be made generic.

It is possible that an arch can support HAVE_DYNAMIC_FTRACE_WITH_ARGS but not HAVE_DYNAMIC_FTRACE_WITH_REGS and then have access to live patching.

Cc Josh Poimboeuf <[email protected]>

Cc Jiri Kosina <[email protected]>

Cc [email protected]

Acked-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Miroslav Benes <[email protected]>

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

---
Changes since v6:
- Updated to use ftrace_instruction_pointer_set() macro

 arch/powerpc/include/asm/livepatch.h | 4 +++-
 arch/s390/include/asm/livepatch.h    | 5 ++++-
 arch/x86/include/asm/ftrace.h        | 3 +++
 arch/x86/include/asm/livepatch.h     | 4 ++--
 arch/x86/kernel/ftrace_64.S          | 4 ++++
 include/linux/ftrace.h               | 7 +++++++
 kernel/livepatch/Kconfig             | 2 +-
 kernel/livepatch/patch.c             | 9 +++++----
 8 files changed, 29 insertions(+), 9 deletions(-)

arch/powerpc/include/asm/livepatch.h +3 -1

index 4a3d5d25fed5..ae25e6e72997 100644
@@ -12,8 +12,10 @@
 #include <linux/sched/task_stack.h>
 
 #ifdef CONFIG_LIVEPATCH
-static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
+
 	regs->nip = ip;
 }

arch/s390/include/asm/livepatch.h +4 -1

index 818612b784cd..d578a8c76676 100644
@@ -11,10 +11,13 @@
 #ifndef ASM_LIVEPATCH_H
 #define ASM_LIVEPATCH_H
 
+#include <linux/ftrace.h>
 #include <asm/ptrace.h>
 
-static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);
+
 	regs->psw.addr = ip;
 }

arch/x86/include/asm/ftrace.h +3 -0

index e00fe88146e0..9f3130f40807 100644
@@ -54,6 +54,9 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
 		return NULL;
 	return &fregs->regs;
 }
+
+#define ftrace_instruction_pointer_set(fregs, _ip)	\
+	do { (fregs)->regs.ip = (_ip); } while (0)
 #endif
 
 #ifdef CONFIG_DYNAMIC_FTRACE

arch/x86/include/asm/livepatch.h +2 -2

index 1fde1ab6559e..7c5cc6660e4b 100644
@@ -12,9 +12,9 @@
 #include <asm/setup.h>
 #include <linux/ftrace.h>
 
-static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
-	regs->ip = ip;
+	ftrace_instruction_pointer_set(fregs, ip);
 }
 
 #endif /* _ASM_X86_LIVEPATCH_H */

arch/x86/kernel/ftrace_64.S +4 -0

index 60e3b64f5ea6..0d54099c2a3a 100644
@@ -157,6 +157,10 @@ SYM_INNER_LABEL(ftrace_caller_op_ptr, SYM_L_GLOBAL)
 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
 
 	/*

include/linux/ftrace.h +7 -0

index 588ea7023a7a..9a8ce28e4485 100644
@@ -97,6 +97,13 @@ struct ftrace_regs {
 };
 #define arch_ftrace_get_regs(fregs) (&(fregs)->regs)
 
+/*
+ * ftrace_instruction_pointer_set() is to be defined by the architecture
+ * if to allow setting of the instruction pointer from the ftrace_regs
+ * when HAVE_DYNAMIC_FTRACE_WITH_ARGS is set and it supports
+ * live kernel patching.
+ */
+#define ftrace_instruction_pointer_set(fregs, ip) do { } while (0)
 #endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
 
 static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs)

kernel/livepatch/Kconfig +1 -1

index 54102deb50ba..53d51ed619a3 100644
@@ -6,7 +6,7 @@ config HAVE_LIVEPATCH
 
 config LIVEPATCH
 	bool "Kernel Live Patching"
-	depends on DYNAMIC_FTRACE_WITH_REGS
+	depends on DYNAMIC_FTRACE_WITH_REGS || DYNAMIC_FTRACE_WITH_ARGS
 	depends on MODULES
 	depends on SYSFS
 	depends on KALLSYMS_ALL

kernel/livepatch/patch.c +5 -4

index f89f9e7e9b07..e8029aea67f1 100644
@@ -42,7 +42,6 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 				       struct ftrace_ops *fops,
 				       struct ftrace_regs *fregs)
 {
-	struct pt_regs *regs = ftrace_get_regs(fregs);
 	struct klp_ops *ops;
 	struct klp_func *func;
 	int patch_state;

@@ -118,7 +117,7 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	if (func->nop)
 		goto unlock;
 
-	klp_arch_set_pc(regs, (unsigned long)func->new_func);
+	klp_arch_set_pc(fregs, (unsigned long)func->new_func);
 
 unlock:
 	preempt_enable_notrace();

@@ -200,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;

Signature · 1 line

2.28.0

Petr Mladek

maintainer

2020-11-19 11:52 UTC #7 lore raw

Re: [PATCH 3/3 v7] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available

Quoted 39 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>
When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS is available, the ftrace call will be able to set the ip of the calling function. This will improve the performance of live kernel patching where it does not need all the regs to be stored just to change the instruction pointer.
If all archs that support live kernel patching also support HAVE_DYNAMIC_FTRACE_WITH_ARGS, then the architecture specific function klp_arch_set_pc() could be made generic.
It is possible that an arch can support HAVE_DYNAMIC_FTRACE_WITH_ARGS but not HAVE_DYNAMIC_FTRACE_WITH_REGS and then have access to live patching.
Cc Josh Poimboeuf <[email protected]>
Cc Jiri Kosina <[email protected]>
Cc [email protected]
Acked-by Peter Zijlstra (Intel) <[email protected]>
Acked-by Miroslav Benes <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>
---
Changes since v6:
- Updated to use ftrace_instruction_pointer_set() macro
 arch/powerpc/include/asm/livepatch.h | 4 +++-
 arch/s390/include/asm/livepatch.h    | 5 ++++-
 arch/x86/include/asm/ftrace.h        | 3 +++
 arch/x86/include/asm/livepatch.h     | 4 ++--
 arch/x86/kernel/ftrace_64.S          | 4 ++++
 include/linux/ftrace.h               | 7 +++++++
 kernel/livepatch/Kconfig             | 2 +-
 kernel/livepatch/patch.c             | 9 +++++----
 8 files changed, 29 insertions(+), 9 deletions(-)
arch/powerpc/include/asm/livepatch.h +0 -0
index 4a3d5d25fed5..ae25e6e72997 100644

arch/powerpc/include/asm/livepatch.h line 17

Quoted 7 lines

arch/powerpc/include/asm/livepatch.h +2 -1

@@ -12,8 +12,10 @@
 #include <linux/sched/task_stack.h>
 
 #ifdef CONFIG_LIVEPATCH
-static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);

Should we check for NULL pointer here?

Quoted 9 lines

arch/powerpc/include/asm/livepatch.h +1 -0
+
 	regs->nip = ip;
 }
 
arch/s390/include/asm/livepatch.h +0 -0
index 818612b784cd..d578a8c76676 100644

arch/s390/include/asm/livepatch.h line 19

Quoted 10 lines

arch/s390/include/asm/livepatch.h +3 -1

@@ -11,10 +11,13 @@
 #ifndef ASM_LIVEPATCH_H
 #define ASM_LIVEPATCH_H
 
+#include <linux/ftrace.h>
 #include <asm/ptrace.h>
 
-static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);

And here?

arch/s390/include/asm/livepatch.h line 23

Quoted 4 lines

arch/s390/include/asm/livepatch.h +1 -0
+
 	regs->psw.addr = ip;
 }
 

Otherwise, it looks for me.

Best Regards,
Petr

Steven Rostedt

author maintainer

2020-11-19 14:12 UTC #8 lore raw

Re: [PATCH 3/3 v7] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available

Quoted 7 lines · On Thu, 19 Nov 2020 12:52:00 +0100 Petr Mladek <[email protected]> wrote:

arch/powerpc/include/asm/livepatch.h line 17
Quoted 5 lines
#ifdef CONFIG_LIVEPATCH
arch/powerpc/include/asm/livepatch.h +2 -1
-static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);  
Should we check for NULL pointer here?

As mentioned in my last email. regs could have been NULL for the same reasons before this patch, and we didn't check it then. Why should we check it now?

The ftrace_get_regs() only makes sure that a ftrace_ops that set FL_SAVE_REGS gets it, and those that did not, don't.

But that's not entirely true either. If there's two callbacks to the same function, and one has FL_SAVE_REGS set, they both can have access to the regs (before and after this patch). It's just that the one that did not have FL_SAVE_REGS set, isn't guaranteed to have it.

-- Steve

Quoted 4 lines

Quoted 4 lines
arch/powerpc/include/asm/livepatch.h +1 -0
+
 	regs->nip = ip;
 }

Petr Mladek

maintainer Acked-by

2020-11-19 16:08 UTC #9 lore raw

Re: [PATCH 3/3 v7] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available

Quoted 15 lines · Steven Rostedt

Quoted 7 lines · On Thu, 19 Nov 2020 12:52:00 +0100 Petr Mladek <[email protected]> wrote:
arch/powerpc/include/asm/livepatch.h line 17
Quoted 5 lines
#ifdef CONFIG_LIVEPATCH
arch/powerpc/include/asm/livepatch.h +2 -1
-static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 {
+	struct pt_regs *regs = ftrace_get_regs(fregs);  
Should we check for NULL pointer here?
As mentioned in my last email. regs could have been NULL for the same reasons before this patch, and we didn't check it then. Why should we check it now?

Quoted 7 lines

The ftrace_get_regs() only makes sure that a ftrace_ops that set FL_SAVE_REGS gets it, and those that did not, don't.
But that's not entirely true either. If there's two callbacks to the same function, and one has FL_SAVE_REGS set, they both can have access to the regs (before and after this patch). It's just that the one that did not have FL_SAVE_REGS set, isn't guaranteed to have it.

Makes sense. Thanks for explanation. Feel free to use:

Acked-by Petr Mladek <[email protected]>

I actually did review of all patches and they looked fine to me. I just did not check all corner cases, assembly, and did not test it, so I give it just my ack. I believe your testing ;-)

Best Regards,
Petr