| Message ID | 1350242593-17761-3-git-send-email-rabin@rab.in (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/14, Rabin Vincent wrote: > > Allow arches to decided to ignore a probe hit. ARM will use this to > only call handlers if the conditions to execute a conditionally executed > instruction are satisfied. Not sure I understand why we shouldn't call handlers in this case, but OK, I know nothing about arm. > static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) > { > struct mm_struct *mm = current->mm; > @@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs) > struct uprobe *uprobe; > unsigned long bp_vaddr; > int uninitialized_var(is_swbp); > + bool ignored = false; > > bp_vaddr = uprobe_get_swbp_addr(regs); > uprobe = find_active_uprobe(bp_vaddr, &is_swbp); > @@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs) > goto cleanup_ret; > } > utask->active_uprobe = uprobe; > + > + if (arch_uprobe_ignore(&uprobe->arch, regs)) { > + ignored = true; > + goto cleanup_ret; > + } > + > handler_chain(uprobe, regs); > if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs)) > goto cleanup_ret; > @@ -1514,7 +1526,7 @@ cleanup_ret: > utask->active_uprobe = NULL; > utask->state = UTASK_RUNNING; > } > - if (!(uprobe->flags & UPROBE_SKIP_SSTEP)) > + if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP)) > > /* > * cannot singlestep; cannot skip instruction; This conflicts with other changes in git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core Could you re-diff? I'll try to read 1-7 tomorrow. Oleg.
2012/10/15 Oleg Nesterov <oleg@redhat.com>: > On 10/14, Rabin Vincent wrote: >> Allow arches to decided to ignore a probe hit. ARM will use this to >> only call handlers if the conditions to execute a conditionally executed >> instruction are satisfied. > > Not sure I understand why we shouldn't call handlers in this case, > but OK, I know nothing about arm. This old discussion about kprobes should be useful: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/046544.html > This conflicts with other changes in > > git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core > > Could you re-diff? OK, I will rebase them.
> static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) > { > struct mm_struct *mm = current->mm; > @@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs) > struct uprobe *uprobe; > unsigned long bp_vaddr; > int uninitialized_var(is_swbp); > + bool ignored = false; > > bp_vaddr = uprobe_get_swbp_addr(regs); > uprobe = find_active_uprobe(bp_vaddr, &is_swbp); > @@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs) > goto cleanup_ret; > } > utask->active_uprobe = uprobe; > + > + if (arch_uprobe_ignore(&uprobe->arch, regs)) { > + ignored = true; > + goto cleanup_ret; > + } > + With Oleg's changes, this should become simple. Something like: if (arch_uprobe_ignore(&uprobe->arch, regs)) goto out; > handler_chain(uprobe, regs); > if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs)) > goto cleanup_ret; > @@ -1514,7 +1526,7 @@ cleanup_ret: > utask->active_uprobe = NULL; > utask->state = UTASK_RUNNING; > } > - if (!(uprobe->flags & UPROBE_SKIP_SSTEP)) > + if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP)) > > /* > * cannot singlestep; cannot skip instruction; > -- > 1.7.9.5 >
On 10/16, Rabin Vincent wrote: > > 2012/10/15 Oleg Nesterov <oleg@redhat.com>: > > > > Not sure I understand why we shouldn't call handlers in this case, > > but OK, I know nothing about arm. > > This old discussion about kprobes should be useful: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html Thanks... Not sure I understand this discussion... And, to clarify, I am not arguing. Just curious. So, is this like cmov on x86? And this patch allows to not report if the condition is not true? Or there are other issues on arm? Oleg.
On Wed, Oct 17, 2012 at 07:35:10PM +0200, Oleg Nesterov wrote: > On 10/16, Rabin Vincent wrote: > > 2012/10/15 Oleg Nesterov <oleg@redhat.com>: > > > Not sure I understand why we shouldn't call handlers in this case, > > > but OK, I know nothing about arm. > > > > This old discussion about kprobes should be useful: > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html > > Thanks... Not sure I understand this discussion... > > And, to clarify, I am not arguing. Just curious. > > So, is this like cmov on x86? And this patch allows to not report if > the condition is not true? Or there are other issues on arm? Yes, I guess this is like CMOV on x86. In the ARM instruction set most instructions can be conditionally executed. In order to set the probe on a conditional instruction, we use an undefined instruction with the same condition as the instruction we replace. However, it is implementation defined whether an undefined instruction with a failing condition code will trigger an undefined instruction exception or just be executed as a NOP. So for those processor implementations where we do get the undefined instruction exception even for a failing condition code, we have to ignore it in order to provide consistent behaviour.
On 10/21, Rabin Vincent wrote: > > On Wed, Oct 17, 2012 at 07:35:10PM +0200, Oleg Nesterov wrote: > > > > And, to clarify, I am not arguing. Just curious. > > > > So, is this like cmov on x86? And this patch allows to not report if > > the condition is not true? Or there are other issues on arm? > > Yes, I guess this is like CMOV on x86. In the ARM instruction set most > instructions can be conditionally executed. > > In order to set the probe on a conditional instruction, we use an > undefined instruction with the same condition as the instruction we > replace. However, it is implementation defined whether an undefined > instruction with a failing condition code will trigger an undefined > instruction exception or just be executed as a NOP. So for those > processor implementations where we do get the undefined instruction > exception even for a failing condition code, we have to ignore it in > order to provide consistent behaviour. OK, I see, thanks for your explanation. Oleg.
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index ac90704..da21b66 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -128,6 +128,7 @@ extern int arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs); extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk); extern int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data); extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs); +extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs); #else /* !CONFIG_UPROBES */ struct uprobes_state { }; diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index db4e3ab..a0e1a38 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1419,6 +1419,11 @@ static void mmf_recalc_uprobes(struct mm_struct *mm) clear_bit(MMF_HAS_UPROBES, &mm->flags); } +bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs) +{ + return false; +} + static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) { struct mm_struct *mm = current->mm; @@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs) struct uprobe *uprobe; unsigned long bp_vaddr; int uninitialized_var(is_swbp); + bool ignored = false; bp_vaddr = uprobe_get_swbp_addr(regs); uprobe = find_active_uprobe(bp_vaddr, &is_swbp); @@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs) goto cleanup_ret; } utask->active_uprobe = uprobe; + + if (arch_uprobe_ignore(&uprobe->arch, regs)) { + ignored = true; + goto cleanup_ret; + } + handler_chain(uprobe, regs); if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs)) goto cleanup_ret; @@ -1514,7 +1526,7 @@ cleanup_ret: utask->active_uprobe = NULL; utask->state = UTASK_RUNNING; } - if (!(uprobe->flags & UPROBE_SKIP_SSTEP)) + if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP)) /* * cannot singlestep; cannot skip instruction;
Allow arches to decided to ignore a probe hit. ARM will use this to only call handlers if the conditions to execute a conditionally executed instruction are satisfied. Signed-off-by: Rabin Vincent <rabin@rab.in> --- include/linux/uprobes.h | 1 + kernel/events/uprobes.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)