[PATCH] kretprobe: kretprobe-booster against 2.6.16-rc1 for i386

[PATCH] kretprobe: kretprobe-booster against 2.6.16-rc1 for i386

[Masami Hiramatsu]

Hi, Andrew

Here is a patch of the kretprobe-booster for i386 arch against
linux-2.6.16-rc1 and also appliable against 2.6.16-rc1-mm4.

Best Regards,

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

Signed-off-by: Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>

 kprobes.c |   56 ++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 20 deletions(-)
diff -Narup a/arch/i386/kernel/kprobes.c b/arch/i386/kernel/kprobes.c
--- a/arch/i386/kernel/kprobes.c	2006-01-24 19:07:26.000000000 +0900
+++ b/arch/i386/kernel/kprobes.c	2006-01-30 18:40:12.000000000 +0900
@@ -255,17 +255,45 @@ no_kprobe:
  * here. When a retprobed function returns, this probe is hit and
  * trampoline_probe_handler() runs, calling the kretprobe's handler.
  */
- void kretprobe_trampoline_holder(void)
+ void __kprobes kretprobe_trampoline_holder(void)
  {
- 	asm volatile (  ".global kretprobe_trampoline\n"
+	 asm volatile ( ".global kretprobe_trampoline\n"
  			"kretprobe_trampoline: \n"
- 			"nop\n");
- }
+			"	subl $8, %esp\n"
+			"	pushf\n"
+			"	subl $20, %esp\n"
+			"	pushl %eax\n"
+			"	pushl %ebp\n"
+			"	pushl %edi\n"
+			"	pushl %esi\n"
+			"	pushl %edx\n"
+			"	pushl %ecx\n"
+			"	pushl %ebx\n"
+			"	movl %esp, %eax\n"
+			"	pushl %eax\n"
+			"	addl $60, %eax\n"
+			"	movl %eax, 56(%esp)\n"
+			"	movl $trampoline_handler, %eax\n"
+			"	call *%eax\n"
+			"	addl $4, %esp\n"
+			"	movl %eax, 56(%esp)\n"
+			"	popl %ebx\n"
+			"	popl %ecx\n"
+			"	popl %edx\n"
+			"	popl %esi\n"
+			"	popl %edi\n"
+			"	popl %ebp\n"
+			"	popl %eax\n"
+			"	addl $20, %esp\n"
+			"	popf\n"
+			"	addl $4, %esp\n"
+			"	ret\n");
+}

 /*
- * Called when we hit the probe point at kretprobe_trampoline
+ * Called from kretprobe_trampoline
  */
-int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
+asmlinkage void *__kprobes trampoline_handler(struct pt_regs *regs)
 {
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
@@ -310,18 +338,11 @@ int __kprobes trampoline_probe_handler(s
 	}

 	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
-	regs->eip = orig_ret_address;

-	reset_current_kprobe();
 	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();

-	/*
-	 * By returning a non-zero value, we are telling
-	 * kprobe_handler() that we don't want the post_handler
-	 * to run (and have re-enabled preemption)
-	 */
-        return 1;
+	return (void*)orig_ret_address;
 }

 /*
@@ -552,12 +573,7 @@ int __kprobes longjmp_break_handler(stru
 	return 0;
 }

-static struct kprobe trampoline_p = {
-	.addr = (kprobe_opcode_t *) &kretprobe_trampoline,
-	.pre_handler = trampoline_probe_handler
-};
-
 int __init arch_init_kprobes(void)
 {
-	return register_kprobe(&trampoline_p);
+	return 0;
 }

Re: [PATCH] kretprobe: kretprobe-booster against 2.6.16-rc1 for i386

[Masami Hiramatsu]

Sorry, also I forgot to remove a solo decrement routine.

Masami Hiramatsu wrote:
> @@ -310,18 +338,11 @@ int __kprobes trampoline_probe_handler(s
>  	}
> 
>  	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
> -	regs->eip = orig_ret_address;
> 
> -	reset_current_kprobe();
>  	spin_unlock_irqrestore(&kretprobe_lock, flags);
>  	preempt_enable_no_resched();
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^  This must cause a trouble.

So, I must remove it (when boosting)
> -	preempt_enable_no_resched();

I attatch the fixed patch to this mail.

> 
> -	/*
> -	 * By returning a non-zero value, we are telling
> -	 * kprobe_handler() that we don't want the post_handler
> -	 * to run (and have re-enabled preemption)
> -	 */
> -        return 1;
> +	return (void*)orig_ret_address;
>  }
> 
>  /*

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

Signed-off-by: Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>

 kprobes.c |   57 ++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 36 insertions(+), 21 deletions(-)
diff -Narup a/arch/i386/kernel/kprobes.c b/arch/i386/kernel/kprobes.c
--- a/arch/i386/kernel/kprobes.c	2006-01-24 19:07:26.000000000 +0900
+++ b/arch/i386/kernel/kprobes.c	2006-01-31 10:26:46.000000000 +0900
@@ -255,17 +255,45 @@ no_kprobe:
  * here. When a retprobed function returns, this probe is hit and
  * trampoline_probe_handler() runs, calling the kretprobe's handler.
  */
