[PATCH 0/7] uprobes: alloc utask/xol_area cleanups and minor fix

[PATCH 0/7] uprobes: alloc utask/xol_area cleanups and minor fix

[Oleg Nesterov]

Hello.

This series fixes the minor bug and cleanups the usage of add_utask()
and xol_alloc_area(). Plus it cleanups the initializaion of ->utask
in handle_swbp() paths.

Anton, this conflicts with your uretprobe patches, but I think we
should do this to avoid the code duplication. IOW, I consider this
series as a minor preparations for uretprobes as well.

Oleg.

 kernel/events/uprobes.c |  150 +++++++++++++++++++++--------------------------
 1 files changed, 68 insertions(+), 82 deletions(-)

[PATCH 1/7] uprobes: Move alloc_page() from xol_add_vma() to xol_alloc_area()

[Oleg Nesterov]

Move alloc_page() from xol_add_vma() to xol_alloc_area() to cleanup
the code. This separates the memory allocations and consolidates the
-EALREADY cleanups and the error handling.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f883813..1456f7d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1041,22 +1041,14 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
 /* Slot allocation for XOL */
 static int xol_add_vma(struct xol_area *area)
 {
-	struct mm_struct *mm;
-	int ret;
-
-	area->page = alloc_page(GFP_HIGHUSER);
-	if (!area->page)
-		return -ENOMEM;
-
-	ret = -EALREADY;
-	mm = current->mm;
+	struct mm_struct *mm = current->mm;
+	int ret = -EALREADY;
 
 	down_write(&mm->mmap_sem);
 	if (mm->uprobes_state.xol_area)
 		goto fail;
 
 	ret = -ENOMEM;
-
 	/* Try to map as high as possible, this is only a hint. */
 	area->vaddr = get_unmapped_area(NULL, TASK_SIZE - PAGE_SIZE, PAGE_SIZE, 0, 0);
 	if (area->vaddr & ~PAGE_MASK) {
@@ -1072,11 +1064,8 @@ static int xol_add_vma(struct xol_area *area)
 	smp_wmb();	/* pairs with get_xol_area() */
 	mm->uprobes_state.xol_area = area;
 	ret = 0;
-
-fail:
+ fail:
 	up_write(&mm->mmap_sem);
-	if (ret)
-		__free_page(area->page);
 
 	return ret;
 }
@@ -1104,21 +1093,26 @@ static struct xol_area *xol_alloc_area(void)
 
 	area = kzalloc(sizeof(*area), GFP_KERNEL);
 	if (unlikely(!area))
-		return NULL;
+		goto out;
 
 	area->bitmap = kzalloc(BITS_TO_LONGS(UINSNS_PER_PAGE) * sizeof(long), GFP_KERNEL);
-
 	if (!area->bitmap)
-		goto fail;
+		goto free_area;
+
+	area->page = area->page = alloc_page(GFP_HIGHUSER);
+	if (!area->page)
+		goto free_bitmap;
 
 	init_waitqueue_head(&area->wq);
 	if (!xol_add_vma(area))
 		return area;
 
-fail:
+	__free_page(area->page);
+ free_bitmap:
 	kfree(area->bitmap);
+ free_area:
 	kfree(area);
-
+ out:
 	return get_xol_area(current->mm);
 }
 
-- 
1.5.5.1

Re: [PATCH 1/7] uprobes: Move alloc_page() from xol_add_vma() to xol_alloc_area()

[Anton Arapov]

On Mon, Dec 31, 2012 at 06:52:12PM +0100, Oleg Nesterov wrote:
> Move alloc_page() from xol_add_vma() to xol_alloc_area() to cleanup
> the code. This separates the memory allocations and consolidates the
> -EALREADY cleanups and the error handling.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |   32 +++++++++++++-------------------
>  1 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index f883813..1456f7d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1104,21 +1093,26 @@ static struct xol_area *xol_alloc_area(void)
>  
>  	area = kzalloc(sizeof(*area), GFP_KERNEL);
>  	if (unlikely(!area))
> -		return NULL;
> +		goto out;
>  
>  	area->bitmap = kzalloc(BITS_TO_LONGS(UINSNS_PER_PAGE) * sizeof(long), GFP_KERNEL);
> -
>  	if (!area->bitmap)
> -		goto fail;
> +		goto free_area;
> +
> +	area->page = area->page = alloc_page(GFP_HIGHUSER);

fyi: didn't review this patch set yet, just caught the string above. ;)

Anton.

Re: [PATCH 1/7] uprobes: Move alloc_page() from xol_add_vma() to xol_alloc_area()

[Oleg Nesterov]

On 01/07, Anton Arapov wrote:
>
> On Mon, Dec 31, 2012 at 06:52:12PM +0100, Oleg Nesterov wrote:
> > -		goto fail;
> > +		goto free_area;
> > +
> > +	area->page = area->page = alloc_page(GFP_HIGHUSER);
>
> fyi: didn't review this patch set yet, just caught the string above. ;)

OOPS, thanks, will fix.

Oleg.

