[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALCETrXwci8L=53UTq8=LEs+5yPgdHnidFf6iz3wZ0SkZwd1Eg@mail.gmail.com>
Date: Mon, 5 Nov 2018 12:05:57 -0800
From: Andy Lutomirski <luto@...capital.net>
To: Nadav Amit <namit@...are.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
"H. Peter Anvin" <hpa@...or.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Andrew Lutomirski <luto@...nel.org>,
Kees Cook <keescook@...omium.org>,
Dave Hansen <dave.hansen@...el.com>,
Masami Hiramatsu <mhiramat@...nel.org>
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@...are.com> wrote:
>
> From: Andy Lutomirski
> Sent: November 5, 2018 at 7:03:49 PM GMT
> > To: Nadav Amit <namit@...are.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>, H. Peter Anvin <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, Andy Lutomirski <luto@...nel.org>, Kees Cook <keescook@...omium.org>, Dave Hansen <dave.hansen@...el.com>, Masami Hiramatsu <mhiramat@...nel.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@...are.com> wrote:
> >>
> >> From: Andy Lutomirski
> >> Sent: November 5, 2018 at 5:22:32 PM GMT
> >>> To: Peter Zijlstra <peterz@...radead.org>
> >>> Cc: Nadav Amit <namit@...are.com>, Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org, x86@...nel.org, H. Peter Anvin <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, Andy Lutomirski <luto@...nel.org>, Kees Cook <keescook@...omium.org>, Dave Hansen <dave.hansen@...el.com>, Masami Hiramatsu <mhiramat@...nel.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@...radead.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
Powered by blists - more mailing lists