uprobes: misc cleanups

PATCH In review Oleg Nesterov 5 participants 16 messages Jun 13, 2012 – Jun 14, 2012

Subsystems (3)

x86 perf events uprobes

lore

Share & tools

lore

Oleg Nesterov

author maintainer

2012-06-13 18:51 UTC #0 lore raw

cover uprobes: misc cleanups

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.

Oleg Nesterov

author maintainer

2012-06-13 18:52 UTC #1 lore raw

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

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 <[email protected]>

---
 kernel/events/uprobes.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

kernel/events/uprobes.c +6 -9

index 54c8780..bb61248 100644
@@ -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;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-06-13 18:52 UTC #2 lore raw

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

In reply to Oleg Nesterov

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

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/events/uprobes.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

kernel/events/uprobes.c +4 -4

index bb61248..9bb5571 100644
@@ -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);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-06-13 18:52 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/events/uprobes.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

kernel/events/uprobes.c +0 -5

index 9bb5571..799d6ed 100644
@@ -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)

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-06-13 18:52 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/events/uprobes.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

kernel/events/uprobes.c +5 -6

index 799d6ed..a4dc9fa 100644
@@ -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;
 	}

Signature · 1 line

1.5.5.1

Oleg Nesterov

author maintainer

2012-06-13 18:53 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/events/uprobes.c |   24 ++++++++----------------
 1 files changed, 8 insertions(+), 16 deletions(-)

kernel/events/uprobes.c +8 ~~-16~~

index a4dc9fa..773bb37 100644
@@ -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

Signature · 1 line

1.5.5.1

Ananth N Mavinakayanahalli

2012-06-14 05:24 UTC #6 lore raw

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

Quoted 7 lines · 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.

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

Ingo Molnar

2012-06-14 08:38 UTC #7 lore raw

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

Quoted 5 lines · Ananth N Mavinakayanahalli

Quoted 4 lines · 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".

Quoted 8 lines

Quoted 3 lines
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

Ananth N Mavinakayanahalli

2012-06-14 08:44 UTC #8 lore raw

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

Quoted 17 lines · Ingo Molnar

Quoted 5 lines · Ananth N Mavinakayanahalli
Quoted 4 lines · 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".
Quoted 8 lines
Quoted 3 lines
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. [...]

Quoted 7 lines

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

Anton Arapov

2012-06-14 10:59 UTC #9 lore raw

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

Quoted 21 lines · Ananth N Mavinakayanahalli

Quoted 20 lines · Ingo Molnar
Quoted 5 lines · Ananth N Mavinakayanahalli
Quoted 4 lines · 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".
Quoted 8 lines
Quoted 3 lines
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:

Quoted 6 lines

Quoted 4 lines
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

Anton Arapov

2012-06-14 11:07 UTC #10 lore raw

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

From: Oleg Nesterov <[email protected]>

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 <[email protected]>

Signed-off-by Anton Arapov <[email protected]>

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

arch/x86/include/asm/uprobes.h +1 -1

index f3971bb..14f0d44 100644
@@ -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);

arch/x86/kernel/uprobes.c +2 -2

index 36fd420..e17b749 100644
@@ -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;

kernel/events/uprobes.c +9 ~~-17~~

index 0e8d28e..a57bb1b 100644
@@ -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

Oleg Nesterov

author maintainer

2012-06-14 17:41 UTC #11 lore raw

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

In reply to Ananth N Mavinakayanahalli

Quoted 6 lines · Ananth N Mavinakayanahalli

Quoted 4 lines · 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".

Quoted 8 lines

Quoted 3 lines
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.

Quoted 3 lines

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.

Oleg Nesterov

author maintainer

2012-06-13 18:53 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/events/uprobes.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

kernel/events/uprobes.c +6 -6

index 773bb37..5ad1d77 100644
@@ -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);

Signature · 1 line

1.5.5.1

Ananth N Mavinakayanahalli

2012-06-14 05:25 UTC #13 lore raw

In reply to Oleg Nesterov

Quoted 8 lines · 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.

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

Ananth

Srikar Dronamraju

Acked-by on the v1 series

2012-06-14 14:59 UTC #14 lore raw

In reply to Oleg Nesterov

Quoted 8 lines

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 <[email protected]>

Oleg Nesterov

author maintainer

2012-06-14 17:46 UTC #15 lore raw

Quoted 7 lines · Srikar Dronamraju

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

Quoted 6 lines

Quoted 2 lines
Srikar, Ananth, please review and tell me what do you think.
The changes look good.
Acked-by Srikar Dronamraju <[email protected]>

Thanks!

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

Oleg.