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

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

[Ananth N Mavinakayanahalli]

From: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

GDB uses a variant of the trap instruction that is different from the
one used by uprobes. Currently, running gdb on a program being traced
by uprobes causes an endless loop since uprobes doesn't understand
that the trap is inserted by some other entity and hence a SIGTRAP needs
to be delivered.

Teach uprobes to ignore breakpoints that doesn't belong to it.

Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: stable@vger.kernel.org
---
 arch/powerpc/kernel/uprobes.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
===================================================================
--- linux-3.9-rc3.orig/arch/powerpc/kernel/uprobes.c
+++ linux-3.9-rc3/arch/powerpc/kernel/uprobes.c
@@ -80,6 +80,16 @@ unsigned long uprobe_get_swbp_addr(struc
 	return instruction_pointer(regs);
 }
 
+/**
+ * is_swbp_insn - check if the instruction is a breakpoint instruction.
+ * @insn: instruction to be checked.
+ * Returns true if @insn is a breakpoint instruction.
+ */
+bool is_swbp_insn(uprobe_opcode_t *insn)
+{
+	return (is_trap(*insn));
+}
+
 /*
  * If xol insn itself traps and generates a signal (SIGILL/SIGSEGV/etc),
  * then detect the case where a singlestepped instruction jumps back to its

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

[Oleg Nesterov]

Hi Ananth,

First of all, let me remind that I know nothing about powerpc ;)

But iirc we already discussed this a bit, I forgot the details but
still I have some concerns...

On 03/20, Ananth N Mavinakayanahalli wrote:
>
> GDB uses a variant of the trap instruction that is different from the
> one used by uprobes. Currently, running gdb on a program being traced
> by uprobes causes an endless loop since uprobes doesn't understand
> that the trap is inserted by some other entity and hence a SIGTRAP needs
> to be delivered.

Yes, and thus is_swbp_at_addr()->is_swbp_insn() called by handle_swbp()
should be updated,

> +bool is_swbp_insn(uprobe_opcode_t *insn)
> +{
> +	return (is_trap(*insn));
> +}

And this patch should fix the problem. (and probably this is fine
for prepare_uprobe()).


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().

And I am just curious, could you explain how X and UPROBE_SWBP_INSN
differ?

Oleg.

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

[Oleg Nesterov]

On 03/20, Oleg Nesterov wrote:
>
> 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().
>
> And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> differ?

IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
Suppose we apply the patch below. Will uprobes on powerpc work?

If yes, then your patch should be fine. If not, we probably need more
changes.

And, forgot to mention. If you change is_swbp_insn(), you can remove
is_trap() from arch_uprobe_analyze_insn().

Oleg.

--- x/arch/powerpc/include/asm/uprobes.h
+++ x/arch/powerpc/include/asm/uprobes.h
@@ -31,7 +31,7 @@ typedef ppc_opcode_t uprobe_opcode_t;
 #define UPROBE_XOL_SLOT_BYTES	(MAX_UINSN_BYTES)
 
 /* The following alias is needed for reference from arch-agnostic code */
-#define UPROBE_SWBP_INSN	BREAKPOINT_INSTRUCTION
+#define UPROBE_SWBP_INSN	TRAP_INSN_USED_BY_GDB
 #define UPROBE_SWBP_INSN_SIZE	4 /* swbp insn size in bytes */
 
 struct arch_uprobe {

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

[Ananth N Mavinakayanahalli]

On Wed, Mar 20, 2013 at 01:26:39PM +0100, Oleg Nesterov wrote:
> Hi Ananth,
> 
> First of all, let me remind that I know nothing about powerpc ;)
> 
> But iirc we already discussed this a bit, I forgot the details but
> still I have some concerns...
> 
> On 03/20, Ananth N Mavinakayanahalli wrote:
> >
> > GDB uses a variant of the trap instruction that is different from the
> > one used by uprobes. Currently, running gdb on a program being traced
> > by uprobes causes an endless loop since uprobes doesn't understand
> > that the trap is inserted by some other entity and hence a SIGTRAP needs
> > to be delivered.
> 
> Yes, and thus is_swbp_at_addr()->is_swbp_insn() called by handle_swbp()
> should be updated,
>
> > +bool is_swbp_insn(uprobe_opcode_t *insn)
> > +{
> > +	return (is_trap(*insn));
> > +}
> 
> And this patch should fix the problem. (and probably this is fine
> for prepare_uprobe()).

