| Message ID | 1381871068-27660-3-git-send-email-dave.long@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/15, David Long wrote: > > @@ -1732,9 +1732,6 @@ static void handle_swbp(struct pt_regs *regs) > return; > } > > - /* change it in advance for ->handler() and restart */ > - instruction_pointer_set(regs, bp_vaddr); > - Well, this looks obviously wrong. This SET_IP() has the comment ;) Note also that with this breaks __skip_sstep() on x86. Oleg.
On 10/19/13 13:02, Oleg Nesterov wrote: > On 10/15, David Long wrote: >> >> @@ -1732,9 +1732,6 @@ static void handle_swbp(struct pt_regs *regs) >> return; >> } >> >> - /* change it in advance for ->handler() and restart */ >> - instruction_pointer_set(regs, bp_vaddr); >> - > > Well, this looks obviously wrong. This SET_IP() has the comment ;) > > Note also that with this breaks __skip_sstep() on x86. > > Oleg. > Yes, and there's a missing weak stub function in there too. It was a surprise to me that declaring an external as weak means that it quietly ignores the fact there is no definition for it at link time, and makes it zero. I think there may be some similar land mines elsewhere in the kernel, unrelated to these changes or uprobes in general. I have an updated version to go out with the v3 patches. It is working with v3.12-rc6 on x86 and ARM, to the extent I'm able to test it. -dl
Sorry for top-posting/formatting, Do you mean arch_uprobe_skip_sstep() ? Yes, this __weak is wrong, already fixed in my tree. See http://marc.info/?l=linux-mips&m=138132052022388&w=2 ----- Original Message ----- From: "David Long" <dave.long@linaro.org> To: "Oleg Nesterov" <oleg@redhat.com> Cc: linux-arm-kernel@lists.infradead.org, "Rabin Vincent" <rabin@rab.in>, "Jon Medhurst (Tixy)" <tixy@linaro.org>, "Srikar Dronamraju" <srikar@linux.vnet.ibm.com>, "Ingo Molnar" <mingo@redhat.com>, linux-kernel@vger.kernel.org Sent: Tuesday, 22 October, 2013 5:45:47 AM Subject: Re: [PATCH v2 02/13] uprobes: allow ignoring of probe hits On 10/19/13 13:02, Oleg Nesterov wrote: > On 10/15, David Long wrote: >> >> @@ -1732,9 +1732,6 @@ static void handle_swbp(struct pt_regs *regs) >> return; >> } >> >> - /* change it in advance for ->handler() and restart */ >> - instruction_pointer_set(regs, bp_vaddr); >> - > > Well, this looks obviously wrong. This SET_IP() has the comment ;) > > Note also that with this breaks __skip_sstep() on x86. > > Oleg. > Yes, and there's a missing weak stub function in there too. It was a surprise to me that declaring an external as weak means that it quietly ignores the fact there is no definition for it at link time, and makes it zero. I think there may be some similar land mines elsewhere in the kernel, unrelated to these changes or uprobes in general. I have an updated version to go out with the v3 patches. It is working with v3.12-rc6 on x86 and ARM, to the extent I'm able to test it. -dl
On 10/22/13 07:25, Oleg Nesterov wrote: > Sorry for top-posting/formatting, > > Do you mean arch_uprobe_skip_sstep() ? > > Yes, this __weak is wrong, already fixed in my tree. See > http://marc.info/?l=linux-mips&m=138132052022388&w=2 > That was one I was looking at, but it seemed to me a general opportunity for runtime failures in the kernel that could have been detected at link time. But that's a topic for discussion elsewhere I should think. -dl
David, sorry for delay. On 10/19, Oleg Nesterov wrote: > > On 10/15, David Long wrote: > > > > @@ -1732,9 +1732,6 @@ static void handle_swbp(struct pt_regs *regs) > > return; > > } > > > > - /* change it in advance for ->handler() and restart */ > > - instruction_pointer_set(regs, bp_vaddr); > > - > > Well, this looks obviously wrong. This SET_IP() has the comment ;) > > Note also that with this breaks __skip_sstep() on x86. Hmm. Thinking more, it seems that this patch has another problem. IIUC, the whole point of arch_uprobe_ignore() is to avoid handler_chain() if the condition was not satisfied, so you need to call it before handler_chain() ? Otherwise this logic should go into can_skip_sstep() and we simply do not need the new hook, just we need to tweak the (ugly) UPROBE_SKIP_SSTEP logic. Oleg.
Sorry for the delay, all this week it's me that's traveling. On 10/28/13 14:54, Oleg Nesterov wrote: > David, sorry for delay. > > On 10/19, Oleg Nesterov wrote: >> >> On 10/15, David Long wrote: >>> >>> @@ -1732,9 +1732,6 @@ static void handle_swbp(struct pt_regs *regs) >>> return; >>> } >>> >>> - /* change it in advance for ->handler() and restart */ >>> - instruction_pointer_set(regs, bp_vaddr); >>> - >> >> Well, this looks obviously wrong. This SET_IP() has the comment ;) >> >> Note also that with this breaks __skip_sstep() on x86. > > Hmm. Thinking more, it seems that this patch has another problem. > > IIUC, the whole point of arch_uprobe_ignore() is to avoid > handler_chain() if the condition was not satisfied, so you need > to call it before handler_chain() ? Yes, you're right. I can see now that is a bad merge of an earlier version of the patch. I have just tested the fix. Thanks for finding this. > > Otherwise this logic should go into can_skip_sstep() and we simply > do not need the new hook, just we need to tweak the (ugly) > UPROBE_SKIP_SSTEP logic. > > Oleg. > -dl
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index 9fd60c5..80116c9 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -133,6 +133,7 @@ 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 unsigned long arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, 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 ad8e1bd..3955172 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1732,9 +1732,6 @@ 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 @@ -1742,16 +1739,26 @@ 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 out; + goto restart; handler_chain(uprobe, regs); + + if (arch_uprobe_ignore(&uprobe->arch, regs)) + goto out; + if (can_skip_sstep(uprobe, regs)) goto out; if (!pre_ssout(uprobe, regs, bp_vaddr)) return; - /* can_skip_sstep() succeeded, or restart if can't singlestep */ +restart: + /* + * cannot singlestep; cannot skip instruction; + * re-execute the instruction. + */ + instruction_pointer_set(regs, bp_vaddr); + out: put_uprobe(uprobe); }