lkml.org 
[lkml]   [2008]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] x86: kprobes change kprobe_handler flow
    Masami Hiramatsu wrote:
    >>> As a result, this function just setups re-entrance.
    >> As you've also pointed out in your previous reply, this case is
    >> peculiar and therefore I believe it should be marked as a BUG(). I've
    >> left the original case, if (kcb->kprobe_status==KPROBE_HIT_SS) &&
    >> (*p->ainsn.insn == BREAKPOINT_INSTRUCTION), untouched and is handled
    >> as it was before. However, if (kcb->kprobe_status==KPROBE_HIT_SS) &&
    >> !(*p->ainsn.insn == BREAKPOINT_INSTRUCTION), then instead of
    >> incrementing nmissed count like before, it should cry out a BUG. This
    >> is not an ordinary recursive probe handling case which should update
    >> the nmissed count.
    >
    > Hmm, I can not agree, because it is possible to insert a kprobe
    > into kprobe's instruction buffer. If it should be a bug, we must
    > check it when registering the kprobe.
    > (And also, in *p->ainsn.insn == BREAKPOINT_INSTRUCTION case, I doubt
    > that the kernel can handle this "orphaned" breakpoint, because the
    > breakpoint address has been changed.)
    >
    > Anyway, if you would like to change the logic, please separate it from
    > cleanup patch.

    Okay. For now, I've restored the original behavior.

    >>>> -out:
    >>>> + if (!ret)
    >>>> + preempt_enable_no_resched();
    >>> I think this is a bit ugly...
    >>> Why would you avoid using mutiple "return"s in this function?
    >>> I think you can remove "ret" from this function.
    >> Hmm...there the are no deeply nested if-elses nor overlapping paths
    >> which need to be avoided. All the nested checks unroll cleanly too.
    >
    > Oh, I just mentioned about this "if (!ret)" condition.
    > Could you try to remove this "ret" variable?
    > I think some "ret=1" could be replaced by "return 1" easily.

    Done. You should find the desired changed in this patch.

    Signed-off-by: Abhishek Sagar <sagar.abhishek@gmail.com>
    Signed-off-by: Quentin Barnes <qbarnes@gmail.com>
    ---

    diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
    index a72e02b..06f8d08 100644
    --- a/arch/x86/kernel/kprobes.c
    +++ b/arch/x86/kernel/kprobes.c
    @@ -441,6 +441,34 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
    /* Replace the return addr with trampoline addr */
    *sara = (unsigned long) &kretprobe_trampoline;
    }
    +
    +static void __kprobes recursive_singlestep(struct kprobe *p,
    + struct pt_regs *regs,
    + struct kprobe_ctlblk *kcb)
    +{
    + save_previous_kprobe(kcb);
    + set_current_kprobe(p, regs, kcb);
    + kprobes_inc_nmissed_count(p);
    + prepare_singlestep(p, regs);
    + kcb->kprobe_status = KPROBE_REENTER;
    +}
    +
    +static void __kprobes setup_singlestep(struct kprobe *p, struct pt_regs *regs,
    + struct kprobe_ctlblk *kcb)
    +{
    +#if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
    + if (p->ainsn.boostable == 1 && !p->post_handler) {
    + /* Boost up -- we can execute copied instructions directly */
    + reset_current_kprobe();
    + regs->ip = (unsigned long)p->ainsn.insn;
    + preempt_enable_no_resched();
    + return;
    + }
    +#endif
    + prepare_singlestep(p, regs);
    + kcb->kprobe_status = KPROBE_HIT_SS;
    +}
    +
    /*
    * We have reentered the kprobe_handler(), since another probe was hit while
    * within the handler. We save the original kprobes variables and just single
    @@ -449,13 +477,9 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
    static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
    struct kprobe_ctlblk *kcb)
    {
    - if (kcb->kprobe_status == KPROBE_HIT_SS &&
    - *p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
    - regs->flags &= ~X86_EFLAGS_TF;
    - regs->flags |= kcb->kprobe_saved_flags;
    - return 0;
    + switch (kcb->kprobe_status) {
    + case KPROBE_HIT_SSDONE:
    #ifdef CONFIG_X86_64
    - } else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
    /* TODO: Provide re-entrancy from post_kprobes_handler() and
    * avoid exception stack corruption while single-stepping on
    * the instruction of the new probe.
    @@ -463,14 +487,26 @@ static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
    arch_disarm_kprobe(p);
    regs->ip = (unsigned long)p->addr;
    reset_current_kprobe();
    - return 1;
    + preempt_enable_no_resched();
    + break;
    #endif
    + case KPROBE_HIT_ACTIVE:
    + recursive_singlestep(p, regs, kcb);
    + break;
    + case KPROBE_HIT_SS:
    + if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
    + regs->flags &= ~TF_MASK;
    + regs->flags |= kcb->kprobe_saved_flags;
    + return 0;
    + } else {
    + recursive_singlestep(p, regs, kcb);
    + }
    + break;
    + default:
    + /* impossible cases */
    + WARN_ON(1);
    }
    - save_previous_kprobe(kcb);
    - set_current_kprobe(p, regs, kcb);
    - kprobes_inc_nmissed_count(p);
    - prepare_singlestep(p, regs);
    - kcb->kprobe_status = KPROBE_REENTER;
    +
    return 1;
    }

    @@ -480,83 +516,66 @@ static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
    */
    static int __kprobes kprobe_handler(struct pt_regs *regs)
    {
    - struct kprobe *p;
    - int ret = 0;
    kprobe_opcode_t *addr;
    + struct kprobe *p;
    struct kprobe_ctlblk *kcb;

    addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));
    + if (*addr != BREAKPOINT_INSTRUCTION) {
    + /*
    + * The breakpoint instruction was removed right
    + * after we hit it. Another cpu has removed
    + * either a probepoint or a debugger breakpoint
    + * at this address. In either case, no further
    + * handling of this interrupt is appropriate.
    + * Back up over the (now missing) int3 and run
    + * the original instruction.
    + */
    + regs->ip = (unsigned long)addr;
    + return 1;
    + }

    /*
    * We don't want to be preempted for the entire
    - * duration of kprobe processing
    + * duration of kprobe processing. We conditionally
    + * re-enable preemption at the end of this function,
    + * and also in reenter_kprobe() and setup_singlestep().
    */
    preempt_disable();
    - kcb = get_kprobe_ctlblk();

    + kcb = get_kprobe_ctlblk();
    p = get_kprobe(addr);
    +
    if (p) {
    - /* Check we're not actually recursing */
    if (kprobe_running()) {
    - ret = reenter_kprobe(p, regs, kcb);
    - if (kcb->kprobe_status == KPROBE_REENTER)
    - {
    - ret = 1;
    - goto out;
    - }
    - goto preempt_out;
    + if (reenter_kprobe(p, regs, kcb))
    + return 1;
    } else {
    set_current_kprobe(p, regs, kcb);
    kcb->kprobe_status = KPROBE_HIT_ACTIVE;
    - if (p->pre_handler && p->pre_handler(p, regs))
    - {
    - /* handler set things up, skip ss setup */
    - ret = 1;
    - goto out;
    - }
    - }
    - } else {
    - if (*addr != BREAKPOINT_INSTRUCTION) {
    +
    /*
    - * The breakpoint instruction was removed right
    - * after we hit it. Another cpu has removed
    - * either a probepoint or a debugger breakpoint
    - * at this address. In either case, no further
    - * handling of this interrupt is appropriate.
    - * Back up over the (now missing) int3 and run
    - * the original instruction.
    + * If we have no pre-handler or it returned 0, we
    + * continue with normal processing. If we have a
    + * pre-handler and it returned non-zero, it prepped
    + * for calling the break_handler below on re-entry
    + * for jprobe processing, so get out doing nothing
    + * more here.
    */
    - regs->ip = (unsigned long)addr;
    - ret = 1;
    - goto preempt_out;
    + if (!p->pre_handler || !p->pre_handler(p, regs))
    + setup_singlestep(p, regs, kcb);
    + return 1;
    }
    - if (kprobe_running()) {
    - p = __get_cpu_var(current_kprobe);
    - if (p->break_handler && p->break_handler(p, regs))
    - goto ss_probe;
    + } else if (kprobe_running()) {
    + p = __get_cpu_var(current_kprobe);
    + if (p->break_handler && p->break_handler(p, regs)) {
    + setup_singlestep(p, regs, kcb);
    + return 1;
    }
    - /* Not one of ours: let kernel handle it */
    - goto preempt_out;
    - }
    + } /* else: not a kprobe fault; let the kernel handle it */

    -ss_probe:
    - ret = 1;
    -#if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
    - if (p->ainsn.boostable == 1 && !p->post_handler) {
    - /* Boost up -- we can execute copied instructions directly */
    - reset_current_kprobe();
    - regs->ip = (unsigned long)p->ainsn.insn;
    - goto preempt_out;
    - }
    -#endif
    - prepare_singlestep(p, regs);
    - kcb->kprobe_status = KPROBE_HIT_SS;
    - goto out;
    -
    -preempt_out:
    preempt_enable_no_resched();
    -out:
    - return ret;
    + return 0;
    }

    /*



    \
     
     \ /
      Last update: 2008-01-03 19:21    [W:0.034 / U:30.200 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site