[PATCH 0/4] uprobes: remove is_swbp_at_addr() from register/unregister

[PATCH 0/4] uprobes: remove is_swbp_at_addr() from register/unregister

[Oleg Nesterov]

Hello.

1. the usage of is_swbp_at_addr() is confusing. This series tries
   to answer the questions we got during the discussion about
   powerpc port which has multiple trap insns.

   Ananth, please review too ;) at least the changelog in 1/4.

   Note: the changelog doesn't mention is_swbp_insn(). If powerpc
   wants to change it, we need 2 helpers for verify_opcode() and
   for is_swbp_at_addr().

2. This is also preparation for the pre-filtering I am working on.

   In particular, this series tries to make clear the fact that
   register/unregister should be "idempotent".

Oleg.

 kernel/events/uprobes.c |  165 +++++++++++++++++------------------------------
 1 files changed, 59 insertions(+), 106 deletions(-)

[PATCH 1/4] uprobes: Kill set_swbp()->is_swbp_at_addr()

[Oleg Nesterov]

A separate patch for better documentation.

set_swbp()->is_swbp_at_addr() is not needed for correctness, it is
harmless to do the unnecessary __replace_page(old_page, new_page)
when these 2 pages are identical.

And it can not be counted as optimization. mmap/register races are
very unlikely, while in the likely case is_swbp_at_addr() adds the
extra get_user_pages() even if the caller is uprobe_mmap(current->mm)
and returns false.

Note also that the semantics/usage of is_swbp_at_addr() in uprobe.c
is confusing. set_swbp() uses it to detect the case when this insn
was already modified by uprobes, that is why it should always compare
the opcode with UPROBE_SWBP_INSN even if the hardware (like powerpc)
has other trap insns. It doesn't matter if this "int3" was in fact
installed by gdb or application itself, we are going to "steal" this
breakpoint anyway and execute the original insn from vm_file even if
it no longer matches the memory.

OTOH, handle_swbp()->find_active_uprobe() uses is_swbp_at_addr() to
figure out whether we need to send SIGTRAP or not if we can not find
uprobe, so in this case it should return true for all trap variants,
not only for UPROBE_SWBP_INSN.

This patch removes set_swbp()->is_swbp_at_addr(), the next patches
will remove it from set_orig_insn() which is similar to set_swbp()
in this respect. So the only caller will be handle_swbp() and we
can make its semantics clear.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 78364a2..04f3259 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -321,17 +321,6 @@ out:
  */
 int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	int result;
-	/*
-	 * See the comment near uprobes_hash().
-	 */
-	result = is_swbp_at_addr(mm, vaddr);
-	if (result == 1)
-		return 0;
-
-	if (result)
-		return result;
-
 	return write_opcode(auprobe, mm, vaddr, UPROBE_SWBP_INSN);
 }
 
-- 
1.5.5.1

[PATCH 2/4] uprobes: Introduce copy_opcode(), kill read_opcode()

[Oleg Nesterov]

No functional changes, preparations.

1. Extract the kmap-and-memcpy code from read_opcode() into the
   new trivial helper, copy_opcode(). The next patch will add
   another user.

2. read_opcode() becomes really trivial, fold it into its single
   caller, is_swbp_at_addr().

3. Remove "auprobe" argument from write_opcode(), it is not used
   since f403072c6.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   63 ++++++++++++++--------------------------------
 1 files changed, 19 insertions(+), 44 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 04f3259..100a920 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -183,19 +183,25 @@ bool __weak is_swbp_insn(uprobe_opcode_t *insn)
 	return *insn == UPROBE_SWBP_INSN;
 }
 
