(live-patching ML CCed, keeping the complete email for reference)
Hi,
a nit concerning the subject. We use just "livepatch:" as a prefix.
On Wed, 28 Oct 2020, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
>
> 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.
I think this is a nice idea, which could easily lead to removing
FTRACE_WITH_REGS altogether. I'm really looking forward to that because
every consolidation step is welcome.
My only remark is for the config. LIVEPATCH now depends on
DYNAMIC_FTRACE_WITH_REGS which is not completely true after this change,
so we should probably make it depend on DYNAMIC_FTRACE_WITH_REGS ||
DYNAMIC_FTRACE_WITH_ARGS, just to reflect the situation better.
Anyway, it passed my tests too and the patch looks good to me, so
Acked-by: Miroslav Benes <mbenes@xxxxxxx>
M
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
> 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/patch.c | 9 +++++----
> 7 files changed, 28 insertions(+), 8 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 6b175c2468e6..7042e80941e5 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -62,6 +62,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
>
> #endif /* 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 c4177bd00cd2..d00806dd8eed 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/patch.c b/kernel/livepatch/patch.c
> index 9af0a7c8a255..722266472903 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
>
>
