Re: [PATCH v4 12/16] x86/kprobes: Fix ordering

[Peter Zijlstra]

On Tue, Oct 22, 2019 at 10:35:21AM +0900, Masami Hiramatsu wrote:
> On Fri, 18 Oct 2019 09:35:37 +0200
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> > Kprobes does something like:
> > 
> > register:
> > 	arch_arm_kprobe()
> > 	  text_poke(INT3)
> >           /* guarantees nothing, INT3 will become visible at some point, maybe */
> > 
> >         kprobe_optimizer()
> > 	  /* guarantees the bytes after INT3 are unused */
> > 	  syncrhonize_rcu_tasks();
> > 	  text_poke_bp(JMP32);
> > 	  /* implies IPI-sync, kprobe really is enabled */
> > 
> > 
> > unregister:
> > 	__disarm_kprobe()
> > 	  unoptimize_kprobe()
> > 	    text_poke_bp(INT3 + tail);
> > 	    /* implies IPI-sync, so tail is guaranteed visible */
> >           arch_disarm_kprobe()
> >             text_poke(old);
> > 	    /* guarantees nothing, old will maybe become visible */
> > 
> > 	synchronize_rcu()
> > 
> >         free-stuff
> 
> Note that this is only for the case of optimized kprobe.
> (On some probe points we can not optimize it)

Of course..

> > Now the problem is that on register, the synchronize_rcu_tasks() does
> > not imply sufficient to guarantee all CPUs have already observed INT3
> > (although in practise this is exceedingly unlikely not to have
> > happened) (similar to how MEMBARRIER_CMD_PRIVATE_EXPEDITED does not
> > imply MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE).
> 
> OK, so the sync_core() after int3 is needed to guarantee the probe
> is enabled on each core.

Indeed, AFAIU the old instruction can stay in I$ or micro-ops caches
which are not (fully) cache coherent.

So without forcing a serializing instruction (CPUID, CR3 write, etc..)
there is no guarantee.

> > Worse, even if it did, we'd have to do 2 synchronize calls to provide
> > the guarantee we're looking for, the first to ensure INT3 is visible,
> > the second to guarantee nobody is then still using the instruction
> > bytes after INT3.
> 
> I think this 2nd guarantee is done by syncrhonize_rcu() if we
> put sync_core() after int3. syncrhonize_rcu() ensures that
> all cores once scheduled and all interrupts have done.

Right, with IPI it all works, without IPI it might be that the INT3 is
still visible when we start synchronize_rcu() and thereby violate the
RCU guarantees.

> > --- a/arch/x86/kernel/kprobes/opt.c
> > +++ b/arch/x86/kernel/kprobes/opt.c
> > @@ -444,14 +444,10 @@ void arch_optimize_kprobes(struct list_h
> >  /* Replace a relative jump with a breakpoint (int3).  */
> >  void arch_unoptimize_kprobe(struct optimized_kprobe *op)
> >  {
> > -	u8 insn_buff[JMP32_INSN_SIZE];
> > -
> > -	/* Set int3 to first byte for kprobes */
> > -	insn_buff[0] = INT3_INSN_OPCODE;
> > -	memcpy(insn_buff + 1, op->optinsn.copied_insn, DISP32_SIZE);
> > -
> > -	text_poke_bp(op->kp.addr, insn_buff, JMP32_INSN_SIZE,
> > -		     text_gen_insn(JMP32_INSN_OPCODE, op->kp.addr, op->optinsn.insn));
> > +	arch_arm_kprobe(&op->kp);
> > +	text_poke(op->kp.addr + INT3_INSN_SIZE,
> > +		  op->optinsn.copied_insn, DISP32_SIZE);
> > +	text_poke_sync();
> >  }
> 
> For this part, I thought it was same as what text_poke_bp() does.
> But, indeed, this looks better (simpler & lighter) than using
> text_poke_bp()...

Indeed. The reason I wrote it as such is that now the
text_poke_bp(.emulate) argument is unused, which I can go remove in a
future patch.

The whole reason I went looking here is that I was going to write a
comment on how this was correct; it seems I've still to do that..

/me adds the two entries to the TODO list.

> So, in total, this looks good to me.
> 
> Acked-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Thanks!