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

[Satoshi Oshima <soshima at redhat dot com>]

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