lkml.org 
[lkml]   [2019]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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().

    \
     
     \ /
      Last update: 2019-10-15 17:52    [W:5.816 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site