Re: [PATCH 1/7] uprobes: Move alloc_page() from xol_add_vma() to xol_alloc_area()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:12]:

> Move alloc_page() from xol_add_vma() to xol_alloc_area() to cleanup
> the code. This separates the memory allocations and consolidates the
> -EALREADY cleanups and the error handling.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

(One simple check below)

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


> ---
>  kernel/events/uprobes.c |   32 +++++++++++++-------------------
>  1 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index f883813..1456f7d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1041,22 +1041,14 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
>  /* Slot allocation for XOL */
>  static int xol_add_vma(struct xol_area *area)
>  {
> -	struct mm_struct *mm;
> -	int ret;
> -
> -	area->page = alloc_page(GFP_HIGHUSER);
> -	if (!area->page)
> -		return -ENOMEM;
> -
> -	ret = -EALREADY;
> -	mm = current->mm;
> +	struct mm_struct *mm = current->mm;
> +	int ret = -EALREADY;
> 
>  	down_write(&mm->mmap_sem);
>  	if (mm->uprobes_state.xol_area)
>  		goto fail;
> 
>  	ret = -ENOMEM;
> -
>  	/* Try to map as high as possible, this is only a hint. */
>  	area->vaddr = get_unmapped_area(NULL, TASK_SIZE - PAGE_SIZE, PAGE_SIZE, 0, 0);
>  	if (area->vaddr & ~PAGE_MASK) {
> @@ -1072,11 +1064,8 @@ static int xol_add_vma(struct xol_area *area)
>  	smp_wmb();	/* pairs with get_xol_area() */
>  	mm->uprobes_state.xol_area = area;
>  	ret = 0;
> -
> -fail:
> + fail:

Not sure if the space before label is intentional?
Its true of other labels below also.

>  	up_write(&mm->mmap_sem);
> -	if (ret)
> -		__free_page(area->page);
> 
>  	return ret;
>  }
> @@ -1104,21 +1093,26 @@ static struct xol_area *xol_alloc_area(void)
> 
>  	area = kzalloc(sizeof(*area), GFP_KERNEL);
>  	if (unlikely(!area))
> -		return NULL;
> +		goto out;
> 
>  	area->bitmap = kzalloc(BITS_TO_LONGS(UINSNS_PER_PAGE) * sizeof(long), GFP_KERNEL);
> -
>  	if (!area->bitmap)
> -		goto fail;
> +		goto free_area;
> +
> +	area->page = area->page = alloc_page(GFP_HIGHUSER);
> +	if (!area->page)
> +		goto free_bitmap;
> 
>  	init_waitqueue_head(&area->wq);
>  	if (!xol_add_vma(area))
>  		return area;
> 
> -fail:
> +	__free_page(area->page);
> + free_bitmap:
>  	kfree(area->bitmap);
> + free_area:
>  	kfree(area);
> -
> + out:
>  	return get_xol_area(current->mm);
>  }
> 
> -- 
> 1.5.5.1
> 

Re: [PATCH 1/7] uprobes: Move alloc_page() from xol_add_vma() to xol_alloc_area()

[Oleg Nesterov]

On 01/08, Srikar Dronamraju wrote:
>
> (One simple check below)
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks,

> > @@ -1072,11 +1064,8 @@ static int xol_add_vma(struct xol_area *area)
> >  	smp_wmb();	/* pairs with get_xol_area() */
> >  	mm->uprobes_state.xol_area = area;
> >  	ret = 0;
> > -
> > -fail:
> > + fail:
>
> Not sure if the space before label is intentional?
> Its true of other labels below also.

Yes, this was intentional although almost subconscious.

Personally I prefer to add a space before the label, this helps
/usr/bin/diff to not confuse this label with the function name.

But I can stop this if you dislike.

Oleg.

Re: [PATCH 1/7] uprobes: Move alloc_page() from xol_add_vma() to xol_alloc_area()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-01-08 18:58:22]:

> On 01/08, Srikar Dronamraju wrote:
> >
> > (One simple check below)
> >
> > Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> Thanks,
> 
> > > @@ -1072,11 +1064,8 @@ static int xol_add_vma(struct xol_area *area)
> > >  	smp_wmb();	/* pairs with get_xol_area() */
> > >  	mm->uprobes_state.xol_area = area;
> > >  	ret = 0;
> > > -
> > > -fail:
> > > + fail:
> >
> > Not sure if the space before label is intentional?
> > Its true of other labels below also.
> 
> Yes, this was intentional although almost subconscious.
> 
> Personally I prefer to add a space before the label, this helps
> /usr/bin/diff to not confuse this label with the function name.
> 
> But I can stop this if you dislike.

I dont have any preference. Just wanted to check if it was intentional
or accidental. I thought checkpatch.pl cribs for this.
I think somebody while reviewing my patches had pointed out the same to me.
I cant remember who and probably my format wasnt just a single space
before label.

> 
> Oleg.
> 

[PATCH 2/7] uprobes: Fold xol_alloc_area() into get_xol_area()

[Oleg Nesterov]

