This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC][PATCH][take2]kretprobe: kretprobe-booster against 2.6.15-rc5-mm3 for i386
- From: Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>
- To: Masami Hiramatsu <hiramatu at sdl dot hitachi dot co dot jp>
- Cc: maneesh at in dot ibm dot com, anil dot s dot keshavamurthy at intel dot com, systemtap at sources dot redhat dot com, Yumiko Sugita <sugita at sdl dot hitachi dot co dot jp>, Satoshi Oshima <soshima at redhat dot com>, Hideo Aoki <haoki at redhat dot com>
- Date: Tue, 24 Jan 2006 14:52:05 +0530
- Subject: Re: [RFC][PATCH][take2]kretprobe: kretprobe-booster against 2.6.15-rc5-mm3 for i386
- References: <43AAA8FF.1070404@sdl.hitachi.co.jp>
- Reply-to: ananth at in dot ibm dot com
On Thu, Dec 22, 2005 at 10:24:15PM +0900, Masami Hiramatsu wrote:
> Hi,
>
> Here is a patch of the kretprobe-booster for i386 arch.
> I expect this patch works fine with preemptible kernel.
> But it needs more review.
>
> Kretprobe make a target function return to tranpoline code.
> With kretprobe-booster, the tranpoline code calls kretprobe's
> handler directly instead of causing break interruption.
> And tranpoline code returns again to original return address.
>
> This new tranpoline code saves and restores registers, so it
> has much compatibility with normal kretprobe.
Looks good (I trust you've tested it in most common scenarios). Look for
some minor comments (^^^) below
Ananth
>
> Best Regards,
>
> --
> Masami HIRAMATSU
> 2nd Research Dept.
> Hitachi, Ltd., Systems Development Laboratory
> E-mail: hiramatu@sdl.hitachi.co.jp
>
> kprobes.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------------
> 1 files changed, 40 insertions(+), 20 deletions(-)
> diff -Narup a/arch/i386/kernel/kprobes.c b/arch/i386/kernel/kprobes.c
> --- a/arch/i386/kernel/kprobes.c 2005-12-20 19:55:42.000000000 +0900
> +++ b/arch/i386/kernel/kprobes.c 2005-12-20 20:11:47.000000000 +0900
> @@ -237,22 +237,53 @@ no_kprobe:
> return ret;
> }
>
> +static asmlinkage void *__kprobes trampoline_handler(struct pt_regs *regs);
> +static void *arch_trampoline_callee;
> +
> /*
> * For function-return probes, init_kprobes() establishes a probepoint
> * 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 arch_trampoline_callee, %eax\n"
^^^^
Why do we need this aliasing of trampoline_handler to
arch_trampoline_callee? Can't we just invoke trampoline_handler()
instead?
> + " 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)
> +static asmlinkage void *__kprobes trampoline_handler(struct pt_regs *regs)
> {
> struct kretprobe_instance *ri = NULL;
> struct hlist_head *head;
> @@ -297,18 +328,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;
> }
>
> /*
> @@ -539,12 +563,8 @@ 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);
> + arch_trampoline_callee= trampoline_handler;
> + return 0;
> }
^^^
This __init call can possibly do away with if we don't need the
arch_trampoline_callee obfuscation, but may need to be a {} for arch
compatibility sake.