Re: [PATCH] powerpc/uprobes: teach uprobes to ignore gdb breakpoints

[Oleg Nesterov <oleg@redhat.com>]

On 03/20, Ananth N Mavinakayanahalli wrote:
>
> On Wed, Mar 20, 2013 at 01:26:39PM +0100, Oleg Nesterov wrote:
> > But, at the same time, is the new definition fine for verify_opcode()?
> >
> > IOW, powerpc has another is_trap() insn(s) used by gdb, lets denote it X.
> > X != UPROBE_SWBP_INSN.
> >
> > Suppose that gdb installs the trap X at some addr, and then uprobe_register()
> > tries to install uprobe at the same address. Then set_swbp() will do nothing,
> > assuming the uprobe was already installed.
> >
> > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to
> > verify. If not, we need 2 definitions. is_uprobe_insn() should still check
> > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap().
>
> is_trap() checks for all trap variants on powerpc, including the one
> uprobe uses. It returns true if the instruction is *any* trap variant.

I understand,

> So, install_breakpoint()->prepare_uprobe()->is_swbp_insn() will return
> ENOTSUPP. In fact, arch_uprobe_analyze_insn() will also do the same.

Yes and the check in arch_uprobe_analyze_insn() should go away.

But you missed my point. Please forget about prepare_uprobe(), it is
wrong anyway. And, prepare_uprobe() inspects the _original_ insn from
the file, this has nothing install_breakpoint/etc.

I meant verify_opcode() called by install_breakpoint/etc.

> This itself should take care of all the cases.
>
> > And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> > differ?
>
> Powerpc has numerous variants of the trap instruction based on
> comparison of two registers or a regsiter and immediate value and a condition
> (less than, greater than, [signed forms thereof], or equal to).
>
> Uprobes uses 0x7fe0008 which is 'tw 31,0,0'  which essentially is an
> unconditional trap.
>
> Gdb uses many traps, one of which is 0x7d821008 which is twge r2,r2,
> which is basically trap if r2 greater than or equal to r2.

OK. So, if I understand correctly, gdb can use some conditional
breakpoint, and it is possible that this insn won't generate the
trap?

Then this patch is not right, or at least we need another change
on top?

Once again. Suppose that gdb installs the TRAP_IF_R1_GT_R2.

After that uprobe_register() is called, but it won't change this
insn because verify_opcode() returns 0.

Then the probed task hits this breakoint with "r1 < r2" and we do
not report this event.


So. I still think that we actually need something like below, and
powerpc should reimplement is_trap_insn() to use is_trap(insn).

No?

Oleg.

--- x/kernel/events/uprobes.c
+++ x/kernel/events/uprobes.c
@@ -532,6 +532,12 @@ static int copy_insn(struct uprobe *upro
 	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
+bool __weak is_trap_insn(uprobe_opcode_t *insn)
+{
+	// powerpc: should use is_trap()
+	return is_swbp_insn(insn);
+}
+
 static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 				struct mm_struct *mm, unsigned long vaddr)
 {
@@ -550,7 +556,7 @@ static int prepare_uprobe(struct uprobe 
 		goto out;
 
 	ret = -ENOTSUPP;
-	if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+	if (is_trap_insn((uprobe_opcode_t *)uprobe->arch.insn))
 		goto out;
 
 	ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
@@ -1452,7 +1458,7 @@ static int is_swbp_at_addr(struct mm_str
 	copy_opcode(page, vaddr, &opcode);
 	put_page(page);
  out:
-	return is_swbp_insn(&opcode);
+	return is_trap_insn(&opcode);
 }
 
 static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)