- void kretprobe_trampoline_holder(void)
+ void __kprobes kretprobe_trampoline_holder(void)
  {
- 	asm volatile (  ".global kretprobe_trampoline\n"
+	 asm volatile ( ".global kretprobe_trampoline\n"
  			"kretprobe_trampoline: \n"
- 			"nop\n");
- }
+			"	subl $8, %esp\n"
+			"	pushf\n"
+			"	subl $20, %esp\n"
+			"	pushl %eax\n"
+			"	pushl %ebp\n"
+			"	pushl %edi\n"
+			"	pushl %esi\n"
+			"	pushl %edx\n"
+			"	pushl %ecx\n"
+			"	pushl %ebx\n"
+			"	movl %esp, %eax\n"
+			"	pushl %eax\n"
+			"	addl $60, %eax\n"
+			"	movl %eax, 56(%esp)\n"
+			"	movl $trampoline_handler, %eax\n"
+			"	call *%eax\n"
+			"	addl $4, %esp\n"
+			"	movl %eax, 56(%esp)\n"
+			"	popl %ebx\n"
+			"	popl %ecx\n"
+			"	popl %edx\n"
+			"	popl %esi\n"
+			"	popl %edi\n"
+			"	popl %ebp\n"
+			"	popl %eax\n"
+			"	addl $20, %esp\n"
+			"	popf\n"
+			"	addl $4, %esp\n"
+			"	ret\n");
+}

 /*
- * Called when we hit the probe point at kretprobe_trampoline
+ * Called from kretprobe_trampoline
  */
