Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-07 Thread Srikar Dronamraju 
* Oleg Nesterov  [2012-10-06 20:59:49]:

> On 10/06, Srikar Dronamraju wrote:
> >
> > Yeah prepare_uprobe() looks good for me.
> >
> > Acked-by: Srikar Dronamraju 
> 
> OK, renamed.
> 
> The next patches updated accordinly, I hope I can keep your acks.

Yes, please keep my acks as I understand that the hunks in the rest of
the patches will have trivial modifications.

-- 
Thanks and Regards
Srikar

> 
> --
> [PATCH] uprobes: Introduce prepare_uprobe()
> 
> Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
> from install_breakpoint() into the new helper, prepare_uprobe().
> 
> And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
> else can use them anyway.
> 
> Signed-off-by: Oleg Nesterov 
> Acked-by: Srikar Dronamraju 
> ---
>  include/linux/uprobes.h |   10 
>  kernel/events/uprobes.c |   60 
> ---
>  2 files changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 18d839d..2459457 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -35,16 +35,6 @@ struct inode;
>  # include 
>  #endif
> 
> -/* flags that denote/change uprobes behaviour */
> -
> -/* Have a copy of original instruction */
> -#define UPROBE_COPY_INSN 0x1
> -
> -/* Dont run handlers when first register/ last unregister in progress*/
> -#define UPROBE_RUN_HANDLER   0x2
> -/* Can skip singlestep */
> -#define UPROBE_SKIP_SSTEP0x4
> -
>  struct uprobe_consumer {
>   int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
>   /*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index dbbca3a..d6fa332 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
>   */
>  static atomic_t uprobe_events = ATOMIC_INIT(0);
> 
> +/* Have a copy of original instruction */
> +#define UPROBE_COPY_INSN 0x1
> +/* Dont run handlers when first register/ last unregister in progress*/
> +#define UPROBE_RUN_HANDLER   0x2
> +/* Can skip singlestep */
> +#define UPROBE_SKIP_SSTEP0x4
> +
>  struct uprobe {
>   struct rb_node  rb_node;/* node in the rb tree */
>   atomic_tref;
> @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
> *filp)
>   return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, 
> uprobe->offset);
>  }
> 
> +static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
> + struct mm_struct *mm, unsigned long vaddr)
> +{
> + int ret = 0;
> +
> + if (uprobe->flags & UPROBE_COPY_INSN)
> + return ret;
> +
> + ret = copy_insn(uprobe, file);
> + if (ret)
> + goto out;
> +
> + ret = -ENOTSUPP;
> + if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> + goto out;
> +
> + ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
> + if (ret)
> + goto out;
> +
> + /* write_opcode() assumes we don't cross page boundary */
> + BUG_ON((uprobe->offset & ~PAGE_MASK) +
> + UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> +
> + smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> + uprobe->flags |= UPROBE_COPY_INSN;
> +
> + out:
> + return ret;
> +}
> +
>  static int
>  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>   struct vm_area_struct *vma, unsigned long vaddr)
> @@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct 
> mm_struct *mm,
>   if (!uprobe->consumers)
>   return 0;
> 
> - if (!(uprobe->flags & UPROBE_COPY_INSN)) {
> - ret = copy_insn(uprobe, vma->vm_file);
> - if (ret)
> - return ret;
> -
> - if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> - return -ENOTSUPP;
> -
> - ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
> - 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);
> -
> - smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> - uprobe->flags |= UPROBE_COPY_INSN;
> - }
> + ret = prepare_uprobe(uprobe, vma->vm_file, mm, vaddr);
> + if (ret)
> + return ret;
> 
>   /*
>* set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
> -- 
> 1.5.5.1
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-07 Thread Srikar Dronamraju 
* Oleg Nesterov o...@redhat.com [2012-10-06 20:59:49]:

 On 10/06, Srikar Dronamraju wrote:
 
  Yeah prepare_uprobe() looks good for me.
 
  Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
 
 OK, renamed.
 
 The next patches updated accordinly, I hope I can keep your acks.

Yes, please keep my acks as I understand that the hunks in the rest of
the patches will have trivial modifications.

-- 
Thanks and Regards
Srikar

 
 --
 [PATCH] uprobes: Introduce prepare_uprobe()
 
 Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
 from install_breakpoint() into the new helper, prepare_uprobe().
 
 And move uprobe-flags defines from uprobes.h to uprobes.c, nobody
 else can use them anyway.
 
 Signed-off-by: Oleg Nesterov o...@redhat.com
 Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
 ---
  include/linux/uprobes.h |   10 
  kernel/events/uprobes.c |   60 
 ---
  2 files changed, 41 insertions(+), 29 deletions(-)
 
 diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
 index 18d839d..2459457 100644
 --- a/include/linux/uprobes.h
 +++ b/include/linux/uprobes.h
 @@ -35,16 +35,6 @@ struct inode;
  # include asm/uprobes.h
  #endif
 
 -/* flags that denote/change uprobes behaviour */
 -
 -/* Have a copy of original instruction */
 -#define UPROBE_COPY_INSN 0x1
 -
 -/* Dont run handlers when first register/ last unregister in progress*/
 -#define UPROBE_RUN_HANDLER   0x2
 -/* Can skip singlestep */
 -#define UPROBE_SKIP_SSTEP0x4
 -
  struct uprobe_consumer {
   int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
   /*
 diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
 index dbbca3a..d6fa332 100644
 --- a/kernel/events/uprobes.c
 +++ b/kernel/events/uprobes.c
 @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
   */
  static atomic_t uprobe_events = ATOMIC_INIT(0);
 
 +/* Have a copy of original instruction */
 +#define UPROBE_COPY_INSN 0x1
 +/* Dont run handlers when first register/ last unregister in progress*/
 +#define UPROBE_RUN_HANDLER   0x2
 +/* Can skip singlestep */
 +#define UPROBE_SKIP_SSTEP0x4
 +
  struct uprobe {
   struct rb_node  rb_node;/* node in the rb tree */
   atomic_tref;
 @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
 *filp)
   return __copy_insn(mapping, filp, uprobe-arch.insn, bytes, 
 uprobe-offset);
  }
 
 +static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 + struct mm_struct *mm, unsigned long vaddr)
 +{
 + int ret = 0;
 +
 + if (uprobe-flags  UPROBE_COPY_INSN)
 + return ret;
 +
 + ret = copy_insn(uprobe, file);
 + if (ret)
 + goto out;
 +
 + ret = -ENOTSUPP;
 + if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
 + goto out;
 +
 + ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
 + if (ret)
 + goto out;
 +
 + /* write_opcode() assumes we don't cross page boundary */
 + BUG_ON((uprobe-offset  ~PAGE_MASK) +
 + UPROBE_SWBP_INSN_SIZE  PAGE_SIZE);
 +
 + smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 + uprobe-flags |= UPROBE_COPY_INSN;
 +
 + out:
 + return ret;
 +}
 +
  static int
  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
   struct vm_area_struct *vma, unsigned long vaddr)
 @@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct 
 mm_struct *mm,
   if (!uprobe-consumers)
   return 0;
 
 - if (!(uprobe-flags  UPROBE_COPY_INSN)) {
 - ret = copy_insn(uprobe, vma-vm_file);
 - if (ret)
 - return ret;
 -
 - if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
 - return -ENOTSUPP;
 -
 - ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
 - 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);
 -
 - smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 - uprobe-flags |= UPROBE_COPY_INSN;
 - }
 + ret = prepare_uprobe(uprobe, vma-vm_file, mm, vaddr);
 + if (ret)
 + return ret;
 
   /*
* set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
 -- 
 1.5.5.1
 
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Oleg Nesterov 
On 10/06, Srikar Dronamraju wrote:
>
> Yeah prepare_uprobe() looks good for me.
>
> Acked-by: Srikar Dronamraju 

OK, renamed.

The next patches updated accordinly, I hope I can keep your acks.

--
[PATCH] uprobes: Introduce prepare_uprobe()

Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, prepare_uprobe().

And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

Signed-off-by: Oleg Nesterov 
Acked-by: Srikar Dronamraju 
---
 include/linux/uprobes.h |   10 
 kernel/events/uprobes.c |   60 ---
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include 
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN   0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER 0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP  0x4
-
 struct uprobe_consumer {
int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dbbca3a..d6fa332 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN   0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER 0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP  0x4
+
 struct uprobe {
struct rb_node  rb_node;/* node in the rb tree */
atomic_tref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
*filp)
return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, 
uprobe->offset);
 }
 