Currently only xol_get_insn_slot() does get_xol_area() + xol_alloc_area(),
but this will have more users and we do not want to copy-and-paste this
code. This patch simply moves xol_alloc_area() into get_xol_area() to
simplify the current and future code.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 1456f7d..ef81162 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1070,27 +1070,21 @@ static int xol_add_vma(struct xol_area *area)
 	return ret;
 }
 
-static struct xol_area *get_xol_area(struct mm_struct *mm)
-{
-	struct xol_area *area;
-
-	area = mm->uprobes_state.xol_area;
-	smp_read_barrier_depends();	/* pairs with wmb in xol_add_vma() */
-
-	return area;
-}
-
 /*
- * xol_alloc_area - Allocate process's xol_area.
- * This area will be used for storing instructions for execution out of
- * line.
+ * get_alloc_area - Allocate process's xol_area if necessary.
+ * This area will be used for storing instructions for execution out of line.
  *
  * Returns the allocated area or NULL.
  */
-static struct xol_area *xol_alloc_area(void)
+static struct xol_area *get_xol_area(void)
 {
+	struct mm_struct *mm = current->mm;
 	struct xol_area *area;
 
+	area = mm->uprobes_state.xol_area;
+	if (area)
+		goto ret;
+
 	area = kzalloc(sizeof(*area), GFP_KERNEL);
 	if (unlikely(!area))
 		goto out;
@@ -1113,7 +1107,10 @@ static struct xol_area *xol_alloc_area(void)
  free_area:
 	kfree(area);
  out:
-	return get_xol_area(current->mm);
+	area = mm->uprobes_state.xol_area;
+ ret:
+	smp_read_barrier_depends();     /* pairs with wmb in xol_add_vma() */
+	return area;
 }
 
 /*
@@ -1189,14 +1186,11 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
 	unsigned long offset;
 	void *vaddr;
 
-	area = get_xol_area(current->mm);
-	if (!area) {
-		area = xol_alloc_area();
-		if (!area)
-			return 0;
-	}
-	current->utask->xol_vaddr = xol_take_insn_slot(area);
+	area = get_xol_area();
+	if (!area)
+		return 0;
 
+	current->utask->xol_vaddr = xol_take_insn_slot(area);
 	/*
 	 * Initialize the slot if xol_vaddr points to valid
 	 * instruction slot.
-- 
1.5.5.1

Re: [PATCH 2/7] uprobes: Fold xol_alloc_area() into get_xol_area()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:16]:

> Currently only xol_get_insn_slot() does get_xol_area() + xol_alloc_area(),
> but this will have more users and we do not want to copy-and-paste this
> code. This patch simply moves xol_alloc_area() into get_xol_area() to
> simplify the current and future code.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |   38 ++++++++++++++++----------------------
>  1 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 1456f7d..ef81162 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1070,27 +1070,21 @@ static int xol_add_vma(struct xol_area *area)
>  	return ret;
>  }
> 
> -static struct xol_area *get_xol_area(struct mm_struct *mm)
> -{
> -	struct xol_area *area;
> -
> -	area = mm->uprobes_state.xol_area;
> -	smp_read_barrier_depends();	/* pairs with wmb in xol_add_vma() */
> -
> -	return area;
> -}
> -
>  /*
> - * xol_alloc_area - Allocate process's xol_area.
> - * This area will be used for storing instructions for execution out of
> - * line.
> + * get_alloc_area - Allocate process's xol_area if necessary.
> + * This area will be used for storing instructions for execution out of line.
>   *
>   * Returns the allocated area or NULL.
>   */
> -static struct xol_area *xol_alloc_area(void)
> +static struct xol_area *get_xol_area(void)
>  {
> +	struct mm_struct *mm = current->mm;
>  	struct xol_area *area;
> 
> +	area = mm->uprobes_state.xol_area;
> +	if (area)
> +		goto ret;
> +
>  	area = kzalloc(sizeof(*area), GFP_KERNEL);
>  	if (unlikely(!area))
>  		goto out;
> @@ -1113,7 +1107,10 @@ static struct xol_area *xol_alloc_area(void)
>   free_area:
>  	kfree(area);
>   out:
> -	return get_xol_area(current->mm);
> +	area = mm->uprobes_state.xol_area;
> + ret:
> +	smp_read_barrier_depends();     /* pairs with wmb in xol_add_vma() */
> +	return area;
>  }
> 
>  /*
> @@ -1189,14 +1186,11 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
>  	unsigned long offset;
>  	void *vaddr;
> 
> -	area = get_xol_area(current->mm);
> -	if (!area) {
> -		area = xol_alloc_area();
> -		if (!area)
> -			return 0;
> -	}
> -	current->utask->xol_vaddr = xol_take_insn_slot(area);
> +	area = get_xol_area();
> +	if (!area)
> +		return 0;
> 
> +	current->utask->xol_vaddr = xol_take_insn_slot(area);
>  	/*
>  	 * Initialize the slot if xol_vaddr points to valid
>  	 * instruction slot.
> -- 
> 1.5.5.1
> 

Re: [PATCH 2/7] uprobes: Fold xol_alloc_area() into get_xol_area()

[Anton Arapov]

On Mon, Dec 31, 2012 at 06:52:16PM +0100, Oleg Nesterov wrote:
> Currently only xol_get_insn_slot() does get_xol_area() + xol_alloc_area(),
> but this will have more users and we do not want to copy-and-paste this
> code. This patch simply moves xol_alloc_area() into get_xol_area() to
> simplify the current and future code.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |   38 ++++++++++++++++----------------------
>  1 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 1456f7d..ef81162 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1070,27 +1070,21 @@ static int xol_add_vma(struct xol_area *area)
>  	return ret;
>  }
>  
> -static struct xol_area *get_xol_area(struct mm_struct *mm)
> -{
> -	struct xol_area *area;
> -
> -	area = mm->uprobes_state.xol_area;
> -	smp_read_barrier_depends();	/* pairs with wmb in xol_add_vma() */
> -
> -	return area;
> -}
> -
>  /*
> - * xol_alloc_area - Allocate process's xol_area.
> - * This area will be used for storing instructions for execution out of
> - * line.
> + * get_alloc_area - Allocate process's xol_area if necessary.
> + * This area will be used for storing instructions for execution out of line.
>   *
>   * Returns the allocated area or NULL.
>   */