+static void copy_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *opcode)
+{
+	void *kaddr = kmap_atomic(page);
+	memcpy(opcode, kaddr + (vaddr & ~PAGE_MASK), UPROBE_SWBP_INSN_SIZE);
+	kunmap_atomic(kaddr);
+}
+
 /*
  * NOTE:
  * Expect the breakpoint instruction to be the smallest size instruction for
  * the architecture. If an arch has variable length instruction and the
  * breakpoint instruction is not of the smallest length instruction
- * supported by that architecture then we need to modify read_opcode /
+ * supported by that architecture then we need to modify is_swbp_at_addr and
  * write_opcode accordingly. This would never be a problem for archs that
  * have fixed length instructions.
  */
 
 /*
  * write_opcode - write the opcode at a given virtual address.
- * @auprobe: arch breakpointing information.
  * @mm: the probed process address space.
  * @vaddr: the virtual address to store the opcode.
  * @opcode: opcode to be written at @vaddr.
@@ -206,8 +212,8 @@ bool __weak is_swbp_insn(uprobe_opcode_t *insn)
  * For mm @mm, write the opcode at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
-			unsigned long vaddr, uprobe_opcode_t opcode)
+static int write_opcode(struct mm_struct *mm, unsigned long vaddr,
+			uprobe_opcode_t opcode)
 {
 	struct page *old_page, *new_page;
 	void *vaddr_old, *vaddr_new;
@@ -253,40 +259,9 @@ put_old:
 	return ret;
 }
 
-/**
- * read_opcode - read the opcode at a given virtual address.
- * @mm: the probed process address space.
- * @vaddr: the virtual address to read the opcode.
- * @opcode: location to store the read opcode.
- *
- * Called with mm->mmap_sem held (for read and with a reference to
- * mm.
- *
- * For mm @mm, read the opcode at @vaddr and store it in @opcode.
- * Return 0 (success) or a negative errno.
- */
-static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t *opcode)
-{
-	struct page *page;
-	void *vaddr_new;
-	int ret;
-
-	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 1, &page, NULL);
-	if (ret <= 0)
-		return ret;
-
-	vaddr_new = kmap_atomic(page);
-	vaddr &= ~PAGE_MASK;
-	memcpy(opcode, vaddr_new + vaddr, UPROBE_SWBP_INSN_SIZE);
-	kunmap_atomic(vaddr_new);
-
-	put_page(page);
-
-	return 0;
-}
-
 static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)
 {
+	struct page *page;
 	uprobe_opcode_t opcode;
 	int result;
 
@@ -300,14 +275,14 @@ static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)
 			goto out;
 	}
 
-	result = read_opcode(mm, vaddr, &opcode);
-	if (result)
+	result = get_user_pages(NULL, mm, vaddr, 1, 0, 1, &page, NULL);
+	if (result < 0)
 		return result;
-out:
-	if (is_swbp_insn(&opcode))
-		return 1;
 
-	return 0;
+	copy_opcode(page, vaddr, &opcode);
+	put_page(page);
+ out:
+	return is_swbp_insn(&opcode);
 }
 
 /**
@@ -321,7 +296,7 @@ out:
  */
 int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	return write_opcode(auprobe, mm, vaddr, UPROBE_SWBP_INSN);