-int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
+asmlinkage void *__kprobes trampoline_handler(struct pt_regs *regs)
 {
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
@@ -310,18 +338,10 @@ int __kprobes trampoline_probe_handler(s
 	}

 	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
-	regs->eip = orig_ret_address;

-	reset_current_kprobe();
 	spin_unlock_irqrestore(&kretprobe_lock, flags);
-	preempt_enable_no_resched();

-	/*
-	 * By returning a non-zero value, we are telling
-	 * kprobe_handler() that we don't want the post_handler
-	 * to run (and have re-enabled preemption)
-	 */
-        return 1;
+	return (void*)orig_ret_address;
 }

 /*
@@ -552,12 +572,7 @@ int __kprobes longjmp_break_handler(stru
 	return 0;
 }

-static struct kprobe trampoline_p = {
-	.addr = (kprobe_opcode_t *) &kretprobe_trampoline,
-	.pre_handler = trampoline_probe_handler
-};
-
 int __init arch_init_kprobes(void)
 {
-	return register_kprobe(&trampoline_p);
+	return 0;
 }

Re: [PATCH] kretprobe: kretprobe-booster against 2.6.16-rc1 for i386

[Andrew Morton]

Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp> wrote:
>
> -	regs->eip = orig_ret_address;
> 
> -	reset_current_kprobe();
>  	spin_unlock_irqrestore(&kretprobe_lock, flags);
> -	preempt_enable_no_resched();

Again, the patch removes a preempt_enable() and doesn't add a
preempt_disable().  Maybe this is to balance the earlier patch.  If so,
they should both be in the same patch so the kernel works OK at each stage.

You didn't include a description of what this patch actually does.

After all the corrections I'm not terribly confident that the three patches
which I ended up with are correct.  Please check them carefully.

Re: [PATCH] kretprobe: kretprobe-booster against 2.6.16-rc1 for i386

[Masami Hiramatsu]

Hi, Andrew
Andrew Morton wrote:
> Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp> wrote:
>>-	regs->eip = orig_ret_address;
>>
>>-	reset_current_kprobe();
>> 	spin_unlock_irqrestore(&kretprobe_lock, flags);
>>-	preempt_enable_no_resched();
>
> Again, the patch removes a preempt_enable() and doesn't add a
> preempt_disable().  Maybe this is to balance the earlier patch.  If so,
> they should both be in the same patch so the kernel works OK at each stage.
> You didn't include a description of what this patch actually does.

That is not to balance the previous patch. Here is the reason and
the description of kretprobe-booster.

The kretprobe basically invokes kprobe twice as following actions;

At function entrance:
(1) int3 (1st kprobe)
(2) preempt_disable
(3) call pre_handler_kretprobe ()
(3-1) store original return address to a retprobe instance
(3-2) modify return address.
(4) copied instructioin(single step)
(5) preempt_enable

At function exit:
(1) return to kretprobe_trampoline
(2) int3 (2nd kprobe)
(3) preempt_disable()
(4) call trampoline_probe_handler()
(4-1) find the corresponding instance and call true handler.
(4-2) restore original return address to regs->eip
(4-3) preempt_enable()
(5) return to int3 handler (do NOT execute single step)

The first kprobe is to modify return address of the function,
and the second is to call the true kretprobe's handler from
the function return point. The first kprobe is executed
normally. But the second does not execute single-step,
because the copied instruction of the probe is always "nop".

In the other hand, kretprobe-booster modifies the process at
function exit as following actions;

At function exit:
(1) return to kretprobe_trampoline
(2) store registers
(3) call trampoline_handler()
(3-1) find the corresponding instance and call true handler.
(3-2) return original return address
(4) restore registers and the original return address.
(5) return to original function caller.

There are no kprobes, and any instructions are not removed.
So, there is no need to disable preemption.
This is the reason why I removed preempt_enable().

Best regards,


-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

[PATCH][take 2] kretprobe: kretprobe-booster against 2.6.16-rc2 for i386

[Masami Hiramatsu]

Hi, Andrew

Here is a patch of kretprobe-booster for i386 against linux-2.6.16-rc2.
In the previous kretprobe-booster patch, I had a mistake about stack
register. In this patch, the bug is fixed.

This bug was pointed by Chuck Ebbert, and there were two different
patches(Chuck's and mine) to fix it. But both patches were dropped
from -mm tree. So, I merged my patch into kretprobe-booster patch
and attached it to this mail.

Could you replace the previous kretprobe-booster patch with this
patch?

The description of kretprobe-booster:
====================================
In the normal operation, kretprobe make a target function return
to trampoline code. A kprobe (called trampoline_probe) has
been inserted at the trampoline code. When the kernel hits this
kprobe, it calls kretprobe's handler and it returns to original
return address.

Kretprobe-booster patch removes the trampoline_probe. It allows
the tranpoline code to call kretprobe's handler directly instead
of invoking kprobe. The trampoline code returns to original return
address.

This new tranpoline code stores and restores registers, so the
kretprobe handler is still able to access those registers.

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

Signed-off-by: Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>

 kprobes.c |   56 +++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 35 insertions(+), 21 deletions(-)
diff -Narup a/arch/i386/kernel/kprobes.c b/arch/i386/kernel/kprobes.c
--- a/arch/i386/kernel/kprobes.c	2006-02-13 15:01:31.000000000 +0900
+++ b/arch/i386/kernel/kprobes.c	2006-02-13 15:06:37.000000000 +0900
@@ -255,17 +255,44 @@ no_kprobe:
  * here. When a retprobed function returns, this probe is hit and
  * trampoline_probe_handler() runs, calling the kretprobe's handler.
  */
