Re: [PATCH] x86: kprobes change kprobe_handler flow

[Abhishek Sagar]

On 1/2/08, Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> I think setup_singlestep() in your first patch is better, because it avoided
> code duplication(*).

Will retain it then.

> >  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;
> > +     int ret = 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
> > +             /* TODO: Provide re-entrancy from
> > +              * post_kprobes_handler() and avoid exception
> > +              * stack corruption while single-stepping on
>
> Why would you modify it?

Do you mean the comment? I had this code in kprobe_handler earlier and
it consistently exceeded 80 columns in my case. Will fix it anyways.

> > +             ret = 1;
> > +             break;
> >  #endif
> > +     case KPROBE_HIT_ACTIVE:
> > +             /* a probe has been hit inside a
> > +              * user handler */
> > +             save_previous_kprobe(kcb);
> > +             set_current_kprobe(p, regs, kcb);
> > +             kprobes_inc_nmissed_count(p);
> > +             prepare_singlestep(p, regs);
> > +             kcb->kprobe_status = KPROBE_REENTER;
> > +             ret = 1;
> > +             break;
> > +     case KPROBE_HIT_SS:
> > +             if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> > +                     regs->flags &= ~TF_MASK;
> > +                     regs->flags |= kcb->kprobe_saved_flags;
> > +             } else {
> > +                     /* BUG? */
> > +             }
> > +             break;
>
> If my thought is correct, we don't need to use swich-case here,
> Because there are only 2 cases, KPROBE_HIT_SSDONE (x86-64 only)
> or others.
> 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.

> > +     default:
> > +             /* impossible cases */
> > +             BUG();
>
> I think no need to check that here.

It may not get hit at runtime but is quite informative.

> >  static int __kprobes kprobe_handler(struct pt_regs *regs)
> >  {
> > -     struct kprobe *p;
> >       int ret = 0;
> >       kprobe_opcode_t *addr;
> > +     struct kprobe *p, *cur;
> >       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;
> > +     }
>
> I think this block changing would better be separated from this patch,
> because it changes code flow logically.

Agreed. I'll address this in another thread, but for now it is safest
to check it for all cases right at the entry of kprobe_handler().

> >
> > -     /*
> > -      * We don't want to be preempted for the entire
> > -      * duration of kprobe processing
> > -      */
> >       preempt_disable();
> >       kcb = get_kprobe_ctlblk();
> > -
> > +     cur = kprobe_running();
>
> I think you don't need "cur", because kprobe_running() is called
> just once on each path.

Will get rid of that.

> > -                     regs->ip = (unsigned long)addr;
> > +                     if (!p->pre_handler || !p->pre_handler(p, regs)) {
> > +                             if (setup_boost(p, regs)) {
> > +                                     prepare_singlestep(p, regs);
> > +                                     kcb->kprobe_status = KPROBE_HIT_SS;
>
> (*) duplication 1
>
> > +                             }
> > +                     }
> >                       ret = 1;
> > -                     goto preempt_out;
> >               }
> > -             if (kprobe_running()) {
> > -                     p = __get_cpu_var(current_kprobe);
> > -                     if (p->break_handler && p->break_handler(p, regs))
> > -                             goto ss_probe;
> > +     } else if (cur) {
> > +             p = __get_cpu_var(current_kprobe);
> > +             if (p->break_handler && p->break_handler(p, regs)) {
> > +                     if (setup_boost(p, regs)) {
> > +                             prepare_singlestep(p, regs);
> > +                             kcb->kprobe_status = KPROBE_HIT_SS;
>
> (*) duplication 2

Will replace it with setup_singlestep().

> > +                     }
> > +                     ret = 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:
> > +     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.

> Thank you,
>
> --
> Masami Hiramatsu
>
> Software Engineer
> Hitachi Computer Products (America) Inc.
> Software Solutions Division
>
> e-mail: mhiramat@xxxxxxxxxx, masami.hiramatsu.pt@xxxxxxxxxxx
--

Thanks for your review!
Abhishek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/