+	return write_opcode(mm, vaddr, UPROBE_SWBP_INSN);
 }
 
 /**
@@ -345,7 +320,7 @@ set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long v
 	if (result != 1)
 		return result;
 
-	return write_opcode(auprobe, mm, vaddr, *(uprobe_opcode_t *)auprobe->insn);
+	return write_opcode(mm, vaddr, *(uprobe_opcode_t *)auprobe->insn);
 }
 
 static int match_uprobe(struct uprobe *l, struct uprobe *r)
-- 
1.5.5.1

[PATCH 3/4] uprobes: Kill set_orig_insn()->is_swbp_at_addr()

[Oleg Nesterov]

Unlike set_swbp(), set_orig_insn()->is_swbp_at_addr() makes sense,
although it can't prevent all confusions.

But the usage of is_swbp_at_addr() is equally confusing, and it adds
the extra get_user_pages() we can avoid.

This patch removes set_orig_insn()->is_swbp_at_addr() but changes
write_opcode() to do the necessary checks before replace_page().

Perhaps it also makes sense to ensure PAGE_MAPPING_ANON in unregister
case.

find_active_uprobe() becomes the only user of is_swbp_at_addr(),
we can change its semantics.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   32 +++++++++++++++++++++++---------
 1 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 100a920..17454fb 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -190,6 +190,25 @@ static void copy_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
 	kunmap_atomic(kaddr);
 }
 
+static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode)
+{
+	uprobe_opcode_t old_opcode;
+	bool is_swbp;
+
+	copy_opcode(page, vaddr, &old_opcode);
+	is_swbp = is_swbp_insn(&old_opcode);
+
+	if (is_swbp_insn(new_opcode)) {
+		if (is_swbp)		/* register: already installed? */
+			return 0;
+	} else {
+		if (!is_swbp)		/* unregister: was it changed by us? */
+			return -EINVAL;
+	}
+
+	return 1;
+}
+
 /*
  * NOTE:
  * Expect the breakpoint instruction to be the smallest size instruction for
@@ -226,6 +245,10 @@ retry:
 	if (ret <= 0)
 		return ret;
 
+	ret = verify_opcode(old_page, vaddr, &opcode);
+	if (ret <= 0)
+		goto put_old;
+
 	ret = -ENOMEM;
 	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vaddr);
 	if (!new_page)
@@ -311,15 +334,6 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
 int __weak
 set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	int result;
-
-	result = is_swbp_at_addr(mm, vaddr);
-	if (!result)
-		return -EINVAL;
-
-	if (result != 1)
-		return result;
-
 	return write_opcode(mm, vaddr, *(uprobe_opcode_t *)auprobe->insn);
 }
 
-- 
1.5.5.1

[PATCH 4/4] uprobes: Simplify is_swbp_at_addr(), remove stale comments

[Oleg Nesterov]

After the previous change is_swbp_at_addr() is always called with
current->mm. Remove this check and move it close to its single caller.

Also, remove the obsolete comment about is_swbp_at_addr() and
uprobe_state.count.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   73 +++++++++++++++-------------------------------
 1 files changed, 24 insertions(+), 49 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 17454fb..a741ba7 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -282,32 +282,6 @@ put_old:
 	return ret;
 }
 
-static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)
-{
-	struct page *page;
-	uprobe_opcode_t opcode;
-	int result;
-
-	if (current->mm == mm) {
-		pagefault_disable();
-		result = __copy_from_user_inatomic(&opcode, (void __user*)vaddr,
-								sizeof(opcode));
-		pagefault_enable();
-
-		if (likely(result == 0))
-			goto out;
-	}
-
-	result = get_user_pages(NULL, mm, vaddr, 1, 0, 1, &page, NULL);
-	if (result < 0)
-		return result;
-
-	copy_opcode(page, vaddr, &opcode);
-	put_page(page);
- out:
-	return is_swbp_insn(&opcode);
-}
-
 /**
  * set_swbp - store breakpoint at a given address.
  * @auprobe: arch specific probepoint information.
@@ -589,29 +563,6 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
 	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
-/*
- * How mm->uprobes_state.count gets updated
- * uprobe_mmap() increments the count if
- * 	- it successfully adds a breakpoint.
- * 	- it cannot add a breakpoint, but sees that there is a underlying
- * 	  breakpoint (via a is_swbp_at_addr()).
- *
- * uprobe_munmap() decrements the count if
- * 	- it sees a underlying breakpoint, (via is_swbp_at_addr)
- * 	  (Subsequent uprobe_unregister wouldnt find the breakpoint
- * 	  unless a uprobe_mmap kicks in, since the old vma would be
- * 	  dropped just after uprobe_munmap.)
- *
- * uprobe_register increments the count if:
- * 	- it successfully adds a breakpoint.
- *
- * uprobe_unregister decrements the count if:
- * 	- it sees a underlying breakpoint and removes successfully.
- * 	  (via is_swbp_at_addr)
- * 	  (Subsequent uprobe_munmap wouldnt find the breakpoint
- * 	  since there is no underlying breakpoint after the
- * 	  breakpoint removal.)
- */
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 			struct vm_area_struct *vma, unsigned long vaddr)
@@ -1389,6 +1340,30 @@ static void mmf_recalc_uprobes(struct mm_struct *mm)
 	clear_bit(MMF_HAS_UPROBES, &mm->flags);
 }
 
