[PATCH 0/6] uprobes: misc cleanups

[PATCH 0/6] uprobes: misc cleanups

[Oleg Nesterov]

Hello,

On top of the previous uprobes patches I sent.

I was going to cleanup write_opcode(), but it turns out we
should cleanup the callers first.

Srikar, Ananth, please review and tell me what do you think.

Oleg.

[PATCH 1/6] uprobes: copy_insn() shouldn't depend on mm/vma/vaddr

[Oleg Nesterov]

1. copy_insn() doesn't need "addr", it can use uprobe->offset.
   Remove this argument.

2. Change copy_insn/__copy_insn to accept "struct file*" instead
   of vma.

copy_insn() is called only once and mm/vma/vaddr are random, it
shouldn't depend on them.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 54c8780..bb61248 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -590,10 +590,9 @@ static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
 }
 
 static int
-__copy_insn(struct address_space *mapping, struct vm_area_struct *vma, char *insn,
+__copy_insn(struct address_space *mapping, struct file *filp, char *insn,
 			unsigned long nbytes, unsigned long offset)
 {
-	struct file *filp = vma->vm_file;
 	struct page *page;
 	void *vaddr;
 	unsigned long off1;
@@ -623,15 +622,13 @@ __copy_insn(struct address_space *mapping, struct vm_area_struct *vma, char *ins
 	return 0;
 }
 
-static int
-copy_insn(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long addr)
+static int copy_insn(struct uprobe *uprobe, struct file *filp)
 {
 	struct address_space *mapping;
 	unsigned long nbytes;
 	int bytes;
 
-	addr &= ~PAGE_MASK;
-	nbytes = PAGE_SIZE - addr;
+	nbytes = PAGE_SIZE - (uprobe->offset & ~PAGE_MASK);
 	mapping = uprobe->inode->i_mapping;
 
 	/* Instruction at end of binary; copy only available bytes */
@@ -642,13 +639,13 @@ copy_insn(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long addr)
 
 	/* Instruction at the page-boundary; copy bytes in second page */
 	if (nbytes < bytes) {
-		if (__copy_insn(mapping, vma, uprobe->arch.insn + nbytes,
+		if (__copy_insn(mapping, filp, uprobe->arch.insn + nbytes,
 				bytes - nbytes, uprobe->offset + nbytes))
 			return -ENOMEM;
 
 		bytes = nbytes;
 	}
-	return __copy_insn(mapping, vma, uprobe->arch.insn, bytes, uprobe->offset);
+	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
 }
 
 /*
@@ -694,7 +691,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	addr = (unsigned long)vaddr;
 
 	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
-		ret = copy_insn(uprobe, vma, addr);
+		ret = copy_insn(uprobe, vma->vm_file);
 		if (ret)
 			return ret;
 
-- 
1.5.5.1

[PATCH 2/6] uprobes: copy_insn() should not return -ENOMEM if __copy_insn() fails

[Oleg Nesterov]

copy_insn() returns -ENOMEM if the first __copy_insn() fails,
it should return the correct error code.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index bb61248..9bb5571 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -639,10 +639,10 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
 
 	/* Instruction at the page-boundary; copy bytes in second page */
 	if (nbytes < bytes) {
-		if (__copy_insn(mapping, filp, uprobe->arch.insn + nbytes,
-				bytes - nbytes, uprobe->offset + nbytes))
-			return -ENOMEM;
-
+		int err = __copy_insn(mapping, filp, uprobe->arch.insn + nbytes,
+				bytes - nbytes, uprobe->offset + nbytes);
+		if (err)
+			return err;
 		bytes = nbytes;
 	}
 	return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, uprobe->offset);
-- 
1.5.5.1

[PATCH 3/6] uprobes: no need to re-check vma_address() in write_opcode()

[Oleg Nesterov]

write_opcode() is called by register_for_each_vma() and uprobe_mmap()
paths. In both cases the caller has already verified this vaddr under
mmap_sem, no need to re-check.

Note also that this check is wrong anyway, we should not truncate
loff_t returned by vma_address() if we do not trust this mapping.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 9bb5571..799d6ed 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -211,7 +211,6 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	struct vm_area_struct *vma;
 	struct uprobe *uprobe;
 	unsigned long pgoff;
-	loff_t addr;
 	int ret;
 retry:
 	/* Read the page with vaddr into memory */
@@ -235,10 +234,6 @@ retry:
 	if (mapping != vma->vm_file->f_mapping)
 		goto put_out;
 
-	addr = vma_address(vma, uprobe->offset);
-	if (vaddr != (unsigned long)addr)
-		goto put_out;
-
 	ret = -ENOMEM;
 	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vaddr);
 	if (!new_page)
-- 
1.5.5.1

[PATCH 4/6] uprobes: move BUG_ON(UPROBE_SWBP_INSN_SIZE) from write_opcode() to install_breakpoint()

[Oleg Nesterov]

write_opcode() ensures that UPROBE_SWBP_INSN doesn't cross the
page boundary. This looks a bit confusing, the check does not
depend on vaddr and it is enough to do it only once right after
install_breakpoint()->arch_uprobe_analyze_insn().

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 799d6ed..a4dc9fa 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -210,7 +210,6 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	void *vaddr_old, *vaddr_new;
 	struct vm_area_struct *vma;
 	struct uprobe *uprobe;
-	unsigned long pgoff;
 	int ret;
 retry:
 	/* Read the page with vaddr into memory */
@@ -251,11 +250,7 @@ retry:
 	vaddr_new = kmap_atomic(new_page);
 
 	memcpy(vaddr_new, vaddr_old, PAGE_SIZE);
-
-	/* poke the new insn in, ASSUMES we don't cross page boundary */
-	pgoff = (vaddr & ~PAGE_MASK);
-	BUG_ON(pgoff + UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
-	memcpy(vaddr_new + pgoff, &opcode, UPROBE_SWBP_INSN_SIZE);
+	memcpy(vaddr_new + (vaddr & ~PAGE_MASK), &opcode, UPROBE_SWBP_INSN_SIZE);
 
 	kunmap_atomic(vaddr_new);
 	kunmap_atomic(vaddr_old);
@@ -697,6 +692,10 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		if (ret)
 			return ret;
 
+		/* write_opcode() assumes we don't cross page boundary */
+		BUG_ON((uprobe->offset & ~PAGE_MASK) +
+				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+
 		uprobe->flags |= UPROBE_COPY_INSN;
 	}
 
-- 
1.5.5.1

[PATCH 5/6] uprobes: don't use loff_t for the valid virtual address

[Oleg Nesterov]

loff_t looks confusing when it is used for the virtual address.
Change map_info and install_breakpoint/remove_breakpoint paths
to use "unsigned long".

The patch doesn't change vma_address(), it can't return "long"
because it is used to verify the mapping. But probably this
needs some cleanups too.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a4dc9fa..773bb37 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -663,9 +663,8 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
  */
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
-			struct vm_area_struct *vma, loff_t vaddr)
+			struct vm_area_struct *vma, unsigned long vaddr)
 {
-	unsigned long addr;
 	int ret;
 
 	/*
@@ -678,8 +677,6 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	if (!uprobe->consumers)
 		return -EEXIST;
 
-	addr = (unsigned long)vaddr;
-
 	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
 		ret = copy_insn(uprobe, vma->vm_file);
 		if (ret)
@@ -708,7 +705,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	 * Hence increment before and decrement on failure.
 	 */
 	atomic_inc(&mm->uprobes_state.count);
-	ret = set_swbp(&uprobe->arch, mm, addr);
+	ret = set_swbp(&uprobe->arch, mm, vaddr);
 	if (ret)
 		atomic_dec(&mm->uprobes_state.count);
 
@@ -716,9 +713,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 }
 
 static void
-remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, loff_t vaddr)
+remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	if (!set_orig_insn(&uprobe->arch, mm, (unsigned long)vaddr, true))
+	if (!set_orig_insn(&uprobe->arch, mm, vaddr, true))
 		atomic_dec(&mm->uprobes_state.count);
 }
 
