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

[Masami Hiramatsu <hiramatu at sdl dot hitachi dot co dot jp>]

Hi, Anil

Thank you for your review.

Keshavamurthy Anil S wrote:
> 	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.

OK, I hope the issues can be solved.

> 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.

I think old djprobe has the same issue. So, it will be solved by using
safety check routine after removing breakpoint. Current kprobe uses
synchronize_sched() for waiting rcu processing in unregister_kprobe().
In my opinion, the solution is that introducing safety check same as
djprobe instead of synchronize_sched() in unregister_kprobe().

The safety check routine of djprobe waits until all cpus execute the works,
and the works are executed from the keventd.
So we can ensure followings when a work is executed:
- interrupt handlers are finished on the cpu.
- p->ainsn.insn is not executed on the cpu.

And also, on preemptible kernel, the booster is not enabled where the
kernel preemption is enabled. So, there are no preempted threads on
p->ainsn.insn.

>>   @@ -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.

Please look this conditional sentence. (copy_eip == p->ainsn.insn)

>>   +               if ( regs->eip > copy_eip &&
                                 ^^^
Thus the kprobe-booster does not write jmp opcode on the 1st instruction
of p->ainsn.insn. Single step execution starts with the 1st byte of
p->ainsn.insn. And only 1st instruction is executed by single step
execution.
So, I think the second one you pointed out is not a problem.

Best Regards,

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