From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11942 invoked by alias); 28 Feb 2006 14:37:08 -0000 Received: (qmail 11930 invoked by uid 22791); 28 Feb 2006 14:37:07 -0000 X-Spam-Status: No, hits=-0.8 required=5.0 tests=AWL,BAYES_00,DNS_FROM_RFC_ABUSE,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from e36.co.us.ibm.com (HELO e36.co.us.ibm.com) (32.97.110.154) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 28 Feb 2006 14:37:04 +0000 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e36.co.us.ibm.com (8.12.11/8.12.11) with ESMTP id k1SEb2GV015193 for ; Tue, 28 Feb 2006 09:37:02 -0500 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.12.10/NCO/VER6.8) with ESMTP id k1SEdkl0161690 for ; Tue, 28 Feb 2006 07:39:46 -0700 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.12.11/8.13.3) with ESMTP id k1SEb2tZ022334 for ; Tue, 28 Feb 2006 07:37:02 -0700 Received: from newton.in.ibm.com ([9.124.35.47]) by d03av02.boulder.ibm.com (8.12.11/8.12.11) with ESMTP id k1SEb0O4022236; Tue, 28 Feb 2006 07:37:01 -0700 Received: by newton.in.ibm.com (Postfix, from userid 500) id 0456CCE3; Tue, 28 Feb 2006 20:08:36 +0530 (IST) Date: Tue, 28 Feb 2006 14:37:00 -0000 From: Prasanna S Panchamukhi To: Keshavamurthy Anil S Cc: systemtap@sources.redhat.com Subject: Re: [PATCH] Kprobes- robust fault handling for i386 Message-ID: <20060228143836.GA24545@in.ibm.com> Reply-To: prasanna@in.ibm.com References: <44BDAFB888F59F408FAE3CC35AB470410313D68D@orsmsx409> <20060227092535.GC6586@in.ibm.com> <20060227170155.A5891@unix-os.sc.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20060227170155.A5891@unix-os.sc.intel.com> User-Agent: Mutt/1.4.1i X-IsSubscribed: yes Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2006-q1/txt/msg00658.txt.bz2 Anil, Thanks for your review comments. Please see the updated patch below, this patch is only for i386 architecture and once we are ok with it, we will port it to other architectures. > > > Over all the the logic seems to good, except I did not > did not see where you are handling multiple sequenital faults > that can happen in pre/post handler. i.e once the fault happens > in say pre_handler, then the status goes to KPROBE_HIT_FAULT, > and say this fault is recovered and the pre_handler continues and > again before returning from pre_handler their can be another fault > and this fault is not being handed currently. The patch below takes care of multiple faults within the same pre/post_handler. > Also I did not see why you are not changing the status back to > original status if the fault is recovered properly. i.e > KPROBE_HIT_ACTIVE -> KPROBE_HIT_FAULT. In KPROBE_HIT_FAULT state > if this recovers, why not change this back to KPROBE_HIT_ACTIVE? > Anyreason for not doing this? > The only reason was to avoid post_handler being executed in case if the user-defined pre-handler faulted. Now the patch below avoids corresponding user-defined post_handler without introducing any new state. The main reason to avoid post_handler execution in this case is to avoid any incosistant data references between pre and post handlers. Thanks Prasanna This patch provides proper kprobes fault handling, if a user-specified pre/post handlers tries to access user address space, through copy_from_user(), get_user() etc. The user-specified fault handler gets called only if the fault occurs while executing user-specified handlers. In such a case user-specified handler is allowed to fix it first, later if the user-specifed fault handler does not fix it, we try to fix it by calling fix_exception(). Also we set the "kprobe_faulted" instance if user-specified pre handler faults, so that corresponding user-specified post_handler can be skipped. The user-specified handler will not be called if the fault happens when single stepping the original instruction, instead we reset the current probe and allow the system page fault handler to fix it up. Signed-off-by: Prasanna S Panchamukhi arch/i386/kernel/kprobes.c | 66 +++++++++++++++++++++++++++++++++++++++------ include/asm-i386/kprobes.h | 1 kernel/kprobes.c | 14 ++++++++- 3 files changed, 72 insertions(+), 9 deletions(-) diff -puN include/asm-i386/kprobes.h~kprobes-i386-pagefault-handling include/asm-i386/kprobes.h --- linux-2.6.16-rc4-mm2/include/asm-i386/kprobes.h~kprobes-i386-pagefault-handling 2006-02-28 18:00:20.000000000 +0530 +++ linux-2.6.16-rc4-mm2-prasanna/include/asm-i386/kprobes.h 2006-02-28 18:01:16.000000000 +0530 @@ -74,6 +74,7 @@ struct kprobe_ctlblk { long *jprobe_saved_esp; struct pt_regs jprobe_saved_regs; kprobe_opcode_t jprobes_stack[MAX_STACK_SIZE]; + struct kprobe *kprobe_faulted; struct prev_kprobe prev_kprobe; }; diff -puN arch/i386/kernel/kprobes.c~kprobes-i386-pagefault-handling arch/i386/kernel/kprobes.c --- linux-2.6.16-rc4-mm2/arch/i386/kernel/kprobes.c~kprobes-i386-pagefault-handling 2006-02-28 09:47:48.000000000 +0530 +++ linux-2.6.16-rc4-mm2-prasanna/arch/i386/kernel/kprobes.c 2006-02-28 19:34:20.000000000 +0530 @@ -35,6 +35,7 @@ #include #include #include +#include void jprobe_return_end(void); @@ -523,7 +524,8 @@ static inline int post_kprobe_handler(st if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) { kcb->kprobe_status = KPROBE_HIT_SSDONE; - cur->post_handler(cur, regs, 0); + if (kcb->kprobe_faulted != cur) + cur->post_handler(cur, regs, 0); } resume_execution(cur, regs, kcb); @@ -554,15 +556,63 @@ static inline int kprobe_fault_handler(s struct kprobe *cur = kprobe_running(); struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr)) - return 1; - - if (kcb->kprobe_status & KPROBE_HIT_SS) { - resume_execution(cur, regs, kcb); + switch(kcb->kprobe_status) { + case KPROBE_HIT_SS: + case KPROBE_REENTER: + /* + * We are here because the instruction being single + * stepped caused a page fault. We reset the current + * kprobe and the eip points back to the probe address + * and allow the page fault handler to continue as a + * normal page fault. + */ + regs->eip = (unsigned long)cur->addr; regs->eflags |= kcb->kprobe_old_eflags; - - reset_current_kprobe(); + if (kcb->kprobe_status == KPROBE_REENTER) + restore_previous_kprobe(kcb); + else + reset_current_kprobe(); preempt_enable_no_resched(); + break; + case KPROBE_HIT_ACTIVE: + /* + * Set appropriate kprobe instance, so that corresponding + * post_handler can be skipped in order to avoid any + * inconsistant data. + */ + kcb->kprobe_faulted = cur; + case KPROBE_HIT_SSDONE: + /* + * We increment the nmissed count for accounting, + * we can also use npre/npostfault count for accouting + * these specific fault cases. + */ + kprobes_inc_nmissed_count(cur); + + /* + * We come here because instructions in the pre/post + * handler caused the page_fault, this could happen + * if handler tries to access user space by + * copy_from_user(), get_user() etc. Let the + * user-specified handler try to fix it first. + */ + if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr)) + return 1; + + /* + * In case the user-specified fault handler returned + * zero, try to fix up. + */ + if (fixup_exception(regs)) + return 1; + + /* + * fixup_exception() could not handle it, + * Let do_page_fault() fix it. + */ + break; + default: + break; } return 0; } diff -puN kernel/kprobes.c~kprobes-i386-pagefault-handling kernel/kprobes.c --- linux-2.6.16-rc4-mm2/kernel/kprobes.c~kprobes-i386-pagefault-handling 2006-02-28 18:04:09.000000000 +0530 +++ linux-2.6.16-rc4-mm2-prasanna/kernel/kprobes.c 2006-02-28 19:27:33.000000000 +0530 @@ -208,9 +208,14 @@ static void __kprobes aggr_post_handler( unsigned long flags) { struct kprobe *kp; + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); list_for_each_entry_rcu(kp, &p->list, list) { - if (kp->post_handler) { + /* + * Check if the corresponding pre_handler had faulted, avoid + * the post_handler in such a case. + */ + if (kp->post_handler && (kcb->kprobe_faulted != kp)) { set_kprobe_instance(kp); kp->post_handler(kp, regs, flags); reset_kprobe_instance(); @@ -223,12 +228,19 @@ static int __kprobes aggr_fault_handler( int trapnr) { struct kprobe *cur = __get_cpu_var(kprobe_instance); + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); /* * if we faulted "during" the execution of a user specified * probe handler, invoke just that probe's fault handler */ if (cur && cur->fault_handler) { + /* + * Set kprobe_faulted to appropriate kprobe instance, so that + * corresponding post handler can be skipped if the fault + * happened due to pre_handler. + */ + kcb->kprobe_faulted = cur; if (cur->fault_handler(cur, regs, trapnr)) return 1; } _ -- Prasanna S Panchamukhi Linux Technology Center India Software Labs, IBM Bangalore Email: prasanna@in.ibm.com Ph: 91-80-51776329