[PATCH 1/4] x86: Clean up remove_breakpoint() in ftrace code

[Petr Mladek]

The ftrace recovery code does not work on x86_64. I tried to
debug it and got a bit confused by remove_breakpoint(). One problem
was reusing the "nop" pointer for different types of code. Also
the combination of two level if-condition and goto was a bit
tricky.

Signed-off-by: Petr Mladek <pmladek@xxxxxxx>
---
 arch/x86/kernel/ftrace.c | 58 ++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index e6253195a301..e7f3f3f565de 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -405,17 +405,16 @@ static int add_breakpoints(struct dyn_ftrace *rec, int enable)
 
 /*
  * On error, we need to remove breakpoints. This needs to
- * be done caefully. If the address does not currently have a
+ * be done carefully. If the address does not currently have a
  * breakpoint, we know we are done. Otherwise, we look at the
- * remaining 4 bytes of the instruction. If it matches a nop
- * we replace the breakpoint with the nop. Otherwise we replace
- * it with the call instruction.
+ * remaining 4 bytes of the instruction. It has to be a valid
+ * code. If not, don't touch the breakpoint, we would just
+ * create a disaster.
  */
 static int remove_breakpoint(struct dyn_ftrace *rec)
 {
 	unsigned char ins[MCOUNT_INSN_SIZE];
-	unsigned char brk = BREAKPOINT_INSTRUCTION;
-	const unsigned char *nop;
+	const unsigned char *valid_ins;
 	unsigned long ftrace_addr;
 	unsigned long ip = rec->ip;
 
@@ -424,38 +423,33 @@ static int remove_breakpoint(struct dyn_ftrace *rec)
 		return -EFAULT;
 
 	/* If this does not have a breakpoint, we are done */
-	if (ins[0] != brk)
+	if (ins[0] != BREAKPOINT_INSTRUCTION)
 		return -1;
 
-	nop = ftrace_nop_replace();
+	/* Check if it is nop instruction */
+	valid_ins = ftrace_nop_replace();
+	if (memcmp(&ins[1], &valid_ins[1], MCOUNT_INSN_SIZE - 1) == 0)
+		goto update;
 
-	/*
-	 * If the last 4 bytes of the instruction do not match
-	 * a nop, then we assume that this is a call to ftrace_addr.
-	 */
-	if (memcmp(&ins[1], &nop[1], MCOUNT_INSN_SIZE - 1) != 0) {
-		/*
-		 * For extra paranoidism, we check if the breakpoint is on
-		 * a call that would actually jump to the ftrace_addr.
-		 * If not, don't touch the breakpoint, we make just create
-		 * a disaster.
-		 */
-		ftrace_addr = get_ftrace_addr(rec);
-		nop = ftrace_call_replace(ip, ftrace_addr);
-
-		if (memcmp(&ins[1], &nop[1], MCOUNT_INSN_SIZE - 1) == 0)
-			goto update;
-
-		/* Check both ftrace_addr and ftrace_old_addr */
-		ftrace_addr = get_ftrace_old_addr(rec);
-		nop = ftrace_call_replace(ip, ftrace_addr);
 
-		if (memcmp(&ins[1], &nop[1], MCOUNT_INSN_SIZE - 1) != 0)
-			return -EINVAL;
-	}
+	/* Or it might be ftrace call instruction */
+	ftrace_addr = get_ftrace_addr(rec);
+	valid_ins = ftrace_call_replace(ip, ftrace_addr);
+	if (memcmp(&ins[1], &valid_ins[1], MCOUNT_INSN_SIZE - 1) == 0)
+		goto update;
+
+	/* Last chance, it might be the old ftrace call instruction */
+	ftrace_addr = get_ftrace_old_addr(rec);
+	valid_ins = ftrace_call_replace(ip, ftrace_addr);
+	if (memcmp(&ins[1], &valid_ins[1], MCOUNT_INSN_SIZE - 1) == 0)
+		goto update;
+
+	/* Hmm, it is an unknown code. Rather bail out. */
+	return -EINVAL;
 
  update:
-	return probe_kernel_write((void *)ip, &nop[0], 1);
+	/* Put back the first byte */
+	return probe_kernel_write((void *)ip, valid_ins, 1);
 }
 
 static int add_update_code(unsigned long ip, unsigned const char *new)
-- 
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/