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

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

[Masami Hiramatsu]

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.

The purpose of this patchset is unifying kprobes_[32|64].[c|h] to kprobes.[c|h]
for simplifying code maintenance.

kprobe-booster and kretprobe-booster were explained in:
http://marc.info/?l=linux-kernel&m=113862526017068&w=2

Currently, these patches do unification as following order.
1. Clean up and fix bugs in kprobes[1/6, 2/6].
2. Introduce kprobe-booster and kretprobe-booster for x86-64[3/6, 4/6].
   (x86-32 kprobes already has same functionalities)
3. Prepare unification[5/6].
4. Unify kprobes code[6/6].

If you have any comment, please let me know.

Arjan,
I have added your signed-off on 5th patch. Is it OK?
It is based on your patch.

Best Regards,

-- 
Masami Hiramatsu

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

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

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

[Jim Keniston]

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

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

[Masami Hiramatsu]

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

Re: [RFC -mm][PATCH 0/6] kprobes x86 code unification and boosters (review part 2)

[Jim Keniston]

FYI, here are review comments I sent privately to Masami yesterday.  (I
was in a hurry and didn't cc everybody.)  He has already addressed them.

Jim
-----
Patch #5:
+#define C ,
This is a creative solution to the problem of test_bit() wanting
an unsigned long* arg.  It may be a little TOO creative for some
reviewers.  If so, remember that you can make the tables all u64
or u32, and just add (const unsigned long*) casts to the test_bit()
calls.

In kprobes_64.c...

+#undef  C
s/  / /

+/*
+ * 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)
I assume you're confident about this.  If not, verify with somebody
like Andi Kleen.  I tried to verify this by sifting through traps_64.c
and desc_64.c and the AMD manuals, but gave up after a while.

Patch #6:
Regarding the historical comments in kprobes.c and kprobes.h...

1. What's the current policy regarding such comments?  Should they just
be removed?  If not...

2. Seems like historical notes for kprobe boosters and kretprobe
boosters
should be added.  (Modesty should not obscure history. :-))

3. You could probably remove the historical comments from kprobes.h,
and/or add a ref to the (more nearly complete) history in kprobes.c.

4. Since each file reflects both i386 and x86_64 now, you need to
clarify some comments:

  a. In kprobes.h, the 2004-Oct comment should end with something
  like "x86_64 adopted from i386".

  b. In kprobes.c, the 2005-May comment about Rusty Lynch should say
  "Added x86_64 function-return probes".  Also, it should follow the
  other 2005-May comment, since we implemented i386 first.

5. s/kenistoj/jkenisto/

Re: [RFC -mm][PATCH 0/6] kprobes x86 code unification and boosters (review part 3)

[Jim Keniston]

Here's the rest of my review.  I didn't find anything broken, so I'm
willing to ack the patch set (in however many pieces it's posted).

Thanks for all your great work, Masami.

Jim
-----
+#define MIN_STACK_SIZE(ADDR) (((MAX_STACK_SIZE) < \
+       (((unsigned long)current_thread_info()) + THREAD_SIZE -
(unsigned long)(ADDR))) \
+       ? (MAX_STACK_SIZE) \
+       : (((unsigned long)current_thread_info()) + THREAD_SIZE -
(unsigned long)(ADDR)))

How about making MIN_STACK_SIZE(ADDR) an inline function?  Seems like
it'd clarify what we're trying to do.
static unsigned long saved_stack_size(unsigned long sp)
{
	unsigned long cur_stack_size =
		((unsigned long)current_thread_info())
		+ THREAD_SIZE - sp;
	return min(cur_stack_size, MAX_STACK_SIZE);
}
But maybe such a change should be packaged separately, and include s390.

+struct arch_specific_insn {
+	/* copy of the original instruction */
+	kprobe_opcode_t *insn;
+	/*
+	 * If this flag is 1, this kprobe can be boosted when its
+	 * post_handler and break_handler is not set.
+	 */

How about:
	/*
	 * boostable = -1: This instruction type is not boostable.
	 * boostable = 0: This instruction type is boostable.
	 * boostable = 1: This instruction has been boosted: we have
	 * added a relative jump after the instruction copy in insn,
	 * so no single-step and fixup are needed (unless there's
	 * a post_handler or break_handler).
	 */
+	int boostable;
+};

