Re: [PATCH] x86: kprobes change kprobe_handler flow

[Masami Hiramatsu <mhiramat@redhat.com>]

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