On 08/21, Ananth N Mavinakayanahalli wrote: > > On Fri, Aug 17, 2012 at 05:00:31PM +0200, Oleg Nesterov wrote: > > > > We should also take > > > care of the in-memory copy, in case gdb had inserted a breakpoint at the > > > same location, right? > > > > gdb (or even the application itself) and uprobes can obviously confuse > > each other, in many ways, and we can do nothing at least currently. > > Just we should ensure that the kernel can't crash/hang/etc. > > Absolutely. The proper fix for this at least from a breakpoint insertion > perspective is to educate gdb (possibly ptrace itself) to fail on a > breakpoint insertion request on an already existing one.
Oh, I don't think this is possible. And there are other problems like this. Uprobe can confuse gdb too, in many ways. For example, uprobe_register() can wrongly _remove_ int3 installed by gdb. The proper fix, I think, is to rework the whole idea about uprobe bps, but this is really "in the long term". install_breakpoint() should only unmap the page and mark its pte as "owned by kernel, FOLL_WRITE should not work". Something like migration or PROT_NONE. The task itself should install bp during the page fault. And we need the "backing store" for the pages with uprobes. Yes, this all is very vague and I can be wrong. Anyway, this is relatively minor, we have more serious problems. > > > Updating is_swbp_insn() per-arch where needed will > > > take care of both the cases, 'cos it gets called before > > > arch_analyze_uprobe_insn() too. > > > > For example. set_swbp()->is_swbp_insn() == T means that (for example) > > uprobe_register() and uprobe_mmap() raced with each other and there is > > no need for set_swbp(). > > This is true for Intel like architectures that have *one* swbp > instruction. On Powerpc, gdb for instance, can insert a trap variant at > the address. Therefore, is_swbp_insn() by definition should return true > for all trap variants. Not in this case, I think. OK, I was going to do this later, but this discussion makes me think I should try to send the patch sooner. set_swbp()->is_swbp_at_addr() is simply unneeded and in fact should be considered as unnecessary pessimization. set_orig_insn()->is_swbp_at_addr() makes more sense, but it can't fix all races with userpace. Still it should die. > OK. I will separate out the is_swbp_insn() change into a separate patch. Great thanks. And if we remove is_swbp_insn() from set_swbp() and set_orig_insn() then the semantics of is_swbp_insn() will much more clear, and in this case I powerpc probably really needs to change it. Oleg. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