+static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)
+{
+	struct page *page;
+	uprobe_opcode_t opcode;
+	int result;
+
+	pagefault_disable();
+	result = __copy_from_user_inatomic(&opcode, (void __user*)vaddr,
+							sizeof(opcode));
+	pagefault_enable();
+
+	if (likely(result == 0))
+		goto out;
+
+	result = get_user_pages(NULL, mm, vaddr, 1, 0, 1, &page, NULL);
+	if (result < 0)
+		return result;
+
+	copy_opcode(page, vaddr, &opcode);
+	put_page(page);
+ out:
+	return is_swbp_insn(&opcode);
+}
+
 static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 {
 	struct mm_struct *mm = current->mm;
-- 
1.5.5.1

Re: [PATCH 1/4] uprobes: Kill set_swbp()->is_swbp_at_addr()

[Peter Zijlstra]

On Sun, 2012-09-23 at 22:19 +0200, Oleg Nesterov wrote:
> A separate patch for better documentation.
> 
> set_swbp()->is_swbp_at_addr() is not needed for correctness, it is
> harmless to do the unnecessary __replace_page(old_page, new_page)
> when these 2 pages are identical.
> 
> And it can not be counted as optimization. mmap/register races are
> very unlikely, while in the likely case is_swbp_at_addr() adds the
> extra get_user_pages() even if the caller is uprobe_mmap(current->mm)
> and returns false.

It does save a page of memory though...

> Note also that the semantics/usage of is_swbp_at_addr() in uprobe.c
> is confusing. set_swbp() uses it to detect the case when this insn
> was already modified by uprobes, that is why it should always compare
> the opcode with UPROBE_SWBP_INSN even if the hardware (like powerpc)
> has other trap insns. It doesn't matter if this "int3" was in fact
> installed by gdb or application itself, we are going to "steal" this
> breakpoint anyway and execute the original insn from vm_file even if
> it no longer matches the memory.
> 
> OTOH, handle_swbp()->find_active_uprobe() uses is_swbp_at_addr() to
> figure out whether we need to send SIGTRAP or not if we can not find
> uprobe, so in this case it should return true for all trap variants,
> not only for UPROBE_SWBP_INSN.
> 
> This patch removes set_swbp()->is_swbp_at_addr(), the next patches
> will remove it from set_orig_insn() which is similar to set_swbp()
> in this respect. So the only caller will be handle_swbp() and we
> can make its semantics clear. 

This does leave me with the question of _why_ you're removing it.. the
above says what it does, and maybe gives a clue as to why you think it
is superfluous but I think its better to be clear on this.

Re: [PATCH 1/4] uprobes: Kill set_swbp()->is_swbp_at_addr()

[Ananth N Mavinakayanahalli]

On Sun, Sep 23, 2012 at 10:19:45PM +0200, Oleg Nesterov wrote:
> A separate patch for better documentation.
> 
> set_swbp()->is_swbp_at_addr() is not needed for correctness, it is
> harmless to do the unnecessary __replace_page(old_page, new_page)
> when these 2 pages are identical.
> 
> And it can not be counted as optimization. mmap/register races are
> very unlikely, while in the likely case is_swbp_at_addr() adds the
> extra get_user_pages() even if the caller is uprobe_mmap(current->mm)
> and returns false.
> 
> Note also that the semantics/usage of is_swbp_at_addr() in uprobe.c
> is confusing. set_swbp() uses it to detect the case when this insn
> was already modified by uprobes, that is why it should always compare
> the opcode with UPROBE_SWBP_INSN even if the hardware (like powerpc)
> has other trap insns.

Agreed...

> It doesn't matter if this "int3" was in fact
> installed by gdb or application itself, we are going to "steal" this
> breakpoint anyway and execute the original insn from vm_file even if
> it no longer matches the memory.

Wouldn't this text make more sense:

It doesn't matter if this 'breakpoint' was in fact...

'int3' is still an x86 artifact.

On powerpc, we don't even get to install_breakpoint() ->set_swbp()
->is_swbp_at_addr() because arch_uprobe_analyze_insn() would've already
caused install_breakpoint() to return -ENOTSUPP.

> OTOH, handle_swbp()->find_active_uprobe() uses is_swbp_at_addr() to
> figure out whether we need to send SIGTRAP or not if we can not find
> uprobe, so in this case it should return true for all trap variants,
> not only for UPROBE_SWBP_INSN.

So, we will need a powerpc specific is_swbp_insn()... ok.

Ananth

Re: [PATCH 3/4] uprobes: Kill set_orig_insn()->is_swbp_at_addr()

[Peter Zijlstra]

On Sun, 2012-09-23 at 22:19 +0200, Oleg Nesterov wrote:

> @@ -226,6 +245,10 @@ retry:

Could you use:

$ cat ~/.gitconfig 
[diff "default"]
	xfuncname = "^[[:alpha:]$_].*[^:]$"

This avoids git-diff it using labels as function names.

>  	if (ret <= 0)
>  		return ret;
>  
> +	ret = verify_opcode(old_page, vaddr, &opcode);
> +	if (ret <= 0)
> +		goto put_old;
> +
>  	ret = -ENOMEM;
>  	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vaddr);
>  	if (!new_page)