@@ -742,7 +739,7 @@ static void delete_uprobe(struct uprobe *uprobe)
 struct map_info {
 	struct map_info *next;
 	struct mm_struct *mm;
-	loff_t vaddr;
+	unsigned long vaddr;
 };
 
 static inline struct map_info *free_map_info(struct map_info *info)
@@ -836,7 +833,6 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 	while (info) {
 		struct mm_struct *mm = info->mm;
 		struct vm_area_struct *vma;
-		loff_t vaddr;
 
 		if (err)
 			goto free;
@@ -846,9 +842,8 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 		if (!vma || !valid_vma(vma, is_register))
 			goto unlock;
 
-		vaddr = vma_address(vma, uprobe->offset);
 		if (vma->vm_file->f_mapping->host != uprobe->inode ||
-						vaddr != info->vaddr)
+		    vma_address(vma, uprobe->offset) != info->vaddr)
 			goto unlock;
 
 		if (is_register) {
@@ -1054,10 +1049,8 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	count = 0;
 
 	list_for_each_entry(uprobe, &tmp_list, pending_list) {
-		loff_t vaddr;
-
 		if (!ret) {
-			vaddr = vma_address(vma, uprobe->offset);
+			loff_t vaddr = vma_address(vma, uprobe->offset);
 
 			if (vaddr < vma->vm_start || vaddr >= vma->vm_end) {
 				put_uprobe(uprobe);
@@ -1121,9 +1114,8 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 	build_probe_list(inode, &tmp_list);
 
 	list_for_each_entry(uprobe, &tmp_list, pending_list) {
-		loff_t vaddr;
+		loff_t vaddr = vma_address(vma, uprobe->offset);
 
-		vaddr = vma_address(vma, uprobe->offset);
 		if (vaddr >= start && vaddr < end) {
 			/*
 			 * An unregister could have removed the probe before
-- 
1.5.5.1

Re: [PATCH 5/6] uprobes: don't use loff_t for the valid virtual address

[Ananth N Mavinakayanahalli]

On Wed, Jun 13, 2012 at 08:51:06PM +0200, Oleg Nesterov wrote:
> loff_t looks confusing when it is used for the virtual address.
> Change map_info and install_breakpoint/remove_breakpoint paths
> to use "unsigned long".
> 
> The patch doesn't change vma_address(), it can't return "long"
> because it is used to verify the mapping. But probably this
> needs some cleanups too.

Oleg,

As you mentioned in another email, this conflicts with my [1/2]
preparatory patch for the powerpc port. Do you think it just makes sense
to make the arch_uprobe_analyze_insn() prototype change to take vaddr as
part of this set itself?

I will then rebase the powerpc port when this goes into an upstream
tree.

Ananth

Re: [PATCH 5/6] uprobes: don't use loff_t for the valid virtual address

[Ingo Molnar]

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

> On Wed, Jun 13, 2012 at 08:51:06PM +0200, Oleg Nesterov wrote:
> > loff_t looks confusing when it is used for the virtual address.
> > Change map_info and install_breakpoint/remove_breakpoint paths
> > to use "unsigned long".
> > 
> > The patch doesn't change vma_address(), it can't return "long"
> > because it is used to verify the mapping. But probably this
> > needs some cleanups too.
> 
> Oleg,
> 
> As you mentioned in another email, this conflicts with my 
> [1/2] preparatory patch for the powerpc port. [...]

Note, I already merged your preparatory patch into tip:perf/core 
a couple of days ago:

 7eb9ba5ed312 uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()

So as long as Oleg is working on top of -tip there should be no 
conflict.

Thanks,

	Ingo

Re: [PATCH 5/6] uprobes: don't use loff_t for the valid virtual address

[Ananth N Mavinakayanahalli]

On Thu, Jun 14, 2012 at 10:38:26AM +0200, Ingo Molnar wrote:
> 
> * Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
> 
> > On Wed, Jun 13, 2012 at 08:51:06PM +0200, Oleg Nesterov wrote:
> > > loff_t looks confusing when it is used for the virtual address.
> > > Change map_info and install_breakpoint/remove_breakpoint paths
> > > to use "unsigned long".
> > > 
> > > The patch doesn't change vma_address(), it can't return "long"
> > > because it is used to verify the mapping. But probably this
> > > needs some cleanups too.
> > 
> > Oleg,
> > 
> > As you mentioned in another email, this conflicts with my 
> > [1/2] preparatory patch for the powerpc port. [...]
> 
> Note, I already merged your preparatory patch into tip:perf/core 
> a couple of days ago:
> 
>  7eb9ba5ed312 uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
> 
> So as long as Oleg is working on top of -tip there should be no 
> conflict.

Ah.. thanks Ingo. I missed that tip commit email.

Ananth

Re: [PATCH 5/6] uprobes: don't use loff_t for the valid virtual address

[Anton Arapov]

On Thu, Jun 14, 2012 at 02:13:00PM +0530, Ananth N Mavinakayanahalli wrote:
> On Thu, Jun 14, 2012 at 10:38:26AM +0200, Ingo Molnar wrote:
> > 
> > * Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
> > 
> > > On Wed, Jun 13, 2012 at 08:51:06PM +0200, Oleg Nesterov wrote:
> > > > loff_t looks confusing when it is used for the virtual address.
> > > > Change map_info and install_breakpoint/remove_breakpoint paths
> > > > to use "unsigned long".
> > > > 
> > > > The patch doesn't change vma_address(), it can't return "long"
> > > > because it is used to verify the mapping. But probably this
> > > > needs some cleanups too.
> > > 
> > > Oleg,
> > > 
> > > As you mentioned in another email, this conflicts with my 
> > > [1/2] preparatory patch for the powerpc port. [...]
> > 
> > Note, I already merged your preparatory patch into tip:perf/core 
> > a couple of days ago:
> > 
> >  7eb9ba5ed312 uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
> > 
> > So as long as Oleg is working on top of -tip there should be no 
> > conflict.
> 
> Ah.. thanks Ingo. I missed that tip commit email.

  Seems Oleg didn't use -tip. There are no conflicts though, just a
small neat there ... Fixed in follow up mail.

Anton

[PATCH v2 5/6] uprobes: don't use loff_t for the valid virtual address

[Anton Arapov]

From: Oleg Nesterov <oleg@redhat.com>

loff_t looks confusing when it is used for the virtual address.
Change map_info and install_breakpoint/remove_breakpoint paths
to use "unsigned long".

The patch doesn't change vma_address(), it can't return "long"
because it is used to verify the mapping. But probably this
needs some cleanups too.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Anton Arapov <anton@redhat.com>
---
 arch/x86/include/asm/uprobes.h |    2 +-
 arch/x86/kernel/uprobes.c      |    4 ++--
 kernel/events/uprobes.c        |   26 +++++++++-----------------
 3 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index f3971bb..14f0d44 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -48,7 +48,7 @@ struct arch_uprobe_task {
 #endif
 };
 
-extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
+extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 36fd420..e17b749 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -409,10 +409,10 @@ static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm,
  * arch_uprobe_analyze_insn - instruction analysis including validity and fixups.
  * @mm: the probed address space.
  * @arch_uprobe: the probepoint information.
- * @addr: virtual address at which to install the probepoint
+ * @vaddr: virtual address at which to install the probepoint
  * Return 0 on success or a -ve number on error.
  */
-int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr)
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
 {
 	int ret;
 	struct insn insn;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 0e8d28e..a57bb1b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -677,9 +677,8 @@ static int copy_insn(struct uprobe *uprobe, struct file *filp)
  */
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
-			struct vm_area_struct *vma, loff_t vaddr)
+			struct vm_area_struct *vma, unsigned long vaddr)
 {
-	unsigned long addr;
 	int ret;
 
 	/*
@@ -692,8 +691,6 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	if (!uprobe->consumers)
 		return -EEXIST;
 
-	addr = (unsigned long)vaddr;
-
 	if (!(uprobe->flags & UPROBE_COPY_INSN)) {
 		ret = copy_insn(uprobe, vma->vm_file);
 		if (ret)
@@ -702,7 +699,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
 			return -ENOTSUPP;
 
-		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, addr);
+		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm, vaddr);
 		if (ret)
 			return ret;
 
@@ -722,7 +719,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	 * Hence increment before and decrement on failure.
 	 */
 	atomic_inc(&mm->uprobes_state.count);
-	ret = set_swbp(&uprobe->arch, mm, addr);
+	ret = set_swbp(&uprobe->arch, mm, vaddr);
 	if (ret)
 		atomic_dec(&mm->uprobes_state.count);
 
@@ -730,9 +727,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 }
 
 static void
-remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, loff_t vaddr)
+remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	if (!set_orig_insn(&uprobe->arch, mm, (unsigned long)vaddr, true))
+	if (!set_orig_insn(&uprobe->arch, mm, vaddr, true))
 		atomic_dec(&mm->uprobes_state.count);
 }
 
@@ -756,7 +753,7 @@ static void delete_uprobe(struct uprobe *uprobe)
 struct map_info {
 	struct map_info *next;
 	struct mm_struct *mm;
-	loff_t vaddr;
+	unsigned long vaddr;
 };
 
 static inline struct map_info *free_map_info(struct map_info *info)
@@ -850,7 +847,6 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 	while (info) {
 		struct mm_struct *mm = info->mm;
 		struct vm_area_struct *vma;
-		loff_t vaddr;
 
 		if (err)
 			goto free;
@@ -860,9 +856,8 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 		if (!vma || !valid_vma(vma, is_register))
 			goto unlock;
 
-		vaddr = vma_address(vma, uprobe->offset);
 		if (vma->vm_file->f_mapping->host != uprobe->inode ||
-						vaddr != info->vaddr)
+		    vma_address(vma, uprobe->offset) != info->vaddr)
 			goto unlock;
 
 		if (is_register) {
@@ -1068,10 +1063,8 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	count = 0;
 
 	list_for_each_entry(uprobe, &tmp_list, pending_list) {
-		loff_t vaddr;
-
 		if (!ret) {
-			vaddr = vma_address(vma, uprobe->offset);
+			loff_t vaddr = vma_address(vma, uprobe->offset);
 
 			if (vaddr < vma->vm_start || vaddr >= vma->vm_end) {
 				put_uprobe(uprobe);
@@ -1135,9 +1128,8 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 	build_probe_list(inode, &tmp_list);
 
 	list_for_each_entry(uprobe, &tmp_list, pending_list) {
-		loff_t vaddr;
+		loff_t vaddr = vma_address(vma, uprobe->offset);
 
-		vaddr = vma_address(vma, uprobe->offset);
 		if (vaddr >= start && vaddr < end) {
 			/*
 			 * An unregister could have removed the probe before

Re: [PATCH 5/6] uprobes: don't use loff_t for the valid virtual address

[Oleg Nesterov]

On 06/14, Ananth N Mavinakayanahalli wrote:
>
> On Wed, Jun 13, 2012 at 08:51:06PM +0200, Oleg Nesterov wrote:
> > loff_t looks confusing when it is used for the virtual address.
> > Change map_info and install_breakpoint/remove_breakpoint paths
> > to use "unsigned long".
> >
> > The patch doesn't change vma_address(), it can't return "long"
> > because it is used to verify the mapping. But probably this
> > needs some cleanups too.
>
> Oleg,
>
> As you mentioned in another email, this conflicts with my [1/2]
> preparatory patch for the powerpc port.

Yes, sorry. I didn't notice it was already applied.

As I said, I do not really agree with this change,
arch_uprobe_analyze_insn() shouldn't use addr/vaddr.

However this is minor, we can change this later. And this code needs
more changes anyway.

I'll rebase this series on top of your "1/2" and resend.


> Do you think it just makes sense
> to make the arch_uprobe_analyze_insn() prototype change to take vaddr as
> part of this set itself?

Yes, Anton already did the necessary fixup (thanks Anton ;)

Oleg.

[PATCH 6/6] uprobes: __copy_insn() needs "loff_t offset"

[Oleg Nesterov]

1. __copy_insn() needs "loff_t offset", not "unsigned long",
   to read the file.

2. use pgoff_t for "idx" and remove the unnecessary typecast.

3. fix the typo, "&=" is not what we want

4. can't resist, rename off1 to off.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 773bb37..5ad1d77 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -581,20 +581,20 @@ static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
 
 static int
 __copy_insn(struct address_space *mapping, struct file *filp, char *insn,
-			unsigned long nbytes, unsigned long offset)
+			unsigned long nbytes, loff_t offset)
 {
 	struct page *page;
 	void *vaddr;
-	unsigned long off1;
-	unsigned long idx;
+	unsigned long off;
+	pgoff_t idx;
 
 	if (!filp)
 		return -EINVAL;
 	if (!mapping->a_ops->readpage)
 		return -EIO;
 
-	idx = (unsigned long)(offset >> PAGE_CACHE_SHIFT);
-	off1 = offset &= ~PAGE_MASK;
+	idx = offset >> PAGE_CACHE_SHIFT;
+	off = offset & ~PAGE_MASK;
 
 	/*
 	 * Ensure that the page that has the original instruction is
@@ -605,7 +605,7 @@ __copy_insn(struct address_space *mapping, struct file *filp, char *insn,
 		return PTR_ERR(page);
 
 	vaddr = kmap_atomic(page);
-	memcpy(insn, vaddr + off1, nbytes);
+	memcpy(insn, vaddr + off, nbytes);
 	kunmap_atomic(vaddr);
 	page_cache_release(page);
 
-- 
1.5.5.1

Re: [PATCH 0/6] uprobes: misc cleanups

[Ananth N Mavinakayanahalli]

On Wed, Jun 13, 2012 at 08:49:33PM +0200, Oleg Nesterov wrote:
> Hello,
> 
> On top of the previous uprobes patches I sent.
> 
> I was going to cleanup write_opcode(), but it turns out we
> should cleanup the callers first.
> 
> Srikar, Ananth, please review and tell me what do you think.

I don't have an objection to this set in principle, except for the
'convenience' comment on one of the patches.

Ananth

Re: [PATCH 0/6] uprobes: misc cleanups

[Srikar Dronamraju]

> 
> On top of the previous uprobes patches I sent.
> 
> I was going to cleanup write_opcode(), but it turns out we
> should cleanup the callers first.
> 
> Srikar, Ananth, please review and tell me what do you think.
> 

The changes look good. 

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

Re: [PATCH 0/6] uprobes: misc cleanups

[Oleg Nesterov]

On 06/14, Srikar Dronamraju wrote:
>
> >
> > On top of the previous uprobes patches I sent.
> >
> > I was going to cleanup write_opcode(), but it turns out we
> > should cleanup the callers first.
> >
> > Srikar, Ananth, please review and tell me what do you think.
> >
>
> The changes look good.
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks!

Srikar, I assume you agree with the previous changes as well?
If yes, I'll appreciate if you ask them explicitly.

Oleg.