+static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
+   struct mm_struct *mm, unsigned long vaddr)
+{
+   int ret = 0;
+
+   if (uprobe->flags & UPROBE_COPY_INSN)
+   return ret;
+
+   ret = copy_insn(uprobe, file);
+   if (ret)
+   goto out;
+
+   ret = -ENOTSUPP;
+   if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+   goto out;
+
+   ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
+   if (ret)
+   goto out;
+
+   /* write_opcode() assumes we don't cross page boundary */
+   BUG_ON((uprobe->offset & ~PAGE_MASK) +
+   UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+
+   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+   uprobe->flags |= UPROBE_COPY_INSN;
+
+ out:
+   return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct 
*mm,
if (!uprobe->consumers)
return 0;
 
-   if (!(uprobe->flags & UPROBE_COPY_INSN)) {
-   ret = copy_insn(uprobe, vma->vm_file);
-   if (ret)
-   return ret;
-
-   if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
-   return -ENOTSUPP;
-
-   ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
-   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);
-
-   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-   uprobe->flags |= UPROBE_COPY_INSN;
-   }
+   ret = prepare_uprobe(uprobe, vma->vm_file, mm, vaddr);
+   if (ret)
+   return ret;
 
/*
 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
-- 
1.5.5.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Srikar Dronamraju 
> > uprobe_copy_insn.
> 
> Yes ;) plus it is not only "copy", it does _analyze
> 
> > Not sure test_and_copy_insn() is a good alternative.
> 
> perhaps prepare_uprobe()... But I agree with any naming, just tell me
> what you prefer and I'll update the patch again.


Yeah prepare_uprobe() looks good for me. 



Acked-by: Srikar Dronamraju 

> 
> ==============
> [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()
> 
> Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
> from install_breakpoint() into the new helper, uprobe_copy_insn().
> 
> And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
> else can use them anyway.
> 
> Signed-off-by: Oleg Nesterov 
> ---
>  include/linux/uprobes.h |   10 
>  kernel/events/uprobes.c |   60 
> ---
>  2 files changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 18d839d..2459457 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -35,16 +35,6 @@ struct inode;
>  # include 
>  #endif
> 
> -/* flags that denote/change uprobes behaviour */
> -
> -/* Have a copy of original instruction */
> -#define UPROBE_COPY_INSN 0x1
> -
> -/* Dont run handlers when first register/ last unregister in progress*/
> -#define UPROBE_RUN_HANDLER   0x2
> -/* Can skip singlestep */
> -#define UPROBE_SKIP_SSTEP0x4
> -
>  struct uprobe_consumer {
>   int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
>   /*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index dd44311..98b60bc 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
>   */
>  static atomic_t uprobe_events = ATOMIC_INIT(0);
> 
> +/* Have a copy of original instruction */
> +#define UPROBE_COPY_INSN 0x1
> +/* Dont run handlers when first register/ last unregister in progress*/
> +#define UPROBE_RUN_HANDLER   0x2
> +/* Can skip singlestep */
> +#define UPROBE_SKIP_SSTEP0x4
> +
>  struct uprobe {
>   struct rb_node  rb_node;/* node in the rb tree */
>   atomic_tref;
> @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
> *filp)
>   return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, 
> uprobe->offset);
>  }
> 
> +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
> + struct mm_struct *mm, unsigned long vaddr)
> +{
> + int ret = 0;
> +
> + if (uprobe->flags & UPROBE_COPY_INSN)
> + return ret;
> +
> + ret = copy_insn(uprobe, file);
> + if (ret)
> + goto out;
> +
> + ret = -ENOTSUPP;
> + if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> + goto out;
> +
> + ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
> + if (ret)
> + goto out;
> +
> + /* write_opcode() assumes we don't cross page boundary */
> + BUG_ON((uprobe->offset & ~PAGE_MASK) +
> + UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> +
> + smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> + uprobe->flags |= UPROBE_COPY_INSN;
> +
> + out:
> + return ret;
> +}
> +
>  static int
>  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>   struct vm_area_struct *vma, unsigned long vaddr)
> @@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct 
> mm_struct *mm,
>   if (!uprobe->consumers)
>   return 0;
> 
> - if (!(uprobe->flags & UPROBE_COPY_INSN)) {
> - ret = copy_insn(uprobe, vma->vm_file);
> - if (ret)
> - return ret;
> -
> - if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> - return -ENOTSUPP;
> -
> - ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
> - 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);
> -
> - smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> - uprobe->flags |= UPROBE_COPY_INSN;
> - }
> + ret = uprobe_copy_insn(uprobe, vma->vm_file, mm, vaddr);
> + if (ret)
> + return ret;
> 
>   /*
>* set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
> -- 
> 1.5.5.1
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Oleg Nesterov 
On 10/06, Srikar Dronamraju wrote:
>
> * Oleg Nesterov  [2012-09-30 21:42:17]:
>
> > +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
> > +   struct mm_struct *mm, unsigned long vaddr)
> > +{
> > +   int ret = 0;
> > +
> > +   if (uprobe->flags & UPROBE_COPY_INSN)
> > +   return ret;
> > +
> > +   ret = copy_insn(uprobe, file);
> > +   if (ret)
> > +   goto out;
> > +
> > +   ret = -ENOTSUPP;
> > +   if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> > +   goto out;
> > +
> > +   ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
> > +   if (ret)
> > +   goto out;
> > +
> > +   /* write_opcode() assumes we don't cross page boundary */
> > +   BUG_ON((uprobe->offset & ~PAGE_MASK) +
> > +   UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> > +
> > +   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> > +   uprobe->flags |= UPROBE_COPY_INSN;
> > +   ret = 0;
> > + out:
> > +   return ret;
> > +}
> > +
>
> 2 nits:
>  why do we need to reset ret before out label? I think its redudant.
>  arch_uprobe_analyze_insn() should have set it to 0 already. No?