+ * get_xol_area ....

Anton.

> -static struct xol_area *xol_alloc_area(void)
> +static struct xol_area *get_xol_area(void)
>  {
> +	struct mm_struct *mm = current->mm;
>  	struct xol_area *area;
>  
> +	area = mm->uprobes_state.xol_area;
> +	if (area)
> +		goto ret;
> +
>  	area = kzalloc(sizeof(*area), GFP_KERNEL);
>  	if (unlikely(!area))
>  		goto out;
> @@ -1113,7 +1107,10 @@ static struct xol_area *xol_alloc_area(void)
>   free_area:
>  	kfree(area);
>   out:
> -	return get_xol_area(current->mm);
> +	area = mm->uprobes_state.xol_area;
> + ret:
> +	smp_read_barrier_depends();     /* pairs with wmb in xol_add_vma() */
> +	return area;
>  }
>  
>  /*
> @@ -1189,14 +1186,11 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
>  	unsigned long offset;
>  	void *vaddr;
>  
> -	area = get_xol_area(current->mm);
> -	if (!area) {
> -		area = xol_alloc_area();
> -		if (!area)
> -			return 0;
> -	}
> -	current->utask->xol_vaddr = xol_take_insn_slot(area);
> +	area = get_xol_area();
> +	if (!area)
> +		return 0;
>  
> +	current->utask->xol_vaddr = xol_take_insn_slot(area);
>  	/*
>  	 * Initialize the slot if xol_vaddr points to valid
>  	 * instruction slot.
> -- 
> 1.5.5.1
> 

Re: [PATCH 2/7] uprobes: Fold xol_alloc_area() into get_xol_area()

[Oleg Nesterov]

On 01/09, Anton Arapov wrote:
>
> On Mon, Dec 31, 2012 at 06:52:16PM +0100, Oleg Nesterov wrote:
> > + * get_alloc_area - Allocate process's xol_area if necessary.
> > + * This area will be used for storing instructions for execution out of line.
> >   *
> >   * Returns the allocated area or NULL.
> >   */
>
> + * get_xol_area ....

Ah, comment, yes. Thanks, fixed.

Oleg.

[PATCH 3/7] uprobes: Turn add_utask() into get_utask()

[Oleg Nesterov]

Rename add_utask() into get_utask() and change it to allocate on
demand to simplify the caller. Like get_xol_area() it will have
more users.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ef81162..b09b3ba 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1290,23 +1290,18 @@ void uprobe_copy_process(struct task_struct *t)
 }
 
 /*
- * Allocate a uprobe_task object for the task.
- * Called when the thread hits a breakpoint for the first time.
+ * Allocate a uprobe_task object for the task if if necessary.
+ * Called when the thread hits a breakpoint.
  *
  * Returns:
  * - pointer to new uprobe_task on success
  * - NULL otherwise
  */
-static struct uprobe_task *add_utask(void)
+static struct uprobe_task *get_utask(void)
 {
-	struct uprobe_task *utask;
-
-	utask = kzalloc(sizeof *utask, GFP_KERNEL);
-	if (unlikely(!utask))
-		return NULL;
-
-	current->utask = utask;
-	return utask;
+	if (!current->utask)
+		current->utask = kzalloc(sizeof(struct uprobe_task), GFP_KERNEL);
+	return current->utask;
 }
 
 /* Prepare to single-step probed instruction out of line. */
@@ -1505,13 +1500,9 @@ static void handle_swbp(struct pt_regs *regs)
 	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
 		goto out;
 