Correct

> 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.
So, install_breakpoint()->prepare_uprobe()->is_swbp_insn() will return
ENOTSUPP. In fact, arch_uprobe_analyze_insn() will also do the same.

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.

Ananth

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

[Ananth N Mavinakayanahalli]

On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> On 03/20, Oleg Nesterov wrote:
> >
> > 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().
> >
> > And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> > differ?
> 
> IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> Suppose we apply the patch below. Will uprobes on powerpc work?
> 
> If yes, then your patch should be fine. If not, we probably need more
> changes.

Yes, it will work fine.

> And, forgot to mention. If you change is_swbp_insn(), you can remove
> is_trap() from arch_uprobe_analyze_insn().

Yeah, that check is no longer needed. I'll send a separate cleanup after
this patch gets applied.

Ananth

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

[Oleg Nesterov]

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)

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

[Oleg Nesterov]

On 03/20, Ananth N Mavinakayanahalli wrote:
>
> On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > On 03/20, Oleg Nesterov wrote:
> > >
> > > 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().
> > >
> > > And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> > > differ?
> >
> > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > Suppose we apply the patch below. Will uprobes on powerpc work?
> >
> > If yes, then your patch should be fine. If not, we probably need more
> > changes.
>
> Yes, it will work fine.

Even if this new insn is conditional?

Oleg.

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

[Ananth N Mavinakayanahalli]

On Wed, Mar 20, 2013 at 05:06:44PM +0100, Oleg Nesterov wrote:
> 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.

I think that is not right... see below...

> > > 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().

Its fine from gdb's perspective with my patch.

> > 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.

For the case where X already exists, verify_opcode() currently returns 0.
IMO, it should return -EEXIST, unless you are proposing that uprobes
should ride on the existing trap (even if its a variant).

If you are proposing that uprobes ride on X if it already exists, that's
not always possible and is a big can of worms... see below...

> > 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?

Yes it is possible if the condition is not met. If the condition is
met, the instruction will generate a trap, and uprobes will do a
send_sig(SIGTRAP) from handle_swbp().

> 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.

At this time, the condition for the trap is not satisfied, so no
exception occurs. If the expectation is that the trap always trigger,
then all such trap variants need to be replaced with the unconditional
trap and we should either add logic to re-execute the condional trap
after uprobe handling and send_sig() via handle_swbp() or emulate the
condition in software and do a send_sig() if needed.

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

I don't see how this will help, especially since the gdb<->uprobes is
fraught with races.

With your proposed patch, we refuse to insert a uprobe if the underlying
instruction is a UPROBE_SWBP_INSTRUCTION; changing is_swbp_at_addr()
will need changes in handle_swbp() too. But, unlike x86, we cannot
expect a uprobe with an underlying trap variant (X) to always trigger.

IMHO, its not a good idea to do that for x86 either, since you'll run
into many other complications (what if the entity that put the original
breakpoint, removed it, etc).

IMHO, I really think we should not allow uprobe_register() to succeed if
the underlying instruction is a breakpoint (or a variant thereof).

Ananth

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

[Ananth N Mavinakayanahalli]

On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote:
> On 03/20, Ananth N Mavinakayanahalli wrote:
> >
> > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > > On 03/20, Oleg Nesterov wrote:
> > > >
> > > > 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().
> > > >
> > > > And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> > > > differ?
> > >
> > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > > Suppose we apply the patch below. Will uprobes on powerpc work?
> > >
> > > If yes, then your patch should be fine. If not, we probably need more
> > > changes.
> >
> > Yes, it will work fine.
> 
> Even if this new insn is conditional?

Yes.

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