OK,

> blank line above out:

OK, see the updated patch below.

> Currently only extern functions start with uprobe_ but we already have
> copy_insn, and __copy_insn, So can think of any names for
> uprobe_copy_insn.

Yes ;) plus it is not only "copy", it does _analyze

> Not sure test_and_copy_insn() is a good alternative.

perhaps prepare_uprobe()... But I agree with any naming, just tell me
what you prefer and I'll update the patch again.

==
[PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, uprobe_copy_insn().

And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

Signed-off-by: Oleg Nesterov 
---
 include/linux/uprobes.h |   10 
 kernel/events/uprobes.c |   60 ---
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include 
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN   0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER 0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP  0x4
-
 struct uprobe_consumer {
int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dd44311..98b60bc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN   0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER 0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP  0x4
+
 struct uprobe {
struct rb_node  rb_node;/* node in the rb tree */
atomic_tref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
*filp)
return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, 
uprobe->offset);
 }
 
+static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
+   struct mm_struct *mm, unsigned long vaddr)
+{
+   int ret = 0;
+
+   if (uprobe->flags & UPROBE_COPY_INSN)
+   return ret;
+
+   ret = copy_insn(uprobe, file);
+   if (ret)
+   goto out;
+
+   ret = -ENOTSUPP;
+   if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+   goto out;
+
+   ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
+   if (ret)
+   goto out;
+
+   /* write_opcode() assumes we don't cross page boundary */
+   BUG_ON((uprobe->offset & ~PAGE_MASK) +
+   UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+
+   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+   uprobe->flags |= UPROBE_COPY_INSN;
+
+ out:
+   return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Srikar Dronamraju 
* Oleg Nesterov  [2012-09-30 21:42:17]:

> Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
> from install_breakpoint() into the new helper, uprobe_copy_insn().
> 
> And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
> else can use them anyway.
> 
> Signed-off-by: Oleg Nesterov 
> ---
>  include/linux/uprobes.h |   10 
>  kernel/events/uprobes.c |   60 
> ---
>  2 files changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 18d839d..2459457 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -35,16 +35,6 @@ struct inode;
>  # include 
>  #endif
> 
> -/* flags that denote/change uprobes behaviour */
> -
> -/* Have a copy of original instruction */
> -#define UPROBE_COPY_INSN 0x1
> -
> -/* Dont run handlers when first register/ last unregister in progress*/
> -#define UPROBE_RUN_HANDLER   0x2
> -/* Can skip singlestep */
> -#define UPROBE_SKIP_SSTEP0x4
> -
>  struct uprobe_consumer {
>   int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
>   /*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index a81080f..5c0c1b0 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
>   */
>  static atomic_t uprobe_events = ATOMIC_INIT(0);
> 
> +/* Have a copy of original instruction */
> +#define UPROBE_COPY_INSN 0x1
> +/* Dont run handlers when first register/ last unregister in progress*/
> +#define UPROBE_RUN_HANDLER   0x2
> +/* Can skip singlestep */
> +#define UPROBE_SKIP_SSTEP0x4
> +
>  struct uprobe {
>   struct rb_node  rb_node;/* node in the rb tree */
>   atomic_tref;
> @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
> *filp)
>   return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, 
> uprobe->offset);
>  }
> 
> +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
> + struct mm_struct *mm, unsigned long vaddr)
> +{
> + int ret = 0;
> +
> + if (uprobe->flags & UPROBE_COPY_INSN)
> + return ret;
> +
> + ret = copy_insn(uprobe, file);
> + if (ret)
> + goto out;
> +
> + ret = -ENOTSUPP;
> + if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> + goto out;
> +
> + ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
> + if (ret)
> + goto out;
> +
> + /* write_opcode() assumes we don't cross page boundary */
> + BUG_ON((uprobe->offset & ~PAGE_MASK) +
> + UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
> +
> + smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
> + uprobe->flags |= UPROBE_COPY_INSN;
> + ret = 0;
> + out:
> + return ret;
> +}
> +

2 nits: 
 why do we need to reset ret before out label? I think its redudant.
 arch_uprobe_analyze_insn() should have set it to 0 already. No?

blank line above out:

Currently only extern functions start with uprobe_ but we already have
copy_insn, and __copy_insn, So can think of any names for
uprobe_copy_insn. Not sure test_and_copy_insn() is a good alternative.

-- 
thanks and regards
Srikar

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Srikar Dronamraju 
* Oleg Nesterov o...@redhat.com [2012-09-30 21:42:17]:

 Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
 from install_breakpoint() into the new helper, uprobe_copy_insn().
 
 And move uprobe-flags defines from uprobes.h to uprobes.c, nobody
 else can use them anyway.
 
 Signed-off-by: Oleg Nesterov o...@redhat.com
 ---
  include/linux/uprobes.h |   10 
  kernel/events/uprobes.c |   60 
 ---
  2 files changed, 41 insertions(+), 29 deletions(-)
 
 diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
 index 18d839d..2459457 100644
 --- a/include/linux/uprobes.h
 +++ b/include/linux/uprobes.h
 @@ -35,16 +35,6 @@ struct inode;
  # include asm/uprobes.h
  #endif
 
 -/* flags that denote/change uprobes behaviour */
 -
 -/* Have a copy of original instruction */
 -#define UPROBE_COPY_INSN 0x1
 -
 -/* Dont run handlers when first register/ last unregister in progress*/
 -#define UPROBE_RUN_HANDLER   0x2
 -/* Can skip singlestep */
 -#define UPROBE_SKIP_SSTEP0x4
 -
  struct uprobe_consumer {
   int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
   /*
 diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
 index a81080f..5c0c1b0 100644
 --- a/kernel/events/uprobes.c
 +++ b/kernel/events/uprobes.c
 @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
   */
  static atomic_t uprobe_events = ATOMIC_INIT(0);
 
 +/* Have a copy of original instruction */
 +#define UPROBE_COPY_INSN 0x1
 +/* Dont run handlers when first register/ last unregister in progress*/
 +#define UPROBE_RUN_HANDLER   0x2
 +/* Can skip singlestep */
 +#define UPROBE_SKIP_SSTEP0x4
 +
  struct uprobe {
   struct rb_node  rb_node;/* node in the rb tree */
   atomic_tref;
 @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
 *filp)
   return __copy_insn(mapping, filp, uprobe-arch.insn, bytes, 
 uprobe-offset);
  }
 
 +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 + struct mm_struct *mm, unsigned long vaddr)
 +{
 + int ret = 0;
 +
 + if (uprobe-flags  UPROBE_COPY_INSN)
 + return ret;
 +
 + ret = copy_insn(uprobe, file);
 + if (ret)
 + goto out;
 +
 + ret = -ENOTSUPP;
 + if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
 + goto out;
 +
 + ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
 + if (ret)
 + goto out;
 +
 + /* write_opcode() assumes we don't cross page boundary */
 + BUG_ON((uprobe-offset  ~PAGE_MASK) +
 + UPROBE_SWBP_INSN_SIZE  PAGE_SIZE);
 +
 + smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 + uprobe-flags |= UPROBE_COPY_INSN;
 + ret = 0;
 + out:
 + return ret;
 +}
 +

