Re: [RFC -mm][PATCH 0/6] kprobes x86 code unification and boosters

[Jim Keniston <jkenisto at us dot ibm dot com>]

On Mon, 2007-12-10 at 17:52 -0500, Masami Hiramatsu wrote:
> Hello all,
> 
> I developed a series of patches which unifies kprobes code on x86
> and introduces boosters on x86-64. These patches can be applied to 2.6.24-rc4-mm1.
> 
...

Here's a partial review.  Just nits so far.

Patches #0-1: Fine.

Patch #2:
The call absolute instruction (0x9a) is invalid for x86_64 in 64-bit
mode.  So it's not really correct to add it in this patch.  You could
just make sure it's there as part of the 32/64 merge patch.

Patch #3:
This is sort of off topic, but the comment for set_jmp_op() could be
clearer.  Something like:
/* Insert a jump instruction at address from that jumps to address to.
*/

+ *
+ * This function also checks instruction size for preparing direct
execution.
Off topic again, but wouldn't this be clearer?
+ * If this is the first time we've single-stepped the instruction at
+ * this probepoint, and the instruction is boostable, boost it: add a
+ * jump instruction after the instruction copy, that jumps to the next
+ * instruction after the probepoint.

Patch #4:
+ * When a retprobed function returns, this code saves registers and
+ * calls trampoline_handler() runs, calling the kretprobe's handler.
That 2nd line has a typo.  Maybe:
+ * calls trampoline_handler(), which calls the kretprobe's handler.

Add a comment here: We don't bother saving the ss register.
+ 			"	pushq %rsp\n"
+			"	pushfq\n"
+			/* skip cs, ip, orig_ax */
How about:	/* trampoline_handler() will plug in cs, ip, orig_ax values
*/
+			"	subq $24, %rsp\n"
+			"	pushq %rdi\n"
+			"	pushq %rsi\n"
+			"	pushq %rdx\n"
+			"	pushq %rcx\n"
+			"	pushq %rax\n"
+			"	pushq %r8\n"
+			"	pushq %r9\n"
+			"	pushq %r10\n"
+			"	pushq %r11\n"
+			"	pushq %rbx\n"
+			"	pushq %rbp\n"
+			"	pushq %r12\n"
+			"	pushq %r13\n"
+			"	pushq %r14\n"
+			"	pushq %r15\n"
+			"	movq %rsp, %rdi\n"
+			"	call trampoline_handler\n"
+			/* save true return address on sp */
How about:		/* Replace saved sp with true return address. */

+			"	movq %rax, 152(%rsp)\n"
+			"	popq %r15\n"
...

[I didn't get around to reviewing all of patches #5 and #6 yet.  Let me
know if you want me to finish tomorrow.]

Patch #6:
+void __kprobes jprobe_return(void)
+{
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+#ifdef CONFIG_X86_64
+	asm volatile ("       xchg   %%rbx,%%rsp     \n"
+#else
+	asm volatile ("       xchgl   %%ebx,%%esp     \n"
+#endif

The above code introduces a parentheses imbalance that affects (among
other things) vi's paren/brace matching.  I suggest:
	asm volatile(
+#ifdef CONFIG_X86_64
+	"       xchg   %%rbx,%%rsp     \n"
+#else
+	"       xchgl   %%ebx,%%esp     \n"
+#endif

Also, replace spaces with tabs here.  kretprobe_trampoline_holder() is
a good example.

Jim