[Oleg Nesterov]

On 03/21, Ananth N Mavinakayanahalli wrote:
>
> On Wed, Mar 20, 2013 at 05:06:44PM +0100, Oleg Nesterov wrote:
>
> > > > 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().
>
> Its fine from gdb's perspective with my patch.

Yes, but this doesn't look right from uprobe's perspective.

> > > 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.
>
> For the case where X already exists, verify_opcode() currently returns 0.
> IMO, it should return -EEXIST,

Oh, this is debatable. Currently we assume that uprobe should "win".
See below...

> unless you are proposing that uprobes
> should ride on the existing trap (even if its a variant).

Yes. And this is what the current implementation does.

> If you are proposing that uprobes ride on X if it already exists, that's
> not always possible and is a big can of worms... see below...

Sure. Whatever we do uprobe and gdb can confuse each other. Unless we
rework the vm code completely (not sure this is realistic).

> > OK. So, if I understand correctly, gdb can use some conditional
> > breakpoint, and it is possible that this insn won't generate the
> > trap?
>
> Yes it is possible if the condition is not met. If the condition is
> met, the instruction will generate a trap, and uprobes will do a
> send_sig(SIGTRAP) from handle_swbp().

Unless there is uprobe at the same address. Once again, uprobe wins.

Your patch only fixes the case when the task hits a non-UPROBE_SWBP_INSN
breakpoint and there is no uprobe at the same address.

> > 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.
>
> At this time, the condition for the trap is not satisfied, so no
> exception occurs.

Yes, and thus uprobe->handler() is not called, this was my point.

> If the expectation is that the trap always trigger,
> then all such trap variants need to be replaced with the unconditional
> trap

Yes. that is why I suggested the patch which doesn't affect verify_opcode().
uprobe_register() should replace the conditional trap with the unconditional
UPROBE_SWBP_INSN. uprobes should win.

> and we should either add logic to re-execute the condional trap
> after uprobe handling and send_sig() via handle_swbp() or emulate the
> condition in software and do a send_sig() if needed.

Unlikely this is possible. Or even desirable.

> > So. I still think that we actually need something like below, and
> > powerpc should reimplement is_trap_insn() to use is_trap(insn).
> >
> > No?
>
> I don't see how this will help,

Hmm. This should equally fix this particular problem? handle_swbp()
will send the trap if is_trap() == T?

Again, unless there is uprobe, but this was always true.

> especially since the gdb<->uprobes is
> fraught with races.

They can race anyway, whatever we do.

Unless we rework write_opcode/etc completely.

> With your proposed patch, we refuse to insert a uprobe if the underlying
> instruction is a UPROBE_SWBP_INSTRUCTION;

If "underlying" means the original insn in vma->file, this is already
true. My patch doesn't change this logic.

Otherwise - no, we do not refuse to insert a uprobe if this insn was
already changed by gdb.

> changing is_swbp_at_addr()
> will need changes in handle_swbp() too.

I don't think so. Why?

> But, unlike x86, we cannot
> expect a uprobe with an underlying trap variant (X) to always trigger.

And that is why I think write_opcode() should rewrite the conditional
trap.

> IMHO, its not a good idea to do that for x86 either,

This change has no effect fo x86.

> IMHO, I really think we should not allow uprobe_register() to succeed if
> the underlying instruction is a breakpoint (or a variant thereof).

I disagree.

Just for example. Suppose we change install_breakpoint() so that it fails
if the underlying instruction is int3. (once again, "underlying" does not
mean the original insn from vma->vm_file).

First of all, this is very much nontrivial. I simply do not see how we
can do this. If nothing else, uprobe_register() can race with uprobe_mmap()
and install_breakpoint() can be called twice with the same vaddr. With
this change register or mmap can fail.

But suppose you can do this. Now you can write the trivial application
which mmaps glibc and inserts int3 into, say, getpid()'s vaddr. Voila,
this makes "perf probe -x /lib/libc.so.6" impossible, uprobe_register()
will fail.

