* [PATCH 3/3 v5] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available [not found] <20201112011516.589846126@goodmis.org> @ 2020-11-12 1:15 ` Steven Rostedt 2020-11-12 8:21 ` Peter Zijlstra 0 siblings, 1 reply; 4+ messages in thread From: Steven Rostedt @ 2020-11-12 1:15 UTC (permalink / raw) To: linux-kernel Cc: Ingo Molnar, Andrew Morton, Miroslav Benes, Peter Zijlstra, Thomas Gleixner, Masami Hiramatsu, Josh Poimboeuf, Jiri Kosina, live-patching From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> 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 <jpoimboe@redhat.com> Cc: Jiri Kosina <jikos@kernel.org> Cc: live-patching@vger.kernel.org Acked-by: Miroslav Benes <mbenes@suse.cz> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- 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(-) diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h index 4a3d5d25fed5..ae25e6e72997 100644 --- a/arch/powerpc/include/asm/livepatch.h +++ b/arch/powerpc/include/asm/livepatch.h @@ -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; } diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h index 818612b784cd..d578a8c76676 100644 --- a/arch/s390/include/asm/livepatch.h +++ b/arch/s390/include/asm/livepatch.h @@ -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; } diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h index e00fe88146e0..235385a38bd9 100644 --- a/arch/x86/include/asm/ftrace.h +++ b/arch/x86/include/asm/ftrace.h @@ -54,6 +54,9 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs) return NULL; return &fregs->regs; } + +#define ftrace_regs_set_ip(fregs, _ip) \ + do { (fregs)->regs.ip = (_ip); } while (0) #endif #ifdef CONFIG_DYNAMIC_FTRACE diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h index 1fde1ab6559e..59a08d5c6f1d 100644 --- a/arch/x86/include/asm/livepatch.h +++ b/arch/x86/include/asm/livepatch.h @@ -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_regs_set_ip(fregs, ip); } #endif /* _ASM_X86_LIVEPATCH_H */ diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index 60e3b64f5ea6..0d54099c2a3a 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -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 /* diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 588ea7023a7a..b4eb962e2be9 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -97,6 +97,13 @@ struct ftrace_regs { }; #define arch_ftrace_get_regs(fregs) (&(fregs)->regs) +/* + * ftrace_regs_set_ip() 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_regs_set_ip(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) diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig index 54102deb50ba..53d51ed619a3 100644 --- a/kernel/livepatch/Kconfig +++ b/kernel/livepatch/Kconfig @@ -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 diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c index f89f9e7e9b07..e8029aea67f1 100644 --- a/kernel/livepatch/patch.c +++ b/kernel/livepatch/patch.c @@ -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; -- 2.28.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 3/3 v5] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available 2020-11-12 1:15 ` [PATCH 3/3 v5] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available Steven Rostedt @ 2020-11-12 8:21 ` Peter Zijlstra 2020-11-12 11:39 ` Peter Zijlstra 2020-11-12 14:59 ` Steven Rostedt 0 siblings, 2 replies; 4+ messages in thread From: Peter Zijlstra @ 2020-11-12 8:21 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Ingo Molnar, Andrew Morton, Miroslav Benes, Thomas Gleixner, Masami Hiramatsu, Josh Poimboeuf, Jiri Kosina, live-patching On Wed, Nov 11, 2020 at 08:15:19PM -0500, Steven Rostedt wrote: > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h > index e00fe88146e0..235385a38bd9 100644 > --- a/arch/x86/include/asm/ftrace.h > +++ b/arch/x86/include/asm/ftrace.h > @@ -54,6 +54,9 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs) > return NULL; > return &fregs->regs; > } > + > +#define ftrace_regs_set_ip(fregs, _ip) \ > + do { (fregs)->regs.ip = (_ip); } while (0) > #endif > > #ifdef CONFIG_DYNAMIC_FTRACE > diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h > index 1fde1ab6559e..59a08d5c6f1d 100644 > --- a/arch/x86/include/asm/livepatch.h > +++ b/arch/x86/include/asm/livepatch.h > @@ -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_regs_set_ip(fregs, ip); > } > The normal variant is called instruction_pointer_set(), should this be called ftrace_instruction_pointer_set() ? (and yes, I hate the long name too). Also, do you want something like: unsigned long ftrace_regs_get_register(struct ftrace_regs *regs, unsigned int offset) { switch (offset / sizeof(long)) { case 4: /* RBP */ case 8: /* R9 */ case 9: /* R8 */ case 10: /* RAX */ case 11: /* RCX */ case 12: /* RDX */ case 13: /* RSI */ case 14: /* RDI */ case 15: /* ORIG_RAX */ case 16: /* RIP */ return *(unsigned long *)regs->regs + offset; default: WARN_ON_ONCE(1); } return 0; } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3/3 v5] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available 2020-11-12 8:21 ` Peter Zijlstra @ 2020-11-12 11:39 ` Peter Zijlstra 2020-11-12 14:59 ` Steven Rostedt 1 sibling, 0 replies; 4+ messages in thread From: Peter Zijlstra @ 2020-11-12 11:39 UTC (permalink / raw) To: Steven Rostedt Cc: linux-kernel, Ingo Molnar, Andrew Morton, Miroslav Benes, Thomas Gleixner, Masami Hiramatsu, Josh Poimboeuf, Jiri Kosina, live-patching On Thu, Nov 12, 2020 at 09:21:44AM +0100, Peter Zijlstra wrote: > Also, do you want something like: > > unsigned long ftrace_regs_get_register(struct ftrace_regs *regs, unsigned int offset) > { I forgot the full regs case: if (regs->regs.cs) return regs_get_register(regs->regs, offset); > switch (offset / sizeof(long)) { > case 4: /* RBP */ > > case 8: /* R9 */ > case 9: /* R8 */ > case 10: /* RAX */ > case 11: /* RCX */ > case 12: /* RDX */ > case 13: /* RSI */ > case 14: /* RDI */ > case 15: /* ORIG_RAX */ > case 16: /* RIP */ > return *(unsigned long *)regs->regs + offset; > > default: > WARN_ON_ONCE(1); > } > return 0; > } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3/3 v5] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available 2020-11-12 8:21 ` Peter Zijlstra 2020-11-12 11:39 ` Peter Zijlstra @ 2020-11-12 14:59 ` Steven Rostedt 1 sibling, 0 replies; 4+ messages in thread From: Steven Rostedt @ 2020-11-12 14:59 UTC (permalink / raw) To: Peter Zijlstra Cc: linux-kernel, Ingo Molnar, Andrew Morton, Miroslav Benes, Thomas Gleixner, Masami Hiramatsu, Josh Poimboeuf, Jiri Kosina, live-patching On Thu, 12 Nov 2020 09:21:44 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Nov 11, 2020 at 08:15:19PM -0500, Steven Rostedt wrote: > > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h > > index e00fe88146e0..235385a38bd9 100644 > > --- a/arch/x86/include/asm/ftrace.h > > +++ b/arch/x86/include/asm/ftrace.h > > @@ -54,6 +54,9 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs) > > return NULL; > > return &fregs->regs; > > } > > + > > +#define ftrace_regs_set_ip(fregs, _ip) \ > > + do { (fregs)->regs.ip = (_ip); } while (0) > > #endif > > > > #ifdef CONFIG_DYNAMIC_FTRACE > > diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h > > index 1fde1ab6559e..59a08d5c6f1d 100644 > > --- a/arch/x86/include/asm/livepatch.h > > +++ b/arch/x86/include/asm/livepatch.h > > @@ -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_regs_set_ip(fregs, ip); > > } > > > > The normal variant is called instruction_pointer_set(), should this be > called ftrace_instruction_pointer_set() ? Sure, I can change that. > > (and yes, I hate the long name too). ftrace_regs_ip_set()? ;-) > > Also, do you want something like: > > unsigned long ftrace_regs_get_register(struct ftrace_regs *regs, unsigned int offset) > { I haven't gotten this far yet. I'm looking at generic use cases on how to get args across archs. Each arch will have its own method. > switch (offset / sizeof(long)) { > case 4: /* RBP */ > > case 8: /* R9 */ > case 9: /* R8 */ > case 10: /* RAX */ > case 11: /* RCX */ > case 12: /* RDX */ > case 13: /* RSI */ > case 14: /* RDI */ > case 15: /* ORIG_RAX */ > case 16: /* RIP */ > return *(unsigned long *)regs->regs + offset; > > default: > WARN_ON_ONCE(1); Not sure we even want to warn. Perhaps have this as: bool ftrace_regs_get_register(struct ftrace_regs *regs, unsigned int offset, unsigned long *val) { if (regs->cs) { *val = regs_get_register(regs->regs, offset); return true; } switch (offset / sizeof(long)) { case ...: *val = *(unsigned long *)regs->regs + offset; return true; default; return false; > } -- Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-12 14:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20201112011516.589846126@goodmis.org>
2020-11-12 1:15 ` [PATCH 3/3 v5] livepatch: Use the default ftrace_ops instead of REGS when ARGS is available Steven Rostedt
2020-11-12 8:21 ` Peter Zijlstra
2020-11-12 11:39 ` Peter Zijlstra
2020-11-12 14:59 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox