x86: kprobes change kprobe_handler flow

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.

Quoted 5 lines · Harvey Harrison

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.

hm, this patch does not apply to x86.git#mm, due to the fixes, unifications and cleanups done there. Could you send a patch against -mm or against x86.git? (see the tree-fetching instructions below) Thanks,

Ingo

--------------{ x86.git instructions }---------->

git-clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git linux-2.6.git cd linux-2.6.git
git-branch x86
git-checkout x86
git-pull git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git mm

(do subsequent pulls via "git-pull --force", as we frequently rebase the git tree. NOTE: this might override your own local changes, so do this only if you dont mind about losing thse changes in that tree.)

Thanks for pointing me to the right tree. I have made some code re-arrangements in this one.

Signed-off-by Abhishek Sagar <[email protected]>

Signed-off-by Quentin Barnes <[email protected]>

---

arch/x86/kernel/kprobes.c +10 -0

index a72e02b..45adc8e 100644
@@ -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;

arch/x86/kernel/kprobes.c +37 -16

 /*
  * 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
 		 * 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();
+		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;
+	default:
+		/* impossible cases */
+		BUG();
 	}
-	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

arch/x86/kernel/kprobes.c +25 -50

-	/*
-	 * 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);
+
 	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;
+				}
+			}
 			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;
+			}
+			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;

Quoted 9 lines

arch/x86/kernel/kprobes.c +9 -0

+#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;

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 463

Quoted 10 lines

arch/x86/kernel/kprobes.c +10 -0

+     }
+     return 1;
+}
+#else
+static __always_inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
+{
+     return 1;
+}
+#endif
+

In the kernel __always_inline == inline, also I think it's nicer to only have one function declaration, and then ifdef the body of the function.

Something like:

static inline int setup_boost(struct kprobe *p, struct pt_regs *regs) {

#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;
                preempt_enable_no_resched();
                return 0;
        }
#endif
        return 1;
}

Quoted 12 lines

static int __kprobes kprobe_handler(struct pt_regs *regs) {

arch/x86/kernel/kprobes.c +4 -1

-     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

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 539

Quoted 10 lines

arch/x86/kernel/kprobes.c +10 -0

+              * 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;
+     }

This return is fine I guess, but after the preempt_disable() I like the goto approach as it will be easier to see what paths enable preemption again and which don't....bonus points if we can move this to the caller or make sure we reenable in all cases before returning and pull in the code in the caller that does this for us.

But I guess your approach of using ret to test whether we need to reenable preemption or not would work as a signal to the caller that they need to reenable preemption.

Cheers,

Harvey

arch/x86/kernel/kprobes.c +9 -0

@@ -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();

arch/x86/kernel/kprobes.c +2 -5

+
 /*
  * 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;

arch/x86/kernel/kprobes.c +10 -0

+		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;

arch/x86/kernel/kprobes.c +6 -7

 	}
-	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

arch/x86/kernel/kprobes.c +5 -24

 	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

arch/x86/kernel/kprobes.c +2 -1

+				}
+			}
 			ret = 1;
-			goto preempt_out;
 		}

arch/x86/kernel/kprobes.c +3 -12

+			}
+			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;
-	}

I think setup_singlestep() in your first patch is better, because it avoided code duplication(*).

Quoted 9 lines

static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
struct kprobe_ctlblk *kcb)
{

arch/x86/kernel/kprobes.c +1 -5

-     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;

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 479

Quoted 10 lines

arch/x86/kernel/kprobes.c +6 -3

+
+     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?

Quoted 11 lines

arch/x86/kernel/kprobes.c +10 -0

+             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;

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 506

Quoted 10 lines

arch/x86/kernel/kprobes.c +10 -0

+             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.

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 509

Quoted 3 lines

arch/x86/kernel/kprobes.c +3 -0

+     default:
+             /* impossible cases */
+             BUG();

I think no need to check that here.

Quoted 12 lines

static int __kprobes kprobe_handler(struct pt_regs *regs) {

arch/x86/kernel/kprobes.c +4 -1

-     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

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 539

Quoted 10 lines

arch/x86/kernel/kprobes.c +10 -0

+              * 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.

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 543

Quoted 9 lines

arch/x86/kernel/kprobes.c +1 -5

-     /*
-      * 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.

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 564

Quoted 5 lines

arch/x86/kernel/kprobes.c +4 -1

-                     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

Quoted 5 lines

arch/x86/kernel/kprobes.c +2 -1

+                             }
+                     }
                      ret = 1;
-                     goto preempt_out;
              }

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 574

Quoted 10 lines

arch/x86/kernel/kprobes.c +6 -4

-             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

Quoted 23 lines

arch/x86/kernel/kprobes.c +3 -18

+                     }
+                     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:

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 581

Quoted 4 lines

arch/x86/kernel/kprobes.c +2 -2

-     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.

Thank you,

--
Masami Hiramatsu

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

e-mail: [email protected], [email protected]

Quoted 5 lines

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 509

Quoted 3 lines

arch/x86/kernel/kprobes.c +3 -0

+     default:
+             /* impossible cases */
+             BUG();

I think no need to check that here.

It may not get hit at runtime but is quite informative.

Quoted 2 lines · Masami Hiramatsu

I think setup_singlestep() in your first patch is better, because it avoided code duplication(*).

Will retain it then.

Quoted 19 lines

Quoted 19 lines

static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
struct kprobe_ctlblk *kcb)
{

arch/x86/kernel/kprobes.c +7 -8

-     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

Quoted 1 line

Why would you modify it?

Do you mean the comment? I had this code in kprobe_handler earlier and it consistently exceeded 80 columns in my case. Will fix it anyways.

Quoted 21 lines

Quoted 11 lines

arch/x86/kernel/kprobes.c +10 -0

+             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;

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 506

Quoted 10 lines

arch/x86/kernel/kprobes.c +10 -0

+             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;

Quoted 4 lines

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.

As you've also pointed out in your previous reply, this case is peculiar and therefore I believe it should be marked as a BUG(). I've left the original case, if (kcb->kprobe_status==KPROBE_HIT_SS) && (*p->ainsn.insn == BREAKPOINT_INSTRUCTION), untouched and is handled as it was before. However, if (kcb->kprobe_status==KPROBE_HIT_SS) && !(*p->ainsn.insn == BREAKPOINT_INSTRUCTION), then instead of incrementing nmissed count like before, it should cry out a BUG. This is not an ordinary recursive probe handling case which should update the nmissed count.

Quoted 4 lines

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 509

Quoted 3 lines

arch/x86/kernel/kprobes.c +3 -0

+     default:
+             /* impossible cases */
+             BUG();

I think no need to check that here.

It may not get hit at runtime but is quite informative.

Quoted 23 lines

Quoted 23 lines

static int __kprobes kprobe_handler(struct pt_regs *regs) {

arch/x86/kernel/kprobes.c +14 -1

-     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;
+     }

Quoted 2 lines

I think this block changing would better be separated from this patch, because it changes code flow logically.

Agreed. I'll address this in another thread, but for now it is safest to check it for all cases right at the entry of kprobe_handler().

Quoted 14 lines

Quoted 14 lines

arch/x86/kernel/kprobes.c +0 -14

-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;
-

Quoted 8 lines

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 581

Quoted 5 lines

arch/x86/kernel/kprobes.c +2 -3

-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.

Hmm...there the are no deeply nested if-elses nor overlapping paths which need to be avoided. All the nested checks unroll cleanly too.

Thanks for your review!
Abhishek

Quoted 12 lines

Quoted 8 lines

arch/x86/kernel/kprobes.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 506

Quoted 8 lines

arch/x86/kernel/kprobes.c +8 -0

+     case KPROBE_HIT_SS:
+             if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
+                     regs->flags &= ~TF_MASK;
+                     regs->flags |= kcb->kprobe_saved_flags;
+             } else {
+                     /* BUG? */
+             }
+             break;

Quoted 4 lines

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.

Quoted 9 lines