Re: [PATCH 1/4] uprobes: Kill set_swbp()->is_swbp_at_addr()

[Oleg Nesterov]

On 09/24, Peter Zijlstra wrote:
>
> On Sun, 2012-09-23 at 22:19 +0200, Oleg Nesterov wrote:
> > A separate patch for better documentation.
> >
> > set_swbp()->is_swbp_at_addr() is not needed for correctness, it is
> > harmless to do the unnecessary __replace_page(old_page, new_page)
> > when these 2 pages are identical.
> >
> > And it can not be counted as optimization. mmap/register races are
> > very unlikely, while in the likely case is_swbp_at_addr() adds the
> > extra get_user_pages() even if the caller is uprobe_mmap(current->mm)
> > and returns false.
>
> It does save a page of memory though...

No, it doesn't, I think.

If this page has int3 it is not file-backed, it was already COW'ed by
uprobes or gdb.

Note the !UPROBE_COPY_INSN code in install breakpoint which has another
is_swbp_insn(). Yes, this logic is not 100% correct and needs more fixes.

So it can only prevent the unnecessary alloc_page() + replace_page() +
free_page(old_page), but only in unlikely case.

And please note that 3/4 restores this optimization, but avoids the
extra get_user_pages(). This will be more important when we add the
filtering, uprobe_register() will need to call register_for_each_vma()
every time when the new consumer comes.

> > Note also that the semantics/usage of is_swbp_at_addr() in uprobe.c
> > is confusing. set_swbp() uses it to detect the case when this insn
> > was already modified by uprobes, that is why it should always compare
> > the opcode with UPROBE_SWBP_INSN even if the hardware (like powerpc)
> > has other trap insns. It doesn't matter if this "int3" was in fact
> > installed by gdb or application itself, we are going to "steal" this
> > breakpoint anyway and execute the original insn from vm_file even if
> > it no longer matches the memory.
> >
> > OTOH, handle_swbp()->find_active_uprobe() uses is_swbp_at_addr() to
> > figure out whether we need to send SIGTRAP or not if we can not find
> > uprobe, so in this case it should return true for all trap variants,
> > not only for UPROBE_SWBP_INSN.
> >
> > This patch removes set_swbp()->is_swbp_at_addr(), the next patches
> > will remove it from set_orig_insn() which is similar to set_swbp()
> > in this respect. So the only caller will be handle_swbp() and we
> > can make its semantics clear.
>
> This does leave me with the question of _why_ you're removing it..

