Re: [PATCH v3 2/7] x86/jump_label: Use text_poke_early() during early_init

[Nadav Amit]

From: Andy Lutomirski
Sent: November 5, 2018 at 7:03:49 PM GMT
> To: Nadav Amit <namit@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>, Ingo Molnar <mingo@xxxxxxxxxx>, LKML <linux-kernel@xxxxxxxxxxxxxxx>, X86 ML <x86@xxxxxxxxxx>, H. Peter Anvin <hpa@xxxxxxxxx>, Thomas Gleixner <tglx@xxxxxxxxxxxxx>, Borislav Petkov <bp@xxxxxxxxx>, Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>, Andy Lutomirski <luto@xxxxxxxxxx>, Kees Cook <keescook@xxxxxxxxxxxx>, Dave Hansen <dave.hansen@xxxxxxxxx>, Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> 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@xxxxxxxxxx> wrote:
>> 
>> From: Andy Lutomirski
>> Sent: November 5, 2018 at 5:22:32 PM GMT
>>> To: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>>> Cc: Nadav Amit <namit@xxxxxxxxxx>, Ingo Molnar <mingo@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, x86@xxxxxxxxxx, H. Peter Anvin <hpa@xxxxxxxxx>, Thomas Gleixner <tglx@xxxxxxxxxxxxx>, Borislav Petkov <bp@xxxxxxxxx>, Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>, Andy Lutomirski <luto@xxxxxxxxxx>, Kees Cook <keescook@xxxxxxxxxxxx>, Dave Hansen <dave.hansen@xxxxxxxxx>, Masami Hiramatsu <mhiramat@xxxxxxxxxx>
>>> 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@xxxxxxxxxxxxx> 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().

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?

Thanks,
Nadav