On Fri, Oct 04, 2019 at 10:45:40PM +0900, Masami Hiramatsu wrote:
> > > > > text_poke_bp(op->kp.addr, insn_buff, RELATIVEJUMP_SIZE,
> > > > > - op->optinsn.insn);
> > > > > + emulate_buff);
> > > > > }
> > >
> > > As argued in a previous thread, text_poke_bp() is broken when it changes
> > > more than a single instruction at a time.
> > >
> > > Now, ISTR optimized kprobes does something like:
> > >
> > > poke INT3
> >
> > Hmm, it does this using text_poke(), but lacks a
> > on_each_cpu(do_sync_core, NULL, 1), which I suppose is OK-ish IFF you do
> > that synchronize_rcu_tasks() after it, but less so if you don't.
> >
> > That is, without either, you can't really tell if the kprobe is in
> > effect or not.
>
> Yes, it doesn't wait the change by design at this moment.
Right, this might surprise some, I suppose, and I might've found a small
issue with it, see below.
> > > synchronize_rcu_tasks() /* waits for all tasks to schedule
> > > guarantees instructions after INT3
> > > are unused */
> > > install optimized probe /* overwrites multiple instrctions with
> > > JMP.d32 */
> > >
> > > And the above then undoes that by:
> > >
> > > poke INT3 on top of the optimzed probe
> > >
> > > poke tail instructions back /* guaranteed safe because the
> > > above INT3 poke ensures the
> > > JMP.d32 instruction is unused */
> > >
> > > poke head byte back
>
> Yes, anyway, the last poke should recover another INT3... (for kprobe)
It does indeed.
> > > Is this correct? If so, we should probably put a comment in there
> > > explaining how all this is unusual but safe.
So from what I can tell of kernel/kprobes.c, what it does is something like:
ARM: (__arm_kprobe)
text_poke(INT3)
/* guarantees nothing, INT3 will become visible at some point, maybe */
(kprobe_optimizer)
if (opt) {
/* guarantees the bytes after INT3 are unused */
syncrhonize_rcu_tasks();
text_poke_bp(JMP32);
/* implies IPI-sync, kprobe really is enabled */
}
DISARM: (__unregister_kprobe_top)
if (opt) {
text_poke_bp(INT3 + tail);
/* implies IPI-sync, so tail is guaranteed visible */
}
text_poke(old);
FREE: (__unregister_kprobe_bottom)
/* guarantees 'old' is visible and the kprobe really is unused, maybe */
synchronize_rcu();
free();
Now the problem is that I don't think the synchronize_rcu() at free
implies enough to guarantee 'old' really is visible on all CPUs.
Similarly, I don't think synchronize_rcu_tasks() is sufficient on the
ARM side either. It only provides the guarantee -provided- the INT3 is
actually visible. If it is not, all bets are off.
I'd feel much better if we switch arch_arm_kprobe() over to using
text_poke_bp(). Or at the very least add the on_each_cpu(do_sync_core)
to it.
Hmm?
