Messages in this thread | | | Date | Tue, 15 Oct 2019 17:51:07 +0200 | From | Jessica Yu <> | Subject | Re: [PATCH v3 5/6] x86/ftrace: Use text_poke() |
| |
+++ 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 >little.
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().
| |