Re: [PATCH -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs->ip
(2014/06/19 21:34), Namhyung Kim wrote: > Hi Masami, > > 2014-06-17 (화), 11:04 +, Masami Hiramatsu: >> +static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip, >> + int *ref) >> +{ >> +int ret; >> + >> +/* Try to set given ip to filter */ >> +ret = ftrace_set_filter_ip(ops, ip, 0, 0); >> +if (ret >= 0) { > > Hmm.. this doesn't look comfortable. What not using usual pattern? > > if (ret < 0) > return ret; > > This way we can reduce a indent level. OK, I'll do so :) > > >> +(*ref)++; >> +if (*ref == 1) { >> +ret = register_ftrace_function(ops); >> +if (ret < 0) { >> +/* Rollback refcounter and filter */ >> +(*ref)--; >> +ftrace_set_filter_ip(ops, ip, 1, 0); >> +} >> +} >> +} >> + >> +return ret; >> +} >> + >> +static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long >> ip, >> + int *ref) >> +{ >> +int ret = 0; >> + >> +(*ref)--; >> +if (*ref == 0) >> +ret = unregister_ftrace_function(ops); >> +if (ret >= 0) >> +/* Try to remove given ip to filter */ >> +ret = ftrace_set_filter_ip(ops, ip, 1, 0); > > I think any failure at this time can be problematic. Since we already > unregistered the ops but the refcount will get increased again, future > attemp to register won't enable the ops anymore IMHO. Agreed. > I think it'd better just ignoring faiure of filter removal here. We'll > miss a filter entry but it'll be usable anyway. > > What about this? OK, I'll use your v2 code :) Thank you for review! -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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 -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs->ip
On Thu, 19 Jun 2014 21:34:31 +0900, Namhyung Kim wrote: > What about this? > > static int __ftrace_remove_filter_ip(...) > { > if (*ref == 1) { > int ret = unregister_ftrace_function(ops); > if (ret < 0) > return ret; > > ftrace_set_filter_ip(ops, ip, 1, 0); > } > > (*ref)--; > return 0; > } Hmm.. I missed removing in *ref > 1 case. Here's a v2. :) static int __ftrace_remove_filter_ip(...) { int ret; if (*ref == 1) { ret = unregister_ftrace_function(ops); if (ret < 0) return ret; /* ignore failure here */ ftrace_set_filter_ip(ops, ip, 1, 0); } else ret = ftrace_set_filter_ip(ops, ip, 1, 0); if (ret < 0) return ret; } (*ref)--; return 0; } -- 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 -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs->ip
Hi Masami, 2014-06-17 (화), 11:04 +, Masami Hiramatsu: > +static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip, > + int *ref) > +{ > + int ret; > + > + /* Try to set given ip to filter */ > + ret = ftrace_set_filter_ip(ops, ip, 0, 0); > + if (ret >= 0) { Hmm.. this doesn't look comfortable. What not using usual pattern? if (ret < 0) return ret; This way we can reduce a indent level. > + (*ref)++; > + if (*ref == 1) { > + ret = register_ftrace_function(ops); > + if (ret < 0) { > + /* Rollback refcounter and filter */ > + (*ref)--; > + ftrace_set_filter_ip(ops, ip, 1, 0); > + } > + } > + } > + > + return ret; > +} > + > +static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long > ip, > + int *ref) > +{ > + int ret = 0; > + > + (*ref)--; > + if (*ref == 0) > + ret = unregister_ftrace_function(ops); > + if (ret >= 0) > + /* Try to remove given ip to filter */ > + ret = ftrace_set_filter_ip(ops, ip, 1, 0); I think any failure at this time can be problematic. Since we already unregistered the ops but the refcount will get increased again, future attemp to register won't enable the ops anymore IMHO. I think it'd better just ignoring faiure of filter removal here. We'll miss a filter entry but it'll be usable anyway. What about this? static int __ftrace_remove_filter_ip(...) { if (*ref == 1) { int ret = unregister_ftrace_function(ops); if (ret < 0) return ret; ftrace_set_filter_ip(ops, ip, 1, 0); } (*ref)--; return 0; } Thanks, Namhyung > + if (ret < 0)/* Rollback refcounter if an error occurs */ > + (*ref)++; > + > + return ret; > +} -- 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 -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs-ip
Hi Masami, 2014-06-17 (화), 11:04 +, Masami Hiramatsu: +static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip, + int *ref) +{ + int ret; + + /* Try to set given ip to filter */ + ret = ftrace_set_filter_ip(ops, ip, 0, 0); + if (ret = 0) { Hmm.. this doesn't look comfortable. What not using usual pattern? if (ret 0) return ret; This way we can reduce a indent level. + (*ref)++; + if (*ref == 1) { + ret = register_ftrace_function(ops); + if (ret 0) { + /* Rollback refcounter and filter */ + (*ref)--; + ftrace_set_filter_ip(ops, ip, 1, 0); + } + } + } + + return ret; +} + +static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long ip, + int *ref) +{ + int ret = 0; + + (*ref)--; + if (*ref == 0) + ret = unregister_ftrace_function(ops); + if (ret = 0) + /* Try to remove given ip to filter */ + ret = ftrace_set_filter_ip(ops, ip, 1, 0); I think any failure at this time can be problematic. Since we already unregistered the ops but the refcount will get increased again, future attemp to register won't enable the ops anymore IMHO. I think it'd better just ignoring faiure of filter removal here. We'll miss a filter entry but it'll be usable anyway. What about this? static int __ftrace_remove_filter_ip(...) { if (*ref == 1) { int ret = unregister_ftrace_function(ops); if (ret 0) return ret; ftrace_set_filter_ip(ops, ip, 1, 0); } (*ref)--; return 0; } Thanks, Namhyung + if (ret 0)/* Rollback refcounter if an error occurs */ + (*ref)++; + + return ret; +} -- 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 -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs-ip
On Thu, 19 Jun 2014 21:34:31 +0900, Namhyung Kim wrote: What about this? static int __ftrace_remove_filter_ip(...) { if (*ref == 1) { int ret = unregister_ftrace_function(ops); if (ret 0) return ret; ftrace_set_filter_ip(ops, ip, 1, 0); } (*ref)--; return 0; } Hmm.. I missed removing in *ref 1 case. Here's a v2. :) static int __ftrace_remove_filter_ip(...) { int ret; if (*ref == 1) { ret = unregister_ftrace_function(ops); if (ret 0) return ret; /* ignore failure here */ ftrace_set_filter_ip(ops, ip, 1, 0); } else ret = ftrace_set_filter_ip(ops, ip, 1, 0); if (ret 0) return ret; } (*ref)--; return 0; } -- 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 -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs-ip
(2014/06/19 21:34), Namhyung Kim wrote: Hi Masami, 2014-06-17 (화), 11:04 +, Masami Hiramatsu: +static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip, + int *ref) +{ +int ret; + +/* Try to set given ip to filter */ +ret = ftrace_set_filter_ip(ops, ip, 0, 0); +if (ret = 0) { Hmm.. this doesn't look comfortable. What not using usual pattern? if (ret 0) return ret; This way we can reduce a indent level. OK, I'll do so :) +(*ref)++; +if (*ref == 1) { +ret = register_ftrace_function(ops); +if (ret 0) { +/* Rollback refcounter and filter */ +(*ref)--; +ftrace_set_filter_ip(ops, ip, 1, 0); +} +} +} + +return ret; +} + +static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long ip, + int *ref) +{ +int ret = 0; + +(*ref)--; +if (*ref == 0) +ret = unregister_ftrace_function(ops); +if (ret = 0) +/* Try to remove given ip to filter */ +ret = ftrace_set_filter_ip(ops, ip, 1, 0); I think any failure at this time can be problematic. Since we already unregistered the ops but the refcount will get increased again, future attemp to register won't enable the ops anymore IMHO. Agreed. I think it'd better just ignoring faiure of filter removal here. We'll miss a filter entry but it'll be usable anyway. What about this? OK, I'll use your v2 code :) Thank you for review! -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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 -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs->ip
Set FTRACE_OPS_FL_IPMODIFY flag only for the probes which can change regs->ip, which has kprobe->break_handler. Currently only jprobe modifies regs->ip to hook a function entry and jumps to a user handler which has same prototype of the original function. In other cases, kprobes restores original regs->ip when returning to ftrace. Note about the implementation: This uses a dummy ftrace_ops to reserve IPMODIFY flag on the given ftrace address, for the case that we have a enabled kprobe on a function entry and a jprobe is added on the same point. In that case, we already have a ftrace_ops without IPMODIFY flag on the entry, and we have to add another ftrace_ops with IPMODIFY on the same address. If we put a same handler on both ftrace_ops, the handler can be called twice on that entry until the first one is removed. This means that the kprobe and the jprobe are called twice too, and that will not what kprobes expected. Thus I added a dummy ftrace_ops just for reserving IPMODIFY flag. Signed-off-by: Masami Hiramatsu Cc: Ananth N Mavinakayanahalli Cc: Steven Rostedt Cc: Josh Poimboeuf Cc: Namhyung Kim --- Documentation/kprobes.txt| 12 ++-- arch/x86/kernel/kprobes/ftrace.c |9 ++- kernel/kprobes.c | 117 +- 3 files changed, 112 insertions(+), 26 deletions(-) diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt index 4bbeca8..177f051 100644 --- a/Documentation/kprobes.txt +++ b/Documentation/kprobes.txt @@ -264,15 +264,13 @@ stop-machine method that ksplice uses for supporting a CONFIG_PREEMPT=y kernel. NOTE for geeks: -The jump optimization changes the kprobe's pre_handler behavior. -Without optimization, the pre_handler can change the kernel's execution +The jump optimization (and ftrace-based kprobes) changes the kprobe's +pre_handler behavior. +Without optimizations, the pre_handler can change the kernel's execution path by changing regs->ip and returning 1. However, when the probe is optimized, that modification is ignored. Thus, if you want to -tweak the kernel's execution path, you need to suppress optimization, -using one of the following techniques: -- Specify an empty function for the kprobe's post_handler or break_handler. - or -- Execute 'sysctl -w debug.kprobes_optimization=n' +tweak the kernel's execution path, you need to suppress optimization or +notify your handler will modify regs->ip by setting p->break_handler. 1.5 Blacklist diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c index 717b02a..5f8f0b3 100644 --- a/arch/x86/kernel/kprobes/ftrace.c +++ b/arch/x86/kernel/kprobes/ftrace.c @@ -27,7 +27,7 @@ static nokprobe_inline int __skip_singlestep(struct kprobe *p, struct pt_regs *regs, - struct kprobe_ctlblk *kcb) + struct kprobe_ctlblk *kcb, unsigned long orig_ip) { /* * Emulate singlestep (and also recover regs->ip) @@ -39,6 +39,8 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs, p->post_handler(p, regs, 0); } __this_cpu_write(current_kprobe, NULL); + if (orig_ip) + regs->ip = orig_ip; return 1; } @@ -46,7 +48,7 @@ int skip_singlestep(struct kprobe *p, struct pt_regs *regs, struct kprobe_ctlblk *kcb) { if (kprobe_ftrace(p)) - return __skip_singlestep(p, regs, kcb); + return __skip_singlestep(p, regs, kcb, 0); else return 0; } @@ -71,13 +73,14 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, if (kprobe_running()) { kprobes_inc_nmissed_count(p); } else { + unsigned long orig_ip = regs->ip; /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */ regs->ip = ip + sizeof(kprobe_opcode_t); __this_cpu_write(current_kprobe, p); kcb->kprobe_status = KPROBE_HIT_ACTIVE; if (!p->pre_handler || !p->pre_handler(p, regs)) - __skip_singlestep(p, regs, kcb); + __skip_singlestep(p, regs, kcb, orig_ip); /* * If pre_handler returns !0, it sets regs->ip and * resets current kprobe. diff --git a/kernel/kprobes.c b/kernel/kprobes.c index e52d86f..8ba1798 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -915,10 +915,85 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p) #ifdef CONFIG_KPROBES_ON_FTRACE static struct ftrace_ops kprobe_ftrace_ops __read_mostly = { .func = kprobe_ftrace_handler, - .flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY, + .flags = FTRACE_OPS_FL_SAVE_REGS, }; static int kprobe_ftrace_enabled; +static void kprobe_ftrace_stub(unsigned long a0, unsigned long a1, + struct ftrace_ops *op, struct pt_regs *regs) +{ + /* Do
[PATCH -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs-ip
Set FTRACE_OPS_FL_IPMODIFY flag only for the probes which can change regs-ip, which has kprobe-break_handler. Currently only jprobe modifies regs-ip to hook a function entry and jumps to a user handler which has same prototype of the original function. In other cases, kprobes restores original regs-ip when returning to ftrace. Note about the implementation: This uses a dummy ftrace_ops to reserve IPMODIFY flag on the given ftrace address, for the case that we have a enabled kprobe on a function entry and a jprobe is added on the same point. In that case, we already have a ftrace_ops without IPMODIFY flag on the entry, and we have to add another ftrace_ops with IPMODIFY on the same address. If we put a same handler on both ftrace_ops, the handler can be called twice on that entry until the first one is removed. This means that the kprobe and the jprobe are called twice too, and that will not what kprobes expected. Thus I added a dummy ftrace_ops just for reserving IPMODIFY flag. Signed-off-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com Cc: Ananth N Mavinakayanahalli ana...@in.ibm.com Cc: Steven Rostedt rost...@goodmis.org Cc: Josh Poimboeuf jpoim...@redhat.com Cc: Namhyung Kim namhy...@kernel.org --- Documentation/kprobes.txt| 12 ++-- arch/x86/kernel/kprobes/ftrace.c |9 ++- kernel/kprobes.c | 117 +- 3 files changed, 112 insertions(+), 26 deletions(-) diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt index 4bbeca8..177f051 100644 --- a/Documentation/kprobes.txt +++ b/Documentation/kprobes.txt @@ -264,15 +264,13 @@ stop-machine method that ksplice uses for supporting a CONFIG_PREEMPT=y kernel. NOTE for geeks: -The jump optimization changes the kprobe's pre_handler behavior. -Without optimization, the pre_handler can change the kernel's execution +The jump optimization (and ftrace-based kprobes) changes the kprobe's +pre_handler behavior. +Without optimizations, the pre_handler can change the kernel's execution path by changing regs-ip and returning 1. However, when the probe is optimized, that modification is ignored. Thus, if you want to -tweak the kernel's execution path, you need to suppress optimization, -using one of the following techniques: -- Specify an empty function for the kprobe's post_handler or break_handler. - or -- Execute 'sysctl -w debug.kprobes_optimization=n' +tweak the kernel's execution path, you need to suppress optimization or +notify your handler will modify regs-ip by setting p-break_handler. 1.5 Blacklist diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c index 717b02a..5f8f0b3 100644 --- a/arch/x86/kernel/kprobes/ftrace.c +++ b/arch/x86/kernel/kprobes/ftrace.c @@ -27,7 +27,7 @@ static nokprobe_inline int __skip_singlestep(struct kprobe *p, struct pt_regs *regs, - struct kprobe_ctlblk *kcb) + struct kprobe_ctlblk *kcb, unsigned long orig_ip) { /* * Emulate singlestep (and also recover regs-ip) @@ -39,6 +39,8 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs, p-post_handler(p, regs, 0); } __this_cpu_write(current_kprobe, NULL); + if (orig_ip) + regs-ip = orig_ip; return 1; } @@ -46,7 +48,7 @@ int skip_singlestep(struct kprobe *p, struct pt_regs *regs, struct kprobe_ctlblk *kcb) { if (kprobe_ftrace(p)) - return __skip_singlestep(p, regs, kcb); + return __skip_singlestep(p, regs, kcb, 0); else return 0; } @@ -71,13 +73,14 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, if (kprobe_running()) { kprobes_inc_nmissed_count(p); } else { + unsigned long orig_ip = regs-ip; /* Kprobe handler expects regs-ip = ip + 1 as breakpoint hit */ regs-ip = ip + sizeof(kprobe_opcode_t); __this_cpu_write(current_kprobe, p); kcb-kprobe_status = KPROBE_HIT_ACTIVE; if (!p-pre_handler || !p-pre_handler(p, regs)) - __skip_singlestep(p, regs, kcb); + __skip_singlestep(p, regs, kcb, orig_ip); /* * If pre_handler returns !0, it sets regs-ip and * resets current kprobe. diff --git a/kernel/kprobes.c b/kernel/kprobes.c index e52d86f..8ba1798 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -915,10 +915,85 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p) #ifdef CONFIG_KPROBES_ON_FTRACE static struct ftrace_ops kprobe_ftrace_ops __read_mostly = { .func = kprobe_ftrace_handler, - .flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY, + .flags = FTRACE_OPS_FL_SAVE_REGS, }; static int kprobe_ftrace_enabled; +static void kprobe_ftrace_stub(unsigned long a0, unsigned long