Again, 3/4 restores this optimization, and imho this series can be
counted as a cleanup/simplification and makes sense anyway.

But the main reason is dufferent. Once again. Lets ignore the problems
with gdb which can install breakpoints too.


set_swbp() and set_orig_insn() use is_swbp_at_addr() to figure out
whether this opcode was modified by uprobes or not. So in this case
is_swbp_insn() has to compare the opcode with UPROBE_SWBP_INSN used
by set_swbp().

But handle_swbp()->find_active_uprobe()->is_swbp_at_addr() is different,
it needs to decide should we send SIGTRAP or not if uprobe was not
found. On x86 this is the same, but powerpc has other insns which
can trigger powerpc's do_int3() counterpart, so in this case
is_swbp_insn() should return true for all trap variants.

Oleg.

Re: [PATCH 3/4] uprobes: Kill set_orig_insn()->is_swbp_at_addr()

[Oleg Nesterov]

On 09/24, Peter Zijlstra wrote:
>
> On Sun, 2012-09-23 at 22:19 +0200, Oleg Nesterov wrote:
>
> > @@ -226,6 +245,10 @@ retry:
>
> Could you use:
>
> $ cat ~/.gitconfig
> [diff "default"]
> 	xfuncname = "^[[:alpha:]$_].*[^:]$"
>
> This avoids git-diff it using labels as function names.

Aha. Will do, thanks!

Oleg.

Re: [PATCH 1/4] uprobes: Kill set_swbp()->is_swbp_at_addr()

[Oleg Nesterov]

On 09/24, Ananth N Mavinakayanahalli wrote:
>
> On Sun, Sep 23, 2012 at 10:19:45PM +0200, Oleg Nesterov wrote:
>
> > It doesn't matter if this "int3" was in fact
> > installed by gdb or application itself, we are going to "steal" this
> > breakpoint anyway and execute the original insn from vm_file even if
> > it no longer matches the memory.
>
> Wouldn't this text make more sense:
>
> It doesn't matter if this 'breakpoint' was in fact...
>
> 'int3' is still an x86 artifact.

Agreed, will update the changelog.

> On powerpc, we don't even get to install_breakpoint() ->set_swbp()
> ->is_swbp_at_addr() because arch_uprobe_analyze_insn() would've already
> caused install_breakpoint() to return -ENOTSUPP.
>
> > OTOH, handle_swbp()->find_active_uprobe() uses is_swbp_at_addr() to
> > figure out whether we need to send SIGTRAP or not if we can not find
> > uprobe, so in this case it should return true for all trap variants,
> > not only for UPROBE_SWBP_INSN.
>
> So, we will need a powerpc specific is_swbp_insn()... ok.

I think yes.

But again, we need 2 helpers. One for is_swbp_at_addr(), and another
for verify_opcode() which only checks UPROBE_SWBP_INSN. IOW, something
like the patch below (on top of this series).

Do you agree?

Oleg.

