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

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

[Anton Arapov]

  There are RFC uretprobes implementation. I'd be grateful for any review.

thanks,

Anton Arapov (6):
  uretprobes/x86: hijack return address
  uretprobes: trampoline implementation
  uretprobes: return probe entry, prepare uretprobe
  uretprobes: invoke return probe handlers
  uprobes: add bp_vaddr argument to consumer handler
  uretprobes: register() and unregister() implementation

 arch/x86/include/asm/uprobes.h |   6 +
 arch/x86/kernel/uprobes.c      |  57 ++++++++
 include/linux/uprobes.h        |  28 +++-
 kernel/events/uprobes.c        | 305 ++++++++++++++++++++++++++++++++++++++---
 kernel/trace/trace_uprobe.c    |  20 +--
 5 files changed, 383 insertions(+), 33 deletions(-)

-- 
1.8.0.2

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

[Anton Arapov]

Function to hijack the return address, replace it with a "trampoline"
and functio to predict the stack pointer address value at return.

Signed-off-by: Anton Arapov <anton@redhat.com>
Signed-off-by: Anithra P Janakiraman <anithra@linux.vnet.ibm.com>
---
 arch/x86/include/asm/uprobes.h |  6 +++++
 arch/x86/kernel/uprobes.c      | 57 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/uprobes.h        | 11 +++++++-
 kernel/events/uprobes.c        |  5 +++-
 4 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8ff8be7..9517c55 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -18,9 +18,11 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
  *
  * Copyright (C) IBM Corporation, 2008-2011
+ * Copyright (C) Red Hat, Inc. 2011-2012
  * Authors:
  *	Srikar Dronamraju
  *	Jim Keniston
+ *	Anton Arapov (uretprobes)
  */
 
 #include <linux/notifier.h>
@@ -55,4 +57,7 @@ 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);
+extern unsigned long arch_uretprobe_predict_sp_at_return(struct pt_regs *regs, struct task_struct *tsk);
 #endif	/* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index c71025b..1a30607 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -16,9 +16,11 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
  *
  * Copyright (C) IBM Corporation, 2008-2011
+ * Copyright (C) Red Hat, Inc. 2011-2012
  * Authors:
  *	Srikar Dronamraju
  *	Jim Keniston
+ *	Anton Arapov (uretprobes)
  */
 #include <linux/kernel.h>
 #include <linux/sched.h>
@@ -695,3 +697,57 @@ 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)
+{
+	struct uprobe_task *utask = current->utask;
+	int rasize, ncopied;
+	unsigned long orig_return_vaddr = 0; /* clear high bits for 32-bit apps */
+
+	if (is_ia32_task())
+		rasize = 4;
+	else
+		rasize = 8;
+
+	ncopied = copy_from_user(&orig_return_vaddr, (void __user *)regs->sp, rasize);
+	if (unlikely(ncopied))
+		return -EFAULT;
+
+	/* check whether address has been already hijacked */
+	if (orig_return_vaddr == rp_trampoline_vaddr)
+		return orig_return_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);
+			utask->doomed = true;
+		}
+		return -EFAULT;
+	}
+
+	return orig_return_vaddr;
+}
+
+/*
+ * On x86_32, if a function returns a struct or union, the return
+ * value is copied into an area created by the caller. The address
+ * of this area is passed on the stack as a "hidden" first argument.
+ * When such a function returns, it uses a "ret $4" instruction to pop
+ * not only the return address but also the hidden arg.  To accommodate
+ * such functions, we add 4 bytes of slop when predicting the return
+ * address. See PR #10078.
+ */
+#define STRUCT_RETURN_SLOP 4
+
+extern unsigned long
+arch_uretprobe_predict_sp_at_return(struct pt_regs *regs, struct task_struct *tsk)
+{
+	if (test_tsk_thread_flag(tsk, TIF_IA32))
+		return (unsigned long) (regs->sp + 4 + STRUCT_RETURN_SLOP);
+	else
+		return (unsigned long) (regs->sp + 8);
+}
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 83742b9..a5db480 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -18,10 +18,13 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
  *
  * Copyright (C) IBM Corporation, 2008-2012
+ * Copyright (C) Red Hat, Inc. 2011-2012
  * Authors:
  *	Srikar Dronamraju
  *	Jim Keniston
- * Copyright (C) 2011-2012 Red Hat, Inc., Peter Zijlstra <pzijlstr@redhat.com>
+ *	Oleg Nesterov
+ *	Peter Zijlstra <pzijlstr@redhat.com>
+ *	Anton Arapov (uretprobes)
  */
 
 #include <linux/errno.h>
@@ -60,6 +63,12 @@ struct uprobe_task {
 
 	unsigned long			xol_vaddr;
 	unsigned long			vaddr;
+
+	/*
+	 * Unexpected error in probe point handling has left task's
+	 * text or stack corrupted. Kill task ASAP.
+	 */
+	bool				doomed;
 };
 
 /*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index c3b65d1..30f2ebc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -16,10 +16,12 @@
  * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
  *
  * Copyright (C) IBM Corporation, 2008-2012
+ * Copyright (C) Red Hat, Inc. 2011-2012
  * Authors:
  *	Srikar Dronamraju
  *	Jim Keniston
- * Copyright (C) 2011-2012 Red Hat, Inc., Peter Zijlstra <pzijlstr@redhat.com>
+ *	Peter Zijlstra <pzijlstr@redhat.com>
+ *	Anton Arapov (uretprobes)
  */
 
 #include <linux/kernel.h>
@@ -1298,6 +1300,7 @@ static struct uprobe_task *add_utask(void)
 	if (unlikely(!utask))
 		return NULL;
 
+	utask->doomed = false;
 	current->utask = utask;
 	return utask;
 }
-- 
1.8.0.2

Re: [RFC PATCH 1/6] uretprobes/x86: hijack return address

[Oleg Nesterov]

