From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Rostedt Subject: Re: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next) Date: Tue, 13 Jan 2015 21:12:23 -0500 Message-ID: <20150113211223.18908f21@grimm.local.home> References: <20141223094607.GA16445@infradead.org> <20141223151056.GA4789@treble.redhat.com> <20141226155613.36dd95b9@canb.auug.org.au> <20150107144317.61ab2080877a4d8227990551@linux-foundation.org> <20150107153006.60ed354e3458f402e6819b9e@linux-foundation.org> <20150107155701.4839545f63701412003edd88@linux-foundation.org> <54B3C1DD.6090400@hitachi.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.225]:53257 "EHLO cdptpa-oedge-vip.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751389AbbANCMN (ORCPT ); Tue, 13 Jan 2015 21:12:13 -0500 In-Reply-To: Sender: linux-next-owner@vger.kernel.org List-ID: To: Jiri Kosina Cc: Masami Hiramatsu , Andrew Morton , Stephen Rothwell , Josh Poimboeuf , Christoph Hellwig , linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, linux-next@vger.kernel.org On Tue, 13 Jan 2015 23:47:57 +0100 (CET) Jiri Kosina wrote: > From: Jiri Kosina > Subject: [PATCH] ftrace: don't allow IPMODIFY without proper compiler > support > > Using IPMODIFY needs to be allowed only with compilers which are > guaranteed to generate function prologues compatible with function > redirection through changing instruction pointer in saved regs. > > For example changing regs->ip on x86_64 in cases when gcc is using > mcount (and not fentry) is not allowed, because at the time mcount > call is issued, the original function's prologue has already been > executed, which leads to all kinds of inconsistent havoc. > > There is currently no way to express dependency on gcc features in > Kconfig, (CC_USING_FENTRY is defined only during build, so it's not > possible for Kconfig symbol to depend on it) so this needs to be > checked in runtime. > > Mark x86_64 with fentry supported for now. Other archs can be added > gradually. > > Signed-off-by: Jiri Kosina > --- > arch/x86/include/asm/ftrace.h | 2 ++ > include/linux/ftrace.h | 4 ++++ > kernel/trace/ftrace.c | 5 +++++ > 3 files changed, 11 insertions(+) > > diff --git a/arch/x86/include/asm/ftrace.h > b/arch/x86/include/asm/ftrace.h index f45acad..29fa417 100644 > --- a/arch/x86/include/asm/ftrace.h > +++ b/arch/x86/include/asm/ftrace.h > @@ -4,8 +4,10 @@ > #ifdef CONFIG_FUNCTION_TRACER > #ifdef CC_USING_FENTRY > # define MCOUNT_ADDR ((long)(__fentry__)) > +# define arch_ftrace_ipmodify_compiler_support(void) ({ 1; }) > #else > # define MCOUNT_ADDR ((long)(mcount)) > +#define arch_ftrace_ipmodify_compiler_support(void) ({ 0; }) > #endif > #define MCOUNT_INSN_SIZE 5 /* sizeof mcount call */ > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 1da6029..655ba99 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -244,6 +244,10 @@ static inline int > ftrace_function_local_disabled(struct ftrace_ops *ops) extern void > ftrace_stub(unsigned long a0, unsigned long a1, struct ftrace_ops > *op, struct pt_regs *regs); > +#ifndef arch_ftrace_ipmodify_compiler_support > +/* let's not make any implicit assumptions about profiling call > placement */ +# define arch_ftrace_ipmodify_compiler_support() { 0; } Is there any reason that this is a macro function? Why not just define: #define ftrace_ipmodify_supported 0 ? -- Steve > +#endif > #else /* !CONFIG_FUNCTION_TRACER */ > /* > * (un)register_ftrace_function must be a macro since the ops > parameter diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 929a733..11370fd 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -1809,6 +1809,11 @@ static int > __ftrace_hash_update_ipmodify(struct ftrace_ops *ops, if > (!(ops->flags & FTRACE_OPS_FL_IPMODIFY)) return 0; > > + if (!arch_ftrace_ipmodify_compiler_support()) { > + WARN(1, "Your compiler doesn't support features > necessary for IPMODIFY"); > + return 0; > + } > + > /* > * Since the IPMODIFY is a very address sensitive action, we > do not > * allow ftrace_ops to set all functions to new hash. >