-	utask = current->utask;
-	if (!utask) {
-		utask = add_utask();
-		/* Cannot allocate; re-execute the instruction. */
-		if (!utask)
-			goto out;
-	}
+	utask = get_utask();
+	if (!utask)
+		goto out;	/* re-execute the instruction. */
 
 	handler_chain(uprobe, regs);
 	if (can_skip_sstep(uprobe, regs))
-- 
1.5.5.1

Re: [PATCH 3/7] uprobes: Turn add_utask() into get_utask()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:20]:

> Rename add_utask() into get_utask() and change it to allocate on
> demand to simplify the caller. Like get_xol_area() it will have
> more users.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |   27 +++++++++------------------
>  1 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index ef81162..b09b3ba 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1290,23 +1290,18 @@ void uprobe_copy_process(struct task_struct *t)
>  }
> 
>  /*
> - * Allocate a uprobe_task object for the task.
> - * Called when the thread hits a breakpoint for the first time.
> + * Allocate a uprobe_task object for the task if if necessary.
> + * Called when the thread hits a breakpoint.
>   *
>   * Returns:
>   * - pointer to new uprobe_task on success
>   * - NULL otherwise
>   */
> -static struct uprobe_task *add_utask(void)
> +static struct uprobe_task *get_utask(void)
>  {
> -	struct uprobe_task *utask;
> -
> -	utask = kzalloc(sizeof *utask, GFP_KERNEL);
> -	if (unlikely(!utask))
> -		return NULL;
> -
> -	current->utask = utask;
> -	return utask;
> +	if (!current->utask)
> +		current->utask = kzalloc(sizeof(struct uprobe_task), GFP_KERNEL);
> +	return current->utask;
>  }
> 
>  /* Prepare to single-step probed instruction out of line. */
> @@ -1505,13 +1500,9 @@ static void handle_swbp(struct pt_regs *regs)
>  	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
>  		goto out;
> 
> -	utask = current->utask;
> -	if (!utask) {
> -		utask = add_utask();
> -		/* Cannot allocate; re-execute the instruction. */
> -		if (!utask)
> -			goto out;
> -	}
> +	utask = get_utask();
> +	if (!utask)
> +		goto out;	/* re-execute the instruction. */
> 
>  	handler_chain(uprobe, regs);
>  	if (can_skip_sstep(uprobe, regs))
> -- 
> 1.5.5.1
> 

[PATCH 4/7] uprobes: Do not play with utask in xol_get_insn_slot()

[Oleg Nesterov]

pre_ssout()->xol_get_insn_slot() path is confusing and buggy. This patch
cleanups the code, the next one fixes the bug.

Change xol_get_insn_slot() to only allocate the slot and do nothing more,
move the initialization of utask->xol_vaddr/vaddr into pre_ssout().

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b09b3ba..2ed6239 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1176,30 +1176,26 @@ static unsigned long xol_take_insn_slot(struct xol_area *area)
 }
 
 /*
- * xol_get_insn_slot - If was not allocated a slot, then
- * allocate a slot.
+ * xol_get_insn_slot - allocate a slot for xol.
  * Returns the allocated slot address or 0.
  */
-static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot_addr)
+static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
 {
 	struct xol_area *area;
 	unsigned long offset;
+	unsigned long xol_vaddr;
 	void *vaddr;
 
 	area = get_xol_area();
 	if (!area)
 		return 0;
 
-	current->utask->xol_vaddr = xol_take_insn_slot(area);
-	/*
-	 * Initialize the slot if xol_vaddr points to valid
-	 * instruction slot.
-	 */
-	if (unlikely(!current->utask->xol_vaddr))
+	xol_vaddr = xol_take_insn_slot(area);
+	if (unlikely(!xol_vaddr))
 		return 0;
 
-	current->utask->vaddr = slot_addr;
-	offset = current->utask->xol_vaddr & ~PAGE_MASK;
+	/* Initialize the slot */
+	offset = xol_vaddr & ~PAGE_MASK;
 	vaddr = kmap_atomic(area->page);
 	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
 	kunmap_atomic(vaddr);
@@ -1209,7 +1205,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
 	 */
 	flush_dcache_page(area->page);
 
-	return current->utask->xol_vaddr;
+	return xol_vaddr;
 }
 
 /*
@@ -1306,12 +1302,21 @@ static struct uprobe_task *get_utask(void)
 
 /* Prepare to single-step probed instruction out of line. */
 static int
-pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long vaddr)
+pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
 {
-	if (xol_get_insn_slot(uprobe, vaddr) && !arch_uprobe_pre_xol(&uprobe->arch, regs))
-		return 0;
+	struct uprobe_task *utask;
+	unsigned long xol_vaddr;
+
+	utask = current->utask;
+
+	xol_vaddr = xol_get_insn_slot(uprobe);
+	if (!xol_vaddr)
+		return -ENOMEM;
+
+	utask->xol_vaddr = xol_vaddr;
+	utask->vaddr = bp_vaddr;
 
-	return -EFAULT;
+	return arch_uprobe_pre_xol(&uprobe->arch, regs);
 }
 
 /*
-- 
1.5.5.1

Re: [PATCH 4/7] uprobes: Do not play with utask in xol_get_insn_slot()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:23]:

> pre_ssout()->xol_get_insn_slot() path is confusing and buggy. This patch
> cleanups the code, the next one fixes the bug.
> 
> Change xol_get_insn_slot() to only allocate the slot and do nothing more,
> move the initialization of utask->xol_vaddr/vaddr into pre_ssout().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |   37 +++++++++++++++++++++----------------
>  1 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b09b3ba..2ed6239 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1176,30 +1176,26 @@ static unsigned long xol_take_insn_slot(struct xol_area *area)
>  }
> 
>  /*
> - * xol_get_insn_slot - If was not allocated a slot, then
> - * allocate a slot.
> + * xol_get_insn_slot - allocate a slot for xol.
>   * Returns the allocated slot address or 0.
>   */
> -static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot_addr)
> +static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
>  {
>  	struct xol_area *area;
>  	unsigned long offset;
> +	unsigned long xol_vaddr;
>  	void *vaddr;
> 
>  	area = get_xol_area();
>  	if (!area)
>  		return 0;
> 
> -	current->utask->xol_vaddr = xol_take_insn_slot(area);
> -	/*
> -	 * Initialize the slot if xol_vaddr points to valid
> -	 * instruction slot.
> -	 */
> -	if (unlikely(!current->utask->xol_vaddr))
> +	xol_vaddr = xol_take_insn_slot(area);
> +	if (unlikely(!xol_vaddr))
>  		return 0;
> 
> -	current->utask->vaddr = slot_addr;
> -	offset = current->utask->xol_vaddr & ~PAGE_MASK;
> +	/* Initialize the slot */
> +	offset = xol_vaddr & ~PAGE_MASK;
>  	vaddr = kmap_atomic(area->page);
>  	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
>  	kunmap_atomic(vaddr);
> @@ -1209,7 +1205,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
>  	 */
>  	flush_dcache_page(area->page);
> 
> -	return current->utask->xol_vaddr;
> +	return xol_vaddr;
>  }
> 
>  /*
> @@ -1306,12 +1302,21 @@ static struct uprobe_task *get_utask(void)
> 
>  /* Prepare to single-step probed instruction out of line. */
>  static int
> -pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long vaddr)
> +pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
>  {
> -	if (xol_get_insn_slot(uprobe, vaddr) && !arch_uprobe_pre_xol(&uprobe->arch, regs))
> -		return 0;
> +	struct uprobe_task *utask;
> +	unsigned long xol_vaddr;
> +
> +	utask = current->utask;
> +
> +	xol_vaddr = xol_get_insn_slot(uprobe);
> +	if (!xol_vaddr)
> +		return -ENOMEM;
> +
> +	utask->xol_vaddr = xol_vaddr;
> +	utask->vaddr = bp_vaddr;
> 
> -	return -EFAULT;
> +	return arch_uprobe_pre_xol(&uprobe->arch, regs);
>  }
> 
>  /*
> -- 
> 1.5.5.1
> 

[PATCH 5/7] uprobes: Fix utask->xol_vaddr leak in pre_ssout()

[Oleg Nesterov]

pre_ssout() should do xol_free_insn_slot() if arch_uprobe_pre_xol()
fails, otherwise nobody will free the allocated slot.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2ed6239..bd94d2c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1306,6 +1306,7 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
 {
 	struct uprobe_task *utask;
 	unsigned long xol_vaddr;
+	int err;
 
 	utask = current->utask;
 
@@ -1316,7 +1317,13 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
 	utask->xol_vaddr = xol_vaddr;
 	utask->vaddr = bp_vaddr;
 
-	return arch_uprobe_pre_xol(&uprobe->arch, regs);
+	err = arch_uprobe_pre_xol(&uprobe->arch, regs);
+	if (unlikely(err)) {
+		xol_free_insn_slot(current);
+		return err;
+	}
+
+	return 0;
 }
 
 /*
-- 
1.5.5.1

Re: [PATCH 5/7] uprobes: Fix utask->xol_vaddr leak in pre_ssout()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:26]:

> pre_ssout() should do xol_free_insn_slot() if arch_uprobe_pre_xol()
> fails, otherwise nobody will free the allocated slot.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

(one nit below)
> ---
>  kernel/events/uprobes.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 2ed6239..bd94d2c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1306,6 +1306,7 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
>  {
>  	struct uprobe_task *utask;
>  	unsigned long xol_vaddr;
> +	int err;
> 
>  	utask = current->utask;
> 
> @@ -1316,7 +1317,13 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
>  	utask->xol_vaddr = xol_vaddr;
>  	utask->vaddr = bp_vaddr;
> 
> -	return arch_uprobe_pre_xol(&uprobe->arch, regs);
> +	err = arch_uprobe_pre_xol(&uprobe->arch, regs);
> +	if (unlikely(err)) {
> +		xol_free_insn_slot(current);
> +		return err;
> +	}
> +
> +	return 0;
>  }

Nit: we could reduce a line or two with

	err = arch_uprobe_pre_xol(&uprobe->arch, regs);
	if (unlikely(err))
		xol_free_insn_slot(current);

	return err;
> 
>  /*
> -- 
> 1.5.5.1
> 

Re: [PATCH 5/7] uprobes: Fix utask->xol_vaddr leak in pre_ssout()

[Oleg Nesterov]

On 01/08, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:26]:
>
> > pre_ssout() should do xol_free_insn_slot() if arch_uprobe_pre_xol()
> > fails, otherwise nobody will free the allocated slot.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks!

> (one nit below)
> ...
> > @@ -1316,7 +1317,13 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
> >  	utask->xol_vaddr = xol_vaddr;
> >  	utask->vaddr = bp_vaddr;
> >
> > -	return arch_uprobe_pre_xol(&uprobe->arch, regs);
> > +	err = arch_uprobe_pre_xol(&uprobe->arch, regs);
> > +	if (unlikely(err)) {
> > +		xol_free_insn_slot(current);
> > +		return err;
> > +	}
> > +
> > +	return 0;
> >  }
>
> Nit: we could reduce a line or two with
>
> 	err = arch_uprobe_pre_xol(&uprobe->arch, regs);
> 	if (unlikely(err))
> 		xol_free_insn_slot(current);
>
> 	return err;

Yes, but this is also preparation for the next patch which adds more
code after arch_uprobe_pre_xol() == 0.

Oleg.

Re: [PATCH 5/7] uprobes: Fix utask->xol_vaddr leak in pre_ssout()

[Srikar Dronamraju]

> > >  	utask->vaddr = bp_vaddr;
> > >
> > > -	return arch_uprobe_pre_xol(&uprobe->arch, regs);
> > > +	err = arch_uprobe_pre_xol(&uprobe->arch, regs);
> > > +	if (unlikely(err)) {
> > > +		xol_free_insn_slot(current);
> > > +		return err;
> > > +	}
> > > +
> > > +	return 0;
> > >  }
> >
> > Nit: we could reduce a line or two with
> >
> > 	err = arch_uprobe_pre_xol(&uprobe->arch, regs);
> > 	if (unlikely(err))
> > 		xol_free_insn_slot(current);
> >
> > 	return err;
> 
> Yes, but this is also preparation for the next patch which adds more
> code after arch_uprobe_pre_xol() == 0.
> 

Agree

-- 
Thanks and Regards
Srikar

[PATCH 6/7] uprobes: Do not allocate current->utask unnecessary

[Oleg Nesterov]

handle_swbp() does get_utask() before can_skip_sstep() for no reason,
we do not need ->utask if can_skip_sstep() succeeds.

Move get_utask() to pre_ssout() who actually starts to use it. Move
the initialization of utask->active_uprobe/state as well. This way
the whole initialization is consolidated in pre_ssout().

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index bd94d2c..ad1245d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1308,7 +1308,9 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
 	unsigned long xol_vaddr;
 	int err;
 
-	utask = current->utask;
+	utask = get_utask();
+	if (!utask)
+		return -ENOMEM;
 
 	xol_vaddr = xol_get_insn_slot(uprobe);
 	if (!xol_vaddr)
@@ -1323,6 +1325,8 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
 		return err;
 	}
 
+	utask->active_uprobe = uprobe;
+	utask->state = UTASK_SSTEP;
 	return 0;
 }
 
@@ -1474,7 +1478,6 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
  */
 static void handle_swbp(struct pt_regs *regs)
 {
-	struct uprobe_task *utask;
 	struct uprobe *uprobe;
 	unsigned long bp_vaddr;
 	int uninitialized_var(is_swbp);
@@ -1512,19 +1515,12 @@ static void handle_swbp(struct pt_regs *regs)
 	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
 		goto out;
 
-	utask = get_utask();
-	if (!utask)
-		goto out;	/* re-execute the instruction. */
-
 	handler_chain(uprobe, regs);
 	if (can_skip_sstep(uprobe, regs))
 		goto out;
 
-	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
-		utask->active_uprobe = uprobe;
-		utask->state = UTASK_SSTEP;
+	if (!pre_ssout(uprobe, regs, bp_vaddr))
 		return;
-	}
 
 	/* can_skip_sstep() succeeded, or restart if can't singlestep */
 out:
