[RFC PATCH v4 0/6] uprobes: return probe implementation

[RFC PATCH v4 0/6] uprobes: return probe implementation

[Anton Arapov]

Hello,

  RFC v4 uretprobes implementation. I'd be grateful for review.
  /* Oleg, this one is more quirky than previous, don't beat me. */

  These patches extending uprobes by enabling tools, such as perf(trace_event),
set a breakpoint on probed function's return address.

v4:
 - get rid of area->rp_trampoline_vaddr as it always the same as ->vaddr
 - preallocate slot, as the first one in xol_add_vma()
 - cleanup ->return_uprobes list in uprobe_free_utask(), because the
   task can exit from inside the ret-probe'd function(s).
 - in find_active_uprobe(): Once we inserted "int3" we must ensure that
   handle_swbp() will be called even if this uprobe goes away. We have
   the reference but it only protects uprobe itself, it can't protect
   agains delete_uprobe().
   IOW, we must ensure that uprobe_pre_sstep_notifier() can't return 0.
 - check, whether utask is not NULL in handle_uretprobe()
   ? do we want a printk() for this case?
 - minor handle_uretprobe() fixups

v3 changes:
  - removed uretprobe_bypass logic, it will be better to send it as
    independent patch
  - unified xol_get_trampoline_slot() and xol_get_insn_slot()
  - protected uprobe with refcounter in prepare_uretprobe()
  - uprobe_register() routine fails now, if neither consumer is set
  - enclosed implementation into 1/6, 6/6 patches -ENOSYS bits

v2 changes:
  - introduced rp_handler(), get rid of return_consumers
  - get rid of uretprobe_[un]register()
  - introduced arch_uretprobe_get_sp()
  - removed uprobe_task->doomed, kill task immediately
  - fix arch_uretprobe_hijack_return_addr()'s returns
  - address the v1 minor issues

integrated patchset:
  http://github.com/arapov/linux-aa/commits/uretprobes_v3

previous implementations:
  RFCv3: https://lkml.org/lkml/2013/2/28/148
  RFCv2: https://lkml.org/lkml/2013/1/9/157
  RFCv1: https://lkml.org/lkml/2012/12/21/133

thanks,
Anton

Anton Arapov (6):
  uretprobes: preparation patch
  uretprobes/x86: hijack return address
  uretprobes: generalize xol_get_insn_slot()
  uretprobes: return probe entry, prepare uretprobe
  uretprobes: invoke return probe handlers
  uretprobes: implemented, thus remove -ENOSYS

 arch/x86/include/asm/uprobes.h |   6 ++
 arch/x86/kernel/uprobes.c      |  29 +++++++++
 include/linux/uprobes.h        |   5 ++
 kernel/events/uprobes.c        | 134 ++++++++++++++++++++++++++++++++++++++---
 4 files changed, 166 insertions(+), 8 deletions(-)

-- 
1.8.1.2

[RFC PATCH v4 1/6] uretprobes: preparation patch

[Anton Arapov]

  1/6 and 6/6 patches are here to enclose return probes implementation as well
as prohibit uprobe_register() routine with no consumer set.

v3 changes: (the patch is introduced in v3)
  - check whether at least one of the consumer's handlers were set
  - a 'TODO' cap that will be removed once return probes be implemented

Signed-off-by: Anton Arapov <anton@redhat.com>
---
 include/linux/uprobes.h | 1 +
 kernel/events/uprobes.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 02b83db..a28bdee 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -46,6 +46,7 @@ enum uprobe_filter_ctx {
 
 struct uprobe_consumer {
 	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
+	int (*rp_handler)(struct uprobe_consumer *self, struct pt_regs *regs);
 	bool (*filter)(struct uprobe_consumer *self,
 				enum uprobe_filter_ctx ctx,
 				struct mm_struct *mm);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a567c8c..d904164 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -828,6 +828,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 	struct uprobe *uprobe;
 	int ret;
 
+	/* Uprobe must have at least one set consumer */
+	if (!uc->handler && !uc->rp_handler)
+		return -EINVAL;
+
+	/* TODO: Implement return probes */
+	if (uc->rp_handler)
+		return -ENOSYS;
+
 	/* Racy, just to catch the obvious mistakes */
 	if (offset > i_size_read(inode))
 		return -EINVAL;
-- 
1.8.1.2

[RFC PATCH v4 2/6] uretprobes/x86: hijack return address

[Anton Arapov]

  hijack the return address and replace it with a "trampoline"

v2:
  - remove ->doomed flag, kill task immediately

Signed-off-by: Anton Arapov <anton@redhat.com>
---
 arch/x86/include/asm/uprobes.h |  1 +
 arch/x86/kernel/uprobes.c      | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8ff8be7..c353555 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -55,4 +55,5 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long rp_trampoline_vaddr, struct pt_regs *regs);
 #endif	/* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0ba4cfb..85e2153 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 		send_sig(SIGTRAP, current, 0);
 	return ret;
 }
