Hello Abhishek,
Abhishek Sagar wrote:
> Harvey Harrison wrote:
>> Make the control flow of kprobe_handler more obvious.
>>
>> Collapse the separate if blocks/gotos with if/else blocks
>> this unifies the duplication of the check for a breakpoint
>> instruction race with another cpu.
>
> This is a patch derived from kprobe_handler of the ARM kprobes
> port. This further simplifies the current x86 kprobe_handler.
> The resulting definition is smaller, more readable, has no
> goto's and contains only a single call to get_kprobe.
Thank you.
As far as I can see, this patch did more than cleanup.
Could you separate changing logic from cleanup and explain
why the logic should be changed?
That will be very useful for reviewers.
> Signed-off-by: Abhishek Sagar <[EMAIL PROTECTED]>
> Signed-off-by: Quentin Barnes <[EMAIL PROTECTED]>
> ---
>
> diff --git a/arch/x86/kernel/kprobes_32.c b/arch/x86/kernel/kprobes_32.c
> index 3a020f7..5a626fd 100644
> --- a/arch/x86/kernel/kprobes_32.c
> +++ b/arch/x86/kernel/kprobes_32.c
> @@ -240,108 +240,110 @@ void __kprobes arch_prepare_kretprobe(struct
> kretprobe_instance *ri,
> *sara = (unsigned long) &kretprobe_trampoline;
> }
>
> +static __always_inline void 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->eip = (unsigned long)p->ainsn.insn;
> + preempt_enable_no_resched();
> + } else {
> + prepare_singlestep(p, regs);
> + kcb->kprobe_status = KPROBE_HIT_SS;
> + }
> +#else
> + prepare_singlestep(p, regs);
> + kcb->kprobe_status = KPROBE_HIT_SS;
please avoid code duplication.
> +#endif
> +}
> +
> /*
> * Interrupts are disabled on entry as trap3 is an interrupt gate and they
> * remain disabled thorough out this function.
> */
> 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;
> + }
>
> - /*
> - * We don't want to be preempted for the entire
> - * duration of kprobe processing
> - */
> preempt_disable();
> kcb = get_kprobe_ctlblk();
> + cur = kprobe_running();
> + p = get_kprobe(addr);
>
> - /* 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? */
> + }
> + break;
> + default:
> + /* impossible cases */
> + BUG();
> }
> - }
> - goto no_kprobe;
> - }
> + } else {
> + set_current_kprobe(p, regs, kcb);
> + kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>
> - p = get_kprobe(addr);
> - if (!p) {
> - 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->eip -= sizeof(kprobe_opcode_t);
> + if (!p->pre_handler || !p->pre_handler(p, regs))
> + setup_singlestep(p, regs, kcb);
> ret = 1;
> }
> - /* Not one of ours: let kernel handle it */
> - goto no_kprobe;
> - }
> -
> - set_current_kprobe(p, regs, kcb);
> - kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> -
> - if (p->pre_handler && p->pre_handler(p, regs))
> - /* handler has already set things up, so skip ss setup */
> - return 1;
> + } else if (cur) {
> + p = __get_cpu_var(current_kprobe);
> + if (p->break_handler && p->break_handler(p, regs)) {
> + setup_singlestep(p, regs, kcb);
> + ret = 1;
> + }
> + } /* else: not a kprobe fault; let the kernel handle it */
>
> -ss_probe:
> -#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->eip = (unsigned long)p->ainsn.insn;
> + if (!ret)
> preempt_enable_no_resched();
> - return 1;
> - }
> -#endif
> - prepare_singlestep(p, regs);
> - kcb->kprobe_status = KPROBE_HIT_SS;
> - return 1;
> -
> -no_kprobe:
> - preempt_enable_no_resched();
> return ret;
> }
>
> diff --git a/arch/x86/kernel/kprobes_64.c b/arch/x86/kernel/kprobes_64.c
> index 5df19a9..788114c 100644
> --- a/arch/x86/kernel/kprobes_64.c
> +++ b/arch/x86/kernel/kprobes_64.c
> @@ -278,30 +278,47 @@ void __kprobes arch_prepare_kretprobe(struct
> kretprobe_instance *ri,
> *sara = (unsigned long) &kretprobe_trampoline;
> }
>
> -int __kprobes kprobe_handler(struct pt_regs *regs)
> +static int __kprobes kprobe_handler(struct pt_regs *regs)
> {
> - struct kprobe *p;
> int ret = 0;
> - kprobe_opcode_t *addr = (kprobe_opcode_t *)(regs->rip -
> sizeof(kprobe_opcode_t));
> + kprobe_opcode_t *addr;
> + struct kprobe *p, *cur;
> struct kprobe_ctlblk *kcb;
>
> - /*
> - * We don't want to be preempted for the entire
> - * duration of kprobe processing
> - */
> + addr = (kprobe_opcode_t *)(regs->rip - 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->rip = (unsigned long)addr;
> + return 1;
> + }
> +
> preempt_disable();
> kcb = get_kprobe_ctlblk();
> + cur = kprobe_running();
> + p = get_kprobe(addr);
>
> - /* 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_rflags;
> - goto no_kprobe;
> - } else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
> + if (p) {
> + if (cur) {
> + switch (kcb->kprobe_status) {
> + 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_SSDONE:
> /* TODO: Provide re-entrancy from
> * post_kprobes_handler() and avoid exception
> * stack corruption while single-stepping on
> @@ -311,72 +328,49 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
> regs->rip = (unsigned long)p->addr;
> reset_current_kprobe();
> ret = 1;
> - } else {
> - /* We have reentered the kprobe_handler(), since
> - * another probe was hit while within the
> - * handler. We here save the original kprobe
> - * variables and just single step on 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;
> + break;
> + case KPROBE_HIT_SS:
> + if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> + regs->eflags &= ~TF_MASK;
> + regs->eflags |=
> + kcb->kprobe_saved_eflags;
> + } else {
> + /* BUG? */
> + }
> + break;
> + default:
> + /* impossible cases */
> + BUG();
> }
> } 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->rip = (unsigned long)addr;
> - ret = 1;
> - goto no_kprobe;
> - }
> - p = __get_cpu_var(current_kprobe);
> - if (p->break_handler && p->break_handler(p, regs)) {
> - goto ss_probe;
> - }
> - }
> - goto no_kprobe;
> - }
> + set_current_kprobe(p, regs, kcb);
> + kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>
> - p = get_kprobe(addr);
> - if (!p) {
> - 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->rip = (unsigned long)addr;
> + if (!p->pre_handler || !p->pre_handler(p, regs)) {
> + prepare_singlestep(p, regs);
> + kcb->kprobe_status = KPROBE_HIT_SS;
> + }
> ret = 1;
> }
> - /* Not one of ours: let kernel handle it */
> - goto no_kprobe;
> - }
> -
> - set_current_kprobe(p, regs, kcb);
> - kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> -
> - if (p->pre_handler && p->pre_handler(p, regs))
> - /* handler has already set things up, so skip ss setup */
> - return 1;
> -
> -ss_probe:
> - prepare_singlestep(p, regs);
> - kcb->kprobe_status = KPROBE_HIT_SS;
> - return 1;
> + } else if (cur) {
> + p = __get_cpu_var(current_kprobe);
> + if (p->break_handler && p->break_handler(p, regs)) {
> + prepare_singlestep(p, regs);
> + kcb->kprobe_status = KPROBE_HIT_SS;
> + ret = 1;
> + }
> + } /* else: not a kprobe fault; let the kernel handle it */
>
> -no_kprobe:
> - preempt_enable_no_resched();
> + if (!ret)
> + preempt_enable_no_resched();
> return ret;
> }
>
>
--
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/