- void kretprobe_trampoline_holder(void)
+ void __kprobes kretprobe_trampoline_holder(void)
  {
- 	asm volatile (  ".global kretprobe_trampoline\n"
+	 asm volatile ( ".global kretprobe_trampoline\n"
  			"kretprobe_trampoline: \n"
- 			"nop\n");
- }
+			"	pushf\n"
+			/* skip cs, eip, orig_eax, es, ds */
+			"	subl $20, %esp\n"
+			"	pushl %eax\n"
+			"	pushl %ebp\n"
+			"	pushl %edi\n"
+			"	pushl %esi\n"
+			"	pushl %edx\n"
+			"	pushl %ecx\n"
+			"	pushl %ebx\n"
+			"	movl %esp, %eax\n"
+			"	call trampoline_handler\n"
+			/* move eflags to cs */
+			"	movl 48(%esp), %edx\n"
+			"	movl %edx, 44(%esp)\n"
+			/* save true return address on eflags */
+			"	movl %eax, 48(%esp)\n"
+			"	popl %ebx\n"
+			"	popl %ecx\n"
+			"	popl %edx\n"
+			"	popl %esi\n"
+			"	popl %edi\n"
+			"	popl %ebp\n"
+			"	popl %eax\n"
+			/* skip eip, orig_eax, es, ds */
+			"	addl $16, %esp\n"
+			"	popf\n"
+			"	ret\n");
+}

 /*
- * Called when we hit the probe point at kretprobe_trampoline
+ * Called from kretprobe_trampoline
  */
