Re: [PATCH] x86: kprobes change kprobe_handler flow

["Abhishek Sagar" <sagar.abhishek@gmail.com>]

On 1/2/08, Harvey Harrison <harvey.harrison@gmail.com> wrote:
> > +#if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
> > +static __always_inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
> > +{
> > +     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 0;
> > +     }
> > +     return 1;
> > +}
> > +#else
> > +static __always_inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
> > +{
> > +     return 1;
> > +}
> > +#endif
> > +
> In the kernel __always_inline == inline, also I think it's nicer to only
> have one function declaration, and then ifdef the body of the function.
>
> Something like:
>
> static inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
> {
> #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 0;
>         }
> #endif
>         return 1;
> }

Ok...will include this after I pick up some more comments.

> >  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;
> > +     }
>
> This return is fine I guess, but after the preempt_disable() I like
> the goto approach as it will be easier to see what paths enable
> preemption again and which don't....bonus points if we can move this
> to the caller or make sure we reenable in all cases before returning
> and pull in the code in the caller that does this for us.
>
> But I guess your approach of using ret to test whether we need to
> reenable preemption or not would work as a signal to the caller that
> they need to reenable preemption.

Hmm...since enabling preemption is tied to 'ret', anyone reading
kprobe_handler will have to follow around all calls which modify it.
There are some checks in the current kprobe_handler definition made
just to do what you're saying, i.e, to push all preemption
enable/disables in krpobe_handler. LIke this one (from the current x86
kprobe_handler):

------------
ret = reenter_kprobe(p, regs, kcb);
if (kcb->kprobe_status == KPROBE_REENTER)
{
    ret = 1;
    goto out;
}
goto preempt_out;

-------------

This is just confusing because we're not actually making any
exceptions here for the KPROBE_REENTER case (which has been partially
handled in reenter_kprobe), rather just tricking our way out of
preemption enabling for a cpl of cases in reenter_kprobe.

> Cheers,
>
> Harvey

Thanks,
Abhishek