x86/kprobes bug? (was: [PATCH 1/3] x86/alternatives: Teach text_poke_bp() to emulate instructions)

[Peter Zijlstra <peterz@infradead.org>]

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?