[PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)

[Jiri Kosina]

On Mon, 12 Jan 2015, Masami Hiramatsu wrote:

> > In any case, Masami, I really think you would like to do something 
> > like that for IPMODIFY as well ... or are you deliberately defering 
> > the responsibility to handle the possible mcount fallout to the 
> > ftrace_ops owner?
> 
> Ah, good point. I just tried to use ftrace and WARN if not possible
> to use it. I'll see it tomorrow. Anyway, I'd prefer to have this
> kind of checking functionality in ftrace.

Okay, so how about something like this, for example ... ?



From: Jiri Kosina <jkosina@xxxxxxx>
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 <jkosina@xxxxxxx>
---
 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; }
+#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.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/