Re: [PATCH] x86: kprobes change kprobe_handler flow

[Masami Hiramatsu <mhiramat@redhat.com>]

Abhishek Sagar wrote:
> Masami Hiramatsu wrote:
...
> Done. You should find the desired changed in this patch.

Well done!
This cleans it up very well.
I have just one more comment.

> @@ -463,14 +487,26 @@ static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
>  		arch_disarm_kprobe(p);
>  		regs->ip = (unsigned long)p->addr;
>  		reset_current_kprobe();
> -		return 1;
> +		preempt_enable_no_resched();
> +		break;
>  #endif
> +	case KPROBE_HIT_ACTIVE:
> +		recursive_singlestep(p, regs, kcb);
> +		break;
> +	case KPROBE_HIT_SS:
> +		if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> +			regs->flags &= ~TF_MASK;
> +			regs->flags |= kcb->kprobe_saved_flags;
> +			return 0;
> +		} else {
> +			recursive_singlestep(p, regs, kcb);
> +		}
> +		break;
> +	default:
> +		/* impossible cases */
> +		WARN_ON(1);

WARN_ON() does not call panic(). Since the kernel doesn't stop,
we need to prepare singlestep after that.

How about this?
---
+	case KPROBE_HIT_ACTIVE:
+		break;
+	case KPROBE_HIT_SS:
+		if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
+			regs->flags &= ~TF_MASK;
+			regs->flags |= kcb->kprobe_saved_flags;
+			return 0;
+		}
+		break;
+	default:
+		/* impossible cases */
+		WARN_ON(1);
 	}
 	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;
 }
---


Thank you,

-- 
Masami Hiramatsu

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

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