* [PATCH 0/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() @ 2012-12-30 15:46 Oleg Nesterov 2012-12-30 15:47 ` [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn Oleg Nesterov 2012-12-30 15:47 ` [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov 0 siblings, 2 replies; 8+ messages in thread From: Oleg Nesterov @ 2012-12-30 15:46 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju Cc: Ananth N Mavinakayanahalli, Anton Arapov, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel Hello. Inspired by the recent uretrpobe RFC's. I think it simply makes no sense to call handler/skip/arch_uprobe_pre_xol with regs->ip pointing to the "random" address after the task hits bp. Ananth, could you please confirm 2/2 can't break powerpc? It "obviously" shouldn't because uprobe_get_swbp_addr() is a "nop" on ppc, but still your ack is welcomed. Oleg. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn 2012-12-30 15:46 [PATCH 0/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov @ 2012-12-30 15:47 ` Oleg Nesterov 2013-01-02 13:16 ` Anton Arapov ` (2 more replies) 2012-12-30 15:47 ` [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov 1 sibling, 3 replies; 8+ messages in thread From: Oleg Nesterov @ 2012-12-30 15:47 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju Cc: Ananth N Mavinakayanahalli, Anton Arapov, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel __skip_sstep() doesn't update regs->ip. Currently this is correct but only "by accident" and it doesn't skip the whole insn. Change it to advance ->ip by the length of the detected 0x66*0x90 sequence. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- arch/x86/kernel/uprobes.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c index c71025b..4e33a35 100644 --- a/arch/x86/kernel/uprobes.c +++ b/arch/x86/kernel/uprobes.c @@ -680,8 +680,11 @@ static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) if (auprobe->insn[i] == 0x66) continue; - if (auprobe->insn[i] == 0x90) + if (auprobe->insn[i] == 0x90) { + regs->ip = uprobe_get_swbp_addr(regs); + regs->ip += i + 1; return true; + } break; } -- 1.5.5.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn 2012-12-30 15:47 ` [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn Oleg Nesterov @ 2013-01-02 13:16 ` Anton Arapov 2013-01-03 12:01 ` Srikar Dronamraju 2013-01-08 11:24 ` Srikar Dronamraju 2 siblings, 0 replies; 8+ messages in thread From: Anton Arapov @ 2013-01-02 13:16 UTC (permalink / raw) To: Oleg Nesterov Cc: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju, Ananth N Mavinakayanahalli, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel On Sun, Dec 30, 2012 at 04:47:19PM +0100, Oleg Nesterov wrote: > __skip_sstep() doesn't update regs->ip. Currently this is correct > but only "by accident" and it doesn't skip the whole insn. Change > it to advance ->ip by the length of the detected 0x66*0x90 sequence. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > arch/x86/kernel/uprobes.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index c71025b..4e33a35 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -680,8 +680,11 @@ static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > if (auprobe->insn[i] == 0x66) > continue; > > - if (auprobe->insn[i] == 0x90) > + if (auprobe->insn[i] == 0x90) { > + regs->ip = uprobe_get_swbp_addr(regs); > + regs->ip += i + 1; > return true; > + } This change/routine is not obvious without the patch description, I would appreciate some comment in the code. :) I believe it could help some random developer in future. / I leave it to your judgement though / Anton. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn 2012-12-30 15:47 ` [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn Oleg Nesterov 2013-01-02 13:16 ` Anton Arapov @ 2013-01-03 12:01 ` Srikar Dronamraju 2013-01-08 11:24 ` Srikar Dronamraju 2 siblings, 0 replies; 8+ messages in thread From: Srikar Dronamraju @ 2013-01-03 12:01 UTC (permalink / raw) To: Oleg Nesterov Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli, Anton Arapov, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel * Oleg Nesterov <oleg@redhat.com> [2012-12-30 16:47:19]: > __skip_sstep() doesn't update regs->ip. Currently this is correct > but only "by accident" and it doesn't skip the whole insn. Change > it to advance ->ip by the length of the detected 0x66*0x90 sequence. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com> > --- > arch/x86/kernel/uprobes.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index c71025b..4e33a35 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -680,8 +680,11 @@ static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > if (auprobe->insn[i] == 0x66) > continue; > > - if (auprobe->insn[i] == 0x90) > + if (auprobe->insn[i] == 0x90) { > + regs->ip = uprobe_get_swbp_addr(regs); > + regs->ip += i + 1; > return true; > + } > > break; > } > -- > 1.5.5.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn 2012-12-30 15:47 ` [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn Oleg Nesterov 2013-01-02 13:16 ` Anton Arapov 2013-01-03 12:01 ` Srikar Dronamraju @ 2013-01-08 11:24 ` Srikar Dronamraju 2 siblings, 0 replies; 8+ messages in thread From: Srikar Dronamraju @ 2013-01-08 11:24 UTC (permalink / raw) To: Oleg Nesterov Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli, Anton Arapov, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel * Oleg Nesterov <oleg@redhat.com> [2012-12-30 16:47:19]: > __skip_sstep() doesn't update regs->ip. Currently this is correct > but only "by accident" and it doesn't skip the whole insn. Change > it to advance ->ip by the length of the detected 0x66*0x90 sequence. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com> > --- > arch/x86/kernel/uprobes.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index c71025b..4e33a35 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -680,8 +680,11 @@ static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > if (auprobe->insn[i] == 0x66) > continue; > > - if (auprobe->insn[i] == 0x90) > + if (auprobe->insn[i] == 0x90) { > + regs->ip = uprobe_get_swbp_addr(regs); > + regs->ip += i + 1; > return true; > + } > > break; > } > -- > 1.5.5.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() 2012-12-30 15:46 [PATCH 0/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov 2012-12-30 15:47 ` [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn Oleg Nesterov @ 2012-12-30 15:47 ` Oleg Nesterov 2012-12-31 8:35 ` Ananth N Mavinakayanahalli 2013-01-08 11:39 ` Srikar Dronamraju 1 sibling, 2 replies; 8+ messages in thread From: Oleg Nesterov @ 2012-12-30 15:47 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju Cc: Ananth N Mavinakayanahalli, Anton Arapov, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel Change handle_swbp() to set regs->ip = bp_vaddr in advance, this is what consumer->handler() needs but uprobe_get_swbp_addr() is not exported. This also simplifies the code and makes it more consistent across the supported architectures. handle_swbp() becomes the only caller of uprobe_get_swbp_addr(). Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- arch/x86/kernel/uprobes.c | 1 - kernel/events/uprobes.c | 15 +++++++-------- kernel/trace/trace_uprobe.c | 4 ++-- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c index 4e33a35..0ba4cfb 100644 --- a/arch/x86/kernel/uprobes.c +++ b/arch/x86/kernel/uprobes.c @@ -681,7 +681,6 @@ static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) continue; if (auprobe->insn[i] == 0x90) { - regs->ip = uprobe_get_swbp_addr(regs); regs->ip += i + 1; return true; } diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 59b6e97..f883813 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1504,6 +1504,10 @@ static void handle_swbp(struct pt_regs *regs) } return; } + + /* change it in advance for ->handler() and restart */ + instruction_pointer_set(regs, bp_vaddr); + /* * TODO: move copy_insn/etc into _register and remove this hack. * After we hit the bp, _unregister + _register can install the @@ -1511,14 +1515,14 @@ static void handle_swbp(struct pt_regs *regs) */ smp_rmb(); /* pairs with wmb() in install_breakpoint() */ if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags))) - goto restart; + goto out; utask = current->utask; if (!utask) { utask = add_utask(); /* Cannot allocate; re-execute the instruction. */ if (!utask) - goto restart; + goto out; } handler_chain(uprobe, regs); @@ -1531,12 +1535,7 @@ static void handle_swbp(struct pt_regs *regs) return; } -restart: - /* - * cannot singlestep; cannot skip instruction; - * re-execute the instruction. - */ - instruction_pointer_set(regs, bp_vaddr); + /* can_skip_sstep() succeeded, or restart if can't singlestep */ out: put_uprobe(uprobe); } diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index 38eee92..52caead 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -488,7 +488,7 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs) return; entry = ring_buffer_event_data(event); - entry->ip = uprobe_get_swbp_addr(task_pt_regs(current)); + entry->ip = instruction_pointer(task_pt_regs(current)); data = (u8 *)&entry[1]; for (i = 0; i < tu->nr_args; i++) call_fetch(&tu->args[i].fetch, regs, data + tu->args[i].offset); @@ -663,7 +663,7 @@ static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs) if (!entry) goto out; - entry->ip = uprobe_get_swbp_addr(task_pt_regs(current)); + entry->ip = instruction_pointer(task_pt_regs(current)); data = (u8 *)&entry[1]; for (i = 0; i < tu->nr_args; i++) call_fetch(&tu->args[i].fetch, regs, data + tu->args[i].offset); -- 1.5.5.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() 2012-12-30 15:47 ` [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov @ 2012-12-31 8:35 ` Ananth N Mavinakayanahalli 2013-01-08 11:39 ` Srikar Dronamraju 1 sibling, 0 replies; 8+ messages in thread From: Ananth N Mavinakayanahalli @ 2012-12-31 8:35 UTC (permalink / raw) To: Oleg Nesterov Cc: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju, Anton Arapov, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel On Sun, Dec 30, 2012 at 04:47:22PM +0100, Oleg Nesterov wrote: > Change handle_swbp() to set regs->ip = bp_vaddr in advance, this is > what consumer->handler() needs but uprobe_get_swbp_addr() is not > exported. > > This also simplifies the code and makes it more consistent across > the supported architectures. handle_swbp() becomes the only caller > of uprobe_get_swbp_addr(). The arch agnostic bits look fine to me. > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() 2012-12-30 15:47 ` [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov 2012-12-31 8:35 ` Ananth N Mavinakayanahalli @ 2013-01-08 11:39 ` Srikar Dronamraju 1 sibling, 0 replies; 8+ messages in thread From: Srikar Dronamraju @ 2013-01-08 11:39 UTC (permalink / raw) To: Oleg Nesterov Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli, Anton Arapov, Frank Eigler, Josh Stone, Suzuki K. Poulose, linux-kernel * Oleg Nesterov <oleg@redhat.com> [2012-12-30 16:47:22]: > Change handle_swbp() to set regs->ip = bp_vaddr in advance, this is > what consumer->handler() needs but uprobe_get_swbp_addr() is not > exported. > > This also simplifies the code and makes it more consistent across > the supported architectures. handle_swbp() becomes the only caller > of uprobe_get_swbp_addr(). > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com> > --- > arch/x86/kernel/uprobes.c | 1 - > kernel/events/uprobes.c | 15 +++++++-------- > kernel/trace/trace_uprobe.c | 4 ++-- > 3 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index 4e33a35..0ba4cfb 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -681,7 +681,6 @@ static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > continue; > > if (auprobe->insn[i] == 0x90) { > - regs->ip = uprobe_get_swbp_addr(regs); > regs->ip += i + 1; > return true; > } > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 59b6e97..f883813 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1504,6 +1504,10 @@ static void handle_swbp(struct pt_regs *regs) > } > return; > } > + > + /* change it in advance for ->handler() and restart */ > + instruction_pointer_set(regs, bp_vaddr); > + > /* > * TODO: move copy_insn/etc into _register and remove this hack. > * After we hit the bp, _unregister + _register can install the > @@ -1511,14 +1515,14 @@ static void handle_swbp(struct pt_regs *regs) > */ > smp_rmb(); /* pairs with wmb() in install_breakpoint() */ > if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags))) > - goto restart; > + goto out; > > utask = current->utask; > if (!utask) { > utask = add_utask(); > /* Cannot allocate; re-execute the instruction. */ > if (!utask) > - goto restart; > + goto out; > } > > handler_chain(uprobe, regs); > @@ -1531,12 +1535,7 @@ static void handle_swbp(struct pt_regs *regs) > return; > } > > -restart: > - /* > - * cannot singlestep; cannot skip instruction; > - * re-execute the instruction. > - */ > - instruction_pointer_set(regs, bp_vaddr); > + /* can_skip_sstep() succeeded, or restart if can't singlestep */ > out: > put_uprobe(uprobe); > } > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index 38eee92..52caead 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -488,7 +488,7 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs) > return; > > entry = ring_buffer_event_data(event); > - entry->ip = uprobe_get_swbp_addr(task_pt_regs(current)); > + entry->ip = instruction_pointer(task_pt_regs(current)); > data = (u8 *)&entry[1]; > for (i = 0; i < tu->nr_args; i++) > call_fetch(&tu->args[i].fetch, regs, data + tu->args[i].offset); > @@ -663,7 +663,7 @@ static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs) > if (!entry) > goto out; > > - entry->ip = uprobe_get_swbp_addr(task_pt_regs(current)); > + entry->ip = instruction_pointer(task_pt_regs(current)); > data = (u8 *)&entry[1]; > for (i = 0; i < tu->nr_args; i++) > call_fetch(&tu->args[i].fetch, regs, data + tu->args[i].offset); > -- > 1.5.5.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-01-08 11:39 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-30 15:46 [PATCH 0/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov 2012-12-30 15:47 ` [PATCH 1/2] uprobes/x86: Change __skip_sstep() to actually skip the whole insn Oleg Nesterov 2013-01-02 13:16 ` Anton Arapov 2013-01-03 12:01 ` Srikar Dronamraju 2013-01-08 11:24 ` Srikar Dronamraju 2012-12-30 15:47 ` [PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain() Oleg Nesterov 2012-12-31 8:35 ` Ananth N Mavinakayanahalli 2013-01-08 11:39 ` Srikar Dronamraju
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).