Message-ID: <20191114135311.GW4131@hirez.programming.kicks-ass.net>
Date:   Thu, 14 Nov 2019 14:53:11 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     x86 <x86@...nel.org>, linux-kernel <linux-kernel@...r.kernel.org>,
        rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        bristot <bristot@...hat.com>, jbaron@...mai.com,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>, Nadav Amit <namit@...are.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        paulmck <paulmck@...nel.org>
Subject: Re: [PATCH -v5 12/17] x86/kprobes: Fix ordering

On Wed, Nov 13, 2019 at 10:42:32AM -0500, Mathieu Desnoyers wrote:
> ----- On Nov 11, 2019, at 8:13 AM, Peter Zijlstra peterz@...radead.org 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();
> 
> syncrhonize -> synchronize

Fixed.

> >	  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
> > 
> > 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
> 
> practise -> practice

And fixed.

> > happened) (similar to how MEMBARRIER_CMD_PRIVATE_EXPEDITED does not
> > imply MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE).
> > 
> > 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'm not entirely convinced about this last statement though. AFAIU:
> 
> - Swapping between some instruction and INT3 is OK,
> - Swapping back between that INT3 and _original_ instruction is OK,
> - Anything else needs to have explicit core serialization.

So far, agreed.

> So I understand the part about requiring the synchronize call to guarantee
> that nobody is then still using the instruction bytes following INT3, but not
> the rationale for the first synchronization. What operation would theoretically
> follow this first synchronize call ?

I'm not completely sure what you're asking, so I'm going to explain too
much and hope I answered your question along the way. If not, please be
a little more specific.

First:

So what can happen with optimized kprobes is that the original
instruction is <5 bytes and we want to write a JMP.d32 (5 bytes).
Something like:

	83e:   48 89 e5                mov    %rsp,%rbp
	841:   48 85 c0                test   %rax,%rax

And we put a kprobe on the MOV. Then we poke INT3 at 0x83e and IPI-sync.
At that point the kprobe is active:

	83e:   cc 89 e5                int3 ; 2 byte garbage
	841:   48 85 c0                test   %rax,%rax

Now we want to optimize that thing. But imagine a task being preempted
at 0x841. If we poke in the JMP.d32 the above turns into gibberish

	83e:   e9 12 34 56 78		jmp +0x12345678

Then our task resumes, loads the instruction at 0x841, which then reads:

	841:   56 78 c0

And goes *bang*.

So what we do, after enabling the regular kprobe, is call
synchronize_rcu_tasks() to wait for each task to have passed through
schedule(). That guarantees no task is preempted inside the kprobe
shadow (when it triggers it ensures it resumes execution at an
instruction boundary further than 5 bytes away).


Second:

The argument I was making was that if we didn't IPI-sync, and if
synchronize_rcu_tasks() would imply the IPI-sync, we would still need
two invocations of it to achieve the desired result. Because only after
the implied IPI-sync must we guarantee no tasks is still inside the
kprobe 'shadow'.


Did that answer your question?

> I understand that this patch modifies arch_{arm,disarm}_kprobe to add
> core serialization after inserting/removing the INT3 even in the case
> where no optimized kprobes are used, which is heavier than what is
> strictly required without optimized kprobes.

I disagree, it is required even for normal kprobes, namely:

Without this additional sync it is uncertain when the kprobe would've
taken effect (if ever).

Suppose a CPU is stuck in a tight loop around the probed instruction,
then it would never have to re-fetch the changed text and thus never
trigger.

Powered by blists - more mailing lists