Date: Sat, 13 Jan 2018 07:33:20 +0100
From: Willy Tarreau <w@1wt.eu>
To: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Borislav Petkov <bp@alien8.de>,
        Laura Abbott <labbott@redhat.com>, X86 ML <x86@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        stable <stable@vger.kernel.org>
Subject: Re: Yet another KPTI regression with 4.14.x series in a VM
Message-ID: <20180113063320.GC16821@1wt.eu>
References: <c7ecacdc-993e-d24f-b8d9-2af7615fd6f6@redhat.com>
 <alpine.DEB.2.20.1801121949220.2371@nanos>
 <9eb15489-da09-7a4c-0700-7b6eb99e6f7b@redhat.com>
 <alpine.DEB.2.20.1801122250080.2371@nanos>
 <CALCETrWOpg_OXThyMC66JQu7JF=TxzRwFrEgunUZt3H+VUfxrQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALCETrWOpg_OXThyMC66JQu7JF=TxzRwFrEgunUZt3H+VUfxrQ@mail.gmail.com>
User-Agent: Mutt/1.6.1 (2016-04-27)
Sender: linux-kernel-owner@vger.kernel.org

On Fri, Jan 12, 2018 at 10:08:20PM -0800, Andy Lutomirski wrote:
> In fact, it looks like this code is totally bogus and has never been
> correct at all.  Even in:
> 
> commit 4b1d5ae3b103eda43f9d0f85c355bb6995b03a30
> Author: Peter Zijlstra <peterz@infradead.org>
> Date:   Mon Dec 4 15:07:59 2017 +0100
> 
>     x86/mm: Use/Fix PCID to optimize user/kernel switches
> 
> We have:
> 
> .macro SWITCH_TO_USER_CR3_NOSTACK scratch_reg:req scratch_reg2:req
>         ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
>         mov     %cr3, \scratch_reg
> 
>         ALTERNATIVE "jmp .Lwrcr3_\@", "", X86_FEATURE_PCID
> 
> ...
> 
> .Lwrcr3_\@:
>         /* Flip the PGD and ASID to the user version */
>         orq     $(PTI_SWITCH_MASK), \scratch_reg
>         mov     \scratch_reg, %cr3
> .Lend_\@:
> 
> That's bogus.  PTI_SWITCH_MASK is 0x1800, which has PCID = 0x800.
> 
> This should probably use an alternative to select between 0x1000 and
> 0x800 depending on X86_FEATURE_PCID or just use an entirely different
> label for the !PCID case.
> 
> FWIW, this bit in SAVE_AND_SWITCH_TO_KERNEL_CR3
> 
>         testq   $(PTI_SWITCH_MASK), \scratch_reg
>         jz      .Ldone_\@
> 
> is a bit silly, too.  It's *correct* (I think), but shouldn't that
> just be bt $(PTI_SWITCH_PGTABLES_BIT), \scratch_reg, with the obvious
> caveat that the headers don't actually define PTI_SWITCH_PGTABLES_BIT?

I wondered the same initially when reading this but thought there was
surely a good reason that I could not understand due to my lack of
knowledge and stopped wondering. BTW your PTI_SWITCH_PGTABLES_BIT would
in fact be PAGE_SHIFT :-)

> Was this code *ever* tested with nopcid?

At least it booted fine in qemu on my machine where pcid was initially
disabled by default, and on an Atom D510 which doesn't have PCID. I've
worked on the initial per-task PTI code with this. That doesn't mean
it's valid, just that there are situations where it works fine :-)

Willy