This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]