This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
- From: Keshavamurthy Anil S <anil dot s dot keshavamurthy at intel dot com>
- To: Masami Hiramatsu <hiramatu at sdl dot hitachi dot co dot jp>
- Cc: 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: Mon, 12 Dec 2005 11:48:45 -0800
- Subject: Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
- References: <438B14E1.3010301@sdl.hitachi.co.jp>
- Reply-to: Keshavamurthy Anil S <anil dot s dot keshavamurthy at intel dot com>
On Mon, Nov 28, 2005 at 06:32:01AM -0800, Masami Hiramatsu wrote:
>
> Hi,
>
> This patch adds kprobe-booster function for i386 arch.
> I fixed mistakes in previous patch.
Hi Masami,
As promised I finished reviewing your patch, at the moment
I see two issues w.r.t your patch. I think both the issues that
I have mentioned below is very hard to fix and I don;t have the
good solution to suggest at the moment.
My comments inline.
> @@ -235,6 +283,14 @@ static int __kprobes kprobe_handler(stru
> /* handler has already set things up, so skip ss setup
> */
> return 1;
>
> + if (p->ainsn.boostable == 1 &&
> + !p->post_handler && !p->break_handler ) {
> + /* Boost up -- we can execute copied instructions
> directly */
> + reset_current_kprobe();
> + regs->eip = (unsigned long)&p->ainsn.insn;
> + return 1;
Issue No 1:
In the above code patch, once you call reset_current_kprobes() and
preempt_enable_no_resched() ( I see preeempt_enable_no_resched()
in your fix to this code), you are not suppose to relay on
&p->ainsn.insn, the reason is that, on the other cpu technically
this memory can get freed due to unregister_kprobe() call.
In other words, by the time this cpu tries to execte the instruction
at regs->eip, that memory might have gone and system is bound to crash.
> @@ -367,18 +426,33 @@ static void __kprobes resume_execution(s
> + if (p->ainsn.boostable == 0) {
> + if ( regs->eip > copy_eip &&
> + (regs->eip - copy_eip) + 5 < MAX_INSN_SIZE) {
> + /* these instructions can be executed directly
> if it
> + jumps back to correct address. */
> + set_jmp_op((void *)regs->eip,
> + (void *)orig_eip + (regs->eip -
> copy_eip));
Issue No 2:
Since Kprobes is now highly parallel(thanks to RCU changes),
the kprobe_handler() can be in execution on multiple cpu's.
Due this parallel kprobe_handler() execution nature, it is also possible
to have some other cpu trying to single step on the copied instruction
at &p->ainsn.insn, at this very instant if set_jmp_op() in the above
call makes changs to that location, I am not sure how badly processor
can behave.
(In the above call to set_jmp_op(), I am assuming that regs->eip is same as
&p->ainsn.insn, and modifying this instruction without understanding
where the other processor are, is very dangerous)
Cheers,
Anil Keshavamurthy