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: Masami Hiramatsu <hiramatu at sdl dot hitachi dot co dot jp>
- To: Keshavamurthy Anil S <anil dot s dot keshavamurthy at intel dot com>
- 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: Wed, 14 Dec 2005 13:05:20 +0900
- Subject: Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
- References: <438B14E1.3010301@sdl.hitachi.co.jp> <20051212114845.A5922@unix-os.sc.intel.com>
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