-int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
+fastcall void *__kprobes trampoline_handler(struct pt_regs *regs)
 {
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
@@ -310,18 +337,10 @@ int __kprobes trampoline_probe_handler(s
 	}

 	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
-	regs->eip = orig_ret_address;

-	reset_current_kprobe();
 	spin_unlock_irqrestore(&kretprobe_lock, flags);
-	preempt_enable_no_resched();

-	/*
-	 * By returning a non-zero value, we are telling
-	 * kprobe_handler() that we don't want the post_handler
-	 * to run (and have re-enabled preemption)
-	 */
-        return 1;
+	return (void*)orig_ret_address;
 }

 /*
@@ -552,12 +571,7 @@ int __kprobes longjmp_break_handler(stru
 	return 0;
 }

-static struct kprobe trampoline_p = {
-	.addr = (kprobe_opcode_t *) &kretprobe_trampoline,
-	.pre_handler = trampoline_probe_handler
-};
-
 int __init arch_init_kprobes(void)
 {
-	return register_kprobe(&trampoline_p);
+	return 0;
 }

Re: [PATCH][take 2] kretprobe: kretprobe-booster against 2.6.16-rc2 for i386

[Andrew Morton]

Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp> wrote:
>
> Hi, Andrew
> 
> Here is a patch of kretprobe-booster for i386 against linux-2.6.16-rc2.

This patch is identical to what I have now in -mm.

> In the previous kretprobe-booster patch, I had a mistake about stack
> register. In this patch, the bug is fixed.
> 
> This bug was pointed by Chuck Ebbert, and there were two different
> patches(Chuck's and mine) to fix it. But both patches were dropped
> from -mm tree. So, I merged my patch into kretprobe-booster patch
> and attached it to this mail.

That was just me folding the fixup patches into the base patch.

I've updated the covering text in those email notifications.

Re: [PATCH] kretprobe: kretprobe-booster against 2.6.16-rc1 for i386

[Chuck Ebbert]

In-Reply-To: <43DE0A53.3060801@sdl.hitachi.co.jp>

On Mon, 30 Jan 2006 at 21:45:07 +0900, Masami Hiramatsu wrote:

> Here is a patch of the kretprobe-booster for i386 arch against
> linux-2.6.16-rc1 and also appliable against 2.6.16-rc1-mm4.

> --- a/arch/i386/kernel/kprobes.c      2006-01-24 19:07:26.000000000 +0900
> +++ b/arch/i386/kernel/kprobes.c      2006-01-30 18:40:12.000000000 +0900
> @@ -255,17 +255,45 @@ no_kprobe:
>   * here. When a retprobed function returns, this probe is hit and
>   * trampoline_probe_handler() runs, calling the kretprobe's handler.
>   */
> - void kretprobe_trampoline_holder(void)
> + void __kprobes kretprobe_trampoline_holder(void)
>   {
> -     asm volatile (  ".global kretprobe_trampoline\n"
> +      asm volatile ( ".global kretprobe_trampoline\n"
>                       "kretprobe_trampoline: \n"
> -                     "nop\n");
> - }
> +                     "       subl $8, %esp\n"

        There is no need to reserve these 8 bytes; they wouldn't be
there if INT3 were done from kernel space anyway.

> +                     "       pushf\n"
> +                     "       subl $20, %esp\n"
> +                     "       pushl %eax\n"
> +                     "       pushl %ebp\n"
> +                     "       pushl %edi\n"
> +                     "       pushl %esi\n"
> +                     "       pushl %edx\n"
> +                     "       pushl %ecx\n"
> +                     "       pushl %ebx\n"
> +                     "       movl %esp, %eax\n"
> +                     "       pushl %eax\n"

        If you make trampoline_probe_handler "fastcall" you can just
pass eax to the handler directly.

> +                     "       addl $60, %eax\n"
> +                     "       movl %eax, 56(%esp)\n"

        No need for this either, since oldesp isn't there on INT3 call anyway. 

> +                     "       movl $trampoline_handler, %eax\n"
> +                     "       call *%eax\n"

        Why not just "call trampoline_handler"?

> +                     "       addl $4, %esp\n"
> +                     "       movl %eax, 56(%esp)\n"
> +                     "       popl %ebx\n"
> +                     "       popl %ecx\n"
> +                     "       popl %edx\n"
> +                     "       popl %esi\n"
> +                     "       popl %edi\n"
> +                     "       popl %ebp\n"
> +                     "       popl %eax\n"
> +                     "       addl $20, %esp\n"
> +                     "       popf\n"
> +                     "       addl $4, %esp\n"

        This "add" corrupts the flags you just popped from the stack.
This can be fixed by moving the return address up 4 bytes on the stack
and doing "ret 4" or by putting the address in regs->eip and just doing
an "iret" to return to the caller.

> +                     "       ret\n");
> +}


Patch for 2.6.16-rc1-mm5 follows.


Fix and clean up kretprobe-kretprobe-booster by:

        - Not reserving 8 bytes at stack bottom
        - Making trampoline_handler fastcall and calling it directly
        - Using "iret" to return to original caller

Using "iret" is slightly slower than direct return; time went from 1030 CPU cycles
overhead per kretprobe to 1050 cycles (2%), but the code is cleaner.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

---

 arch/i386/kernel/kprobes.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)

