[PATCH v2 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
oleg at redhat.com
Tue Jun 12 02:12:15 EST 2012
I think the patch is correct and I am sorry for nit-picking,
this is really minor.
On 06/08, Ananth N Mavinakayanahalli wrote:
> Changes in V2:
> Pass (unsigned long)addr
Well, IMHO, this is confusing.
First of all, why do we have this "addr" or even "vaddr"? It should
not exists. We pass it to copy_insn(), but for what?? copy_insn()
should simply use uprobe->offset, the virtual address for this
particular mapping does not matter at all. I am going to send
Note also that we should move this !UPROBE_COPY_INSN from
install_breakpoint() to somewhere near alloc_uprobe(). This code
is called only once, it looks a bit strange to use the "random" mm
(the first mm vma_prio_tree_foreach() finds) and its mapping to
verify the insn. In fact this is simply not correct and should be
fixed, note that on x86 arch_uprobe_analyze_insn() checks
IOW, Perhaps uprobe->offset makes more sense?
> --- linux-3.5-rc1.orig/kernel/events/uprobes.c
> +++ linux-3.5-rc1/kernel/events/uprobes.c
> @@ -697,7 +697,7 @@ install_breakpoint(struct uprobe *uprobe
> if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> return -EEXIST;
> - ret = arch_uprobe_analyze_insn(&uprobe->arch, mm);
> + ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, addr);
Just fyi, this conflicts with
"[PATCH 1/3] uprobes: install_breakpoint() should fail if is_swbp_insn() == T"
I sent, but the conflict is trivial.
More information about the Linuxppc-dev