2 nits: 
 why do we need to reset ret before out label? I think its redudant.
 arch_uprobe_analyze_insn() should have set it to 0 already. No?

blank line above out:

Currently only extern functions start with uprobe_ but we already have
copy_insn, and __copy_insn, So can think of any names for
uprobe_copy_insn. Not sure test_and_copy_insn() is a good alternative.

-- 
thanks and regards
Srikar

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Oleg Nesterov 
On 10/06, Srikar Dronamraju wrote:

 * Oleg Nesterov o...@redhat.com [2012-09-30 21:42:17]:

  +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
  +   struct mm_struct *mm, unsigned long vaddr)
  +{
  +   int ret = 0;
  +
  +   if (uprobe-flags  UPROBE_COPY_INSN)
  +   return ret;
  +
  +   ret = copy_insn(uprobe, file);
  +   if (ret)
  +   goto out;
  +
  +   ret = -ENOTSUPP;
  +   if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
  +   goto out;
  +
  +   ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
  +   if (ret)
  +   goto out;
  +
  +   /* write_opcode() assumes we don't cross page boundary */
  +   BUG_ON((uprobe-offset  ~PAGE_MASK) +
  +   UPROBE_SWBP_INSN_SIZE  PAGE_SIZE);
  +
  +   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
  +   uprobe-flags |= UPROBE_COPY_INSN;
  +   ret = 0;
  + out:
  +   return ret;
  +}
  +

 2 nits:
  why do we need to reset ret before out label? I think its redudant.
  arch_uprobe_analyze_insn() should have set it to 0 already. No?