--- x/kernel/events/uprobes.c
+++ x/kernel/events/uprobes.c
@@ -178,6 +178,11 @@ static int __replace_page(struct vm_area
  * Default implementation of is_swbp_insn
  * Returns true if @insn is a breakpoint instruction.
  */
+static bool is_uprobe_opcode(uprobe_opcode_t *insn)
+{
+	return *insn == UPROBE_SWBP_INSN;
+}
+
 bool __weak is_swbp_insn(uprobe_opcode_t *insn)
 {
 	return *insn == UPROBE_SWBP_INSN;
@@ -196,9 +201,9 @@ static int verify_opcode(struct page *pa
 	bool is_swbp;
 
 	copy_opcode(page, vaddr, &old_opcode);
-	is_swbp = is_swbp_insn(&old_opcode);
+	is_swbp = is_uprobe_opcode(&old_opcode);
 
-	if (is_swbp_insn(new_opcode)) {
+	if (is_uprobe_opcode(new_opcode)) {
 		if (is_swbp)		/* register: already installed? */
 			return 0;
 	} else {
@@ -585,7 +590,7 @@ install_breakpoint(struct uprobe *uprobe
 		if (ret)
 			return ret;
 
-		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+		if (is_uprobe_opcode((uprobe_opcode_t *)uprobe->arch.insn))
 			return -ENOTSUPP;
 
 		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);

Re: [PATCH 1/4] uprobes: Kill set_swbp()->is_swbp_at_addr()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-23 22:19:45]:

> A separate patch for better documentation.
> 
> set_swbp()->is_swbp_at_addr() is not needed for correctness, it is
> harmless to do the unnecessary __replace_page(old_page, new_page)
> when these 2 pages are identical.
> 
> And it can not be counted as optimization. mmap/register races are
> very unlikely, while in the likely case is_swbp_at_addr() adds the
> extra get_user_pages() even if the caller is uprobe_mmap(current->mm)
> and returns false.
> 
> Note also that the semantics/usage of is_swbp_at_addr() in uprobe.c
> is confusing. set_swbp() uses it to detect the case when this insn
> was already modified by uprobes, that is why it should always compare
> the opcode with UPROBE_SWBP_INSN even if the hardware (like powerpc)
> has other trap insns. It doesn't matter if this "int3" was in fact
> installed by gdb or application itself, we are going to "steal" this
> breakpoint anyway and execute the original insn from vm_file even if
> it no longer matches the memory.
> 
> OTOH, handle_swbp()->find_active_uprobe() uses is_swbp_at_addr() to
> figure out whether we need to send SIGTRAP or not if we can not find
> uprobe, so in this case it should return true for all trap variants,
> not only for UPROBE_SWBP_INSN.
> 
> This patch removes set_swbp()->is_swbp_at_addr(), the next patches
> will remove it from set_orig_insn() which is similar to set_swbp()
> in this respect. So the only caller will be handle_swbp() and we
> can make its semantics clear.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Re: [PATCH 2/4] uprobes: Introduce copy_opcode(), kill read_opcode()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-23 22:19:48]:

> No functional changes, preparations.
> 
> 1. Extract the kmap-and-memcpy code from read_opcode() into the
>    new trivial helper, copy_opcode(). The next patch will add
>    another user.
> 
> 2. read_opcode() becomes really trivial, fold it into its single
>    caller, is_swbp_at_addr().
> 
> 3. Remove "auprobe" argument from write_opcode(), it is not used
>    since f403072c6.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Re: [PATCH 4/4] uprobes: Simplify is_swbp_at_addr(), remove stale comments

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-23 22:19:53]:

> After the previous change is_swbp_at_addr() is always called with
> current->mm. Remove this check and move it close to its single caller.
> 
> Also, remove the obsolete comment about is_swbp_at_addr() and
> uprobe_state.count.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---


Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Re: [PATCH 3/4] uprobes: Kill set_orig_insn()->is_swbp_at_addr()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-23 22:19:50]:

> Unlike set_swbp(), set_orig_insn()->is_swbp_at_addr() makes sense,
> although it can't prevent all confusions.
> 
> But the usage of is_swbp_at_addr() is equally confusing, and it adds
> the extra get_user_pages() we can avoid.
> 
> This patch removes set_orig_insn()->is_swbp_at_addr() but changes
> write_opcode() to do the necessary checks before replace_page().
> 
> Perhaps it also makes sense to ensure PAGE_MAPPING_ANON in unregister
> case.
> 
> find_active_uprobe() becomes the only user of is_swbp_at_addr(),
> we can change its semantics.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---



Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>