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

[Masami Hiramatsu <mhiramat at redhat dot com>]

Hello Jim,

Thank you for comments.

Jim Keniston wrote:
> 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.

Indeed, I checked that is true in the intel's developers manual.

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

Thanks, it is really helpful for me as a non-English speaker.
By the way, 'from' and 'to' are a bit confused me without quatation.
So, how about this?
/* Insert a jump instruction at address 'from', which 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.

OK, I applied that.

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

Right.

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

OK.

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

Thank you again.
Now I'm cleaning the code up by using checkpatch.pl and checking a bug
reported from Srinivasa. It will take a while. I'll repost cleaned patches
in this week.

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

OK, I applied it.

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

Best regards.

-- 
Masami Hiramatsu

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

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