Hi Abhishek, Thank you for good work.
Abhishek Sagar wrote: > @@ -441,6 +441,26 @@ void __kprobes arch_prepare_kretprobe(struct > kretprobe_instance *ri, > /* Replace the return addr with trampoline addr */ > *sara = (unsigned long) &kretprobe_trampoline; > } > + > +#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 I think setup_singlestep() in your first patch is better, because it avoided code duplication(*). > + > /* > * 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,29 +469,47 @@ 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; > + 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? > * the instruction of the new probe. > */ > arch_disarm_kprobe(p); > regs->ip = (unsigned long)p->addr; > reset_current_kprobe(); > - return 1; > + preempt_enable_no_resched(); I think preepmt_enable here is good idea. > + 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. > + default: > + /* impossible cases */ > + BUG(); I think no need to check that here. > } > - 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; > + > + return ret; > } > > /* > @@ -480,82 +518,67 @@ 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, *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. > > - /* > - * 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. > p = get_kprobe(addr); > + > if (p) { > - /* Check we're not actually recursing */ > - if (kprobe_running()) { > + if (cur) { > ret = reenter_kprobe(p, regs, kcb); > - if (kcb->kprobe_status == KPROBE_REENTER) > - { > - ret = 1; > - goto out; > - } > - goto preempt_out; > } 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; > + 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 > + } > + 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. > return ret; > } > > Thank you, -- Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [EMAIL PROTECTED], [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/