Whatever you think about this logic, it was desidned to assume that
install_breakpoint() should be "idempotent", and we ignore the races
with gdb. We should only ensure that the kernel can't crash/etc.

And uprobe can "steal" the trap from gdb if they race, again this is by
design. and your patch can't prevent this but complicates the rules.

I already said this many times, but let me repeat. is_swbp_isn() and
its usage is confusing. Lets forget about prepare_uprobe(). Now,

	- verify_opcode()->is_swbp_insn() means:

		is this insn fine for uprobe? (we do not care about
		gdb, we simply ignore this problem)

	- is_swbp_at_addr()->is_swbp_insn() means:

		there is no uprobe, should we send SIGTRAP ?

And the patch I sent separates these 2 cases.


Finally. If we want to eliminate the gdb/uprobes races/confusions,
we can not simply use a PageAnon() page, we shuld rewrite this code
completely. I can quote the very old email I sent you:

	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.

IOW, somehow we should ensure that once uprobe changes the page, nobody
else can change it until uprobe_unregister().

Oleg.

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

[Oleg Nesterov]

On 03/21, Ananth N Mavinakayanahalli wrote:
?
> On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote:
> > On 03/20, Ananth N Mavinakayanahalli wrote:
> > >
> > > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > > > On 03/20, Oleg Nesterov wrote:
> > > > >
> > > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > > > Suppose we apply the patch below. Will uprobes on powerpc work?
> > > >
> > > > If yes, then your patch should be fine. If not, we probably need more
> > > > changes.
> > >
> > > Yes, it will work fine.
> >
> > Even if this new insn is conditional?
>
> Yes.

But this can't be true. If it is conditional, it won't always generate a
trap, this means uprobe won't actually work.

Oleg.

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

[Ananth N Mavinakayanahalli]

On Thu, Mar 21, 2013 at 05:00:02PM +0100, Oleg Nesterov wrote:
> On 03/21, Ananth N Mavinakayanahalli wrote:
> ?
> > On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote:
> > > On 03/20, Ananth N Mavinakayanahalli wrote:
> > > >
> > > > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > > > > On 03/20, Oleg Nesterov wrote:
> > > > > >
> > > > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > > > > Suppose we apply the patch below. Will uprobes on powerpc work?
> > > > >
> > > > > If yes, then your patch should be fine. If not, we probably need more
> > > > > changes.
> > > >
> > > > Yes, it will work fine.
> > >
> > > Even if this new insn is conditional?
> >
> > Yes.
> 
> But this can't be true. If it is conditional, it won't always generate a
> trap, this means uprobe won't actually work.

I meant to say, uprobes if we use a conditional trap instruction as long
as the condition is met.

Ananth

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

[Ananth N Mavinakayanahalli]

On Thu, Mar 21, 2013 at 04:58:09PM +0100, Oleg Nesterov wrote:
> On 03/21, Ananth N Mavinakayanahalli wrote:
> >
> > On Wed, Mar 20, 2013 at 05:06:44PM +0100, Oleg Nesterov wrote:
> >
> > > > > 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().
> >
> > Its fine from gdb's perspective with my patch.
> 
> Yes, but this doesn't look right from uprobe's perspective.
> 
> > > > 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.
> >
> > For the case where X already exists, verify_opcode() currently returns 0.
> > IMO, it should return -EEXIST,
> 
> Oh, this is debatable. Currently we assume that uprobe should "win".

And there is that one case where gdb also uses an unconditional trap...
but that's besides the point if we don't care about gdb.

> See below...
> 
> > unless you are proposing that uprobes
> > should ride on the existing trap (even if its a variant).
> 
> Yes. And this is what the current implementation does.
> 
> > If you are proposing that uprobes ride on X if it already exists, that's
> > not always possible and is a big can of worms... see below...
> 
> Sure. Whatever we do uprobe and gdb can confuse each other. Unless we
> rework the vm code completely (not sure this is realistic).

Agreed.

