Re: [PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

* Oleg Nesterov <oleg@redhat.com> [2013-04-09 22:13:02]:

> On 04/09, Oleg Nesterov wrote:
> >
> > > Should we a check here before using top most ri.
> > > What if the application had done a longjmp and the trampoline he hit
> > > corresponds to something thats below in the stack?
> > >
> > > Not sure if this what you meant by leaking return instances in your next
> > > patch.
> >
> > Oh yes, this should be documented more explicitly in the changelog of
> > this patch or 7/9 (which tries to document the limitations but should
> > be more clear).
> >
> > Currently we do not support longjmp() and we assume that the probed
> > function should do the regular return. We should certainly try to improve
> > this, but I really think that this should go into the next series.
> >
> > Because this is nontrivial, needs more discussion, and I'm afraid should
> > be per-arch. Even on x86 (which can check the stack) this is not simple,
> > in general we can't know how to check that (to simplify) the first frame
> > is already invalid. Just for example, we could check regs->sp and detect
> > that longjmp() was called but sigaltstack() can easily fool this logic.
> >

Yes, its perfectly fine to keep this logic for the next patchset.
Can you tell me why sigaltstack() can fool us if we rely on regs->sp.
I should admit that I am not too familiar with sigaltstack.

> > Or we can change prepare_uretprobe() to alloc the new slot for the
> > trampoline every time (and mark it as "trampoline" for handle_swbp() of
> > course), this way we can easily discard the invalid ret_instance's in
> > handler_uretprobe(). But this doesn't solve all problems and this is
> > not really nice/simple.
> >
> > In short. I think we should document the limitations more clearly, push
> > this functionality, then try to improve things. Do you agree?
> 
> IOW. Will you agree with v2 below?
> 
> Changes:
> 
> 	- s/handler_uretprobe/handle_trampoline/
> 
> 	- s/handler_uretprobe_chain/handle_uretprobe_chain/
> 
> 	- add the TODO: comments into the changelog and
> 	  handle_trampoline().
> 
> 
> -------------------------------------------------------------------------------
> [PATCH v2] uretprobes: Return probe exit, invoke handlers
> 
> 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.
> 
> TODO: handle_trampoline() assumes that ->return_instances is always valid.
> We should teach it to handle longjmp() which can invalidate the pending
> return_instance's. This is nontrivial, we will try to do this in a separate
> series.
> 
> Signed-off-by: Anton Arapov <anton@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  kernel/events/uprobes.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 64 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index e352e18..b9c4325 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1633,6 +1633,62 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>  	up_read(&uprobe->register_rwsem);
>  }
> 
> +static void
> +handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> +{
> +	struct uprobe *uprobe = ri->uprobe;
> +	struct uprobe_consumer *uc;
> +
> +	down_read(&uprobe->register_rwsem);
> +	for (uc = uprobe->consumers; uc; uc = uc->next) {
> +		if (uc->ret_handler)
> +			uc->ret_handler(uc, ri->func, regs);
> +	}
> +	up_read(&uprobe->register_rwsem);
> +}
> +
> +static bool handle_trampoline(struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask;
> +	struct return_instance *ri, *tmp;
> +	bool chained;
> +
> +	utask = current->utask;
> +	if (!utask)
> +		return false;
> +
> +	ri = utask->return_instances;
> +	if (!ri)
> +		return false;
> +
> +	/*
> +	 * TODO: we should throw out return_instance's invalidated by
> +	 * longjmp(), currently we assume that the probed function always
> +	 * returns.
> +	 */
> +	instruction_pointer_set(regs, ri->orig_ret_vaddr);
> +
> +	for (;;) {
> +		handle_uretprobe_chain(ri, regs);
> +
> +		chained = ri->chained;
> +		put_uprobe(ri->uprobe);
> +
> +		tmp = ri;
> +		ri = ri->next;
> +		kfree(tmp);
> +
> +		if (!chained)
> +			break;
> +
> +		BUG_ON(!ri);
> +	}
> +
> +	utask->return_instances = ri;
> +
> +	return true;
> +}
> +
>  /*
>   * Run handler and ask thread to singlestep.
>   * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
> @@ -1644,8 +1700,15 @@ static void handle_swbp(struct pt_regs *regs)
>  	int uninitialized_var(is_swbp);
> 
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
> -	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> +	if (bp_vaddr == get_trampoline_vaddr()) {
> +		if (handle_trampoline(regs))
> +			return;
> 
> +		pr_warn("uprobe: unable to handle uretprobe pid/tgid=%d/%d\n",
> +						current->pid, current->tgid);
> +	}
> +
> +	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
>  	if (!uprobe) {
>  		if (is_swbp > 0) {
>  			/* No matching uprobe; signal SIGTRAP. */
> -- 
> 1.5.5.1
> 
> 

-- 
Thanks and Regards
Srikar Dronamraju