Re: [PATCH v2 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
flat view
From: Oleg Nesterov <oleg@redhat.com>
Date: 2012-06-11 16:15:08
Also in:
lkml
Ananth, Srikar, I think the patch is correct and I am sorry for nit-picking, this is really minor. But, 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 the cleanup. 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 mm->context.ia32_compat. IOW, Perhaps uprobe->offset makes more sense?
quoted hunk ↗ jump to hunk
--- 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. Oleg.