> > > OK. So, if I understand correctly, gdb can use some conditional
> > > breakpoint, and it is possible that this insn won't generate the
> > > trap?
> >
> > Yes it is possible if the condition is not met. If the condition is
> > met, the instruction will generate a trap, and uprobes will do a
> > send_sig(SIGTRAP) from handle_swbp().
> 
> Unless there is uprobe at the same address. Once again, uprobe wins.

In which case, we will need to replace the conditional trap with the
unconditional one when the uprobe register happens. That is doable.

> Your patch only fixes the case when the task hits a non-UPROBE_SWBP_INSN
> breakpoint and there is no uprobe at the same address.

Correct.

> > > 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.
> >
> > At this time, the condition for the trap is not satisfied, so no
> > exception occurs.
> 
> Yes, and thus uprobe->handler() is not called, this was my point.
> 
> > If the expectation is that the trap always trigger,
> > then all such trap variants need to be replaced with the unconditional
> > trap
> 
> Yes. that is why I suggested the patch which doesn't affect verify_opcode().
> uprobe_register() should replace the conditional trap with the unconditional
> UPROBE_SWBP_INSN. uprobes should win.

OK, I see your point.

...

> > But, unlike x86, we cannot
> > expect a uprobe with an underlying trap variant (X) to always trigger.
> 
> And that is why I think write_opcode() should rewrite the conditional
> trap.

OK

> > IMHO, its not a good idea to do that for x86 either,
> 
> This change has no effect fo x86.
> 
> > IMHO, I really think we should not allow uprobe_register() to succeed if
> > the underlying instruction is a breakpoint (or a variant thereof).
> 
> I disagree.
> 
> Just for example. Suppose we change install_breakpoint() so that it fails
> if the underlying instruction is int3. (once again, "underlying" does not
> mean the original insn from vma->vm_file).
> 
> First of all, this is very much nontrivial. I simply do not see how we
> can do this. If nothing else, uprobe_register() can race with uprobe_mmap()
> and install_breakpoint() can be called twice with the same vaddr. With
> this change register or mmap can fail.
> 
> But suppose you can do this. Now you can write the trivial application
> which mmaps glibc and inserts int3 into, say, getpid()'s vaddr. Voila,
> this makes "perf probe -x /lib/libc.so.6" impossible, uprobe_register()
> will fail.
> 
> Whatever you think about this logic, it was desidned to assume that
> install_breakpoint() should be "idempotent", and we ignore the races
> with gdb. We should only ensure that the kernel can't crash/etc.
> 
> And uprobe can "steal" the trap from gdb if they race, again this is by
> design. and your patch can't prevent this but complicates the rules.
> 
> I already said this many times, but let me repeat. is_swbp_isn() and
> its usage is confusing. Lets forget about prepare_uprobe(). Now,
> 
> 	- verify_opcode()->is_swbp_insn() means:
> 
> 		is this insn fine for uprobe? (we do not care about
> 		gdb, we simply ignore this problem)

I will write up a patch for this case.. So, IIUC we do not care to send
gdb a SIGTRAP if we have replaced a conditional trap from gdb with an
unconditional uprobes one, right?

> 	- is_swbp_at_addr()->is_swbp_insn() means:
> 
> 		there is no uprobe, should we send SIGTRAP ?

This part is handled with my patch now...

Thanks for being patient. I'll write up the patches and send across for
review.

Ananth.

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

[Oleg Nesterov]

On 03/22, Ananth N Mavinakayanahalli wrote:
>
> On Thu, Mar 21, 2013 at 04:58:09PM +0100, Oleg Nesterov wrote:
> >
> > 	- verify_opcode()->is_swbp_insn() means:
> >
> > 		is this insn fine for uprobe? (we do not care about
> > 		gdb, we simply ignore this problem)
>
> I will write up a patch for this case.. So, IIUC we do not care to send
> gdb a SIGTRAP if we have replaced a conditional trap from gdb with an
> unconditional uprobes one, right?

Yes.

And just in case, we do not send SIGTRAP if gdb used the same/unconditional
insn. We simply can't know if someone else wants to know that the task hits
this breakpoint.

Oleg.