* RE: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
@ 2005-12-21 18:11 Keshavamurthy, Anil S
2005-12-22 6:07 ` Masami Hiramatsu
0 siblings, 1 reply; 11+ messages in thread
From: Keshavamurthy, Anil S @ 2005-12-21 18:11 UTC (permalink / raw)
To: Masami Hiramatsu, ananth, Satoshi Oshima
Cc: systemtap, Yumiko Sugita, Hideo Aoki
>You are correct. I had misunderstood the synchronize_sched().
>I read the RCU code again, and I understood that the
>synchronize_sched()
>is enough to check safety of the kprobe-booster.
>So, previous kprobe-booster patch is safe, but it is for old
>-mm kernel.
>I update and post it again soon.
>
In previous kprobe-booster patch, I did not see the config option
where you enable kprobe-booster only if kernel preemption is disabled.
So are you going to include this in your new patch?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-12-21 18:11 [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386 Keshavamurthy, Anil S
@ 2005-12-22 6:07 ` Masami Hiramatsu
0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2005-12-22 6:07 UTC (permalink / raw)
To: Keshavamurthy, Anil S
Cc: ananth, Satoshi Oshima, systemtap, Yumiko Sugita, Hideo Aoki
Hi, Anil
Keshavamurthy, Anil S wrote:
>>You are correct. I had misunderstood the synchronize_sched().
>>I read the RCU code again, and I understood that the
>>synchronize_sched()
>>is enough to check safety of the kprobe-booster.
>>So, previous kprobe-booster patch is safe, but it is for old
>>-mm kernel.
>>I update and post it again soon.
>>
>
> In previous kprobe-booster patch, I did not see the config option
> where you enable kprobe-booster only if kernel preemption is disabled.
> So are you going to include this in your new patch?
Yes, I'm going to include the following fix which I posted previously.
With this fix, kprobes checks preempt_count before boosting.
When preempt_count == 1, this kernel thread becomes
preemptable after calling preempt_enable_no_resched().
Then the kernel thread can be preempted on the instruction
buffer. So, we must disable boosting in this case.
This is simpler than disabling whole of booster-patch
on preemptable kernel.
-------- Original Message --------
Subject: Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
Date: Tue, 06 Dec 2005 14:27:23 +0900
From: Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>
To: systemtap@sources.redhat.com
CC: Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>, Yumiko Sugita <sugita@sdl.hitachi.co.jp>, Satoshi Oshima <soshima@redhat.com>, Hideo Aoki <haoki@redhat.com>,
"Keshavamurthy, Anil S" <anil.s.keshavamurthy@intel.com>, ananth@in.ibm.com
References: <43870DDB.8020306@sdl.hitachi.co.jp> <438B142B.5080407@sdl.hitachi.co.jp> <438B148F.9000004@sdl.hitachi.co.jp> <438B14E1.3010301@sdl.hitachi.co.jp>
Hi,
I forgot to enable preemption in previous booster patch.
With this fix, the booster can work safely on the preemptible
kernel.
For safety, the booster is enabled only if the pre-handler
of kprobe is called from where the preemption is prohibited.
Masami Hiramatsu wrote:
> @@ -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 &&
#ifdef CONFIG_PREEMPT
preempt_count() != 1 &&
#endif
> + !p->post_handler && !p->break_handler ) {
> + /* Boost up -- we can execute copied instructions directly */
> + reset_current_kprobe();
> + regs->eip = (unsigned long)&p->ainsn.insn;
preempt_enable_no_resched();
> + return 1;
> + }
> +
> ss_probe:
> prepare_singlestep(p, regs);
> kcb->kprobe_status = KPROBE_HIT_SS;
--
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-12-21 8:30 ` Ananth N Mavinakayanahalli
@ 2005-12-21 17:39 ` Masami Hiramatsu
0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2005-12-21 17:39 UTC (permalink / raw)
To: ananth, Keshavamurthy Anil S, Satoshi Oshima
Cc: systemtap, Yumiko Sugita, Hideo Aoki
Hi, Ananth, Anil, and Satoshi
Ananth N Mavinakayanahalli wrote:
> On Tue, Dec 20, 2005 at 11:01:39AM -0800, Keshavamurthy Anil S wrote:
>
>>On Tue, Dec 20, 2005 at 10:21:14PM +0900, Masami Hiramatsu wrote:
>>
>>>Hi, Anil
>>>
>>>Masami Hiramatsu wrote:
>>>
>>>>>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().
>>>
>>>I found the rcu_barrier() function that works similar to djprobe's
>>>safety check routine.
>>>
>>>
>>>>The safety check routine of djprobe waits until all cpus execute the works,
>>>>and the works are executed from the keventd.
>>>
>>>The rcu_barrier() function waits until all cpus have gone through a
>>>quiescent state.
>>>
>>>- The CPU performs a process switch.
>>>- The CPU starts executing in User Mode.
>>>- The CPU executes the idle loop
>>>In each of these cases, We say that the CPU has gone through
>>>a quiescent state.
>>>
>>>If we call rcu_barrier() after "int3" is removed, we can ensure
>>>followings after the rcu_barrier() called.
>>>- interrupt handlers are finished on all CPUs.
>>>- p->ainsn.insn is not executed on all CPUs.
>>>And we can remove a boosted kprobe safely.
>>>
>>>Current kprobes uses synchronize_sched(). I think this function
>>>checks safety of only current CPU. So, it may not ensure above
>>>conditions. But rcu_barrier() checks safety of all CPUs. So, it
>>>can ensure above conditions.
>>>How would you think about this?
>>
>>You are correct, we need to ensure safety of all CPUs.
>>rcu_barrier() will work.
>
> Unless I've understood RCU incorrectly, rcu_barrier() is not needed in
> the kprobes case. As I read the code, you'll need to use rcu_barrier()
> in cases where you have to run the completion routine on each online cpu.
>
> All we care about in kprobes case is that the other cpus don't hold a
> reference to the just removed kprobe, so we can complete the kprobe
> unregistration. Return from synchronize_sched() ensures that all the
> other cpus have passed through a quiescent state (one of the conditions
> Masami has listed above).
You are correct. I had misunderstood the synchronize_sched().
I read the RCU code again, and I understood that the synchronize_sched()
is enough to check safety of the kprobe-booster.
So, previous kprobe-booster patch is safe, but it is for old -mm kernel.
I update and post it again soon.
> Also remember we don't define our own callback function either.
>
> Ananth
>
Thanks,
--
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-12-20 19:38 ` Keshavamurthy Anil S
@ 2005-12-21 8:30 ` Ananth N Mavinakayanahalli
2005-12-21 17:39 ` Masami Hiramatsu
0 siblings, 1 reply; 11+ messages in thread
From: Ananth N Mavinakayanahalli @ 2005-12-21 8:30 UTC (permalink / raw)
To: Keshavamurthy Anil S
Cc: Masami Hiramatsu, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki
On Tue, Dec 20, 2005 at 11:01:39AM -0800, Keshavamurthy Anil S wrote:
> On Tue, Dec 20, 2005 at 10:21:14PM +0900, Masami Hiramatsu wrote:
> > Hi, Anil
> >
> > Masami Hiramatsu wrote:
> > >>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().
> >
> > I found the rcu_barrier() function that works similar to djprobe's
> > safety check routine.
> >
> > > The safety check routine of djprobe waits until all cpus execute the works,
> > > and the works are executed from the keventd.
> >
> > The rcu_barrier() function waits until all cpus have gone through a
> > quiescent state.
> >
> > - The CPU performs a process switch.
> > - The CPU starts executing in User Mode.
> > - The CPU executes the idle loop
> > In each of these cases, We say that the CPU has gone through
> > a quiescent state.
> >
> > If we call rcu_barrier() after "int3" is removed, we can ensure
> > followings after the rcu_barrier() called.
> > - interrupt handlers are finished on all CPUs.
> > - p->ainsn.insn is not executed on all CPUs.
> > And we can remove a boosted kprobe safely.
> >
> > Current kprobes uses synchronize_sched(). I think this function
> > checks safety of only current CPU. So, it may not ensure above
> > conditions. But rcu_barrier() checks safety of all CPUs. So, it
> > can ensure above conditions.
> > How would you think about this?
>
> You are correct, we need to ensure safety of all CPUs.
> rcu_barrier() will work.
Unless I've understood RCU incorrectly, rcu_barrier() is not needed in
the kprobes case. As I read the code, you'll need to use rcu_barrier()
in cases where you have to run the completion routine on each online cpu.
All we care about in kprobes case is that the other cpus don't hold a
reference to the just removed kprobe, so we can complete the kprobe
unregistration. Return from synchronize_sched() ensures that all the
other cpus have passed through a quiescent state (one of the conditions
Masami has listed above).
Also remember we don't define our own callback function either.
Ananth
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-12-20 13:45 ` Masami Hiramatsu
2005-12-20 17:27 ` Satoshi Oshima
@ 2005-12-20 19:38 ` Keshavamurthy Anil S
2005-12-21 8:30 ` Ananth N Mavinakayanahalli
1 sibling, 1 reply; 11+ messages in thread
From: Keshavamurthy Anil S @ 2005-12-20 19:38 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Keshavamurthy Anil S, systemtap, Yumiko Sugita, Satoshi Oshima,
Hideo Aoki
On Tue, Dec 20, 2005 at 10:21:14PM +0900, Masami Hiramatsu wrote:
> Hi, Anil
>
> Masami Hiramatsu wrote:
> >>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().
>
> I found the rcu_barrier() function that works similar to djprobe's
> safety check routine.
>
> > The safety check routine of djprobe waits until all cpus execute the works,
> > and the works are executed from the keventd.
>
> The rcu_barrier() function waits until all cpus have gone through a
> quiescent state.
>
> - The CPU performs a process switch.
> - The CPU starts executing in User Mode.
> - The CPU executes the idle loop
> In each of these cases, We say that the CPU has gone through
> a quiescent state.
>
> If we call rcu_barrier() after "int3" is removed, we can ensure
> followings after the rcu_barrier() called.
> - interrupt handlers are finished on all CPUs.
> - p->ainsn.insn is not executed on all CPUs.
> And we can remove a boosted kprobe safely.
>
> Current kprobes uses synchronize_sched(). I think this function
> checks safety of only current CPU. So, it may not ensure above
> conditions. But rcu_barrier() checks safety of all CPUs. So, it
> can ensure above conditions.
> How would you think about this?
You are correct, we need to ensure safety of all CPUs.
rcu_barrier() will work.
>
> > 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.
Yes, this is also true, you have to disable booster when kernel
preemption is enabled. So in effect booster will work only when
kernel preemption is disabled.
>
>
> --
> Masami HIRAMATSU
> 2nd Research Dept.
> Hitachi, Ltd., Systems Development Laboratory
> E-mail: hiramatu@sdl.hitachi.co.jp
>
- Anil
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-12-20 13:45 ` Masami Hiramatsu
@ 2005-12-20 17:27 ` Satoshi Oshima
2005-12-20 19:38 ` Keshavamurthy Anil S
1 sibling, 0 replies; 11+ messages in thread
From: Satoshi Oshima @ 2005-12-20 17:27 UTC (permalink / raw)
To: Masami Hiramatsu, Keshavamurthy Anil S
Cc: systemtap, Yumiko Sugita, Hideo Aoki
Hi, Anil and Masami
Masami Hiramatsu wrote:
> Hi, Anil
>
> Masami Hiramatsu wrote:
>
>>>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().
>
> I found the rcu_barrier() function that works similar to djprobe's
> safety check routine.
I had not understood how RCU works. But I have learned it.
rcu_barrier() registers callbacks on all CPU.
synchronize_sched() registers callback on current CPU only.
Safety check of RCU is the same between rcu_barrier() and
synchronize_sched().
So synchronize_sched() is enough for safety checking of
kprobe-booster.
You don't have to use rcu_barrier() instead of
synchronize_sched().
>
>>The safety check routine of djprobe waits until all cpus execute the works,
>>and the works are executed from the keventd.
>
>
> The rcu_barrier() function waits until all cpus have gone through a
> quiescent state.
>
> - The CPU performs a process switch.
> - The CPU starts executing in User Mode.
> - The CPU executes the idle loop
> In each of these cases, We say that the CPU has gone through
> a quiescent state.
>
> If we call rcu_barrier() after "int3" is removed, we can ensure
> followings after the rcu_barrier() called.
> - interrupt handlers are finished on all CPUs.
> - p->ainsn.insn is not executed on all CPUs.
> And we can remove a boosted kprobe safely.
>
> Current kprobes uses synchronize_sched(). I think this function
> checks safety of only current CPU. So, it may not ensure above
> conditions. But rcu_barrier() checks safety of all CPUs. So, it
> can ensure above conditions.
> How would you think about this?
>
>
>>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.
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-12-14 14:19 ` Masami Hiramatsu
@ 2005-12-20 13:45 ` Masami Hiramatsu
2005-12-20 17:27 ` Satoshi Oshima
2005-12-20 19:38 ` Keshavamurthy Anil S
0 siblings, 2 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2005-12-20 13:45 UTC (permalink / raw)
To: Keshavamurthy Anil S
Cc: Masami Hiramatsu, systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki
Hi, Anil
Masami Hiramatsu wrote:
>>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().
I found the rcu_barrier() function that works similar to djprobe's
safety check routine.
> The safety check routine of djprobe waits until all cpus execute the works,
> and the works are executed from the keventd.
The rcu_barrier() function waits until all cpus have gone through a
quiescent state.
- The CPU performs a process switch.
- The CPU starts executing in User Mode.
- The CPU executes the idle loop
In each of these cases, We say that the CPU has gone through
a quiescent state.
If we call rcu_barrier() after "int3" is removed, we can ensure
followings after the rcu_barrier() called.
- interrupt handlers are finished on all CPUs.
- p->ainsn.insn is not executed on all CPUs.
And we can remove a boosted kprobe safely.
Current kprobes uses synchronize_sched(). I think this function
checks safety of only current CPU. So, it may not ensure above
conditions. But rcu_barrier() checks safety of all CPUs. So, it
can ensure above conditions.
How would you think about this?
> 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.
--
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-12-12 19:49 ` Keshavamurthy Anil S
@ 2005-12-14 14:19 ` Masami Hiramatsu
2005-12-20 13:45 ` Masami Hiramatsu
0 siblings, 1 reply; 11+ messages in thread
From: Masami Hiramatsu @ 2005-12-14 14:19 UTC (permalink / raw)
To: Keshavamurthy Anil S; +Cc: systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-11-28 14:32 ` [RFC][Patch 2/2][take2]kprobe: " Masami Hiramatsu
2005-12-06 5:27 ` Masami Hiramatsu
@ 2005-12-12 19:49 ` Keshavamurthy Anil S
2005-12-14 14:19 ` Masami Hiramatsu
1 sibling, 1 reply; 11+ messages in thread
From: Keshavamurthy Anil S @ 2005-12-12 19:49 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: systemtap, Yumiko Sugita, Satoshi Oshima, Hideo Aoki
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-11-28 14:32 ` [RFC][Patch 2/2][take2]kprobe: " Masami Hiramatsu
@ 2005-12-06 5:27 ` Masami Hiramatsu
2005-12-12 19:49 ` Keshavamurthy Anil S
1 sibling, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2005-12-06 5:27 UTC (permalink / raw)
To: systemtap
Cc: Masami Hiramatsu, Yumiko Sugita, Satoshi Oshima, Hideo Aoki,
Keshavamurthy, Anil S, ananth
Hi,
I forgot to enable preemption in previous booster patch.
With this fix, the booster can work safely on the preemptible
kernel.
For safety, the booster is enabled only if the pre-handler
of kprobe is called from where the preemption is prohibited.
Masami Hiramatsu wrote:
> @@ -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 &&
#ifdef CONFIG_PREEMPT
preempt_count() != 1 &&
#endif
> + !p->post_handler && !p->break_handler ) {
> + /* Boost up -- we can execute copied instructions directly */
> + reset_current_kprobe();
> + regs->eip = (unsigned long)&p->ainsn.insn;
preempt_enable_no_resched();
> + return 1;
> + }
> +
> ss_probe:
> prepare_singlestep(p, regs);
> kcb->kprobe_status = KPROBE_HIT_SS;
--
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386
2005-11-28 14:30 ` [RFC][Patch 1/2][take2]kprobe: " Masami Hiramatsu
@ 2005-11-28 14:32 ` Masami Hiramatsu
2005-12-06 5:27 ` Masami Hiramatsu
2005-12-12 19:49 ` Keshavamurthy Anil S
0 siblings, 2 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2005-11-28 14:32 UTC (permalink / raw)
To: systemtap; +Cc: Masami Hiramatsu, Yumiko Sugita, Satoshi Oshima, Hideo Aoki
Hi,
This patch adds kprobe-booster function for i386 arch.
I fixed mistakes in previous patch.
--
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp
Signed-off-by: Masami Hiramatsu <hiramatu@sdl.hitachi.co.jp>
arch/i386/kernel/kprobes.c | 78 +++++++++++++++++++++++++++++++++++++++++++--
include/asm-i386/kprobes.h | 4 ++
2 files changed, 80 insertions(+), 2 deletions(-)
diff -Narup a/arch/i386/kernel/kprobes.c b/arch/i386/kernel/kprobes.c
--- a/arch/i386/kernel/kprobes.c 2005-11-28 11:43:10.000000000 +0900
+++ b/arch/i386/kernel/kprobes.c 2005-11-28 15:57:39.000000000 +0900
@@ -41,6 +41,49 @@ void jprobe_return_end(void);
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+/* insert a jmp code */
+static inline void set_jmp_op(void *from, void *to)
+{
+ struct __arch_jmp_op {
+ char op;
+ long raddr;
+ } __attribute__((packed)) *jop;
+ jop = (struct __arch_jmp_op *)from;
+ jop->raddr = (long)(to) - ((long)(from) + 5);
+ jop->op = RELATIVEJUMP_INSTRUCTION;
+}
+
+/*
+ * returns non-zero if opcodes can be boosted.
+ */
+static inline int can_boost(kprobe_opcode_t opcode)
+{
+ switch (opcode & 0xf0 ) {
+ case 0x70:
+ return 0; /* can't boost conditional jump */
+ case 0x90:
+ /* can't boost call and pushf */
+ return opcode != 0x9a && opcode != 0x9c;
+ case 0xc0:
+ /* can't boost undefined opcodes and soft-interruptions */
+ return (0xc1 < opcode && opcode < 0xc6) ||
+ (0xc7 < opcode && opcode < 0xcc) || opcode == 0xcf;
+ case 0xd0:
+ /* can boost AA* and XLAT */
+ return (opcode == 0xd4 || opcode == 0xd5 || opcode == 0xd7);
+ case 0xe0:
+ /* can boost in/out and (may be) jmps */
+ return (0xe3 < opcode && opcode != 0xe8);
+ case 0xf0:
+ /* clear and set flags can be boost */
+ return (opcode == 0xf5 || (0xf7 < opcode && opcode < 0xfe));
+ default:
+ /* currently, can't boost 2 bytes opcodes */
+ return opcode != 0x0f;
+ }
+}
+
+
/*
* returns non-zero if opcode modifies the interrupt flag.
*/
@@ -65,6 +108,11 @@ void __kprobes arch_copy_kprobe(struct k
{
memcpy(p->ainsn.insn, p->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
p->opcode = *p->addr;
+ if (can_boost(p->opcode)) {
+ p->ainsn.boostable = 0;
+ } else {
+ p->ainsn.boostable = -1;
+ }
}
void __kprobes arch_arm_kprobe(struct kprobe *p)
@@ -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;
+ }
+
ss_probe:
prepare_singlestep(p, regs);
kcb->kprobe_status = KPROBE_HIT_SS;
@@ -340,6 +396,8 @@ int __kprobes trampoline_probe_handler(s
* 2) If the single-stepped instruction was a call, the return address
* that is atop the stack is the address following the copied instruction.
* We need to make it the address following the original instruction.
+ *
+ * This function also checks instruction size for preparing direct execution.
*/
static void __kprobes resume_execution(struct kprobe *p,
struct pt_regs *regs, struct kprobe_ctlblk *kcb)
@@ -360,6 +418,7 @@ static void __kprobes resume_execution(s
case 0xca:
case 0xea: /* jmp absolute -- eip is correct */
/* eip is already adjusted, no more changes required */
+ p->ainsn.boostable = 1;
goto no_change;
case 0xe8: /* call relative - Fix return addr */
*tos = orig_eip + (*tos - copy_eip);
@@ -367,18 +426,33 @@ static void __kprobes resume_execution(s
case 0xff:
if ((p->ainsn.insn[1] & 0x30) == 0x10) {
/* call absolute, indirect */
- /* Fix return addr; eip is correct. */
+ /* Fix return addr; eip is correct
+ But this is not boostable */
*tos = orig_eip + (*tos - copy_eip);
goto no_change;
} else if (((p->ainsn.insn[1] & 0x31) == 0x20) || /* jmp near, absolute indirect */
((p->ainsn.insn[1] & 0x31) == 0x21)) { /* jmp far, absolute indirect */
- /* eip is correct. */
+ /* eip is correct. And this is boostable */
+ p->ainsn.boostable = 1;
goto no_change;
}
default:
break;
}
+ 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));
+ p->ainsn.boostable = 1;
+ } else {
+ p->ainsn.boostable = -1;
+ }
+ }
+
regs->eip = orig_eip + (regs->eip - copy_eip);
no_change:
diff -Narup a/include/asm-i386/kprobes.h b/include/asm-i386/kprobes.h
--- a/include/asm-i386/kprobes.h 2005-11-08 11:51:02.000000000 +0900
+++ b/include/asm-i386/kprobes.h 2005-11-24 21:41:30.000000000 +0900
@@ -31,6 +31,7 @@ struct pt_regs;
typedef u8 kprobe_opcode_t;
#define BREAKPOINT_INSTRUCTION 0xcc
+#define RELATIVEJUMP_INSTRUCTION 0xe9
#define MAX_INSN_SIZE 16
#define MAX_STACK_SIZE 64
#define MIN_STACK_SIZE(ADDR) (((MAX_STACK_SIZE) < \
@@ -47,6 +48,9 @@ void kretprobe_trampoline(void);
struct arch_specific_insn {
/* copy of the original instruction */
kprobe_opcode_t insn[MAX_INSN_SIZE];
+ /* If this flag is not 0, this kprobe can be boost when its
+ post_handler and break_handler is not set. */
+ int boostable;
};
struct prev_kprobe {
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2005-12-22 6:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-12-21 18:11 [RFC][Patch 2/2][take2]kprobe: kprobe-booster against 2.6.14-mm1 for i386 Keshavamurthy, Anil S
2005-12-22 6:07 ` Masami Hiramatsu
-- strict thread matches above, loose matches on Subject: below --
2005-11-25 13:13 [RFC][Patch 0/2]kprobe: " Masami Hiramatsu
2005-11-28 14:29 ` [RFC][Patch 0/2][take2]kprobe: " Masami Hiramatsu
2005-11-28 14:30 ` [RFC][Patch 1/2][take2]kprobe: " Masami Hiramatsu
2005-11-28 14:32 ` [RFC][Patch 2/2][take2]kprobe: " Masami Hiramatsu
2005-12-06 5:27 ` Masami Hiramatsu
2005-12-12 19:49 ` Keshavamurthy Anil S
2005-12-14 14:19 ` Masami Hiramatsu
2005-12-20 13:45 ` Masami Hiramatsu
2005-12-20 17:27 ` Satoshi Oshima
2005-12-20 19:38 ` Keshavamurthy Anil S
2005-12-21 8:30 ` Ananth N Mavinakayanahalli
2005-12-21 17:39 ` Masami Hiramatsu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).