OK,

 blank line above out:

OK, see the updated patch below.

 Currently only extern functions start with uprobe_ but we already have
 copy_insn, and __copy_insn, So can think of any names for
 uprobe_copy_insn.

Yes ;) plus it is not only copy, it does _analyze

 Not sure test_and_copy_insn() is a good alternative.

perhaps prepare_uprobe()... But I agree with any naming, just tell me
what you prefer and I'll update the patch again.

==
[PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, uprobe_copy_insn().

And move uprobe-flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

Signed-off-by: Oleg Nesterov o...@redhat.com
---
 include/linux/uprobes.h |   10 
 kernel/events/uprobes.c |   60 ---
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include asm/uprobes.h
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN   0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER 0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP  0x4
-
 struct uprobe_consumer {
int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dd44311..98b60bc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN   0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER 0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP  0x4
+
 struct uprobe {
struct rb_node  rb_node;/* node in the rb tree */
atomic_tref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
*filp)
return __copy_insn(mapping, filp, uprobe-arch.insn, bytes, 
uprobe-offset);
 }
 
+static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
+   struct mm_struct *mm, unsigned long vaddr)
+{
+   int ret = 0;
+
+   if (uprobe-flags  UPROBE_COPY_INSN)
+   return ret;
+
+   ret = copy_insn(uprobe, file);
+   if (ret)
+   goto out;
+
+   ret = -ENOTSUPP;
+   if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
+   goto out;
+
+   ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
+   if (ret)
+   goto out;
+
+   /* write_opcode() assumes we don't cross page boundary */
+   BUG_ON((uprobe-offset  ~PAGE_MASK) +
+   UPROBE_SWBP_INSN_SIZE  PAGE_SIZE);
+
+   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+   uprobe-flags |= UPROBE_COPY_INSN;
+
+ out:
+   return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct 
*mm,
if (!uprobe-consumers)
return 0;
 
-   if (!(uprobe-flags  UPROBE_COPY_INSN)) {
-   ret = copy_insn(uprobe, vma-vm_file);
-   if (ret)
-   return ret;
-
-   if (is_swbp_insn((uprobe_opcode_t *)uprobe

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Srikar Dronamraju 
  uprobe_copy_insn.
 
 Yes ;) plus it is not only copy, it does _analyze
 
  Not sure test_and_copy_insn() is a good alternative.
 
 perhaps prepare_uprobe()... But I agree with any naming, just tell me
 what you prefer and I'll update the patch again.


Yeah prepare_uprobe() looks good for me. 



Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com

 
 ==
 [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()
 
 Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
 from install_breakpoint() into the new helper, uprobe_copy_insn().
 
 And move uprobe-flags defines from uprobes.h to uprobes.c, nobody
 else can use them anyway.
 
 Signed-off-by: Oleg Nesterov o...@redhat.com
 ---
  include/linux/uprobes.h |   10 
  kernel/events/uprobes.c |   60 
 ---
  2 files changed, 41 insertions(+), 29 deletions(-)
 
 diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
 index 18d839d..2459457 100644
 --- a/include/linux/uprobes.h
 +++ b/include/linux/uprobes.h
 @@ -35,16 +35,6 @@ struct inode;
  # include asm/uprobes.h
  #endif
 
 -/* flags that denote/change uprobes behaviour */
 -
 -/* Have a copy of original instruction */
 -#define UPROBE_COPY_INSN 0x1
 -
 -/* Dont run handlers when first register/ last unregister in progress*/
 -#define UPROBE_RUN_HANDLER   0x2
 -/* Can skip singlestep */
 -#define UPROBE_SKIP_SSTEP0x4
 -
  struct uprobe_consumer {
   int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
   /*
 diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
 index dd44311..98b60bc 100644
 --- a/kernel/events/uprobes.c
 +++ b/kernel/events/uprobes.c
 @@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
   */
  static atomic_t uprobe_events = ATOMIC_INIT(0);
 
 +/* Have a copy of original instruction */
 +#define UPROBE_COPY_INSN 0x1
 +/* Dont run handlers when first register/ last unregister in progress*/
 +#define UPROBE_RUN_HANDLER   0x2
 +/* Can skip singlestep */
 +#define UPROBE_SKIP_SSTEP0x4
 +
  struct uprobe {
   struct rb_node  rb_node;/* node in the rb tree */
   atomic_tref;
 @@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
 *filp)
   return __copy_insn(mapping, filp, uprobe-arch.insn, bytes, 
 uprobe-offset);
  }
 
 +static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
 + struct mm_struct *mm, unsigned long vaddr)
 +{
 + int ret = 0;
 +
 + if (uprobe-flags  UPROBE_COPY_INSN)
 + return ret;
 +
 + ret = copy_insn(uprobe, file);
 + if (ret)
 + goto out;
 +
 + ret = -ENOTSUPP;
 + if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
 + goto out;
 +
 + ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
 + if (ret)
 + goto out;
 +
 + /* write_opcode() assumes we don't cross page boundary */
 + BUG_ON((uprobe-offset  ~PAGE_MASK) +
 + UPROBE_SWBP_INSN_SIZE  PAGE_SIZE);
 +
 + smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 + uprobe-flags |= UPROBE_COPY_INSN;
 +
 + out:
 + return ret;
 +}
 +
  static int
  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
   struct vm_area_struct *vma, unsigned long vaddr)
 @@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct 
 mm_struct *mm,
   if (!uprobe-consumers)
   return 0;
 
 - if (!(uprobe-flags  UPROBE_COPY_INSN)) {
 - ret = copy_insn(uprobe, vma-vm_file);
 - if (ret)
 - return ret;
 -
 - if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
 - return -ENOTSUPP;
 -
 - ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
 - 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);
 -
 - smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 - uprobe-flags |= UPROBE_COPY_INSN;
 - }
 + ret = uprobe_copy_insn(uprobe, vma-vm_file, mm, vaddr);
 + if (ret)
 + return ret;
 
   /*
* set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
 -- 
 1.5.5.1
 
 
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-10-06 Thread Oleg Nesterov 
On 10/06, Srikar Dronamraju wrote:

 Yeah prepare_uprobe() looks good for me.

 Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com

OK, renamed.

The next patches updated accordinly, I hope I can keep your acks.

--
[PATCH] uprobes: Introduce prepare_uprobe()

Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, prepare_uprobe().

And move uprobe-flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

Signed-off-by: Oleg Nesterov o...@redhat.com
Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com
---
 include/linux/uprobes.h |   10 
 kernel/events/uprobes.c |   60 ---
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include asm/uprobes.h
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN   0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER 0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP  0x4
-
 struct uprobe_consumer {
int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dbbca3a..d6fa332 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN   0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER 0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP  0x4
+
 struct uprobe {
struct rb_node  rb_node;/* node in the rb tree */
atomic_tref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
*filp)
return __copy_insn(mapping, filp, uprobe-arch.insn, bytes, 
uprobe-offset);
 }
 
