On Tue, Oct 22, 2019 at 10:35:21AM +0900, Masami Hiramatsu wrote:
> On Fri, 18 Oct 2019 09:35:37 +0200
> Peter Zijlstra <[email protected]> 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 <[email protected]>
Thanks!