As you've also pointed out in your previous reply, this case is peculiar and therefore I believe it should be marked as a BUG(). I've left the original case, if (kcb->kprobe_status==KPROBE_HIT_SS) && (*p->ainsn.insn == BREAKPOINT_INSTRUCTION), untouched and is handled as it was before. However, if (kcb->kprobe_status==KPROBE_HIT_SS) && !(*p->ainsn.insn == BREAKPOINT_INSTRUCTION), then instead of incrementing nmissed count like before, it should cry out a BUG. This is not an ordinary recursive probe handling case which should update the nmissed count.

Hmm, I can not agree, because it is possible to insert a kprobe into kprobe's instruction buffer. If it should be a bug, we must check it when registering the kprobe.

(And also, in *p->ainsn.insn == BREAKPOINT_INSTRUCTION case, I doubt that the kernel can handle this "orphaned" breakpoint, because the breakpoint address has been changed.)

Quoted 3 lines

Hmm, I can not agree, because it is possible to insert a kprobe into kprobe's instruction buffer. If it should be a bug, we must check it when registering the kprobe.

I discussed it with other maintainers and knew that current kprobes does not allow user to insert a kprobe to another kprobe's instruction buffer, because register_kprobe ensures the insertion address is text. Now I changed my mind. I think that case (p && kprobe_running() && kcb->kprobe_status==KPROBE_HIT_SS) is BUG(), even if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION).

I could understand what the original code did at last.
If a kprobe is inserted on a breakpoint which other debugger inserts, it single step inline instead of out-of-line.(this is done in prepare_singlestep) In this case, (p && kprobe_running() && kcb->kprobe_status==KPROBE_HIT_SS) is true and we need pass the control to the debugger.
And if (*p->ainsn.insn != BREAKPOINT_INSTRUCTION) (or (p != kprobe_running())) in that case, there may be some bugs.

Now I think your original suggestion is correct.
Please fix it in another patch.

--
Masami Hiramatsu

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

e-mail: [email protected], [email protected]

Quoted 10 lines

Quoted 1 line

As a result, this function just setups re-entrance.

As you've also pointed out in your previous reply, this case is peculiar and therefore I believe it should be marked as a BUG(). I've left the original case, if (kcb->kprobe_status==KPROBE_HIT_SS) && (*p->ainsn.insn == BREAKPOINT_INSTRUCTION), untouched and is handled as it was before. However, if (kcb->kprobe_status==KPROBE_HIT_SS) && !(*p->ainsn.insn == BREAKPOINT_INSTRUCTION), then instead of incrementing nmissed count like before, it should cry out a BUG. This is not an ordinary recursive probe handling case which should update the nmissed count.

Hmm, I can not agree, because it is possible to insert a kprobe into kprobe's instruction buffer. If it should be a bug, we must check it when registering the kprobe.
(And also, in *p->ainsn.insn == BREAKPOINT_INSTRUCTION case, I doubt that the kernel can handle this "orphaned" breakpoint, because the breakpoint address has been changed.)

Anyway, if you would like to change the logic, please separate it from cleanup patch.

Quoted 6 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +2 -1

-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.

Quoted 2 lines

Hmm...there the are no deeply nested if-elses nor overlapping paths which need to be avoided. All the nested checks unroll cleanly too.

Oh, I just mentioned about this "if (!ret)" condition. Could you try to remove this "ret" variable?
I think some "ret=1" could be replaced by "return 1" easily.

Masami Hiramatsu wrote:

Done. You should find the desired changed in this patch.

arch/x86/kernel/kprobes.c +7 -1

@@ -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) {

arch/x86/kernel/kprobes.c static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs, · line 507

Quoted 12 lines

arch/x86/kernel/kprobes.c +12 -0

+     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.

Masami Hiramatsu

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

e-mail: [email protected], [email protected]

Quoted 5 lines · Harvey Harrison

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.

Signed-off-by Abhishek Sagar <[email protected]>

Signed-off-by Quentin Barnes <[email protected]>

---

arch/x86/kernel/kprobes_32.c +7 -0

index 3a020f7..5a626fd 100644
@@ -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 */

arch/x86/kernel/kprobes_32.c +63 -78

+#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;
 }
 