+
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
+		rp_trampoline_vaddr, struct pt_regs *regs)
+{
+	int rasize, ncopied;
+	unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
+
+	rasize = is_ia32_task() ? 4 : 8;
+	ncopied = copy_from_user(&orig_ret_vaddr, (void __user *)regs->sp, rasize);
+	if (unlikely(ncopied))
+		return 0;
+
+	/* check whether address has been already hijacked */
+	if (orig_ret_vaddr == rp_trampoline_vaddr)
+		return orig_ret_vaddr;
+
+	ncopied = copy_to_user((void __user *)regs->sp, &rp_trampoline_vaddr, rasize);
+	if (unlikely(ncopied)) {
+		if (ncopied != rasize) {
+			printk(KERN_ERR "uretprobe: return address clobbered: "
+					"pid=%d, %%sp=%#lx, %%ip=%#lx\n",
+					current->pid, regs->sp, regs->ip);
+			/* kill task immediately */
+			send_sig(SIGSEGV, current, 0);
+		}
+	}
+
+	return orig_ret_vaddr;
+}
-- 
1.8.1.2

[RFC PATCH v4 3/6] uretprobes: generalize xol_get_insn_slot()

[Anton Arapov]

  Generalize xol_take_insn_slot() to enable more consumers of the
function, e.g. trampoline implementation for return probes.

  The first time a uprobe with return consumer is hit for a process, a
trampoline slot is obtained in the xol_area and initialized with a
breakpoint instruction. This slot is subsequently used by all
uretprobes.

v3:
 - unified xol_get_insn_slot(), thus get rid of xol_get_trampoline_slot()

Signed-off-by: Anton Arapov <anton@redhat.com>
---
 kernel/events/uprobes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index d904164..69bf060 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1221,7 +1221,7 @@ static unsigned long xol_take_insn_slot(struct xol_area *area)
  * 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)
+static unsigned long xol_get_insn_slot(unsigned char *insn)
 {
 	struct xol_area *area;
 	unsigned long offset;
@@ -1239,7 +1239,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)
 	/* Initialize the slot */
 	offset = xol_vaddr & ~PAGE_MASK;
 	vaddr = kmap_atomic(area->page);
-	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
+	memcpy(vaddr + offset, insn, MAX_UINSN_BYTES);
 	kunmap_atomic(vaddr);
 	/*
 	 * We probably need flush_icache_user_range() but it needs vma.
@@ -1353,7 +1353,7 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
 	if (!utask)
 		return -ENOMEM;
 
-	xol_vaddr = xol_get_insn_slot(uprobe);
+	xol_vaddr = xol_get_insn_slot(uprobe->arch.insn);
 	if (!xol_vaddr)
 		return -ENOMEM;
 
-- 
1.8.1.2

[RFC PATCH v4 4/6] uretprobes: return probe entry, prepare uretprobe

[Anton Arapov]

  When a uprobe with return consumer is hit, prepare_uretprobe function is
invoked. It creates return_instance, hijacks return address and replaces
it with the trampoline.

 N.B. it might be a good idea to introduce get_uprobe() to reflect
put_uprobe() later, but it is not a subject of this patchset.

v4:
 - get rid of area->rp_trampoline_vaddr as it always the same as ->vaddr
 - preallocate slot, as the first one in xol_add_vma()
 - cleanup ->return_uprobes list in uprobe_free_utask(), because the
   task can exit from inside the ret-probe'd function(s).
 - in find_active_uprobe(): Once we inserted "int3" we must ensure that
   handle_swbp() will be called even if this uprobe goes away. We have
   the reference but it only protects uprobe itself, it can't protect
   agains delete_uprobe().
   IOW, we must ensure that uprobe_pre_sstep_notifier() can't return 0.
v3:
 - protected uprobe with refcounter. See atomic_inc in prepare_uretprobe()
and put_uprobe() in a following patch in handle_uretprobe()
v2:
 - get rid of ->return_consumers member from struct uprobe, introduce
rp_handler() in consumer

Signed-off-by: Anton Arapov <anton@redhat.com>
---
 include/linux/uprobes.h |  4 +++
 kernel/events/uprobes.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index a28bdee..6b0a309 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -69,6 +69,10 @@ struct uprobe_task {
 	enum uprobe_task_state		state;
 	struct arch_uprobe_task		autask;
 
+	/*
+	 * list for tracking uprobes with return consumers
+	 */
+	struct hlist_head		return_uprobes;
 	struct uprobe			*active_uprobe;
 
 	unsigned long			xol_vaddr;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 69bf060..12acc10 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -57,6 +57,8 @@ static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
 
 static struct percpu_rw_semaphore dup_mmap_sem;
 
