Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752866Ab3DMKHW (ORCPT ); Sat, 13 Apr 2013 06:07:22 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:60086 "EHLO e8.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752482Ab3DMKHU (ORCPT ); Sat, 13 Apr 2013 06:07:20 -0400 Date: Sat, 13 Apr 2013 15:31:13 +0530 From: Srikar Dronamraju To: Oleg Nesterov Cc: Anton Arapov , LKML , Josh Stone , Frank Eigler , Peter Zijlstra , Ingo Molnar , Ananth N Mavinakayanahalli , adrian.m.negreanu@intel.com, Torsten.Polle@gmx.de Subject: Re: [PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers Message-ID: <20130413100113.GC11721@linux.vnet.ibm.com> Reply-To: Srikar Dronamraju References: <1365004839-21982-1-git-send-email-anton@redhat.com> <1365004839-21982-7-git-send-email-anton@redhat.com> <20130407105308.GA2899@linux.vnet.ibm.com> <20130409140530.GA20577@redhat.com> <20130409201302.GA30570@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130409201302.GA30570@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13041310-9360-0000-0000-000011B9769A Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5459 Lines: 170 * Oleg Nesterov [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 > Signed-off-by: Oleg Nesterov Acked-by: Srikar Dronamraju > --- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/