--- 2.6.16-rc1-mm5-386.orig/arch/i386/kernel/kprobes.c
+++ 2.6.16-rc1-mm5-386/arch/i386/kernel/kprobes.c
@@ -325,9 +325,10 @@ no_kprobe:
  {
 	 asm volatile ( ".global kretprobe_trampoline\n"
  			"kretprobe_trampoline: \n"
-			"	subl $8, %esp\n"
 			"	pushf\n"
-			"	subl $20, %esp\n"
+			"	pushl %cs\n"
+			/* skip eip, orig_eax, es, ds */
+			"	subl $16, %esp\n"
 			"	pushl %eax\n"
 			"	pushl %ebp\n"
 			"	pushl %edi\n"
@@ -336,13 +337,9 @@ no_kprobe:
 			"	pushl %ecx\n"
 			"	pushl %ebx\n"
 			"	movl %esp, %eax\n"
-			"	pushl %eax\n"
-			"	addl $60, %eax\n"
-			"	movl %eax, 56(%esp)\n"
-			"	movl $trampoline_handler, %eax\n"
-			"	call *%eax\n"
-			"	addl $4, %esp\n"
-			"	movl %eax, 56(%esp)\n"
+			"	call trampoline_handler\n"
+			/* save new eip */
+			"	movl %eax, 40(%esp)\n"
 			"	popl %ebx\n"
 			"	popl %ecx\n"
 			"	popl %edx\n"
@@ -350,16 +347,15 @@ no_kprobe:
 			"	popl %edi\n"
 			"	popl %ebp\n"
 			"	popl %eax\n"
-			"	addl $20, %esp\n"
-			"	popf\n"
-			"	addl $4, %esp\n"
-			"	ret\n");
+			/* skip ds, es, orig_eax */
+			"	addl $12, %esp\n"
+			"	iret\n");
 }
 
 /*
  * Called from kretprobe_trampoline
  */