+static unsigned long xol_get_insn_slot(unsigned char *insn);
+
 /* Have a copy of original instruction */
 #define UPROBE_COPY_INSN	0
 /* Can skip singlestep */
@@ -75,6 +77,12 @@ struct uprobe {
 	struct arch_uprobe	arch;
 };
 
+struct return_uprobe_i {
+	struct uprobe		*uprobe;
+	struct hlist_node	hlist;		/* node in list */
+	unsigned long		orig_ret_vaddr; /* original return address */
+};
+
 /*
  * valid_vma: Verify if the specified vma is an executable vma
  * Relax restrictions while unregistering: vm_flags might have
@@ -1085,6 +1093,7 @@ static int xol_add_vma(struct xol_area *area)
 {
 	struct mm_struct *mm = current->mm;
 	int ret = -EALREADY;
+	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
 
 	down_write(&mm->mmap_sem);
 	if (mm->uprobes_state.xol_area)
@@ -1106,6 +1115,13 @@ static int xol_add_vma(struct xol_area *area)
 	smp_wmb();	/* pairs with get_xol_area() */
 	mm->uprobes_state.xol_area = area;
 	ret = 0;
+
+	/*
+	 * If we reached this place, we did allocate a new area. We want
+	 * pre-alloc a slot for the return probes here.
+	 */
+	xol_get_insn_slot(&insn);
+
  fail:
 	up_write(&mm->mmap_sem);
 
@@ -1306,6 +1322,9 @@ unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs)
 void uprobe_free_utask(struct task_struct *t)
 {
 	struct uprobe_task *utask = t->utask;
+	struct hlist_head *head;
+	struct hlist_node *tmp;
+	struct return_uprobe_i *ri;
 
 	if (!utask)
 		return;
@@ -1313,6 +1332,13 @@ void uprobe_free_utask(struct task_struct *t)
 	if (utask->active_uprobe)
 		put_uprobe(utask->active_uprobe);
 
+	head = &utask->return_uprobes;
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		put_uprobe(ri->uprobe);
+		hlist_del(&ri->hlist);
+		kfree(ri);
+	}
+
 	xol_free_insn_slot(t);
 	kfree(utask);
 	t->utask = NULL;
@@ -1336,11 +1362,37 @@ void uprobe_copy_process(struct task_struct *t)
  */
 static struct uprobe_task *get_utask(void)
 {
-	if (!current->utask)
+	if (!current->utask) {
 		current->utask = kzalloc(sizeof(struct uprobe_task), GFP_KERNEL);
+		if (current->utask)
+			INIT_HLIST_HEAD(&current->utask->return_uprobes);
+	}
 	return current->utask;
 }
 
+static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
+{
+	struct return_uprobe_i *ri;
+	struct uprobe_task *utask;
+	struct xol_area *area;
+
+	ri = kzalloc(sizeof(struct return_uprobe_i), GFP_KERNEL);
+	if (!ri)
+		return;
+
+	area = get_xol_area();
+	utask = get_utask();
+	ri->orig_ret_vaddr = arch_uretprobe_hijack_return_addr(area->vaddr, regs);
+	if (likely(ri->orig_ret_vaddr)) {
+		/* TODO: uretprobe bypass logic */
+		atomic_inc(&uprobe->ref);
+		ri->uprobe = uprobe;
+		INIT_HLIST_NODE(&ri->hlist);
+		hlist_add_head(&ri->hlist, &utask->return_uprobes);
+	} else
+		kfree(ri);
+}
+
 /* Prepare to single-step probed instruction out of line. */
 static int
 pre_ssout(struct uprobe *uprobe, struct pt_regs *regs, unsigned long bp_vaddr)
