lkml.org 
[lkml]   [2008]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86: kprobes change kprobe_handler flow
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: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com



\
 
 \ /
  Last update: 2008-01-02 19:13    [W:0.074 / U:0.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site