From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753292AbbC3OpX (ORCPT ); Mon, 30 Mar 2015 10:45:23 -0400 Received: from mail-ob0-f178.google.com ([209.85.214.178]:35209 "EHLO mail-ob0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752752AbbC3OpW (ORCPT ); Mon, 30 Mar 2015 10:45:22 -0400 MIME-Version: 1.0 In-Reply-To: References: <1427373731-13056-1-git-send-email-dvlasenk@redhat.com> <55195D3E.4060608@redhat.com> From: Andy Lutomirski Date: Mon, 30 Mar 2015 07:45:01 -0700 Message-ID: Subject: Re: [PATCH] x86/asm/entry/64: better check for canonical address To: Denys Vlasenko Cc: Borislav Petkov , X86 ML , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 30, 2015 at 7:30 AM, Andy Lutomirski wrote: > On Mon, Mar 30, 2015 at 7:27 AM, Denys Vlasenko wrote: >> On 03/26/2015 07:45 PM, Andy Lutomirski wrote: >>> On Thu, Mar 26, 2015 at 5:42 AM, Denys Vlasenko 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 >>>> CC: Borislav Petkov >>>> 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