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

From: Jessica Yu
Date: Tue Oct 15 2019 - 11:51:16 EST

+++ Peter Zijlstra [15/10/19 15:56 +0200]:
On Tue, Oct 15, 2019 at 03:07:40PM +0200, Jessica Yu wrote:
I'm having trouble visualizing what changes you're planning on making.

I want all the text poking (jump_label, ftrace, klp whatever) to happen
_before_ we do the protection changes.

I also want to avoid flipping the protection state around unnecessarily,
because that clearly is just wasted work.

OK, that makes sense, thanks for clarifying.
I get that you want to squash ftrace_module_enable() into
ftrace_module_init(), before module_enable_ro(). I'm fine with that as
long as the races Steven described are addressed for affected arches.

Right, the problem is set_all_modules_text_*(), that relies on COMING
having made the protection changes.

After the x86 changes, there's only 2 more architectures that use that
function. I'll work on getting those converted and then we can delete
that function and worry no more about it.

And livepatch should be OK as long as klp_module_coming() is always
called *after* ftrace_module_enable(). But are you moving
klp_module_coming() before the module_enable_* calls as well? And if
so, why?

I wanted to move the COMING notifier callback before the protection
changes, because that is the easiest way to convert jump_label and
static_call and AFAICT nothing really cared aside from ftrace.

I think I would be fine with this as long as no notifiers/users rely
on the assumption that COMING == module enabled protections already.
I've yet to audit all the module notifiers (but I trust you've done
this already), and I think ftrace was the only user that relied on
this. For livepatch it's probably not immediately fixable without some
serious overhaul.

The alternative is introducing additional module states, which just adds
complexity we don't really need if we can just flip things around a

Yeah, I would prefer not adding more states if possible :-)

> 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.

Not sure if relevant, but just thought I'd clarify: IIRC,
klp_module_coming() is not poking the coming module, but it calls
module_enable_ro() on itself (the livepatch module) so it can apply
relocations and such on the new code, which lives inside the livepatch
module, and it needs to possibly do this numerous times over the
lifetime of the patch module for any coming module it is responsible
for patching (i.e., call module_enable_ro() on the patch module, not
necessarily the coming module). So I am not be sure why
klp_module_coming() should be moved before complete_formation(). I
hope I'm remembering the details correctly, livepatch folks feel free
to chime in if I'm incorrect here.

You mean it does module_disable_ro() ? That would be broken and it needs
to be fixed. Can some livepatch person explain what it does and why?

Gah, sorry, yes I meant module_disable_ro().