+	/*
+	 * It is possible to have multiple instances associated with a given
+	 * task either because an multiple functions in the call path
s/an multiple/multiple/
+	 * have a return probe installed on them, and/or more then one return
s/a return probe/return probes/
s/one return/one/  [Fix "return\nreturn"]
+	 * return probe was registered for a target function.
+	 *
+	 * We can handle this because:
+	 *     - instances are always inserted at the head of the list
s/inserted at/pushed onto/
+	 *     - when multiple return probes are registered for the same
+	 *       function, the first instance's ret_addr will point to the
+	 *       real return address, and all the rest will point to
+	 *       kretprobe_trampoline
How about this?
	 *	 function, the (chronologically) first instance's ret_addr
	 *	 will be the real return address, and all the rest will
	 *	 point to kretprobe_trampoline.
+	 */

+		 * we can also use npre/npostfault count for accouting
s/accouting/accounting/

Re: [RFC -mm][PATCH 0/6] kprobes x86 code unification and boosters (review part 3)

[Masami Hiramatsu]

Hello Jim,

Jim Keniston wrote:
> Here's the rest of my review.  I didn't find anything broken, so I'm
> willing to ack the patch set (in however many pieces it's posted).
> 
> Thanks for all your great work, Masami.
> 
> Jim
> -----
> +#define MIN_STACK_SIZE(ADDR) (((MAX_STACK_SIZE) < \
> +       (((unsigned long)current_thread_info()) + THREAD_SIZE -
> (unsigned long)(ADDR))) \
> +       ? (MAX_STACK_SIZE) \
> +       : (((unsigned long)current_thread_info()) + THREAD_SIZE -
> (unsigned long)(ADDR)))
> 
> How about making MIN_STACK_SIZE(ADDR) an inline function?  Seems like
> it'd clarify what we're trying to do.
> static unsigned long saved_stack_size(unsigned long sp)
> {
> 	unsigned long cur_stack_size =
> 		((unsigned long)current_thread_info())
> 		+ THREAD_SIZE - sp;
> 	return min(cur_stack_size, MAX_STACK_SIZE);
> }
> But maybe such a change should be packaged separately, and include s390.

I agree, and I think it might better merge it after unification
for reducing modification and side-effects.

> +struct arch_specific_insn {
> +	/* copy of the original instruction */
> +	kprobe_opcode_t *insn;
> +	/*
> +	 * If this flag is 1, this kprobe can be boosted when its
> +	 * post_handler and break_handler is not set.
> +	 */
> 
> How about:
> 	/*
> 	 * boostable = -1: This instruction type is not boostable.
> 	 * boostable = 0: This instruction type is boostable.
> 	 * boostable = 1: This instruction has been boosted: we have
> 	 * added a relative jump after the instruction copy in insn,
> 	 * so no single-step and fixup are needed (unless there's
> 	 * a post_handler or break_handler).
> 	 */
> +	int boostable;
> +};

Thanks, it's good to me.

> 
> +	/*
> +	 * It is possible to have multiple instances associated with a given
> +	 * task either because an multiple functions in the call path
> s/an multiple/multiple/
> +	 * have a return probe installed on them, and/or more then one return
> s/a return probe/return probes/
> s/one return/one/  [Fix "return\nreturn"]
> +	 * return probe was registered for a target function.
> +	 *
> +	 * We can handle this because:
> +	 *     - instances are always inserted at the head of the list
> s/inserted at/pushed onto/
> +	 *     - when multiple return probes are registered for the same
> +	 *       function, the first instance's ret_addr will point to the
> +	 *       real return address, and all the rest will point to
> +	 *       kretprobe_trampoline
> How about this?
> 	 *	 function, the (chronologically) first instance's ret_addr
> 	 *	 will be the real return address, and all the rest will
> 	 *	 point to kretprobe_trampoline.
> +	 */
> 
> +		 * we can also use npre/npostfault count for accouting
> s/accouting/accounting/

OK, I'll fix it.

> 
> 

Thank you very much!

-- 
Masami Hiramatsu

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

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