@@ -1468,6 +1520,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	struct mm_struct *mm = current->mm;
 	struct uprobe *uprobe = NULL;
 	struct vm_area_struct *vma;
+	struct uprobe_task *utask;
 
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, bp_vaddr);
@@ -1485,8 +1538,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 		*is_swbp = -EFAULT;
 	}
 
-	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
+	utask = get_utask();
+	if (!uprobe && hlist_empty(&utask->return_uprobes) &&
+	    test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags)) {
 		mmf_recalc_uprobes(mm);
+	}
 	up_read(&mm->mmap_sem);
 
 	return uprobe;
@@ -1494,12 +1550,17 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 
 static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 {
+	int rc = 0;
 	struct uprobe_consumer *uc;
 	int remove = UPROBE_HANDLER_REMOVE;
 
 	down_read(&uprobe->register_rwsem);
 	for (uc = uprobe->consumers; uc; uc = uc->next) {
-		int rc = uc->handler(uc, regs);
+		if (uc->handler)
+			rc = uc->handler(uc, regs);
+
+		if (uc->rp_handler)
+			prepare_uretprobe(uprobe, regs); /* put bp at return */
 
 		WARN(rc & ~UPROBE_HANDLER_MASK,
 			"bad rc=0x%x from %pf()\n", rc, uc->handler);
-- 
1.8.1.2

[RFC PATCH v4 5/6] uretprobes: invoke return probe handlers

[Anton Arapov]

  Uretprobe handlers are invoked when the trampoline is hit, on completion the
trampoline is replaced with the saved return address and the uretprobe instance
deleted.

v4:
 - check, whether utask is not NULL in handle_uretprobe()
   ? do we want a printk() for this case?
 - get rid of area->rp_trampoline_vaddr
 - minor handle_uretprobe() fixups
v3:
 - protected uprobe with refcounter. See put_uprobe() in handle_uretprobe()
that reflects increment in prepare_uretprobe()
v2:
 - get rid of ->return_consumers member from struct uprobe, introduce
rp_handler() in consumer instead

Signed-off-by: Anton Arapov <anton@redhat.com>
---
 arch/x86/include/asm/uprobes.h |  5 ++++
 kernel/events/uprobes.c        | 57 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index c353555..fa9d9de 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -56,4 +56,9 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long rp_trampoline_vaddr, struct pt_regs *regs);
+
+static inline unsigned long arch_uretprobe_get_sp(struct pt_regs *regs)
+{
+	return (unsigned long)regs->sp;
+}
 #endif	/* _ASM_UPROBES_H */
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 12acc10..e86b6ea 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1574,6 +1574,55 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 	up_read(&uprobe->register_rwsem);
 }
 
