uprobes: alloc utask/xol_area cleanups and minor fix

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

---
 kernel/events/uprobes.c |   32 +++++++++++++-------------------
 1 files changed, 13 insertions(+), 19 deletions(-)

kernel/events/uprobes.c +0 -0

index f883813..1456f7d 100644
@@ -1104,21 +1093,26 @@ static struct xol_area *xol_alloc_area(void)
 
 	area = kzalloc(sizeof(*area), GFP_KERNEL);
 	if (unlikely(!area))

kernel/events/uprobes.c static struct xol_area *xol_alloc_area(void) · line 1102

Quoted 4 lines · Oleg Nesterov

kernel/events/uprobes.c +3 -1

-		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. ;)

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

---
 kernel/events/uprobes.c |   32 +++++++++++++-------------------
 1 files changed, 13 insertions(+), 19 deletions(-)

kernel/events/uprobes.c +2 -10

index f883813..1456f7d 100644
@@ -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) {

kernel/events/uprobes.c +10 -7

 	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);
 }
 

Signature · 1 line

1.5.5.1

(One simple check below)

Acked-by Srikar Dronamraju <[email protected]>

kernel/events/uprobes.c static int xol_add_vma(struct xol_area *area) · line 1067

Quoted 7 lines

kernel/events/uprobes.c +1 -2

@@ -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.

Quoted 4 lines · Srikar Dronamraju

(One simple check below)

Acked-by Srikar Dronamraju <[email protected]>

Thanks,

Quoted 10 lines

kernel/events/uprobes.c static int xol_add_vma(struct xol_area *area) · line 1067

Quoted 7 lines

kernel/events/uprobes.c +1 -2

@@ -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.

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

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

kernel/events/uprobes.c +16 -22

index 1456f7d..ef81162 100644
@@ -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.

Signature · 1 line

1.5.5.1

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

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

kernel/events/uprobes.c +0 -9

index 1456f7d..ef81162 100644
@@ -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;
-}

kernel/events/uprobes.c +14 -9

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

Signature · 1 line

1.5.5.1

Quoted 5 lines · Oleg Nesterov

+ * 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 ....

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

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

kernel/events/uprobes.c +9 -18

index ef81162..b09b3ba 100644
@@ -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))

Signature · 1 line

1.5.5.1

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

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

kernel/events/uprobes.c +21 -16

index b09b3ba..2ed6239 100644
@@ -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);
 }
 
 /*

Signature · 1 line

1.5.5.1

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

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

kernel/events/uprobes.c +1 -0

index 2ed6239..bd94d2c 100644
@@ -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;

kernel/events/uprobes.c +0 -0

 /*

Signature · 1 line

1.5.5.1

Quoted 4 lines · 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 <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

(one nit below)
...

kernel/events/uprobes.c pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr) · line 1327

Quoted 13 lines

kernel/events/uprobes.c +7 -1

@@ -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;

Quoted 14 lines

kernel/events/uprobes.c pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr) · line 1327

Quoted 11 lines

kernel/events/uprobes.c +7 -1

 	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

Quoted 5 lines

	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.

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

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

kernel/events/uprobes.c +5 -2

index bd94d2c..ad1245d 100644
@@ -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);

kernel/events/uprobes.c +1 -4

 	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:

Signature · 1 line

1.5.5.1

Quoted 10 lines · Oleg Nesterov

static void handle_swbp(struct pt_regs *regs)
{

kernel/events/uprobes.c +0 -1

-	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;
 

kernel/events/uprobes.c static void handle_swbp(struct pt_regs *regs) · line 1518

Quoted 4 lines

kernel/events/uprobes.c +0 -4

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

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

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

kernel/events/uprobes.c +1 -2

index ad1245d..ed4fcbe 100644
@@ -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;

Signature · 1 line

1.5.5.1

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