Re: [PATCH] x86/asm/entry/64: better check for canonical address

From: Andy Lutomirski <luto@amacapital.net>
To: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>, X86 ML <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86/asm/entry/64: better check for canonical address
Date: Mon, 30 Mar 2015 07:45:01 -0700	[thread overview]
Message-ID: <CALCETrWeGTM-4K4n0Bfh_fd_SrSFYjPvwQ4y-k+jdSANUKW99Q@mail.gmail.com> (raw)
In-Reply-To: <CALCETrV1DAOmV-JrspS-K9Vvz_nD0bnTw8t0Z-P7kno-pgsL5g@mail.gmail.com>

On Mon, Mar 30, 2015 at 7:30 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Mar 30, 2015 at 7:27 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>> On 03/26/2015 07:45 PM, Andy Lutomirski wrote:
>>> On Thu, Mar 26, 2015 at 5:42 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>>> This change makes the check exact (no more false positives
>>>> on kernel addresses).
>>>>
>>>> It isn't really important to be fully correct here -
>>>> almost all addresses we'll ever see will be userspace ones,
>>>> but OTOH it looks to be cheap enough:
>>>> the new code uses two more ALU ops but preserves %rcx,
>>>> allowing to not reload it from pt_regs->cx again.
>>>> On disassembly level, the changes are:
>>>>
>>>> cmp %rcx,0x80(%rsp) -> mov 0x80(%rsp),%r11; cmp %rcx,%r11
>>>> shr $0x2f,%rcx      -> shl $0x10,%rcx; sar $0x10,%rcx; cmp %rcx,%r11
>>>> mov 0x58(%rsp),%rcx -> (eliminated)
>>>>
>>>> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
>>>> CC: Borislav Petkov <bp@alien8.de>
>>>> CC: x86@kernel.org
>>>> CC: linux-kernel@vger.kernel.org
>>>> ---
>>>>
>>>> Andy, I'd undecided myself on the merits of doing this.
>>>> If you like it, feel free to take it in your tree.
>>>> I trimmed CC list to not bother too many people with this trivial
>>>> and quite possibly "useless churn"-class change.
>>>
>>> I suspect that the two added ALU ops are free for all practical
>>> purposes, and the performance of this path isn't *that* critical.
>>>
>>> If anyone is running with vsyscall=native because they need the
>>> performance, then this would be a big win.  Otherwise I don't have a
>>> real preference.  Anyone else have any thoughts here?
>>>
>>> Let me just run through the math quickly to make sure I believe all the numbers:
>>>
>>> Canonical addresses either start with 17 zeros or 17 ones.
>>>
>>> In the old code, we checked that the top (64-47) = 17 bits were all
>>> zero.  We did this by shifting right by 47 bits and making sure that
>>> nothing was left.
>>>
>>> In the new code, we're shifting left by (64 - 48) = 16 bits and then
>>> signed shifting right by the same amount, this propagating the 17th
>>> highest bit to all positions to its left.  If we get the same value we
>>> started with, then we're good to go.
>>>
>>> So it looks okay to me.
>>
>>
>> So please take it into your tree :)
>>
>
> Will do, but not until later this week because I'm on vacation and I'm
> allocating about ten minutes to using the computer :)  Or maybe Ingo
> will beat me.

Actually, before I do that, want to send a test case?  I don't think
it's that important (or easy) to test performance, but testing for
oopses is good.  Basing off of this:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=x86/entry&id=eeac7de873439bfb5cf49b04119f510fcbd5c040

might be reasonable, but it's also entirely optional -- it's just how
I would approach it.

--Andy