On 12/21, Anton Arapov wrote:
>
> +extern unsigned long
> +arch_uretprobe_hijack_return_addr(unsigned long rp_trampoline_vaddr, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +	int rasize, ncopied;
> +	unsigned long orig_return_vaddr = 0; /* clear high bits for 32-bit apps */
> +
> +	if (is_ia32_task())
> +		rasize = 4;
> +	else
> +		rasize = 8;
> +
> +	ncopied = copy_from_user(&orig_return_vaddr, (void __user *)regs->sp, rasize);
> +	if (unlikely(ncopied))
> +		return -EFAULT;

Hmm. The caller (added by 3/6) does

	ri->orig_return_vaddr = arch_uretprobe_hijack_return_addr(...);
	if (likely(ri->orig_return_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);

OK... perhaps we could try to write rasize - ncopied bytes first, but
this is minor.

> +			utask->doomed = true;

But this looks strange. We set ->doomed = true, but the task continues to run.
I think in this case we should send SIGTRAP right now. We should not wait until
handle_swbp() notices this flag (which btw can never happen). And this also
means ->doomed should die.

> +		return -EFAULT;

Again, NULL or fix the caller.

> + * On x86_32, if a function returns a struct or union, the return
> + * value is copied into an area created by the caller. The address
> + * of this area is passed on the stack as a "hidden" first argument.
> + * When such a function returns, it uses a "ret $4" instruction to pop
> + * not only the return address but also the hidden arg.  To accommodate
> + * such functions, we add 4 bytes of slop when predicting the return
> + * address. See PR #10078.
                   ^^^^^^^^^
I'd wish I knew what this "PR" means ;)


> +#define STRUCT_RETURN_SLOP 4
> +
> +extern unsigned long
> +arch_uretprobe_predict_sp_at_return(struct pt_regs *regs, struct task_struct *tsk)
> +{
> +	if (test_tsk_thread_flag(tsk, TIF_IA32))
> +		return (unsigned long) (regs->sp + 4 + STRUCT_RETURN_SLOP);

Somehow I can't understand the logic behind arch_uretprobe_predict_sp_at_return()
at all... I'll try more. but tsk is always current, I see no point to pass the
argument.

> @@ -60,6 +63,12 @@ struct uprobe_task {
>
>  	unsigned long			xol_vaddr;
>  	unsigned long			vaddr;
> +
> +	/*
> +	 * Unexpected error in probe point handling has left task's
> +	 * text or stack corrupted. Kill task ASAP.
                                    ^^^^^^^^^^^^^^
Exactly, so ...

> +	bool				doomed;

must die, I think.

Oleg.

[RFC PATCH 2/6] uretprobes: trampoline implementation

[Anton Arapov]

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.

Signed-off-by: Anton Arapov <anton@redhat.com>
Signed-off-by: Anithra P Janakiraman <anithra@linux.vnet.ibm.com>
---
 include/linux/uprobes.h |  1 +
 kernel/events/uprobes.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index a5db480..68780b6 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -88,6 +88,7 @@ struct xol_area {
 	 * the vma go away, and we must handle that reasonably gracefully.
 	 */
 	unsigned long 		vaddr;		/* Page(s) of instruction slots */
+	unsigned long		rp_trampoline_vaddr; /* address of trampolines */
 };
 
 struct uprobes_state {
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 30f2ebc..f0dbbd0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1246,6 +1246,44 @@ static void xol_free_insn_slot(struct task_struct *tsk)
 	}
 }
 
+/*
+ * xol_get_trampoline_slot - A trampoline slot is obtained the first
+ * time a uprobe with return consumer is hit for a process. Use one
+ * trampoline slot for all probes pertaining to a process, i.e.
+ * one per xol_area.
+ */
+static unsigned long xol_get_trampoline_slot(void)
+{
+	struct xol_area *area;
+	unsigned long offset;
+	void *vaddr;
+	uprobe_opcode_t bp_insn = UPROBE_SWBP_INSN;
+
+	area = get_xol_area(current->mm);
+	if (!area) {
+		area = xol_alloc_area();
+		if (!area)
+			return 0;
+	}
+
+	if (!area->rp_trampoline_vaddr)
+		area->rp_trampoline_vaddr = xol_take_insn_slot(area);
+
+	/*
+	 * Initialize the slot if rp_trampoline_vaddr points to valid
+	 * instruction slot.
+	 */
+	if (unlikely(!area->rp_trampoline_vaddr))
+		return 0;
+
+	offset = area->rp_trampoline_vaddr & ~PAGE_MASK;
+	vaddr = kmap_atomic(area->page);
+	memcpy(vaddr + offset, &bp_insn, UPROBE_SWBP_INSN_SIZE);
+	kunmap_atomic(vaddr);
+
+	return area->rp_trampoline_vaddr;
+}
+
 /**
  * uprobe_get_swbp_addr - compute address of swbp given post-swbp regs
  * @regs: Reflects the saved state of the task after it has hit a breakpoint
-- 
1.8.0.2

Re: [RFC PATCH 2/6] uretprobes: trampoline implementation

[Oleg Nesterov]

On 12/21, Anton Arapov wrote:
>
> +static unsigned long xol_get_trampoline_slot(void)
> +{
> +	struct xol_area *area;
> +	unsigned long offset;
> +	void *vaddr;
> +	uprobe_opcode_t bp_insn = UPROBE_SWBP_INSN;
> +
> +	area = get_xol_area(current->mm);
> +	if (!area) {
> +		area = xol_alloc_area();
> +		if (!area)
> +			return 0;
> +	}
> +
> +	if (!area->rp_trampoline_vaddr)
> +		area->rp_trampoline_vaddr = xol_take_insn_slot(area);
> +
> +	/*
> +	 * Initialize the slot if rp_trampoline_vaddr points to valid
> +	 * instruction slot.
> +	 */
> +	if (unlikely(!area->rp_trampoline_vaddr))
> +		return 0;
> +
> +	offset = area->rp_trampoline_vaddr & ~PAGE_MASK;
> +	vaddr = kmap_atomic(area->page);
> +	memcpy(vaddr + offset, &bp_insn, UPROBE_SWBP_INSN_SIZE);
> +	kunmap_atomic(vaddr);
> +
> +	return area->rp_trampoline_vaddr;
> +}

Oh, this should be unified with xol_get_insn_slot(), we certainly do not
want to copy-and-paste it.

Or. Perhaps even better, do not add this helper at all. xol_alloc_area()
could reserve the first slot/bit for trampoline. And note that in this
case we do not need xol_area->rp_trampoline_vaddr, it is always equal
to xol_area->vaddr.

Oleg.

[RFC PATCH 3/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.

This patch introduces return_consumer field in uprobe_task. It creates
additional overhead in filter_chain(), but this is the least evil at the
moment.

The other approach might be: use only consumers field for both probes and
return probes with additional flag introduction for distinguishing them.
This will remove the overhead in some functions like filter_chain(), however
it will comlicate handler_chain() and consumer_add()/_remove() routines.
And uprobes development moves towards reducing flags number so the field
can be removed eventually.

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 68780b6..1222a2c 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -59,6 +59,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_instances;
 	struct uprobe			*active_uprobe;
 
 	unsigned long			xol_vaddr;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f0dbbd0..af424a4 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -63,6 +63,8 @@ static struct percpu_rw_semaphore dup_mmap_sem;
 /* Can skip singlestep */
 #define UPROBE_SKIP_SSTEP	1
 
+static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs);
+
 struct uprobe {
 	struct rb_node		rb_node;	/* node in the rb tree */
 	atomic_t		ref;
@@ -70,12 +72,20 @@ struct uprobe {
 	struct rw_semaphore	consumer_rwsem;
 	struct list_head	pending_list;
 	struct uprobe_consumer	*consumers;
+	struct uprobe_consumer  *return_consumers;
 	struct inode		*inode;		/* Also hold a ref to inode */
 	loff_t			offset;
 	unsigned long		flags;
 	struct arch_uprobe	arch;
 };
 
+struct return_instance {
+	struct uprobe		*uprobe;
+	struct hlist_node	hlist;		/* node in list */
+	unsigned long		orig_return_vaddr;
+	unsigned long		sp;
+};
+
 /*
  * valid_vma: Verify if the specified vma is an executable vma
  * Relax restrictions while unregistering: vm_flags might have
@@ -424,6 +434,8 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 
 	uprobe->inode = igrab(inode);
 	uprobe->offset = offset;
+	uprobe->consumers = NULL;
+	uprobe->return_consumers = NULL;
 	init_rwsem(&uprobe->register_rwsem);
 	init_rwsem(&uprobe->consumer_rwsem);
 	/* For now assume that the instruction need not be single-stepped */
@@ -447,6 +459,9 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 	struct uprobe_consumer *uc;
 
 	down_read(&uprobe->register_rwsem);
+	if (uprobe->return_consumers)
+		prepare_uretprobe(uprobe, regs);
+
 	for (uc = uprobe->consumers; uc; uc = uc->next)
 		uc->handler(uc, regs);
 	up_read(&uprobe->register_rwsem);
@@ -1284,6 +1299,75 @@ static unsigned long xol_get_trampoline_slot(void)
 	return area->rp_trampoline_vaddr;
 }
 
+/* Returns true if @ri_sp lies outside the stack (beyond @cursp). */
+static inline bool compare_stack_ptrs(unsigned long cursp, unsigned long ri_sp)
+{
+#ifdef CONFIG_STACK_GROWSUP
+	if (cursp < ri_sp)
+		return true;
+#else
+	if (cursp > ri_sp)
+		return true;
+#endif
+	return false;
+}
+
+/*
+ * A longjmp may cause one or more uretprobed functions to terminate without
+ * returning. Those functions' return_instances need to be recycled.
+ * We detect this when any uretprobed function is subsequently called
+ * or returns. A bypassed return_instance's stack pointer is beyond the
+ * current stack.
+ */
+static inline void uretprobe_bypass_instances(unsigned long cursp, struct uprobe_task *utask)
+{
+	struct hlist_node *r1, *r2;
+	struct return_instance *ri;
+	struct hlist_head *head = &utask->return_instances;
+
+	hlist_for_each_entry_safe(ri, r1, r2, head, hlist) {
+		if (compare_stack_ptrs(cursp, ri->sp)) {
+			hlist_del(&ri->hlist);
+			kfree(ri);
+		} else
+			return;
+	}
+}
+
+static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask;
+	struct xol_area *area;
+	struct return_instance *ri;
+	unsigned long rp_trampoline_vaddr = 0;
+
+	utask = current->utask;
+	area = get_xol_area(current->mm);
+	if (area)
+		rp_trampoline_vaddr = area->rp_trampoline_vaddr;
+
+	if (!rp_trampoline_vaddr) {
+		rp_trampoline_vaddr = xol_get_trampoline_slot();
+		if (!rp_trampoline_vaddr)
+			return;
+	}
+
+	ri = (struct return_instance *)kzalloc(sizeof(struct return_instance),
+						GFP_KERNEL);
+	if (!ri)
+		return;
+
+	ri->orig_return_vaddr = arch_uretprobe_hijack_return_addr(rp_trampoline_vaddr, regs);
+	if (likely(ri->orig_return_vaddr)) {
+		ri->sp = arch_uretprobe_predict_sp_at_return(regs, current);
+		uretprobe_bypass_instances(ri->sp, utask);
+		ri->uprobe = uprobe;
+		INIT_HLIST_NODE(&ri->hlist);
+		hlist_add_head(&ri->hlist, &utask->return_instances);
+	} else
+		kfree(ri);
+}
+
 /**
  * uprobe_get_swbp_addr - compute address of swbp given post-swbp regs
  * @regs: Reflects the saved state of the task after it has hit a breakpoint
@@ -1339,6 +1423,7 @@ static struct uprobe_task *add_utask(void)
 		return NULL;
 
 	utask->doomed = false;
+	INIT_HLIST_HEAD(&utask->return_instances);
 	current->utask = utask;
 	return utask;
 }
-- 
1.8.0.2

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

[Oleg Nesterov]

On 12/21, Anton Arapov wrote:
>
>  struct uprobe {
>  	struct rb_node		rb_node;	/* node in the rb tree */
>  	atomic_t		ref;
> @@ -70,12 +72,20 @@ struct uprobe {
>  	struct rw_semaphore	consumer_rwsem;
>  	struct list_head	pending_list;
>  	struct uprobe_consumer	*consumers;
> +	struct uprobe_consumer  *return_consumers;

Probably this needs more discussion, but when I look at the next patches
I think that yes, we should not add ->return_consumers and duplicate the
code add/del/each. Perhaps it would be better to add the RET_CONSUMER flag.

Better yet, we could add 2 bits perhaps... then a single consumer/register
can be used to track both events if needed, hit or/and return. But this
needs additional argument.

So perhaps we should simply add uprobe_consumer->ret_hanlder(). A user
can initialize ->hanlder or ->ret_hanlder or both before register. The
only complication is that we need the new bit in uprobe->flags for
prepare_uprobe(). consumer_add/del should set/clear this bit.

> @@ -424,6 +434,8 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
>
>  	uprobe->inode = igrab(inode);
>  	uprobe->offset = offset;
> +	uprobe->consumers = NULL;
> +	uprobe->return_consumers = NULL;

unneeded.

> +/*
> + * A longjmp may cause one or more uretprobed functions to terminate without
> + * returning.

Yes... Plus, we should protect against other attacks...

Those functions' return_instances need to be recycled.
> + * We detect this when any uretprobed function is subsequently called
> + * or returns. A bypassed return_instance's stack pointer is beyond the
> + * current stack.
> + */
> +static inline void uretprobe_bypass_instances(unsigned long cursp, struct uprobe_task *utask)
> +{
> +	struct hlist_node *r1, *r2;
> +	struct return_instance *ri;
> +	struct hlist_head *head = &utask->return_instances;
> +
> +	hlist_for_each_entry_safe(ri, r1, r2, head, hlist) {
> +		if (compare_stack_ptrs(cursp, ri->sp)) {
> +			hlist_del(&ri->hlist);
> +			kfree(ri);

Not sure this will always work, but lets discuss this later.

If nothing else, I wouldn't trust compare_stack_ptrs()... sigaltstack()
can fool this logic afaics.

So far I do not understand this code in details, but it seems that even
the trivial case like

	void ddos_uretpobe(void)
	{
		return ddos_uretpobe();
	}

can lead to the problem (without tail call optimization). The user-space
stack is huge, we should not allow ->return_instances to grow without
any limits, and note that this memory is not accounted.

> +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask;
> +	struct xol_area *area;
> +	struct return_instance *ri;
> +	unsigned long rp_trampoline_vaddr = 0;
> +
> +	utask = current->utask;
> +	area = get_xol_area(current->mm);
> +	if (area)
> +		rp_trampoline_vaddr = area->rp_trampoline_vaddr;
> +
> +	if (!rp_trampoline_vaddr) {
> +		rp_trampoline_vaddr = xol_get_trampoline_slot();

I already mentioned that we probably do not need xol_get_trampoline_slot().
But at least we need xol_alloc_area(), yes.

However, I do not think it is fine to call it here, under ->register_rwsem.
(xol_get_trampoline_slot is even worse btw) perhaps we should do this before
handler_chain().

I think we should refactor handle_swbp/pre_ssout a bit and do _alloc before
handler_chain(). OK, we will see.

> +		if (!rp_trampoline_vaddr)
> +			return;
> +	}
> +
> +	ri = (struct return_instance *)kzalloc(sizeof(struct return_instance),
> +						GFP_KERNEL);
> +	if (!ri)
> +		return;
> +
> +	ri->orig_return_vaddr = arch_uretprobe_hijack_return_addr(rp_trampoline_vaddr, regs);
> +	if (likely(ri->orig_return_vaddr)) {
> +		ri->sp = arch_uretprobe_predict_sp_at_return(regs, current);
> +		uretprobe_bypass_instances(ri->sp, utask);
> +		ri->uprobe = uprobe;

And what protects ri->uprobe? It can go away. See also my reply to 0/6.

Oleg.

[RFC PATCH 4/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.

There are get stack pointer ( cur_sp = (unsigned long)regs->sp; ) and
restore ip (instruction_pointer_set(regs, orig_return_addr); ) in the
code, however I'm not sure if both is ok for any architecture and
perhaps should be moved to arch-dependent uprobes code otherwise.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index af424a4..a3b3a60 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -467,6 +467,16 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 	up_read(&uprobe->register_rwsem);
 }
 
+static void uretprobe_handler_chain(struct uprobe *uprobe, unsigned long bp_vaddr, struct pt_regs *regs)
+{
+	struct uprobe_consumer *uc;
+
+	down_read(&uprobe->register_rwsem);
+	for (uc = uprobe->return_consumers; uc; uc = uc->next)
+		uc->handler(uc, regs);
+	up_read(&uprobe->register_rwsem);
+}
+
 static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 	down_write(&uprobe->consumer_rwsem);
@@ -608,6 +618,12 @@ static bool filter_chain(struct uprobe *uprobe)
 		if (ret)
 			break;
 	}
+	for (uc = uprobe->return_consumers; uc; uc = uc->next) {
+		/* TODO: ret = uc->filter(...) */
+		ret = true;
+		if (ret)
+			break;
+	}
 	up_read(&uprobe->consumer_rwsem);
 
 	return ret;
@@ -1334,6 +1350,48 @@ static inline void uretprobe_bypass_instances(unsigned long cursp, struct uprobe
 	}
 }
 
+static unsigned long uretprobe_run_handlers(struct pt_regs *regs)
+{
+	struct hlist_head *head;
+	struct hlist_node *r1, *r2;
+
+	struct return_instance *ri;
+	struct uprobe_task *utask;
+
+	struct xol_area *area;
+	unsigned long rp_trampoline_vaddr;
+	unsigned long orig_return_vaddr, cur_sp;
+
+	cur_sp = (unsigned long)regs->sp;
+	utask = current->utask;
+	uretprobe_bypass_instances(cur_sp, utask);
+
+	area = get_xol_area(current->mm);
+	rp_trampoline_vaddr = area->rp_trampoline_vaddr;
+	head = &utask->return_instances;
+	hlist_for_each_entry_safe(ri, r1, r2, head, hlist) {
+		if (ri->uprobe && ri->uprobe->return_consumers)
+			uretprobe_handler_chain(ri->uprobe, ri->orig_return_vaddr, regs);
+
+		orig_return_vaddr = ri->orig_return_vaddr;
+		hlist_del(&ri->hlist);
+		kfree(ri);
+
+		if (orig_return_vaddr != rp_trampoline_vaddr)
+			/*
+			 * This is the first ri (chronologically) pushed for
+			 * this particular instance of the probed function.
+			 */
+			return orig_return_vaddr;
+	}
+
+	printk(KERN_ERR "uretprobe: no instance with original return address!"
+			" pid/tgid=%d/%d", current->pid, current->tgid);
+	utask->doomed = true;
+
+	return 0;
+}
+
 static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
 {
 	struct uprobe_task *utask;
@@ -1567,12 +1625,23 @@ static void handle_swbp(struct pt_regs *regs)
 {
 	struct uprobe_task *utask;
 	struct uprobe *uprobe;
-	unsigned long bp_vaddr;
+	struct xol_area *area;
+	unsigned long bp_vaddr, orig_return_vaddr;
 	int uninitialized_var(is_swbp);
 
 	bp_vaddr = uprobe_get_swbp_addr(regs);
-	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
+	area = get_xol_area(current->mm);
+	if (area) {
+		if (bp_vaddr == area->rp_trampoline_vaddr) {
+			orig_return_vaddr = uretprobe_run_handlers(regs);
+			instruction_pointer_set(regs, orig_return_vaddr);
+			if (current->utask->doomed)
+				send_sig(SIGSEGV, current, 0);
+			return;
+		}
+	}
 
+	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
 	if (!uprobe) {
 		if (is_swbp > 0) {
 			/* No matching uprobe; signal SIGTRAP. */
-- 
1.8.0.2

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

[Oleg Nesterov]

On 12/21, Anton Arapov wrote:
>
> +static unsigned long uretprobe_run_handlers(struct pt_regs *regs)
> +{
> +	struct hlist_head *head;
> +	struct hlist_node *r1, *r2;
> +
> +	struct return_instance *ri;
> +	struct uprobe_task *utask;
> +
> +	struct xol_area *area;
> +	unsigned long rp_trampoline_vaddr;
> +	unsigned long orig_return_vaddr, cur_sp;
> +
> +	cur_sp = (unsigned long)regs->sp;

This depends on arch/.

BTW, this series will break the compilation on powerpc, I guess.

> +	hlist_for_each_entry_safe(ri, r1, r2, head, hlist) {
> +		if (ri->uprobe

it should never be NULL, no?

> && ri->uprobe->return_consumers)
> +			uretprobe_handler_chain(ri->uprobe, ri->orig_return_vaddr, regs);
                                                                ^^^^^^^^^^^^^^^^^
->orig_return_vaddr can be rp_trampoline_vaddr. Not sure ->handler()
wants this value ;)

> @@ -1567,12 +1625,23 @@ static void handle_swbp(struct pt_regs *regs)
>  {
>  	struct uprobe_task *utask;
>  	struct uprobe *uprobe;
> -	unsigned long bp_vaddr;
> +	struct xol_area *area;
> +	unsigned long bp_vaddr, orig_return_vaddr;
>  	int uninitialized_var(is_swbp);
>
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
> -	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> +	area = get_xol_area(current->mm);
> +	if (area) {
> +		if (bp_vaddr == area->rp_trampoline_vaddr) {
> +			orig_return_vaddr = uretprobe_run_handlers(regs);
> +			instruction_pointer_set(regs, orig_return_vaddr);
> +			if (current->utask->doomed)
> +				send_sig(SIGSEGV, current, 0);

Just move this send_sig() into uretprobe_run_handlers() and kill
the ugly utask->doomed.

Oleg.

[RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Anton Arapov]

Add argument to handler() to pass the breakpoint address(bp_vaddr). This makes
uprobes consistent, doesn't require users to calculate the pre-breakpoint
address themselves as well as remove uprobes.h import(see the cuts in
trace_event.c here). It also simplifies implementation of the return
probes(uretprobes).

I can't foresee whether anything else from the u[ret]probes should be exposed
the same way as breakpoint address and bet on it as unlikely, so I think
anything heavier than just a sufficient to pass an address variable is overkill.
Any input are welcome though.

Signed-off-by: Anton Arapov <anton@redhat.com>
---
 include/linux/uprobes.h     |  2 +-
 kernel/events/uprobes.c     |  9 +++++----
 kernel/trace/trace_uprobe.c | 20 +++++++++++---------
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 1222a2c..741cc77 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -39,7 +39,7 @@ struct inode;
 #endif
 
 struct uprobe_consumer {
-	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
+	int (*handler)(struct uprobe_consumer *self, unsigned long bp_vaddr, struct pt_regs *regs);
 
 	struct uprobe_consumer *next;
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a3b3a60..d4016282 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -454,7 +454,8 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 	return uprobe;
 }
 
-static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
+static void
+handler_chain(struct uprobe *uprobe, unsigned long bp_vaddr, struct pt_regs *regs)
 {
 	struct uprobe_consumer *uc;
 
@@ -463,7 +464,7 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 		prepare_uretprobe(uprobe, regs);
 
 	for (uc = uprobe->consumers; uc; uc = uc->next)
-		uc->handler(uc, regs);
+		uc->handler(uc, bp_vaddr, regs);
 	up_read(&uprobe->register_rwsem);
 }
 
@@ -473,7 +474,7 @@ static void uretprobe_handler_chain(struct uprobe *uprobe, unsigned long bp_vadd
 
 	down_read(&uprobe->register_rwsem);
 	for (uc = uprobe->return_consumers; uc; uc = uc->next)
-		uc->handler(uc, regs);
+		uc->handler(uc, bp_vaddr, regs);
 	up_read(&uprobe->register_rwsem);
 }
 
@@ -1676,7 +1677,7 @@ static void handle_swbp(struct pt_regs *regs)
 			goto restart;
 	}
 
-	handler_chain(uprobe, regs);
+	handler_chain(uprobe, bp_vaddr, regs);
 	if (can_skip_sstep(uprobe, regs))
 		goto out;
 
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 920dfd0..999da12 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -20,7 +20,6 @@
 
 #include <linux/module.h>
 #include <linux/uaccess.h>
-#include <linux/uprobes.h>
 #include <linux/namei.h>
 #include <linux/string.h>
 
@@ -62,7 +61,7 @@ static void unregister_uprobe_event(struct trace_uprobe *tu);
 static DEFINE_MUTEX(uprobe_lock);
 static LIST_HEAD(uprobe_list);
 
-static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
+static int uprobe_dispatcher(struct uprobe_consumer *con, unsigned long bp_vaddr, struct pt_regs *regs);
 
 /*
  * Allocate new trace_uprobe and initialize it (including uprobes).
@@ -465,7 +464,8 @@ static const struct file_operations uprobe_profile_ops = {
 };
 
 /* uprobe handler */
-static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
+static void
+uprobe_trace_func(struct trace_uprobe *tu, unsigned long bp_vaddr, struct pt_regs *regs)
 {
 	struct uprobe_trace_entry_head *entry;
 	struct ring_buffer_event *event;
@@ -488,7 +488,7 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 		return;
 
 	entry = ring_buffer_event_data(event);
-	entry->ip = uprobe_get_swbp_addr(task_pt_regs(current));
+	entry->ip = bp_vaddr;
 	data = (u8 *)&entry[1];
 	for (i = 0; i < tu->nr_args; i++)
 		call_fetch(&tu->args[i].fetch, regs, data + tu->args[i].offset);
@@ -642,7 +642,8 @@ static int set_print_fmt(struct trace_uprobe *tu)
 
 #ifdef CONFIG_PERF_EVENTS
 /* uprobe profile handler */
-static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
+static void
+uprobe_perf_func(struct trace_uprobe *tu, unsigned long bp_vaddr, struct pt_regs *regs)
 {
 	struct ftrace_event_call *call = &tu->call;
 	struct uprobe_trace_entry_head *entry;
@@ -663,7 +664,7 @@ static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
 	if (!entry)
 		goto out;
 
-	entry->ip = uprobe_get_swbp_addr(task_pt_regs(current));
+	entry->ip = bp_vaddr;
 	data = (u8 *)&entry[1];
 	for (i = 0; i < tu->nr_args; i++)
 		call_fetch(&tu->args[i].fetch, regs, data + tu->args[i].offset);
@@ -703,7 +704,8 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
 	return 0;
 }
 
-static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
+static int
+uprobe_dispatcher(struct uprobe_consumer *con, unsigned long bp_vaddr, struct pt_regs *regs)
 {
 	struct uprobe_trace_consumer *utc;
 	struct trace_uprobe *tu;
@@ -714,11 +716,11 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 		return 0;
 
 	if (tu->flags & TP_FLAG_TRACE)
-		uprobe_trace_func(tu, regs);
+		uprobe_trace_func(tu, bp_vaddr, regs);
 
 #ifdef CONFIG_PERF_EVENTS
 	if (tu->flags & TP_FLAG_PROFILE)
-		uprobe_perf_func(tu, regs);
+		uprobe_perf_func(tu, bp_vaddr, regs);
 #endif
 	return 0;
 }
-- 
1.8.0.2

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Oleg Nesterov]

On 12/21, Anton Arapov wrote:
>
>  struct uprobe_consumer {
> -	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> +	int (*handler)(struct uprobe_consumer *self, unsigned long bp_vaddr, struct pt_regs *regs);

It seems that we can do better...

Just change regs->ip before calling ->handler(). In both cases we are
going to change it again, so ->ip doesn't matter.

Initially I though we can't do this for uretprobes, but it seems we can.

I won't insist though.

Oleg.

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Oleg Nesterov]

Forgot to ask...

On 12/22, Oleg Nesterov wrote:
>
> On 12/21, Anton Arapov wrote:
> >
> >  struct uprobe_consumer {
> > -	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> > +	int (*handler)(struct uprobe_consumer *self, unsigned long bp_vaddr, struct pt_regs *regs);
>
> It seems that we can do better...
>
> Just change regs->ip before calling ->handler().

Josh, Frank, will it work for you?

I assume that stap never needs the "real" ->ip after bp, it only needs
to know which bp the task hits. And once again, in any case ->ip will
be changed after ->handler() is called.

Oleg.

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Oleg Nesterov]

On 12/22, Oleg Nesterov wrote:
>
> Forgot to ask...
>
> On 12/22, Oleg Nesterov wrote:
> >
> > On 12/21, Anton Arapov wrote:
> > >
> > >  struct uprobe_consumer {
> > > -	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> > > +	int (*handler)(struct uprobe_consumer *self, unsigned long bp_vaddr, struct pt_regs *regs);
> >
> > It seems that we can do better...
> >
> > Just change regs->ip before calling ->handler().
>
> Josh, Frank, will it work for you?

Wait, probably I was confused by this patch and 4/6...

To simplify, lets ignore the normal uprobes. Yes I still think that it
would be better to set "regs->ip = orig_return_vaddr" before calling
->handler() and not pass it as an argument.

But, probably uprobe_consumer also needs to know bp_vaddr? IOW, the
address of the function which we are going to return from? In this case,
yes, we also need another argument. And prepare_uretprobe/etc should
be changed to record bp_vaddr passed from handle_swbp(). And
uretprobe_run_handlers() should pass this bp_vaddr, not orig_return_vaddr.

Or I am confused.

Anton?

Oleg.

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Anton Arapov]

On Sun, Dec 23, 2012 at 04:49:10PM +0100, Oleg Nesterov wrote:
> On 12/22, Oleg Nesterov wrote:
> >
> > Forgot to ask...
> >
> > On 12/22, Oleg Nesterov wrote:
> > >
> > > On 12/21, Anton Arapov wrote:
> > > >
> > > >  struct uprobe_consumer {
> > > > -	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> > > > +	int (*handler)(struct uprobe_consumer *self, unsigned long bp_vaddr, struct pt_regs *regs);
> > >
> > > It seems that we can do better...
> > >
> > > Just change regs->ip before calling ->handler().
> >
> > Josh, Frank, will it work for you?
> 
> Wait, probably I was confused by this patch and 4/6...
> 
> To simplify, lets ignore the normal uprobes. Yes I still think that it
> would be better to set "regs->ip = orig_return_vaddr" before calling
> ->handler() and not pass it as an argument.
> 
> But, probably uprobe_consumer also needs to know bp_vaddr? IOW, the
> address of the function which we are going to return from? In this case,
> yes, we also need another argument. And prepare_uretprobe/etc should
> be changed to record bp_vaddr passed from handle_swbp(). And
> uretprobe_run_handlers() should pass this bp_vaddr, not orig_return_vaddr.
> 
> Or I am confused.
> 
> Anton?

  Oleg, you are absolutely right and change the ip before calling the
->handle is much better then what I 'invented'.
 
  I'm working on v2 to address this and other issues, will post it soon.

Anton.

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Josh Stone]

On 01/08/2013 06:27 AM, Anton Arapov wrote:
> On Sun, Dec 23, 2012 at 04:49:10PM +0100, Oleg Nesterov wrote:
>> On 12/22, Oleg Nesterov wrote:
>>>> Just change regs->ip before calling ->handler().
>>>
>>> Josh, Frank, will it work for you?
>>
>> Wait, probably I was confused by this patch and 4/6...
>>
>> To simplify, lets ignore the normal uprobes. Yes I still think that it
>> would be better to set "regs->ip = orig_return_vaddr" before calling
>> ->handler() and not pass it as an argument.
>>
>> But, probably uprobe_consumer also needs to know bp_vaddr? IOW, the
>> address of the function which we are going to return from? In this case,
>> yes, we also need another argument. And prepare_uretprobe/etc should
>> be changed to record bp_vaddr passed from handle_swbp(). And
>> uretprobe_run_handlers() should pass this bp_vaddr, not orig_return_vaddr.
>>
>> Or I am confused.
>>
>> Anton?
> 
>   Oleg, you are absolutely right and change the ip before calling the
> ->handle is much better then what I 'invented'.
>  
>   I'm working on v2 to address this and other issues, will post it soon.

For uprobes, systemtap wants regs->ip to be the real breakpoint address,
and for uretprobes we want the original return address.  With the
utrace-based uprobes we would munge it by modifying regs->ip before
continuing with the rest of our handler.  If uprobes now wants to
prepare regs->ip for us in the same way, I think that's fine.

Josh

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Oleg Nesterov]

On 01/10, Josh Stone wrote:
>
> For uprobes, systemtap wants regs->ip to be the real breakpoint address,

Just to be sure this didn't escape your attention... Please note that
this becomes true after

	[PATCH 2/2] uprobes: Change handle_swbp() to expose bp_vaddr to handler_chain()
	http://marc.info/?l=linux-kernel&m=135688247630262

so you should no longer use uprobe_get_swbp_addr or inst->vaddr to
change regs->ip.

> and for uretprobes we want the original return address.

Yes, Anton's v2 does this.

But. Don't you also need to know the address of function we are going
to return from?

Probably you do not, uprobe_consumer should know which function (but
not vaddr) it probes, but please confirm.

Oleg.

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Josh Stone]

On 01/12/2013 09:06 AM, Oleg Nesterov wrote:
> On 01/10, Josh Stone wrote:
>> and for uretprobes we want the original return address.
> 
> Yes, Anton's v2 does this.
> 
> But. Don't you also need to know the address of function we are going
> to return from?
> 
> Probably you do not, uprobe_consumer should know which function (but
> not vaddr) it probes, but please confirm.

Right, this is fine.

The main reason we need a fixed-up IP is to have a consistent user state
for unwinding and evaluating other related DWARF expressions.

Setting regs->ip to the entry address of the function we just returned
from would actually be harmful, as it would be completely lying about
the current execution point, and the rest of the register and memory
state wouldn't match that point either.

Maybe it would be useful if regs->ip reflected the address of the RET
instruction we just executed, but only if e.g. regs->sp also got rewound
accordingly.  Since I don't think this is possible, just setting
regs->ip to the return target is good enough.

Josh

Re: [RFC PATCH 5/6] uprobes: add bp_vaddr argument to consumer handler

[Oleg Nesterov]

On 01/15, Josh Stone wrote:
>
> On 01/12/2013 09:06 AM, Oleg Nesterov wrote:
> > On 01/10, Josh Stone wrote:
> >> and for uretprobes we want the original return address.
> >
> > Yes, Anton's v2 does this.
> >
> > But. Don't you also need to know the address of function we are going
> > to return from?
> >
> > Probably you do not, uprobe_consumer should know which function (but
> > not vaddr) it probes, but please confirm.
>
> Right, this is fine.

OK, thanks.

> Setting regs->ip to the entry address of the function we just returned
> from would actually be harmful,

Yes, yes, I understand. I meant, ->ret_hander() could have the additional
argument to tell the address of the function.

> Maybe it would be useful if regs->ip reflected the address of the RET
> instruction we just executed, but only if e.g. regs->sp also got rewound
> accordingly.  Since I don't think this is possible, just setting
> regs->ip to the return target is good enough.

Yes, I guess this is not possible.

Oleg.

[RFC PATCH 6/6] uretprobes: register() and unregister() implementation

[Anton Arapov]

A uretprobe is specified by a file:offset:handler. A uprobe is created
at the offset specified. When this uprobe is hit a uretprobe instance
created and a probe placed instead of the return address of the current
function. The handler specified is invoked before replacing the return
address. A uretprobe therefore places two probes, firstly a uprobe at
offset, whose handler in turn places the second probe at the return
address at task runtime.

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

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 741cc77..ebf957f 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -104,6 +104,8 @@ extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, u
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
+extern int uretprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
+extern void uretprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_mmap(struct vm_area_struct *vma);
 extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void uprobe_start_dup_mmap(void);
@@ -130,6 +132,14 @@ static inline void
 uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
 {
 }
+uretprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+{
+	return -ENOSYS;
+}
+static inline void
+uretprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+{
+}
 static inline int uprobe_mmap(struct vm_area_struct *vma)
 {
 	return 0;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index d4016282..d360077 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -486,6 +486,14 @@ static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
 	up_write(&uprobe->consumer_rwsem);
 }
 
+static void return_consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
+{
+	down_write(&uprobe->consumer_rwsem);
+	uc->next = uprobe->return_consumers;
+	uprobe->return_consumers = uc;
+	up_write(&uprobe->consumer_rwsem);
+}
+
 /*
  * For uprobe @uprobe, delete the consumer @uc.
  * Return true if the @uc is deleted successfully
@@ -509,6 +517,24 @@ static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
 	return ret;
 }
 
+static bool return_consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)
+{
+	struct uprobe_consumer **con;
+	bool ret = false;
+
+	down_write(&uprobe->consumer_rwsem);
+	for (con = &uprobe->return_consumers; *con; con = &(*con)->next) {
+		if (*con == uc) {
+			*con = uc->next;
+			ret = true;
+			break;
+		}
+	}
+	up_write(&uprobe->consumer_rwsem);
+
+	return ret;
+}
+
 static int
 __copy_insn(struct address_space *mapping, struct file *filp, char *insn,
 			unsigned long nbytes, loff_t offset)
@@ -831,30 +857,29 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 	return err;
 }
 
-static int __uprobe_register(struct uprobe *uprobe, struct uprobe_consumer *uc)
-{
-	consumer_add(uprobe, uc);
-	return register_for_each_vma(uprobe, true);
-}
-
-static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
+static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc, bool is_rp)
 {
 	int err;
 
-	if (!consumer_del(uprobe, uc))	/* WARN? */
+	if (is_rp)
+		err = return_consumer_del(uprobe, uc);
+	else
+		err = consumer_del(uprobe, uc);
+	if (!err) /* WARN? */
 		return;
 
 	err = register_for_each_vma(uprobe, false);
 	/* TODO : cant unregister? schedule a worker thread */
-	if (!uprobe->consumers && !err)
+	if (!uprobe->consumers && !uprobe->return_consumers && !err)
 		delete_uprobe(uprobe);
 }
 
 /*
- * uprobe_register - register a probe
+ * __uprobe_register - register a probe
  * @inode: the file in which the probe has to be placed.
  * @offset: offset from the start of the file.
- * @uc: information on howto handle the probe..
+ * @uc: information on howto handle the probe.
+ * @is_rp: if true - return probe.
  *
  * Apart from the access refcount, uprobe_register() takes a creation
  * refcount (thro alloc_uprobe) if and only if this @uprobe is getting
@@ -867,7 +892,7 @@ static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *u
  * Return errno if it cannot successully install probes
  * else return 0 (success)
  */
-int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+int __uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool is_rp)
 {
 	struct uprobe *uprobe;
 	int ret;
@@ -887,9 +912,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 	down_write(&uprobe->register_rwsem);
 	ret = -EAGAIN;
 	if (likely(uprobe_is_active(uprobe))) {
-		ret = __uprobe_register(uprobe, uc);
+		if (is_rp)
+			return_consumer_add(uprobe, uc);
+		else
+			consumer_add(uprobe, uc);
+
+		ret = register_for_each_vma(uprobe, true);
 		if (ret)
-			__uprobe_unregister(uprobe, uc);
+			__uprobe_unregister(uprobe, uc, is_rp);
 	}
 	up_write(&uprobe->register_rwsem);
 	put_uprobe(uprobe);
@@ -900,7 +930,26 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 }
 
 /*
- * uprobe_unregister - unregister a already registered probe.
+ * u[ret]probe_register - register a probe
+ * @inode: the file in which the probe has to be placed.
+ * @offset: offset from the start of the file.
+ * @uc: information on howto handle the probe.
+ *
+ * Return errno if it cannot successully install probes
+ * else return 0 (success)
+ */
+int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+{
+	return __uprobe_register(inode, offset, uc, false);
+}
+
+int uretprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+{
+	return __uprobe_register(inode, offset, uc, true);
+}
+
+/*
+ * u[ret]probe_unregister - unregister a already registered probe.
  * @inode: the file in which the probe has to be removed.
  * @offset: offset from the start of the file.
  * @uc: identify which probe if multiple probes are colocated.
@@ -914,8 +963,24 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 		return;
 
 	down_write(&uprobe->register_rwsem);
-	__uprobe_unregister(uprobe, uc);
+	__uprobe_unregister(uprobe, uc, false);
 	up_write(&uprobe->register_rwsem);
+
+	put_uprobe(uprobe);
+}
+
+void uretprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+{
+	struct uprobe *uprobe;
+
+	uprobe = find_uprobe(inode, offset);
+	if (!uprobe)
+		return;
+
+	down_write(&uprobe->register_rwsem);
+	__uprobe_unregister(uprobe, uc, true);
+	up_write(&uprobe->register_rwsem);
+
 	put_uprobe(uprobe);
 }
 
-- 
1.8.0.2

Re: [RFC PATCH 6/6] uretprobes: register() and unregister() implementation

[Oleg Nesterov]

On 12/21, Anton Arapov wrote:
>
> +int uretprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
> +{
> +	return __uprobe_register(inode, offset, uc, true);
> +}
> ...
> +
> +void uretprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
> +{
> +	struct uprobe *uprobe;
> +
> +	uprobe = find_uprobe(inode, offset);
> +	if (!uprobe)
> +		return;
> +
> +	down_write(&uprobe->register_rwsem);
> +	__uprobe_unregister(uprobe, uc, true);
> +	up_write(&uprobe->register_rwsem);
> +
>  	put_uprobe(uprobe);
>  }

See my reply to 3/6. So far I think that we do not need
uretprobe_register/unregister at all.

Oleg.

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

[Oleg Nesterov]

On 12/21, Anton Arapov wrote:
>
>   There are RFC uretprobes implementation. I'd be grateful for any review.

Anton, I won't look at this series till Monday (at least).

But at first glance it needs a lot of cleanups and _fixes_.

If nothing else, ->return_instances logic looks very wrong (but again,
I didn't really read this series and I already had a beer ;).

Just for example. Suppose that you insert uretprobe at exit() in glibc().
IOW, handle_swbp(rp_trampoline_vaddr) never happens. Who will cleanup
utask->return_instances and kfree() return_instance's?

Or. return_consumer_del() simply removes uprobe_consumer. Again, somehow
we should free the "pending" return_instance's. Plus we should restore
the original return adresses connected to these return_instance's. Just
suppose that uretprobe_run_handlers() is called after uprobe has gone
away. In this case ri->uprobe points to nowhere.

Oleg.