arch/x86/kernel/kprobes_64.c +69 -75

index 5df19a9..788114c 100644
@@ -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;
 }
 

Could you separate changing logic from cleanup and explain why the logic should be changed?

Quoted 11 lines

arch/x86/kernel/kprobes_32.c +11 -0

+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 {

arch/x86/kernel/kprobes_32.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 259

Quoted 6 lines

arch/x86/kernel/kprobes_32.c +6 -0

+             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.

Quoted 8 lines

static int __kprobes kprobe_handler(struct pt_regs *regs) {

arch/x86/kernel/kprobes_32.c +1 -1

-     struct kprobe *p;
      int ret = 0;
      kprobe_opcode_t *addr;
+     struct kprobe *p, *cur;
      struct kprobe_ctlblk *kcb;

Quoted 4 lines

      addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));

arch/x86/kernel/kprobes_32.c +3 -0

+     if (*addr != BREAKPOINT_INSTRUCTION) {
+             /*
+              * The breakpoint instruction was removed right

arch/x86/kernel/kprobes_32.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 287

Quoted 10 lines

arch/x86/kernel/kprobes_32.c +10 -0

+              * 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;
+     }

Quoted 55 lines

arch/x86/kernel/kprobes_32.c +19 -34

-     /* 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 {

arch/x86/kernel/kprobes_32.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 299

Quoted 8 lines

arch/x86/kernel/kprobes_32.c +6 -1

+                                     /* BUG? */
+                             }
+                             break;
+                     default:
+                             /* impossible cases */
+                             BUG();
                      }
-             }

--
Masami Hiramatsu

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

e-mail: [email protected], [email protected]

Quoted 8 lines

Quoted 8 lines

static int __kprobes kprobe_handler(struct pt_regs *regs) {

arch/x86/kernel/kprobes_32.c +1 -1

-     struct kprobe *p;
      int ret = 0;
      kprobe_opcode_t *addr;
+     struct kprobe *p, *cur;
      struct kprobe_ctlblk *kcb;

Quoted 14 lines

Quoted 4 lines

      addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));

arch/x86/kernel/kprobes_32.c +3 -0

+     if (*addr != BREAKPOINT_INSTRUCTION) {
+             /*
+              * The breakpoint instruction was removed right

arch/x86/kernel/kprobes_32.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 287

Quoted 10 lines

arch/x86/kernel/kprobes_32.c +10 -0

+              * 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;
+     }

I have moved the above breakpoint removal check at the beginning of the function. The current check is for '!p' case only, whereas IMO this check should be performed for all cases. An external debugger may very well plant and remove a breakpoint on an address which has probe on it. This check is a race nevertheless, so its relative placing within kprobe_handler() would be best where its duplication can be avoided.

Quoted 48 lines

Quoted 48 lines

arch/x86/kernel/kprobes_32.c +13 -34

-     /* 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;

Quoted 8 lines

arch/x86/kernel/kprobes_32.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 315

Quoted 8 lines

arch/x86/kernel/kprobes_32.c +8 -0

+                     case KPROBE_HIT_SS:
+                             if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
+                                     regs->eflags &= ~TF_MASK;
+                                     regs->eflags |=
+                                             kcb->kprobe_saved_eflags;
+                             } else {
+                                     /* BUG? */
+                             }

Quoted 6 lines

arch/x86/kernel/kprobes_32.c void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 299

Quoted 6 lines

arch/x86/kernel/kprobes_32.c +4 -1

+                             break;
+                     default:
+                             /* impossible cases */
+                             BUG();
                      }
-             }

Replaced deeply nested if-elses with a switch.

Please let me know if there are any changes on which you would like further clarification.

Quoted 7 lines

--
Masami Hiramatsu

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

Quoted 1 line

e-mail: [email protected], [email protected]

--

Thanks,
Abhishek Sagar