Date:   Thu, 19 Nov 2020 13:06:19 +0100
From:   Alexandre Chartre <alexandre.chartre@...cle.com>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Joerg Roedel <jroedel@...e.de>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        jan.setjeeilers@...cle.com, Junaid Shahid <junaids@...gle.com>,
        oweisse@...gle.com, Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Alexander Graf <graf@...zon.de>, mgross@...ux.intel.com,
        kuzuno@...il.com
Subject: Re: [RFC][PATCH v2 12/21] x86/pti: Use PTI stack instead of
 trampoline stack



On 11/19/20 9:05 AM, Alexandre Chartre wrote:
>>>>>>>>>
>>>>>>>>> When entering the kernel from userland, use the per-task PTI stack
>>>>>>>>> instead of the per-cpu trampoline stack. Like the trampoline stack,
>>>>>>>>> the PTI stack is mapped both in the kernel and in the user page-table.
>>>>>>>>> Using a per-task stack which is mapped into the kernel and the user
>>>>>>>>> page-table instead of a per-cpu stack will allow executing more code
>>>>>>>>> before switching to the kernel stack and to the kernel page-table.
>>>>>>>>
>>>>>>>> Why?
>>>>>>>
>>>>>>> When executing more code in the kernel, we are likely to reach a point
>>>>>>> where we need to sleep while we are using the user page-table, so we need
>>>>>>> to be using a per-thread stack.
>>>>>>>
>>>>>>>> I can't immediately evaluate how nasty the page table setup is because
>>>>>>>> it's not in this patch.
>>>>>>>
>>>>>>> The page-table is the regular page-table as introduced by PTI. It is just
>>>>>>> augmented with a few additional mapping which are in patch 11 (x86/pti:
>>>>>>> Extend PTI user mappings).
>>>>>>>
>>>>>>>>     But AFAICS the only thing that this enables is sleeping with user pagetables.
>>>>>>>
>>>>>>> That's precisely the point, it allows to sleep with the user page-table.
>>>>>>>
>>>>>>>> Do we really need to do that?
>>>>>>>
>>>>>>> Actually, probably not with this particular patchset, because I do the page-table
>>>>>>> switch at the very beginning and end of the C handler. I had some code where I
>>>>>>> moved the page-table switch deeper in the kernel handler where you definitively
>>>>>>> can sleep (for example, if you switch back to the user page-table before
>>>>>>> exit_to_user_mode_prepare()).
>>>>>>>
>>>>>>> So a first step should probably be to not introduce the per-task PTI trampoline stack,
>>>>>>> and stick with the existing trampoline stack. The per-task PTI trampoline stack can
>>>>>>> be introduced later when the page-table switch is moved deeper in the C handler and
>>>>>>> we can effectively sleep while using the user page-table.
>>>>>>
>>>>>> Seems reasonable.
>>>>>>
>>>>>
>>>>> I finally remember why I have introduced a per-task PTI trampoline stack right now:
>>>>> that's to be able to move the CR3 switch anywhere in the C handler. To do so, we need
>>>>> a per-task stack to enter (and return) from the C handler as the handler can potentially
>>>>> go to sleep.
>>>>>
>>>>> Without a per-task trampoline stack, we would be limited to call the switch CR3 functions
>>>>> from the assembly entry code before and after calling the C function handler (also called
>>>>> from assembly).
>>>>
>>>> The noinstr part of the C entry code won't sleep.
>>>>
>>>
>>> But the noinstr part of the handler can sleep, and if it does we will need to
>>> preserve the trampoline stack (even if we switch to the per-task kernel stack to
>>> execute the noinstr part).
>>>
>>> Example:
>>>
>>> #define DEFINE_IDTENTRY(func)                                           \
>>> static __always_inline void __##func(struct pt_regs *regs);             \
>>>                                                                           \
>>> __visible noinstr void func(struct pt_regs *regs)                       \
>>> {                                                                       \
>>>           irqentry_state_t state;         -+                              \
>>>                                            |                              \
>>>           user_pagetable_escape(regs);     | use trampoline stack (1)
>>>           state = irqentry_enter(regs);    |                              \
>>>           instrumentation_begin();        -+                              \
>>>           run_idt(__##func, regs);       |===| run __func() on kernel stack (this can sleep)
>>>           instrumentation_end();          -+                              \
>>>           irqentry_exit(regs, state);      | use trampoline stack (2)
>>>           user_pagetable_return(regs);    -+                              \
>>> }
>>>
>>> Between (1) and (2) we need to preserve and use the same trampoline stack
>>> in case __func() went sleeping.
>>>
>>
>> Why?  Right now, we have the percpu entry stack, and we do just fine
>> if we enter on one percpu stack and exit from a different one.
>> We would need to call from asm to C on the entry stack, return back to
>> asm, and then switch stacks.
>>
> 
> That's the problem: I didn't want to return back to asm, so that the pagetable
> switch can be done anywhere in the C handler.
> 
> So yes, returning to asm to switch the stack is the solution if we want to avoid
> having per-task trampoline stack. The drawback is that this forces to do the
> page-table switch at the beginning and end of the handler; the pagetable switch
> cannot be moved deeper down into the C handler.
> 
> But that's probably a good first step (effectively just moving CR3 switch to C
> without adding per-task trampoline stack). I will update the patches to do that,
> and we can defer the per-task trampoline stack to later if there's an effective
> need for it.
> 

That might not be a good first step after all... Calling CR3 switch C functions
from assembly introduces extra pt_regs copies between the trampoline stack and the
kernel stack.

Currently when entering syscall, we immediately switches CR3 and builds pt_regs
directly on the kernel stack. On return, registers are restored from pt_regs from
the kernel stack, the return frame is built on the trampoline stack and then we
switch CR3.

To call CR3 switch C functions on syscall entry, we need to switch to the trampoline
stack, build pt_regs on the trampoline stack, call CR3 switch, switch to the kernel
stack, copy pt_regs to the kernel stack. On return, we have to copy pt_regs back to
the trampoline stack, call CR3 switch, restore registers.

This is less of an impact for interrupt because we enter on the trampoline stack and
the current code already builds pt_regs on the trampoline stack and copies it to the
kernel stack (although this can certainly be avoided in the current code).

I am not comfortable adding these extra steps in syscall and interrupt as the current
code is fairly optimized. With a per-task trampoline stack, we don't have extra copy
because we can build pt_regs directly on the trampoline stack and it will preserved
even when switching to the kernel stack. On syscall/interrupt return, it also saves
a copy of the iret frame from the kernel stack to the trampoline stack.

alex.

Powered by blists - more mailing lists