Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support
On Fri, 16 Jan 2015 17:41:58 +0100 (CET) Jiri Kosina wrote: > On Fri, 16 Jan 2015, Steven Rostedt wrote: > > > Instead, make live kernel patching fail to load if fentry isn't > > supported. IOW, instead of ftrace_ipmodify_supported, have a > > live_kernel_patching_supported that could be based on fentry being used > > or not. > > I can live with that, we are handling non-fentry case ourselves already. > > You expressed that you would accept such patch for ftrace in a past, so I Right, but then I thought about it more :-) I had a lot of time thinking about things like this while debugging the dance between function graph and jprobes. > thought I'll move it out from livepatching to ftrace. But if you don't > want it now, that's not a big issue, and handling the possible fallout > stays the responsibility of ftrace_ops "owner". > Exactly! -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support
On Fri, 16 Jan 2015, Steven Rostedt wrote: > Instead, make live kernel patching fail to load if fentry isn't > supported. IOW, instead of ftrace_ipmodify_supported, have a > live_kernel_patching_supported that could be based on fentry being used > or not. I can live with that, we are handling non-fentry case ourselves already. You expressed that you would accept such patch for ftrace in a past, so I thought I'll move it out from livepatching to ftrace. But if you don't want it now, that's not a big issue, and handling the possible fallout stays the responsibility of ftrace_ops "owner". -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support
On Thu, 15 Jan 2015 10:50:07 +0100 (CET) Jiri Kosina wrote: > 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. That's actually not true. Sorry, I started thinking about this more, and I disagree with this change. > > 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. No, it only causes havoc if whoever modifies the ip doesn't know it's not at the start of the function. Hence, it's only the user of ftrace that wants to replace functions that needs to worry about this. In fact, there's nothing wrong if kprobes to use ftrace to change ip even if fentry isn't supported. That's because if fentry isn't supported, kprobes will notice that there's no ftrace call at the start of a function and will use a breakpoint instead. If a kprobe user still wants to change the ip after the stack frame was set up, they still can do that, they just need to find where the mcount call is. kprobe does its work based on the kernel address to probe, not where ftrace places its hooks. Now you could argue that there's no reason to change ip if ftrace isn't using fentry, but there's nothing to prevent a user from doing so. It also bothers me that you just prevented all users of kprobes from taking advantage of hooking to a ftrace caller if fentry isn't supported. All kprobes really needs is the REGS version supported. 99% of all kprobes do not modify ip. I have to say NAK on this change. Instead, make live kernel patching fail to load if fentry isn't supported. IOW, instead of ftrace_ipmodify_supported, have a live_kernel_patching_supported that could be based on fentry being used or not. -- Steve > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support
On Thu, 15 Jan 2015 10:50:07 +0100 (CET) Jiri Kosina jkos...@suse.cz wrote: 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. That's actually not true. Sorry, I started thinking about this more, and I disagree with this change. 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. No, it only causes havoc if whoever modifies the ip doesn't know it's not at the start of the function. Hence, it's only the user of ftrace that wants to replace functions that needs to worry about this. In fact, there's nothing wrong if kprobes to use ftrace to change ip even if fentry isn't supported. That's because if fentry isn't supported, kprobes will notice that there's no ftrace call at the start of a function and will use a breakpoint instead. If a kprobe user still wants to change the ip after the stack frame was set up, they still can do that, they just need to find where the mcount call is. kprobe does its work based on the kernel address to probe, not where ftrace places its hooks. Now you could argue that there's no reason to change ip if ftrace isn't using fentry, but there's nothing to prevent a user from doing so. It also bothers me that you just prevented all users of kprobes from taking advantage of hooking to a ftrace caller if fentry isn't supported. All kprobes really needs is the REGS version supported. 99% of all kprobes do not modify ip. I have to say NAK on this change. Instead, make live kernel patching fail to load if fentry isn't supported. IOW, instead of ftrace_ipmodify_supported, have a live_kernel_patching_supported that could be based on fentry being used or not. -- Steve 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 jkos...@suse.cz -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support
On Fri, 16 Jan 2015, Steven Rostedt wrote: Instead, make live kernel patching fail to load if fentry isn't supported. IOW, instead of ftrace_ipmodify_supported, have a live_kernel_patching_supported that could be based on fentry being used or not. I can live with that, we are handling non-fentry case ourselves already. You expressed that you would accept such patch for ftrace in a past, so I thought I'll move it out from livepatching to ftrace. But if you don't want it now, that's not a big issue, and handling the possible fallout stays the responsibility of ftrace_ops owner. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support
On Fri, 16 Jan 2015 17:41:58 +0100 (CET) Jiri Kosina jkos...@suse.cz wrote: On Fri, 16 Jan 2015, Steven Rostedt wrote: Instead, make live kernel patching fail to load if fentry isn't supported. IOW, instead of ftrace_ipmodify_supported, have a live_kernel_patching_supported that could be based on fentry being used or not. I can live with that, we are handling non-fentry case ourselves already. You expressed that you would accept such patch for ftrace in a past, so I Right, but then I thought about it more :-) I had a lot of time thinking about things like this while debugging the dance between function graph and jprobes. thought I'll move it out from livepatching to ftrace. But if you don't want it now, that's not a big issue, and handling the possible fallout stays the responsibility of ftrace_ops owner. Exactly! -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2 v3] 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
---
v1 -> v2: turn macro function into a plain macro
v2 -> v3: return ENOTSUPP and reject registering ftrace_ops in
ftrace_hash_move() already
arch/x86/include/asm/ftrace.h | 2 ++
include/linux/ftrace.h| 5 +
kernel/trace/ftrace.c | 5 +
3 files changed, 12 insertions(+)
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index f45acad..f84eaaf 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 ftrace_ipmodify_supported 1
#else
# define MCOUNT_ADDR ((long)(mcount))
+# define ftrace_ipmodify_supported 0
#endif
#define MCOUNT_INSN_SIZE 5 /* sizeof mcount call */
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1da6029..837153a 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -244,6 +244,11 @@ 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 ftrace_ipmodify_supported
+/* let's not make any implicit assumptions about profiling call placement */
+# define ftrace_ipmodify_supported 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..3c1261c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1378,6 +1378,11 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
if (ops->flags & FTRACE_OPS_FL_IPMODIFY && !enable)
return -EINVAL;
+ if ((ops->flags & FTRACE_OPS_FL_IPMODIFY) &&
!ftrace_ipmodify_supported) {
+ WARN(1, "Your compiler doesn't support features necessary for
IPMODIFY");
+ return -ENOTSUPP;
+ }
+
/*
* If the new source is empty, just free dst and assign it
* the empty_hash.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2 v3] 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 jkos...@suse.cz
---
v1 - v2: turn macro function into a plain macro
v2 - v3: return ENOTSUPP and reject registering ftrace_ops in
ftrace_hash_move() already
arch/x86/include/asm/ftrace.h | 2 ++
include/linux/ftrace.h| 5 +
kernel/trace/ftrace.c | 5 +
3 files changed, 12 insertions(+)
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index f45acad..f84eaaf 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 ftrace_ipmodify_supported 1
#else
# define MCOUNT_ADDR ((long)(mcount))
+# define ftrace_ipmodify_supported 0
#endif
#define MCOUNT_INSN_SIZE 5 /* sizeof mcount call */
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1da6029..837153a 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -244,6 +244,11 @@ 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 ftrace_ipmodify_supported
+/* let's not make any implicit assumptions about profiling call placement */
+# define ftrace_ipmodify_supported 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..3c1261c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1378,6 +1378,11 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
if (ops-flags FTRACE_OPS_FL_IPMODIFY !enable)
return -EINVAL;
+ if ((ops-flags FTRACE_OPS_FL_IPMODIFY)
!ftrace_ipmodify_supported) {
+ WARN(1, Your compiler doesn't support features necessary for
IPMODIFY);
+ return -ENOTSUPP;
+ }
+
/*
* If the new source is empty, just free dst and assign it
* the empty_hash.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
