Message-ID: <CALCETrV9ckeSwyjLn4hxuJeakxnkRQ71RAVf44nTWOFva9EJFA@mail.gmail.com>
Date:   Thu, 19 Nov 2020 08:06:42 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Alexandre Chartre <alexandre.chartre@...cle.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        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 Thu, Nov 19, 2020 at 4:06 AM Alexandre Chartre
<alexandre.chartre@...cle.com> wrote:
>
>
>
> 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.

IIRC during the early KPTI development, we went through a stage with
per-task stacks.  It was a mess, and we got rid of it for good reason.
Your series has a bad diffstat, is a lot of churn, doesn't even handle
the unmap part yet, and, by itself, adds basically no value.

We need to take a big step back and figure out what problem any of
this is solving.  As I understand it, most of the issue is that you
want to be able to switch between kernel and guest mode on one core
without exposing kernel memory to Meltdown or Spectre attacks from
guest code on another core.  If so, it seems to me that a much nicer
solution would be to use a per-cpu or per-vcpu stack for the vm entry.
It could plausibly even use the existing entry stack.  If you used a
per-vcpu stack, though, you could even sleep with some care.

Powered by blists - more mailing lists