This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH] Kprobes- robust fault handling for i386
On Tue, Feb 28, 2006 at 06:38:36AM -0800, Prasanna S Panchamukhi wrote:
>
> 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.
This version looks good with no new Kprobes states.
Makes life easy to understand :-)
> [..]The main reason to avoid post_handler execution in this
> case is to avoid any incosistant data references between pre and post
> handlers.
Okay, I got that point, but if the fault recovery in pre_handler
is *successful*, then in this case you *should* permit calling
post_handler. See my inline comments to address this issue.
Thanks,
Anil
>
> Signed-off-by: Prasanna S Panchamukhi <prasanna@in.ibm.com>
>
> 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;
> };
Good approach.
>
> 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 <asm/cacheflush.h>
> #include <asm/kdebug.h>
> #include <asm/desc.h>
> +#include <asm/uaccess.h>
>
> 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;
if the fault recovery is successful, before returning 1, you
need to reset kcb->kprobe_faulted to NULL;
> +
> + /*
> + * In case the user-specified fault handler returned
> + * zero, try to fix up.
> + */
> + if (fixup_exception(regs))
> + return 1;
same here, before returning 1, you need to reset kcb->kprobe_faulted to NULL;
> +
> + /*
> + * 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;
Move this kcb->kprobe_faulted = cur; before if(curr && cur->handler) {}
The reason is, irrespective of cur->fault_handler, we need to save
kcb->kprobe_faulted, so post handler can be skipped properly.
> 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