[PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area

[PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area

[Oleg Nesterov]

1. Don't include asm/uprobes.h unconditionally, we only need
   it if CONFIG_UPROBES.

2. Move the definition of "struct xol_area" into uprobes.c.

   Perhaps we should simply kill struct uprobes_state, it buys
   nothing.

3. Kill the dummy definition of uprobe_get_swbp_addr(), nobody
   except handle_swbp() needs it.

4. Purely cosmetic, but move the decl of uprobe_get_swbp_addr()
   up, close to other __weak helpers.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/uprobes.h |   31 ++++---------------------------
 kernel/events/uprobes.c |   19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 366b8b2..fe0c0bb 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -33,10 +33,6 @@ struct mm_struct;
 struct inode;
 struct notifier_block;
 
-#ifdef CONFIG_ARCH_SUPPORTS_UPROBES
-# include <asm/uprobes.h>
-#endif
-
 #define UPROBE_HANDLER_REMOVE		1
 #define UPROBE_HANDLER_MASK		1
 
@@ -61,6 +57,8 @@ struct uprobe_consumer {
 };
 
 #ifdef CONFIG_UPROBES
+#include <asm/uprobes.h>
+
 enum uprobe_task_state {
 	UTASK_RUNNING,
 	UTASK_SSTEP,
@@ -93,24 +91,7 @@ struct uprobe_task {
 	unsigned int			depth;
 };
 
-/*
- * On a breakpoint hit, thread contests for a slot.  It frees the
- * slot after singlestep. Currently a fixed number of slots are
- * allocated.
- */
-struct xol_area {
-	wait_queue_head_t 	wq;		/* if all slots are busy */
-	atomic_t 		slot_count;	/* number of in-use slots */
-	unsigned long 		*bitmap;	/* 0 = free slot */
-	struct page 		*page;
-
-	/*
-	 * We keep the vma's vm_start rather than a pointer to the vma
-	 * itself.  The probed process or a naughty kernel module could make
-	 * the vma go away, and we must handle that reasonably gracefully.
-	 */
-	unsigned long 		vaddr;		/* Page(s) of instruction slots */
-};
+struct xol_area;
 
 struct uprobes_state {
 	struct xol_area		*xol_area;
@@ -120,6 +101,7 @@ extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsign
 extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
+extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
 extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
@@ -131,7 +113,6 @@ extern void uprobe_end_dup_mmap(void);
 extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
 extern void uprobe_free_utask(struct task_struct *t);
 extern void uprobe_copy_process(struct task_struct *t, unsigned long flags);
-extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
 extern int uprobe_post_sstep_notifier(struct pt_regs *regs);
 extern int uprobe_pre_sstep_notifier(struct pt_regs *regs);
 extern void uprobe_notify_resume(struct pt_regs *regs);
@@ -187,10 +168,6 @@ static inline bool uprobe_deny_signal(void)
 {
 	return false;
 }
-static inline unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
-{
-	return 0;
-}
 static inline void uprobe_free_utask(struct task_struct *t)
 {
 }
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index acc0317..a7239eb 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -86,6 +86,25 @@ struct return_instance {
 };
 
 /*
+ * On a breakpoint hit, thread contests for a slot.  It frees the
+ * slot after singlestep. Currently a fixed number of slots are
+ * allocated.
+ */
+struct xol_area {
+	wait_queue_head_t 	wq;		/* if all slots are busy */
+	atomic_t 		slot_count;	/* number of in-use slots */
+	unsigned long 		*bitmap;	/* 0 = free slot */
+	struct page 		*page;
+
+	/*
+	 * We keep the vma's vm_start rather than a pointer to the vma
+	 * itself.  The probed process or a naughty kernel module could make
+	 * the vma go away, and we must handle that reasonably gracefully.
+	 */
+	unsigned long 		vaddr;		/* Page(s) of instruction slots */
+};
+
+/*
  * valid_vma: Verify if the specified vma is an executable vma
  * Relax restrictions while unregistering: vm_flags might have
  * changed after breakpoint was inserted.
-- 
1.5.5.1

Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-11-09 20:03:44]:

> 1. Don't include asm/uprobes.h unconditionally, we only need
>    it if CONFIG_UPROBES.
> 
> 2. Move the definition of "struct xol_area" into uprobes.c.
> 
>    Perhaps we should simply kill struct uprobes_state, it buys
>    nothing.
> 
> 3. Kill the dummy definition of uprobe_get_swbp_addr(), nobody
>    except handle_swbp() needs it.
> 
> 4. Purely cosmetic, but move the decl of uprobe_get_swbp_addr()
>    up, close to other __weak helpers.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> +++ b/kernel/events/uprobes.c
> @@ -86,6 +86,25 @@ struct return_instance {
>  };
>  
>  /*
> + * On a breakpoint hit, thread contests for a slot.  It frees the
> + * slot after singlestep. Currently a fixed number of slots are
> + * allocated.
> + */
> +struct xol_area {

So, my main complaint about the uprobes code isn't functional but 
documentational, similar to what I outlined a few days ago: what this 
comment does not explain is exactly what a 'XOL area' is.

You guys are changing code that reads like gobbledygook to people reading 
it for the first time. It's understandable that you want to use 
abbreviations and I don't object against that, but please explain key 
concepts and data structures when they first come up - a very good place 
to do that is in places where key structures are declared.

I didn't find any high level description of the XOL code, one which makes 
clear that how we manage these out of line execution areas:

 comet:~/tip> git grep -i 'out of line' $(find . -name '*uprobe*.[ch]')
 arch/powerpc/kernel/uprobes.c: * arch_uprobe_pre_xol - prepare to execute out of line.
 arch/x86/kernel/uprobes.c: * arch_uprobe_pre_xol - prepare to execute out of line.
 arch/x86/kernel/uprobes.c: * address pushed by a call instruction executed out of line.
 kernel/events/uprobes.c: * This area will be used for storing instructions for execution out of line.
 kernel/events/uprobes.c:/* Prepare to single-step probed instruction out of line. */

The one that comes closest is:

          * This area will be used for storing instructions for execution out of line.

... but that is a single sentence and deep inside the XOL code already.

Really, please make a better job of introducing other kernel hackers to 
the code you are writing ...

Maybe even split the XOL code out into kernel/events/uprobes_xol.c or so? 
That will give a natural place to explain yourselves at the beginning of 
the file.

Thanks,

	Ingo

Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area

[Oleg Nesterov]

On 11/11, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > +++ b/kernel/events/uprobes.c
> > @@ -86,6 +86,25 @@ struct return_instance {
> >  };
> >
> >  /*
> > + * On a breakpoint hit, thread contests for a slot.  It frees the
> > + * slot after singlestep. Currently a fixed number of slots are
> > + * allocated.
> > + */
> > +struct xol_area {
>
> So, my main complaint about the uprobes code isn't functional but
> documentational, similar to what I outlined a few days ago: what this
> comment does not explain is exactly what a 'XOL area' is.
>
> You guys are changing code that reads like gobbledygook to people reading
> it for the first time.

Not that I am trying to defense uprobes, but this is equally true for
any piece of kernel code, at least to me ;)

> It's understandable that you want to use
> abbreviations and I don't object against that, but please explain key
> concepts and data structures when they first come up

Well, this patch only move the definition with the comments, but:

> - a very good place
> to do that is in places where key structures are declared.
>
> I didn't find any high level description of the XOL code, one which makes
> clear that how we manage these out of line execution areas:

I have to agree, all these comments do not really help...

> The one that comes closest is:
>
>           * This area will be used for storing instructions for execution out of line.
>
> ... but that is a single sentence and deep inside the XOL code already.

and even this comment should be probably moved up to the "struct xol_area",


> Really, please make a better job of introducing other kernel hackers to
> the code you are writing ...
>
> Maybe even split the XOL code out into kernel/events/uprobes_xol.c or so?

I do not really think a separate uprobes_xol.c makes sense. I think it would
be nice to have the high-level "uprobes design" doc in uprobetracer.txt, or

> That will give a natural place to explain yourselves at the beginning of
> the file.

or even in the beginning of uprobes.c, I agree.

Don't get me wrong, I am not volunteering ;) But at least I'll try to
pay more attention to the comments when I change the code next time.

Oleg.

Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 11/11, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > +++ b/kernel/events/uprobes.c
> > > @@ -86,6 +86,25 @@ struct return_instance {
> > >  };
> > >
> > >  /*
> > > + * On a breakpoint hit, thread contests for a slot.  It frees the
> > > + * slot after singlestep. Currently a fixed number of slots are
> > > + * allocated.
> > > + */
> > > +struct xol_area {
> >
> > So, my main complaint about the uprobes code isn't functional but 
> > documentational, similar to what I outlined a few days ago: what this 
> > comment does not explain is exactly what a 'XOL area' is.
> >
> > You guys are changing code that reads like gobbledygook to people 
> > reading it for the first time.
> 
> Not that I am trying to defense uprobes, but this is equally true for 
> any piece of kernel code, at least to me ;)

I'm really not suggesting to do overly much - only for some minimal blurb 
like the scheduler has in most places:

/*
 * This is the main, per-CPU runqueue data structure.
 *
 * Locking rule: those places that want to lock multiple runqueues
 * (such as the load balancing or the thread migration code), lock
 * acquire operations must be ordered by ascending &runqueue.
 */
struct rq {
        /* runqueue lock: */
        raw_spinlock_t lock;

But the apparent assumption that the reader knows what 'XOL' means 
triggered my suggest :-)

> > Maybe even split the XOL code out into kernel/events/uprobes_xol.c or 
> > so?
> 
> I do not really think a separate uprobes_xol.c makes sense. [...]

Ok - it's your call really.

> [...] I think it would be nice to have the high-level "uprobes design" 
> doc in uprobetracer.txt, or

Even better if the best parts are integrated into the source code!

Thanks,

	Ingo

[PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol

[Oleg Nesterov]

On 11/11, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 11/11, Ingo Molnar wrote:
> > >
> > > You guys are changing code that reads like gobbledygook to people
> > > reading it for the first time.
> >
> > Not that I am trying to defense uprobes, but this is equally true for
> > any piece of kernel code, at least to me ;)
>
> I'm really not suggesting to do overly much - only for some minimal blurb
> like the scheduler has in most places:

OK. Let me try to make a first step to improve this a little bit...

How about the patch below? Srikar?


Subject: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol

Document xol_area and arch_uprobe.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 51a7f53..b886a5e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -73,6 +73,17 @@ struct uprobe {
 	struct inode		*inode;		/* Also hold a ref to inode */
 	loff_t			offset;
 	unsigned long		flags;
+
+	/*
+	 * The generic code assumes that it has two members of unknown type
+	 * owned by the arch-specific code:
+	 *
+	 * 	insn -	copy_insn() saves the original instruction here for
+	 *		arch_uprobe_analyze_insn().
+	 *
+	 *	ixol -	potentially modified instruction to execute out of
+	 *		line, copied to xol_area by xol_get_insn_slot().
+	 */
 	struct arch_uprobe	arch;
 };
 
@@ -86,6 +97,10 @@ struct return_instance {
 };
 
 /*
+ * Execute out of line area: anonymous executable mapping installed
+ * by the probed task to execute the copy of the original instruction
+ * mangled by set_swbp().
+ *
  * On a breakpoint hit, thread contests for a slot.  It frees the
  * slot after singlestep. Currently a fixed number of slots are
  * allocated.
-- 
1.5.5.1

Re: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 11/11, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > On 11/11, Ingo Molnar wrote:
> > > >
> > > > You guys are changing code that reads like gobbledygook to people
> > > > reading it for the first time.
> > >
> > > Not that I am trying to defense uprobes, but this is equally true for
> > > any piece of kernel code, at least to me ;)
> >
> > I'm really not suggesting to do overly much - only for some minimal blurb
> > like the scheduler has in most places:
> 
> OK. Let me try to make a first step to improve this a little bit...
> 
> How about the patch below? Srikar?
> 
> 
> Subject: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
> 
> Document xol_area and arch_uprobe.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 51a7f53..b886a5e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -73,6 +73,17 @@ struct uprobe {
>  	struct inode		*inode;		/* Also hold a ref to inode */
>  	loff_t			offset;
>  	unsigned long		flags;
> +
> +	/*
> +	 * The generic code assumes that it has two members of unknown type
> +	 * owned by the arch-specific code:
> +	 *
> +	 * 	insn -	copy_insn() saves the original instruction here for
> +	 *		arch_uprobe_analyze_insn().
> +	 *
> +	 *	ixol -	potentially modified instruction to execute out of
> +	 *		line, copied to xol_area by xol_get_insn_slot().
> +	 */
>  	struct arch_uprobe	arch;
>  };
>  
> @@ -86,6 +97,10 @@ struct return_instance {
>  };
>  
>  /*
> + * Execute out of line area: anonymous executable mapping installed
> + * by the probed task to execute the copy of the original instruction
> + * mangled by set_swbp().
> + *
>   * On a breakpoint hit, thread contests for a slot.  It frees the
>   * slot after singlestep. Currently a fixed number of slots are
>   * allocated.

Looks perfect to me!

Thanks,

	Ingo

Re: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol

[Srikar Dronamraju]

> 
> OK. Let me try to make a first step to improve this a little bit...
> 
> How about the patch below? Srikar?
> 
> 
> Subject: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
> 
> Document xol_area and arch_uprobe.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---

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

>  kernel/events/uprobes.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 51a7f53..b886a5e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -73,6 +73,17 @@ struct uprobe {
>  	struct inode		*inode;		/* Also hold a ref to inode */
>  	loff_t			offset;
>  	unsigned long		flags;
> +
> +	/*
> +	 * The generic code assumes that it has two members of unknown type
> +	 * owned by the arch-specific code:
> +	 *
> +	 * 	insn -	copy_insn() saves the original instruction here for
> +	 *		arch_uprobe_analyze_insn().
> +	 *
> +	 *	ixol -	potentially modified instruction to execute out of
> +	 *		line, copied to xol_area by xol_get_insn_slot().
> +	 */
>  	struct arch_uprobe	arch;
>  };
> 
> @@ -86,6 +97,10 @@ struct return_instance {
>  };
> 
>  /*
> + * Execute out of line area: anonymous executable mapping installed
> + * by the probed task to execute the copy of the original instruction
> + * mangled by set_swbp().
> + *
>   * On a breakpoint hit, thread contests for a slot.  It frees the
>   * slot after singlestep. Currently a fixed number of slots are
>   * allocated.
> -- 
> 1.5.5.1
> 
> 

-- 
Thanks and Regards
Srikar Dronamraju