* Oleg Nesterov <o...@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 <an...@redhat.com>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
Acked-by: Srikar Dronamraju <sri...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
