Re: [PATCH v3 5/6] x86/ftrace: Use text_poke()

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 11:54:49AM -0400, Steven Rostedt wrote:
> On Thu, 10 Oct 2019 16:05:13 +0200
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> > > Because we can't do the above once we have more than one CPU running.  
> > 
> > We loose BOOTING _long_ before we gain SMP.
> 
> Ah, yep. But I finally got it working with the following patch:

That looks like it can be done simpler; but my head is exploding, I'll
have to look at this in the AM.

> Is it really important to use text_poke() on text that is coming live?

What is really important is that we never have memory that is both
writable and executable (W^X).

Moving as much poking to before marking it RO (or moving the marking RO
later if that is the same thing) is a sane approach.

But once it gains X, we must never again mark it W, without first
removing X.

> That is, I really hate the above "set_ro" hack. This is because you
> moved the ro setting to create_trampoline() and then forcing the
> text_poke() on text that has just been created. I prefer to just modify
> it and then setting it to ro before it gets executed. Otherwise we need
> to do all these dances.

I thought create_trampoline() finished the whole thing; if it does not,
either make create_trampoline() do everything, or add a
finish_trampoline() callback to mark it complete.

> The same is with the module code. My patch was turning text to
> read-write that is not to be executed yet. Really, what's wrong with
> that?

The fact that it is executable; also the fact that you do it right after
we mark it ro. Flipping the memory protections back and forth is just
really poor everything.

Once this ftrace thing is sorted, we'll change x86 to _refuse_ to make
executable (kernel) memory writable.

Really the best solution is to move all the poking into
ftrace_module_init(), before we mark it RO+X. That's what I'm going to
do for jump_label and static_call as well, I just need to add that extra
notifier callback.