+static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
+   struct mm_struct *mm, unsigned long vaddr)
+{
+   int ret = 0;
+
+   if (uprobe-flags  UPROBE_COPY_INSN)
+   return ret;
+
+   ret = copy_insn(uprobe, file);
+   if (ret)
+   goto out;
+
+   ret = -ENOTSUPP;
+   if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
+   goto out;
+
+   ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
+   if (ret)
+   goto out;
+
+   /* write_opcode() assumes we don't cross page boundary */
+   BUG_ON((uprobe-offset  ~PAGE_MASK) +
+   UPROBE_SWBP_INSN_SIZE  PAGE_SIZE);
+
+   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+   uprobe-flags |= UPROBE_COPY_INSN;
+
+ out:
+   return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct 
*mm,
if (!uprobe-consumers)
return 0;
 
-   if (!(uprobe-flags  UPROBE_COPY_INSN)) {
-   ret = copy_insn(uprobe, vma-vm_file);
-   if (ret)
-   return ret;
-
-   if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
-   return -ENOTSUPP;
-
-   ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
-   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);
-
-   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-   uprobe-flags |= UPROBE_COPY_INSN;
-   }
+   ret = prepare_uprobe(uprobe, vma-vm_file, mm, vaddr);
+   if (ret)
+   return ret;
 
