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

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

[Anton Arapov]

Hello,

  RFC v3 uretprobes implementation. I'd be grateful for review.

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

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:
  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        |   6 +++
 kernel/events/uprobes.c        | 112 ++++++++++++++++++++++++++++++++++++++---
 4 files changed, 146 insertions(+), 7 deletions(-)

-- 
1.8.1.2

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

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 |  5 +++++
 kernel/events/uprobes.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index a28bdee..6aaa1ce 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;
@@ -92,6 +96,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; /* trampoline address */
 };
 
 struct uprobes_state {
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 69bf060..57f70cd 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -75,6 +75,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
@@ -1336,11 +1342,48 @@ 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;
+	unsigned long rp_trampoline_vaddr = 0;
+	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+
+	area = get_xol_area();
+	if (area)
+		rp_trampoline_vaddr = area->rp_trampoline_vaddr;
+	if (!rp_trampoline_vaddr) {
+		rp_trampoline_vaddr = xol_get_insn_slot(&insn);
+		if (!rp_trampoline_vaddr)
+			return;
+	}
+	area->rp_trampoline_vaddr = rp_trampoline_vaddr;
+
+	ri = kzalloc(sizeof(struct return_uprobe_i), GFP_KERNEL);
+	if (!ri)
+		return;
+
+	utask = get_utask();
+	ri->orig_ret_vaddr = arch_uretprobe_hijack_return_addr(rp_trampoline_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)
@@ -1494,12 +1537,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 v3 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.

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        | 50 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 53 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 57f70cd..deacb35 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1561,6 +1561,52 @@ 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 pt_regs *regs)
+{
+	struct hlist_head *head;
+	struct hlist_node *tmp;
+	struct xol_area *area;
+	struct return_uprobe_i *ri;
+	struct uprobe_task *utask;
+
+	/* TODO: uretprobe bypass logic */
+
+	area = get_xol_area();
+	utask = get_utask();
+	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);
+		}
+
+		put_uprobe(ri->uprobe);
+		hlist_del(&ri->hlist);
+		kfree(ri);
+
+		if (ri->orig_ret_vaddr != area->rp_trampoline_vaddr)
+			return;
+	}
+
+	/* TODO: change the message */
+	printk(KERN_ERR "uprobe: no instance found! pid/tgid=%d/%d",
+			current->pid, current->tgid);
+	/* TODO: May be sigtrap? if possible to restore utask */
+	send_sig(SIGSEGV, current, 0);
+}
+
 /*
  * Run handler and ask thread to singlestep.
  * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1576,8 +1622,8 @@ static void handle_swbp(struct pt_regs *regs)
 
 	if (!uprobe) {
 		if (is_swbp > 0) {
-			/* No matching uprobe; signal SIGTRAP. */
-			send_sig(SIGTRAP, current, 0);
+			/* No matching uprobe; Try with uretprobe */
+			handle_uretprobe(regs);
 		} else {
 			/*
 			 * Either we raced with uprobe_unregister() or we can't
-- 
1.8.1.2

[RFC PATCH v3 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 deacb35..7ae338b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -838,10 +838,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 v3 3/6] uretprobes: generalize xol_get_insn_slot()

[Oleg Nesterov]

Anton, I'll try to read this series asap. Just one note,

On 02/28, Anton Arapov wrote:
>
>   Generalize xol_take_insn_slot() to enable more consumers of the
> function, e.g. trampoline implementation for return probes.

Yes, this is can work too.

But I guess you misunderstood me, or I missed something...

Why do you need xol_area->rp_trampoline_vaddr at all? From the very
beginning I am trying to suggest to use the first slot for trampoline:

	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.

Doesn't this look simpler?

Oleg.

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

[Oleg Nesterov]

On 02/28, Anton Arapov wrote:
>
> +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> +{
> +	struct return_uprobe_i *ri;
> +	struct uprobe_task *utask;
> +	struct xol_area *area;
> +	unsigned long rp_trampoline_vaddr = 0;
> +	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> +
> +	area = get_xol_area();
> +	if (area)
> +		rp_trampoline_vaddr = area->rp_trampoline_vaddr;
> +	if (!rp_trampoline_vaddr) {
> +		rp_trampoline_vaddr = xol_get_insn_slot(&insn);
> +		if (!rp_trampoline_vaddr)
> +			return;
> +	}
> +	area->rp_trampoline_vaddr = rp_trampoline_vaddr;

This is called under down_read(), so 2 threads can race with each other
and use the different rp_trampoline_vaddr's if ->rp_trampoline_vaddr was
NULL.

And again, I think ->rp_trampoline_vaddr is simply unneeded, see my
reply to 3/6.

>  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 */

Hmm. I didn't read this series yet. But at first glance I am not
sure prepare_uretprobe() should be called every time we see
->rp_handler != NULL, there could be multiple consumers...

Oleg.

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

[Ananth N Mavinakayanahalli]

On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:
>   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))

What if ncopied < rasize? Agreed that the upper order bits can be 0, but should
you not validate ncopied == rasize?

Ananth

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

[Anton Arapov]

On Fri, Mar 01, 2013 at 11:15:36AM +0530, Ananth N Mavinakayanahalli wrote:
> On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:
> >   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))
> 
> What if ncopied < rasize? Agreed that the upper order bits can be 0, but should
> you not validate ncopied == rasize?

  Function returns 0 in case copy_from_user() was not able to copy
return address entirely, and "if (ncopied)" makes sure of it. We 
can't continue if we have no correct return address.

  copy_from_user() returns number of bytes that were *not* copied,
thus "ncopied == rasize" means copy_from_user() was not able to copy
*all* bytes. I don't see the point of such check here.

  Or am I missing anything?