-asmlinkage void *__kprobes trampoline_handler(struct pt_regs *regs)
+fastcall void *__kprobes trampoline_handler(struct pt_regs *regs)
 {
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
-- 
Chuck

Re: [PATCH] kretprobe: kretprobe-booster against 2.6.16-rc1 for i386

[Masami Hiramatsu]

Hi, Chuck

Chuck Ebbert wrote:
> In-Reply-To: <43DE0A53.3060801@sdl.hitachi.co.jp>
> 
> On Mon, 30 Jan 2006 at 21:45:07 +0900, Masami Hiramatsu wrote:
> 
>> Here is a patch of the kretprobe-booster for i386 arch against
>> linux-2.6.16-rc1 and also appliable against 2.6.16-rc1-mm4.
> 
>> --- a/arch/i386/kernel/kprobes.c      2006-01-24 19:07:26.000000000 +0900
>> +++ b/arch/i386/kernel/kprobes.c      2006-01-30 18:40:12.000000000 +0900
>> @@ -255,17 +255,45 @@ no_kprobe:
>>   * here. When a retprobed function returns, this probe is hit and
>>   * trampoline_probe_handler() runs, calling the kretprobe's handler.
>>   */
>> - void kretprobe_trampoline_holder(void)
>> + void __kprobes kretprobe_trampoline_holder(void)
>>   {
>> -     asm volatile (  ".global kretprobe_trampoline\n"
>> +      asm volatile ( ".global kretprobe_trampoline\n"
>>                       "kretprobe_trampoline: \n"
>> -                     "nop\n");
>> - }
>> +                     "       subl $8, %esp\n"
> 
>         There is no need to reserve these 8 bytes; they wouldn't be
> there if INT3 were done from kernel space anyway.

OK, I agree.

>> +                     "       pushf\n"
>> +                     "       subl $20, %esp\n"
>> +                     "       pushl %eax\n"
>> +                     "       pushl %ebp\n"
>> +                     "       pushl %edi\n"
>> +                     "       pushl %esi\n"
>> +                     "       pushl %edx\n"
>> +                     "       pushl %ecx\n"
>> +                     "       pushl %ebx\n"
>> +                     "       movl %esp, %eax\n"
>> +                     "       pushl %eax\n"
> 
>         If you make trampoline_probe_handler "fastcall" you can just
> pass eax to the handler directly.

It is a nice idea.

>> +                     "       addl $60, %eax\n"
>> +                     "       movl %eax, 56(%esp)\n"
> 
>         No need for this either, since oldesp isn't there on INT3 call anyway. 

OK, I agree too.

>> +                     "       movl $trampoline_handler, %eax\n"
>> +                     "       call *%eax\n"
> 
>         Why not just "call trampoline_handler"?

I forgot to do so... thanks!

>> +                     "       addl $4, %esp\n"
>> +                     "       movl %eax, 56(%esp)\n"
>> +                     "       popl %ebx\n"
>> +                     "       popl %ecx\n"
>> +                     "       popl %edx\n"
>> +                     "       popl %esi\n"
>> +                     "       popl %edi\n"
>> +                     "       popl %ebp\n"
>> +                     "       popl %eax\n"
>> +                     "       addl $20, %esp\n"
>> +                     "       popf\n"
>> +                     "       addl $4, %esp\n"
> 
>         This "add" corrupts the flags you just popped from the stack.

Sure, this "add" may modify the status flags. It should be fixed.

> This can be fixed by moving the return address up 4 bytes on the stack
> and doing "ret 4" or by putting the address in regs->eip and just doing
> an "iret" to return to the caller.

Why would you like to use "iret"?
I worry about its negative side effects, because the "iret" is only for
interrupt handlers and may have some side effects.
In my honest opinion, this can be fixed by moving eflags to cs field,
putting the return address in eflags and using "ret".
I developed a patch and attach it to this mail.

This patch fixes and cleanups kretprobe-kretprobe-booster by:

          - Not reserving 8 bytes at stack bottom
          - Making trampoline_handler fastcall and calling it directly
          - Not changing the status flags

I'm very happy to be fixed it! Thank you very much for pointing it out!

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp

Signed-off-by: Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>

 kprobes.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)
diff -Narup a/arch/i386/kernel/kprobes.c b/arch/i386/kernel/kprobes.c
--- a/arch/i386/kernel/kprobes.c	2006-02-07 11:51:16.000000000 +0900
+++ b/arch/i386/kernel/kprobes.c	2006-02-08 16:05:35.000000000 +0900
@@ -325,8 +325,8 @@ no_kprobe:
  {
 	 asm volatile ( ".global kretprobe_trampoline\n"
  			"kretprobe_trampoline: \n"
-			"	subl $8, %esp\n"
 			"	pushf\n"
+			/* skip cs, eip, orig_eax, es, ds */
 			"	subl $20, %esp\n"
 			"	pushl %eax\n"
 			"	pushl %ebp\n"
@@ -336,13 +336,12 @@ no_kprobe:
 			"	pushl %ecx\n"
 			"	pushl %ebx\n"
 			"	movl %esp, %eax\n"
-			"	pushl %eax\n"
-			"	addl $60, %eax\n"
-			"	movl %eax, 56(%esp)\n"
-			"	movl $trampoline_handler, %eax\n"
-			"	call *%eax\n"
-			"	addl $4, %esp\n"
-			"	movl %eax, 56(%esp)\n"
+			"	call trampoline_handler\n"
+			/* move eflags to cs */
+			"	movl 48(%esp), %edx\n"
+			"	movl %edx, 44(%esp)\n"
+			/* save true return address on eflags */
+			"	movl %eax, 48(%esp)\n"
 			"	popl %ebx\n"
 			"	popl %ecx\n"
 			"	popl %edx\n"
@@ -350,16 +349,16 @@ no_kprobe:
 			"	popl %edi\n"
 			"	popl %ebp\n"
 			"	popl %eax\n"
-			"	addl $20, %esp\n"
+			/* skip eip, orig_eax, es, ds */
+			"	addl $16, %esp\n"
 			"	popf\n"
-			"	addl $4, %esp\n"
 			"	ret\n");
 }

 /*
  * Called from kretprobe_trampoline
  */
-asmlinkage void *__kprobes trampoline_handler(struct pt_regs *regs)
+fastcall void *__kprobes trampoline_handler(struct pt_regs *regs)
 {
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;