Re: [PATCH] x86: kprobes change kprobe_handler flow

[Masami Hiramatsu <mhiramat@redhat.com>]

Hi,

Abhishek Sagar wrote:
>>>  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->eip - 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->eip -= sizeof(kprobe_opcode_t);
>>> +             return 1;
>>> +     }
> 
> I have moved the above breakpoint removal check at the beginning of
> the function. The current check is for '!p' case only, whereas IMO
> this check should be performed for all cases. An external debugger may
> very well plant and remove a breakpoint on an address which has probe
> on it. This check is a race nevertheless, so its relative placing
> within kprobe_handler() would be best where its duplication can be
> avoided.

I think there is another reason; now we have disable_all_kprobes() which
disarms(removes BREAKPOINT instruction) all kprobes. So, this should be commented.

By the way, IMHO, if a debugger causes it, that might be a bug.
Since when the debugger removes a breakpoint, it should prevent the exception
caused by the breakpoint on other processors. In other words, the debugger
should work as transparently as possible.

>>> -     /* Check we're not actually recursing */
>>> -     if (kprobe_running()) {
>>> -             p = get_kprobe(addr);
>>> -             if (p) {
>>> -                     if (kcb->kprobe_status == KPROBE_HIT_SS &&
>>> -                             *p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
>>> -                             regs->eflags &= ~TF_MASK;
>>> -                             regs->eflags |= kcb->kprobe_saved_eflags;
>>> -                             goto no_kprobe;
>>> -                     }
>>> -                     /* We have reentered the kprobe_handler(), since
>>> -                      * another probe was hit while within the handler.
>>> -                      * We here save the original kprobes variables and
>>> -                      * just single step on the instruction of the new probe
>>> -                      * without calling any user handlers.
>>> -                      */
>>> -                     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;
>>> -             } else {
>>> -                     if (*addr != BREAKPOINT_INSTRUCTION) {
>>> -                     /* The breakpoint instruction was removed by
>>> -                      * another cpu right after we hit, no further
>>> -                      * handling of this interrupt is appropriate
>>> -                      */
>>> -                             regs->eip -= sizeof(kprobe_opcode_t);
>>> +     if (p) {
>>> +             if (cur) {
>>> +                     switch (kcb->kprobe_status) {
>>> +                     case KPROBE_HIT_ACTIVE:
>>> +                     case KPROBE_HIT_SSDONE:
>>> +                             /* 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;
>>> -                             goto no_kprobe;
>>> -                     }
>>> -                     p = __get_cpu_var(current_kprobe);
>>> -                     if (p->break_handler && p->break_handler(p, regs)) {
>>> -                             goto ss_probe;
>>> +                             break;
>>> +                     case KPROBE_HIT_SS:
>>> +                             if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
>>> +                                     regs->eflags &= ~TF_MASK;
>>> +                                     regs->eflags |=
>>> +                                             kcb->kprobe_saved_eflags;
>>> +                             } else {
>>> +                                     /* BUG? */
>>> +                             }

I think whole of this case might have a bug. Since "p" is a recursing kprobe on
the instruction buffer of the running kprobe, *p->ainsn.insn is the next singlestep
target. I think KPROBE_HIT_SS should be treated as same as KPROBE_HIT_ACTIVE.

If the author thought a situation that a user inserts a kprobe on a breakpoint which
has been set by other debugger, we have to check it in !p case, because
get_kprobe(running_kprobe->ainsn.insn) will return NULL in that case.


>>> +                             break;
>>> +                     default:
>>> +                             /* impossible cases */
>>> +                             BUG();
>>>                       }
>>> -             }
> 
> Replaced deeply nested if-elses with a switch.

Originally, if (kcb->kprobe_status==KPROBE_HIT_SS) && !(*p->ainsn.insn == BREAKPOINT_INSTRUCTION),
it increments nmissed_count and change the status to KPROBE_REENTER. Please trace the flow carefully.

> 
> Please let me know if there are any changes on which you would like
> further clarification.
> 
>> --
>> Masami Hiramatsu
>>
>> Software Engineer
>> Hitachi Computer Products (America) Inc.
>> Software Solutions Division
>>
>> e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com
> --
> 
> Thanks,
> Abhishek Sagar

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com