-- 
1.5.5.1

Re: [PATCH 6/7] uprobes: Do not allocate current->utask unnecessary

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:29]:

> handle_swbp() does get_utask() before can_skip_sstep() for no reason,
> we do not need ->utask if can_skip_sstep() succeeds.
> 
> Move get_utask() to pre_ssout() who actually starts to use it. Move
> the initialization of utask->active_uprobe/state as well. This way
> the whole initialization is consolidated in pre_ssout().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |   16 ++++++----------
>  1 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index bd94d2c..ad1245d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1308,7 +1308,9 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
>  	unsigned long xol_vaddr;
>  	int err;
> 
> -	utask = current->utask;
> +	utask = get_utask();
> +	if (!utask)
> +		return -ENOMEM;
> 
>  	xol_vaddr = xol_get_insn_slot(uprobe);
>  	if (!xol_vaddr)
> @@ -1323,6 +1325,8 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
>  		return err;
>  	}
> 
> +	utask->active_uprobe = uprobe;
> +	utask->state = UTASK_SSTEP;
>  	return 0;
>  }
> 
> @@ -1474,7 +1478,6 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>   */
>  static void handle_swbp(struct pt_regs *regs)
>  {
> -	struct uprobe_task *utask;
>  	struct uprobe *uprobe;
>  	unsigned long bp_vaddr;
>  	int uninitialized_var(is_swbp);
> @@ -1512,19 +1515,12 @@ static void handle_swbp(struct pt_regs *regs)
>  	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
>  		goto out;
> 
> -	utask = get_utask();
> -	if (!utask)
> -		goto out;	/* re-execute the instruction. */
> -


If get_utask fails with the above change, Dont we end up calling
handler_chain twice(or more)?. I think this is probably true with
previous patch too.

>  	handler_chain(uprobe, regs);
>  	if (can_skip_sstep(uprobe, regs))
>  		goto out;
> 
> -	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
> -		utask->active_uprobe = uprobe;
> -		utask->state = UTASK_SSTEP;
> +	if (!pre_ssout(uprobe, regs, bp_vaddr))
>  		return;
> -	}
> 
>  	/* can_skip_sstep() succeeded, or restart if can't singlestep */
>  out:
> -- 
> 1.5.5.1
> 