thank you!
Anton.

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

[Ananth N Mavinakayanahalli]

On Fri, Mar 01, 2013 at 12:00:43PM +0100, Anton Arapov wrote:
> On Fri, Mar 01, 2013 at 11:15:36AM +0530, Ananth N Mavinakayanahalli wrote:
> > On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:

...

> > > +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))
> > 
> > What if ncopied < rasize? Agreed that the upper order bits can be 0, but should
> > you not validate ncopied == rasize?
> 
>   Function returns 0 in case copy_from_user() was not able to copy
> return address entirely, and "if (ncopied)" makes sure of it. We 
> can't continue if we have no correct return address.
> 
>   copy_from_user() returns number of bytes that were *not* copied,
> thus "ncopied == rasize" means copy_from_user() was not able to copy
> *all* bytes. I don't see the point of such check here.
> 
>   Or am I missing anything?

You are right... my bad.

Ananth

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

[Oleg Nesterov]

On 02/28, Anton Arapov wrote:
>
> @@ -1576,8 +1622,8 @@ static void handle_swbp(struct pt_regs *regs)
>  
>  	if (!uprobe) {
>  		if (is_swbp > 0) {
> -			/* No matching uprobe; signal SIGTRAP. */
> -			send_sig(SIGTRAP, current, 0);
> +			/* No matching uprobe; Try with uretprobe */
> +			handle_uretprobe(regs);

Hmm. at least this looks certainly wrong.

You shifted send_sig(SIGSEGV) into handle_uretprobe(), but if nothing
else printk(KERN_ERR "uprobe: no instance found!") doesn't look nice
if we hit the regular breakoint.

In fact everything handle_uretprobe() does in this case looks wrong.
Including the fact that get_xol_area/get_utask can fail. And unwinding
looks bogus...

I think you need

	if (bp_vaddr == rp_trampoline_vaddr)
		handle_uretprobe()

like the previous version did.

Oleg.

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

[Oleg Nesterov]

On 02/28, Anton Arapov wrote:
>
> +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> +{
> +	struct return_uprobe_i *ri;
> +	struct uprobe_task *utask;
> +	struct xol_area *area;
> +	unsigned long rp_trampoline_vaddr = 0;
> +	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> +
> +	area = get_xol_area();
> +	if (area)
> +		rp_trampoline_vaddr = area->rp_trampoline_vaddr;
> +	if (!rp_trampoline_vaddr) {
> +		rp_trampoline_vaddr = xol_get_insn_slot(&insn);
> +		if (!rp_trampoline_vaddr)
> +			return;
> +	}
> +	area->rp_trampoline_vaddr = rp_trampoline_vaddr;
> +
> +	ri = kzalloc(sizeof(struct return_uprobe_i), GFP_KERNEL);
> +	if (!ri)
> +		return;
> +
> +	utask = get_utask();
> +	ri->orig_ret_vaddr = arch_uretprobe_hijack_return_addr(rp_trampoline_vaddr, regs);
> +	if (likely(ri->orig_ret_vaddr)) {
> +		/* TODO: uretprobe bypass logic */
> +		atomic_inc(&uprobe->ref);

OK, but even this is not enough.

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.

So this patch needs the additional change in find_active_uprobe(),

	-	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
	+	if (!uprobe && hlist_empty(->return_uprobes) &&
	+	    test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))

Oleg.

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

[Oleg Nesterov]

On 02/28, Anton Arapov wrote:
>
> @@ -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;

Forgot to mention, at least you should also change uprobe_free_utask()
to cleanup this list, the task can exit from inside the ret-probe'd
function(s).

Oleg.

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

[Anton Arapov]

On Sun, Mar 03, 2013 at 05:40:29PM +0100, Oleg Nesterov wrote:
> On 02/28, Anton Arapov wrote:
> >
> > @@ -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;
> 
> Forgot to mention, at least you should also change uprobe_free_utask()
> to cleanup this list, the task can exit from inside the ret-probe'd
> function(s).

Yes, right. Thanks for the other mails as well. I will fix it for the
next RFC v4.

Anton.

> Oleg.
> 

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

[Anton Arapov]

On Thu, Feb 28, 2013 at 09:10:24PM +0100, Oleg Nesterov wrote:
> On 02/28, Anton Arapov wrote:
> >
> > +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> > +{
> > +	struct return_uprobe_i *ri;
> > +	struct uprobe_task *utask;
> > +	struct xol_area *area;
> > +	unsigned long rp_trampoline_vaddr = 0;
> > +	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> > +
> > +	area = get_xol_area();
> > +	if (area)
> > +		rp_trampoline_vaddr = area->rp_trampoline_vaddr;
> > +	if (!rp_trampoline_vaddr) {
> > +		rp_trampoline_vaddr = xol_get_insn_slot(&insn);
> > +		if (!rp_trampoline_vaddr)
> > +			return;
> > +	}
> > +	area->rp_trampoline_vaddr = rp_trampoline_vaddr;
> 
> This is called under down_read(), so 2 threads can race with each other
> and use the different rp_trampoline_vaddr's if ->rp_trampoline_vaddr was
> NULL.
> And again, I think ->rp_trampoline_vaddr is simply unneeded, see my
> reply to 3/6.
  Yes, 'fixed' this, I will send quirky v4 in a few, to maintain your attention. :)

> 
> >  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 */
> 
> Hmm. I didn't read this series yet. But at first glance I am not
> sure prepare_uretprobe() should be called every time we see
> ->rp_handler != NULL, there could be multiple consumers...

  I can not come up with a better idea yet, that would narrow down the
number of prepare_uretprobe() calls. 

Anton.