Messages in this thread | | | From | Andy Lutomirski <> | Date | Mon, 5 Nov 2018 12:05:57 -0800 | Subject | Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early_init |
| |
On Mon, Nov 5, 2018 at 11:25 AM Nadav Amit <namit@vmware.com> wrote: > > From: Andy Lutomirski > Sent: November 5, 2018 at 7:03:49 PM GMT > > To: Nadav Amit <namit@vmware.com> > > Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>, H. Peter Anvin <hpa@zytor.com>, Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, Andy Lutomirski <luto@kernel.org>, Kees Cook <keescook@chromium.org>, Dave Hansen <dave.hansen@intel.com>, Masami Hiramatsu <mhiramat@kernel.org> > > Subject: Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early_init > > > > > > > > > >> On Nov 5, 2018, at 9:49 AM, Nadav Amit <namit@vmware.com> wrote: > >> > >> From: Andy Lutomirski > >> Sent: November 5, 2018 at 5:22:32 PM GMT > >>> To: Peter Zijlstra <peterz@infradead.org> > >>> Cc: Nadav Amit <namit@vmware.com>, Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org, x86@kernel.org, H. Peter Anvin <hpa@zytor.com>, Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, Andy Lutomirski <luto@kernel.org>, Kees Cook <keescook@chromium.org>, Dave Hansen <dave.hansen@intel.com>, Masami Hiramatsu <mhiramat@kernel.org> > >>> Subject: Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early_init > >>> > >>> > >>> > >>>>> On Nov 5, 2018, at 6:09 AM, Peter Zijlstra <peterz@infradead.org> wrote: > >>>>> > >>>>> On Fri, Nov 02, 2018 at 04:29:41PM -0700, Nadav Amit wrote: > >>>>> diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c > >>>>> index aac0c1f7e354..367c1d0c20a3 100644 > >>>>> --- a/arch/x86/kernel/jump_label.c > >>>>> +++ b/arch/x86/kernel/jump_label.c > >>>>> @@ -52,7 +52,13 @@ static void __ref __jump_label_transform(struct jump_entry *entry, > >>>>> jmp.offset = jump_entry_target(entry) - > >>>>> (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE); > >>>>> > >>>>> - if (early_boot_irqs_disabled) > >>>>> + /* > >>>>> + * As long as we are in early boot, we can use text_poke_early(), which > >>>>> + * is more efficient: the memory was still not marked as read-only (it > >>>>> + * is only marked after poking_init()). This also prevents us from using > >>>>> + * text_poke() before poking_init() is called. > >>>>> + */ > >>>>> + if (!early_boot_done) > >>>>> poker = text_poke_early; > >>>>> > >>>>> if (type == JUMP_LABEL_JMP) { > >>>> > >>>> It took me a while to untangle init/maze^H^Hin.c... but I think this > >>>> is all we need: > >>>> > >>>> diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c > >>>> index aac0c1f7e354..ed5fe274a7d8 100644 > >>>> --- a/arch/x86/kernel/jump_label.c > >>>> +++ b/arch/x86/kernel/jump_label.c > >>>> @@ -52,7 +52,12 @@ static void __ref __jump_label_transform(struct jump_entry *entry, > >>>> jmp.offset = jump_entry_target(entry) - > >>>> (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE); > >>>> > >>>> - if (early_boot_irqs_disabled) > >>>> + /* > >>>> + * As long as we're UP and not yet marked RO, we can use > >>>> + * text_poke_early; SYSTEM_BOOTING guarantees both, as we switch to > >>>> + * SYSTEM_SCHEDULING before going either. > >>>> + */ > >>>> + if (system_state == SYSTEM_BOOTING) > >>>> poker = text_poke_early; > >>>> > >>>> if (type == JUMP_LABEL_JMP) { > >>> > >>> Can we move this logic into text_poke() and get rid of text_poke_early()? > >> > >> This will negatively affect poking of modules doing module loading, e.g., > >> apply_paravirt(). This can be resolved by keeping track when the module is > >> write-protected and giving a module parameter to text_poke(). Does it worth > >> the complexity? > > > > Probably not. > > > > OTOH, why does alternative patching need text_poke() at all? Can’t it just > > write to the text? > > Good question. According to my understanding, these games of > text_poke_early() are not needed, at least for modules (on Intel). > > Intel SDM 11.6 "SELF-MODIFYING CODE” says: > > "A write to a memory location in a code segment that is currently cached in > the processor causes the associated cache line (or lines) to be invalidated. > This check is based on the physical address of the instruction.” > > Then the manual talks about prefetched instructions, but the modules code is > presumably not be “prefetchable” at this point. So I think it should be > safe, but I guess that you reviewed Intel/AMD manuals better when you wrote > sync_core().
Beats the heck out of me.
Linus, hpa, or Dave, a question for you: suppose I map some page writably, write to it, then upgrade permissions to allow execute. Must I force all CPUs that might execute from it without first serializing to serialize? I suspect this doesn't really affect user code, but it may affect the module loader.
To be safe, shouldn't the module loader broadcast an IPI to sync_core() everywhere after loading a module and before making it runnable, regardless of alternative patching?
IOW, the right sequence of events probably ought to me:
1. Allocate the memory and map it. 2. Copy in the text. 3. Patch alternatives, etc. This is logically just like (2) from an architectural perspective -- we're just writing to memory that won't be executed. 4. Serialize everything. 5. Run it!
> > Anyhow, there should be a function that wraps the memcpy() to keep track > when someone changes the text (for potential future use). > > Does it make sense? Do you want me to give it a spin?
Sure, I guess. Linus, what do you think?
> > Thanks, > Nadav
-- Andy Lutomirski AMA Capital Management, LLC
| |