Re: [PATCH 6/7] uprobes: Do not allocate current->utask unnecessary

[Oleg Nesterov]

On 01/08, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:29]:
>
> >  static void handle_swbp(struct pt_regs *regs)
> >  {
> > -	struct uprobe_task *utask;
> >  	struct uprobe *uprobe;
> >  	unsigned long bp_vaddr;
> >  	int uninitialized_var(is_swbp);
> > @@ -1512,19 +1515,12 @@ static void handle_swbp(struct pt_regs *regs)
> >  	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
> >  		goto out;
> >
> > -	utask = get_utask();
> > -	if (!utask)
> > -		goto out;	/* re-execute the instruction. */
> > -
>
> If get_utask fails with the above change, Dont we end up calling
> handler_chain twice(or more)?.

After restart, yes.

> I think this is probably true with
> previous patch too.

And this can happen with the current code too, if xol_alloc_area()
fails. So I think this is probably fine. Besides, if GFP_KERNEL
fails the task should be oom-killed in practice.

Oleg.

[PATCH 7/7] uprobes: Kill the bogus IS_ERR_VALUE(xol_vaddr) check

[Oleg Nesterov]

utask->xol_vaddr is either zero or valid, remove the bogus
IS_ERR_VALUE() check in xol_free_insn_slot().

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ad1245d..ed4fcbe 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1223,8 +1223,7 @@ static void xol_free_insn_slot(struct task_struct *tsk)
 		return;
 
 	slot_addr = tsk->utask->xol_vaddr;
-
-	if (unlikely(!slot_addr || IS_ERR_VALUE(slot_addr)))
+	if (unlikely(!slot_addr))
 		return;
 
 	area = tsk->mm->uprobes_state.xol_area;
-- 
1.5.5.1

Re: [PATCH 7/7] uprobes: Kill the bogus IS_ERR_VALUE(xol_vaddr) check

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-31 18:52:32]:

> utask->xol_vaddr is either zero or valid, remove the bogus
> IS_ERR_VALUE() check in xol_free_insn_slot().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index ad1245d..ed4fcbe 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1223,8 +1223,7 @@ static void xol_free_insn_slot(struct task_struct *tsk)
>  		return;
> 
>  	slot_addr = tsk->utask->xol_vaddr;
> -
> -	if (unlikely(!slot_addr || IS_ERR_VALUE(slot_addr)))
> +	if (unlikely(!slot_addr))
>  		return;
> 
>  	area = tsk->mm->uprobes_state.xol_area;
> -- 
> 1.5.5.1
> 

Re: [PATCH 0/7] uprobes: alloc utask/xol_area cleanups and minor fix

[Anton Arapov]

On Mon, Dec 31, 2012 at 06:51:50PM +0100, Oleg Nesterov wrote:
> Hello.
> 
> This series fixes the minor bug and cleanups the usage of add_utask()
> and xol_alloc_area(). Plus it cleanups the initializaion of ->utask
> in handle_swbp() paths.
> 
> Anton, this conflicts with your uretprobe patches, but I think we
> should do this to avoid the code duplication. IOW, I consider this
> series as a minor preparations for uretprobes as well.
> 
> Oleg.
> 
>  kernel/events/uprobes.c |  150 +++++++++++++++++++++--------------------------
>  1 files changed, 68 insertions(+), 82 deletions(-)
>

I haven't found anything other than small style problems, I've pointed
to in other mails to this thread.

And this patchset doesn't brake anything on my machine. :)

Acked-by: Anton Arapov <anton@redhat.com>

Anton.