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.
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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
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
* Ananth N Mavinakayanahalli <[email protected]> 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
On Thu, Jun 14, 2012 at 10:38:26AM +0200, Ingo Molnar wrote:
>
> * Ananth N Mavinakayanahalli <[email protected]> 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
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 <[email protected]> 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
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(-)
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
>
> 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]>
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.
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 <[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.