This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]