+static void handler_uretprobe_chain(struct uprobe *uprobe, struct pt_regs *regs)
+{
+	struct uprobe_consumer *uc;
+
+	down_read(&uprobe->register_rwsem);
+	for (uc = uprobe->consumers; uc; uc = uc->next) {
+		if (uc->rp_handler)
+			uc->rp_handler(uc, regs);
+	}
+	up_read(&uprobe->register_rwsem);
+}
+
+static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
+{
+	struct hlist_head *head;
+	struct hlist_node *tmp;
+	struct return_uprobe_i *ri;
+	struct uprobe_task *utask;
+	unsigned long orig_ret_vaddr;
+
+	/* TODO: uretprobe bypass logic */
+
+	utask = get_utask();
+	if (!utask) {
+		/* TODO:RFC task is not probed, do we want printk here? */
+		return;
+	}
+	head = &utask->return_uprobes;
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->uprobe->consumers) {
+			instruction_pointer_set(regs, ri->orig_ret_vaddr);
+			handler_uretprobe_chain(ri->uprobe, regs);
+		}
+
+		orig_ret_vaddr = ri->orig_ret_vaddr;
+		put_uprobe(ri->uprobe);
+		hlist_del(&ri->hlist);
+		kfree(ri);
+
+		if (orig_ret_vaddr != area->vaddr)
+			return;
+	}
+
+	/* TODO: change the message */
+	printk(KERN_ERR "uprobe: no instance found! pid/tgid=%d/%d",
+			current->pid, current->tgid);
+	send_sig(SIGSEGV, current, 0);
+}
+
 /*
  * Run handler and ask thread to singlestep.
  * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1581,6 +1630,7 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 static void handle_swbp(struct pt_regs *regs)
 {
 	struct uprobe *uprobe;
+	struct xol_area *area;
 	unsigned long bp_vaddr;
 	int uninitialized_var(is_swbp);
 
@@ -1589,8 +1639,11 @@ static void handle_swbp(struct pt_regs *regs)
 
 	if (!uprobe) {
 		if (is_swbp > 0) {
-			/* No matching uprobe; signal SIGTRAP. */
-			send_sig(SIGTRAP, current, 0);
+			area = get_xol_area();
+			if (area && bp_vaddr == area->vaddr)
+				handle_uretprobe(area, regs);
+			else
+				send_sig(SIGTRAP, current, 0);
 		} else {
 			/*
 			 * Either we raced with uprobe_unregister() or we can't
-- 
1.8.1.2

[RFC PATCH v4 6/6] uretprobes: implemented, thus remove -ENOSYS

[Anton Arapov]

  1/6 and 6/6 patches are here to enclose return probes implementation
as well as prohibit uprobe_register() routine with no consumer set.

v3 changes: (the patch is introduced in v3)
  - remove 'TODO' as return probes implemented now

Signed-off-by: Anton Arapov <anton@redhat.com>
---
 kernel/events/uprobes.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e86b6ea..b9c51dc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -840,10 +840,6 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 	if (!uc->handler && !uc->rp_handler)
 		return -EINVAL;
 
-	/* TODO: Implement return probes */
-	if (uc->rp_handler)
-		return -ENOSYS;
-
 	/* Racy, just to catch the obvious mistakes */
 	if (offset > i_size_read(inode))
 		return -EINVAL;
-- 
1.8.1.2

Re: [RFC PATCH v4 4/6] uretprobes: return probe entry, prepare uretprobe

[Oleg Nesterov]

On 03/04, Anton Arapov wrote:
>
> @@ -1085,6 +1093,7 @@ static int xol_add_vma(struct xol_area *area)
>  {
>  	struct mm_struct *mm = current->mm;
>  	int ret = -EALREADY;
> +	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
>
>  	down_write(&mm->mmap_sem);
>  	if (mm->uprobes_state.xol_area)
> @@ -1106,6 +1115,13 @@ static int xol_add_vma(struct xol_area *area)
>  	smp_wmb();	/* pairs with get_xol_area() */
>  	mm->uprobes_state.xol_area = area;
>  	ret = 0;
> +
> +	/*
> +	 * If we reached this place, we did allocate a new area. We want
> +	 * pre-alloc a slot for the return probes here.
> +	 */
> +	xol_get_insn_slot(&insn);

Just change get_xol_area() to do set_bit(0, bitmap) and copy_to_page(page, int3)
(extacted from xol_get_insn_slot().

> @@ -1485,8 +1538,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  		*is_swbp = -EFAULT;
>  	}
>  
> -	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
> +	utask = get_utask();
> +	if (!uprobe && hlist_empty(&utask->return_uprobes) &&
> +	    test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags)) {
>  		mmf_recalc_uprobes(mm);

Wait, I was wrong. We should not clear MMF_* if another thread has
->return_uprobes. Perhaps we should change uprobe_pre_sstep_notifier()
instead...

>  	down_read(&uprobe->register_rwsem);
>  	for (uc = uprobe->consumers; uc; uc = uc->next) {
> -		int rc = uc->handler(uc, regs);
> +		if (uc->handler)
> +			rc = uc->handler(uc, regs);
> +
> +		if (uc->rp_handler)
> +			prepare_uretprobe(uprobe, regs); /* put bp at return */

Again, this is not right. We should do this only once after the main
loop.

Oleg.

Re: [RFC PATCH v4 5/6] uretprobes: invoke return probe handlers

[Oleg Nesterov]

On 03/04, Anton Arapov wrote:
>
> +static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
> +{
> +	struct hlist_head *head;
> +	struct hlist_node *tmp;
> +	struct return_uprobe_i *ri;
> +	struct uprobe_task *utask;
> +	unsigned long orig_ret_vaddr;
> +
> +	/* TODO: uretprobe bypass logic */
> +
> +	utask = get_utask();
> +	if (!utask) {
> +		/* TODO:RFC task is not probed, do we want printk here? */
> +		return;
> +	}
> +	head = &utask->return_uprobes;
> +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> +		if (ri->uprobe->consumers) {
> +			instruction_pointer_set(regs, ri->orig_ret_vaddr);

This doesn't look right if ri->orig_ret_vaddr == area->vaddr. We should
splice the list and find orig_ret_vaddr in advance.

> @@ -1589,8 +1639,11 @@ static void handle_swbp(struct pt_regs *regs)
>
>  	if (!uprobe) {
>  		if (is_swbp > 0) {
> -			/* No matching uprobe; signal SIGTRAP. */
> -			send_sig(SIGTRAP, current, 0);
> +			area = get_xol_area();
> +			if (area && bp_vaddr == area->vaddr)
> +				handle_uretprobe(area, regs);
> +			else
> +				send_sig(SIGTRAP, current, 0);

Why? We can check bp_vaddr at the start, before find_active_uprobe().

And I'd suggest to not use area->vaddr directly, imho a trivial helper
makes sense.

Oleg.

Re: [RFC PATCH v4 5/6] uretprobes: invoke return probe handlers

[Ananth N Mavinakayanahalli]

On Mon, Mar 04, 2013 at 03:38:12PM +0100, Anton Arapov wrote:
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index c353555..fa9d9de 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -56,4 +56,9 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
>  extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
>  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long rp_trampoline_vaddr, struct pt_regs *regs);
> +
> +static inline unsigned long arch_uretprobe_get_sp(struct pt_regs *regs)
> +{
> +	return (unsigned long)regs->sp;
> +}

You could use GET_USP() here.

Re: [RFC PATCH v4 0/6] uprobes: return probe implementation

[Ingo Molnar]

* Anton Arapov <anton@redhat.com> wrote:

> Hello,
> 
>   RFC v4 uretprobes implementation. I'd be grateful for review.
>   /* Oleg, this one is more quirky than previous, don't beat me. */

Please include a good high level description of the new feature in one of the 
patches - preferably also stick it somewhere into Documentation/.

Thanks,

	Ingo

Re: [RFC PATCH v4 0/6] uprobes: return probe implementation

[Anton Arapov]

On Tue, Mar 05, 2013 at 01:04:17PM +0100, Ingo Molnar wrote:
> 
> * Anton Arapov <anton@redhat.com> wrote:
> 
> > Hello,
> > 
> >   RFC v4 uretprobes implementation. I'd be grateful for review.
> >   /* Oleg, this one is more quirky than previous, don't beat me. */
> 
> Please include a good high level description of the new feature in one of the 
> patches - preferably also stick it somewhere into Documentation/.

  Yes, I apologize, I did wait for someone's complaint to push myself to
write meaningful description.
 
thanks,
Anton

> Thanks,
> 
> 	Ingo

Re: [RFC PATCH v4 5/6] uretprobes: invoke return probe handlers

[Anton Arapov]

On Tue, Mar 05, 2013 at 12:33:26PM +0530, Ananth N Mavinakayanahalli wrote:
> On Mon, Mar 04, 2013 at 03:38:12PM +0100, Anton Arapov wrote:
> > 
> > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> > index c353555..fa9d9de 100644
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -56,4 +56,9 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
> >  extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
> >  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
> >  extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long rp_trampoline_vaddr, struct pt_regs *regs);
> > +
> > +static inline unsigned long arch_uretprobe_get_sp(struct pt_regs *regs)
> > +{
> > +	return (unsigned long)regs->sp;
> > +}
> 
> You could use GET_USP() here.
perhaps, which in turn is a helper for user_stack_pointer() :) comment
is valid though.

thanks,
Anton.

Re: [RFC PATCH v4 4/6] uretprobes: return probe entry, prepare uretprobe

[Anton Arapov]

On Mon, Mar 04, 2013 at 05:47:53PM +0100, Oleg Nesterov wrote:
> On 03/04, Anton Arapov wrote:
> >
> > @@ -1085,6 +1093,7 @@ static int xol_add_vma(struct xol_area *area)
> >  {
> >  	struct mm_struct *mm = current->mm;
> >  	int ret = -EALREADY;
> > +	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> >
> >  	down_write(&mm->mmap_sem);
> >  	if (mm->uprobes_state.xol_area)
> > @@ -1106,6 +1115,13 @@ static int xol_add_vma(struct xol_area *area)
> >  	smp_wmb();	/* pairs with get_xol_area() */
> >  	mm->uprobes_state.xol_area = area;
> >  	ret = 0;
> > +
> > +	/*
> > +	 * If we reached this place, we did allocate a new area. We want
> > +	 * pre-alloc a slot for the return probes here.
> > +	 */
> > +	xol_get_insn_slot(&insn);
> Just change get_xol_area() to do set_bit(0, bitmap) and copy_to_page(page, int3)
> (extacted from xol_get_insn_slot().
ah yes, right. Thanks for this hint.

> 
> > @@ -1485,8 +1538,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> >  		*is_swbp = -EFAULT;
> >  	}
> >  
> > -	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
> > +	utask = get_utask();
> > +	if (!uprobe && hlist_empty(&utask->return_uprobes) &&
> > +	    test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags)) {
> >  		mmf_recalc_uprobes(mm);
> Wait, I was wrong. We should not clear MMF_* if another thread has
> ->return_uprobes. Perhaps we should change uprobe_pre_sstep_notifier()
> instead...
Will look into it.

> >  	down_read(&uprobe->register_rwsem);
> >  	for (uc = uprobe->consumers; uc; uc = uc->next) {
> > -		int rc = uc->handler(uc, regs);
> > +		if (uc->handler)
> > +			rc = uc->handler(uc, regs);
> > +
> > +		if (uc->rp_handler)
> > +			prepare_uretprobe(uprobe, regs); /* put bp at return */
> Again, this is not right. We should do this only once after the main
> loop.
fixed for v5.

Thanks!
Anton.

Re: [RFC PATCH v4 5/6] uretprobes: invoke return probe handlers

[Anton Arapov]

On Mon, Mar 04, 2013 at 05:51:20PM +0100, Oleg Nesterov wrote:
> On 03/04, Anton Arapov wrote:
> >
> > +static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
> > +{
> > +	struct hlist_head *head;
> > +	struct hlist_node *tmp;
> > +	struct return_uprobe_i *ri;
> > +	struct uprobe_task *utask;
> > +	unsigned long orig_ret_vaddr;
> > +
> > +	/* TODO: uretprobe bypass logic */
> > +
> > +	utask = get_utask();
> > +	if (!utask) {
> > +		/* TODO:RFC task is not probed, do we want printk here? */
> > +		return;
> > +	}
> > +	head = &utask->return_uprobes;
> > +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> > +		if (ri->uprobe->consumers) {
> > +			instruction_pointer_set(regs, ri->orig_ret_vaddr);
> This doesn't look right if ri->orig_ret_vaddr == area->vaddr. We should
> splice the list and find orig_ret_vaddr in advance.
True, this cycle is buggy. I will rework handle_uretprobe().

> > @@ -1589,8 +1639,11 @@ static void handle_swbp(struct pt_regs *regs)
> >
> >  	if (!uprobe) {
> >  		if (is_swbp > 0) {
> > -			/* No matching uprobe; signal SIGTRAP. */
> > -			send_sig(SIGTRAP, current, 0);
> > +			area = get_xol_area();
> > +			if (area && bp_vaddr == area->vaddr)
> > +				handle_uretprobe(area, regs);
> > +			else
> > +				send_sig(SIGTRAP, current, 0);
> 
> Why? We can check bp_vaddr at the start, before find_active_uprobe().
For some reason, I was thinking it is better to hide this logic under
if (!uprobe). Will correct this chunk.

> And I'd suggest to not use area->vaddr directly, imho a trivial helper
> makes sense.
I see the idea behind, with this change it will be more clear and
fragile in case someone change the underneath logic. Will do this.

thank you very much!
Anton.