Messages in this thread | | | Date | Wed, 9 Oct 2019 15:07:54 +0200 | From | Peter Zijlstra <> | Subject | x86/kprobes bug? (was: [PATCH 1/3] x86/alternatives: Teach text_poke_bp() to emulate instructions) |
| |
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?
| |