/*
 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
-- 
1.5.5.1


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-09-30 Thread Oleg Nesterov 
Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, uprobe_copy_insn().

And move uprobe->flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

Signed-off-by: Oleg Nesterov 
---
 include/linux/uprobes.h |   10 
 kernel/events/uprobes.c |   60 ---
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include 
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN   0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER 0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP  0x4
-
 struct uprobe_consumer {
int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a81080f..5c0c1b0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN   0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER 0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP  0x4
+
 struct uprobe {
struct rb_node  rb_node;/* node in the rb tree */
atomic_tref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
*filp)
return __copy_insn(mapping, filp, uprobe->arch.insn, bytes, 
uprobe->offset);
 }
 
+static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
+   struct mm_struct *mm, unsigned long vaddr)
+{
+   int ret = 0;
+
+   if (uprobe->flags & UPROBE_COPY_INSN)
+   return ret;
+
+   ret = copy_insn(uprobe, file);
+   if (ret)
+   goto out;
+
+   ret = -ENOTSUPP;
+   if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
+   goto out;
+
+   ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
+   if (ret)
+   goto out;
+
+   /* write_opcode() assumes we don't cross page boundary */
+   BUG_ON((uprobe->offset & ~PAGE_MASK) +
+   UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+
+   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+   uprobe->flags |= UPROBE_COPY_INSN;
+   ret = 0;
+ out:
+   return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct 
*mm,
if (!uprobe->consumers)
return 0;
 
-   if (!(uprobe->flags & UPROBE_COPY_INSN)) {
-   ret = copy_insn(uprobe, vma->vm_file);
-   if (ret)
-   return ret;
-
-   if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
-   return -ENOTSUPP;
-
-   ret = arch_uprobe_analyze_insn(>arch, mm, vaddr);
-   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);
-
-   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-   uprobe->flags |= UPROBE_COPY_INSN;
-   }
+   ret = uprobe_copy_insn(uprobe, vma->vm_file, mm, vaddr);
+   if (ret)
+   return ret;
 
/*
 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/7] uprobes: Introduce uprobe_copy_insn()

2012-09-30 Thread Oleg Nesterov 
Preparation. Extract the copy_insn/arch_uprobe_analyze_insn code
from install_breakpoint() into the new helper, uprobe_copy_insn().

And move uprobe-flags defines from uprobes.h to uprobes.c, nobody
else can use them anyway.

Signed-off-by: Oleg Nesterov o...@redhat.com
---
 include/linux/uprobes.h |   10 
 kernel/events/uprobes.c |   60 ---
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 18d839d..2459457 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -35,16 +35,6 @@ struct inode;
 # include asm/uprobes.h
 #endif
 
-/* flags that denote/change uprobes behaviour */
-
-/* Have a copy of original instruction */
-#define UPROBE_COPY_INSN   0x1
-
-/* Dont run handlers when first register/ last unregister in progress*/
-#define UPROBE_RUN_HANDLER 0x2
-/* Can skip singlestep */
-#define UPROBE_SKIP_SSTEP  0x4
-
 struct uprobe_consumer {
int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
/*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a81080f..5c0c1b0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -78,6 +78,13 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
  */
 static atomic_t uprobe_events = ATOMIC_INIT(0);
 
+/* Have a copy of original instruction */
+#define UPROBE_COPY_INSN   0x1
+/* Dont run handlers when first register/ last unregister in progress*/
+#define UPROBE_RUN_HANDLER 0x2
+/* Can skip singlestep */
+#define UPROBE_SKIP_SSTEP  0x4
+
 struct uprobe {
struct rb_node  rb_node;/* node in the rb tree */
atomic_tref;
@@ -563,6 +570,37 @@ static int copy_insn(struct uprobe *uprobe, struct file 
*filp)
return __copy_insn(mapping, filp, uprobe-arch.insn, bytes, 
uprobe-offset);
 }
 
+static int uprobe_copy_insn(struct uprobe *uprobe, struct file *file,
+   struct mm_struct *mm, unsigned long vaddr)
+{
+   int ret = 0;
+
+   if (uprobe-flags  UPROBE_COPY_INSN)
+   return ret;
+
+   ret = copy_insn(uprobe, file);
+   if (ret)
+   goto out;
+
+   ret = -ENOTSUPP;
+   if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
+   goto out;
+
+   ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
+   if (ret)
+   goto out;
+
+   /* write_opcode() assumes we don't cross page boundary */
+   BUG_ON((uprobe-offset  ~PAGE_MASK) +
+   UPROBE_SWBP_INSN_SIZE  PAGE_SIZE);
+
+   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
+   uprobe-flags |= UPROBE_COPY_INSN;
+   ret = 0;
+ out:
+   return ret;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long vaddr)
@@ -580,25 +618,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct 
*mm,
if (!uprobe-consumers)
return 0;
 
-   if (!(uprobe-flags  UPROBE_COPY_INSN)) {
-   ret = copy_insn(uprobe, vma-vm_file);
-   if (ret)
-   return ret;
-
-   if (is_swbp_insn((uprobe_opcode_t *)uprobe-arch.insn))
-   return -ENOTSUPP;
-
-   ret = arch_uprobe_analyze_insn(uprobe-arch, mm, vaddr);
-   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);
-
-   smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
-   uprobe-flags |= UPROBE_COPY_INSN;
-   }
+   ret = uprobe_copy_insn(uprobe, vma-vm_file, mm, vaddr);
+   if (ret)
+   return ret;
 
/*
 * set MMF_HAS_UPROBES in advance for uprobe_pre_sstep_notifier(),
-- 
1.5.5.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/