linux-kernel - Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALCETrXo1zLXcRX-tF-MQ5qa7sLrKoAYdn5ox-F-kp-3JhPaXg@mail.gmail.com>
Date:   Fri, 2 Dec 2016 12:41:11 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Borislav Petkov <bp@...en8.de>, Borislav Petkov <bp@...nel.org>,
        Andy Lutomirski <luto@...nel.org>, Peter Anvin <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brian Gerst <brgerst@...il.com>,
        Matthew Whitehead <tedheadster@...il.com>,
        Henrique de Moraes Holschuh <hmh@....eng.br>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Cooper <andrew.cooper3@...rix.com>
Subject: Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 11:35 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Fri, Dec 2, 2016 at 11:30 AM, Andy Lutomirski <luto@...capital.net> wrote:
>>
>> How's this?
>
> Looks ok. I do think that
>
>> I suppose it could be an unconditional IRET-to-self, but that's a good
>> deal slower and not a whole lot simpler.  Although if we start doing
>> it right, performance won't really matter here.
>
> Considering you already got the iret-to-self wrong in the first
> version, I really like the "one single unconditional version" so that
> everybody tests that _one_ thing and there isn't anything subtle going
> on.
>
> Hmm?

Okay, sold.  It makes the patchset much much shorter, too.

>
> And yes, if it turns out that performance matters, we almost certainly
> are doing something really wrong, and we shouldn't be using that
> sync_core() thing in that place anyway.

To play devil's advocate (and definitely out of scope for this
particular patchset), is user code permitted to do:

1. Map a page RX at one address and RW somewhere else (for nice ASLR).
2. Write to the RW mapping.
3. CPUID or IRET-to-self.
4. Jump to the RX mapping.

Because, if so, we should maybe serialize whenever we migrate a
process to a different CPU.  (We *definitely* need to flush the store
buffer when migrating, because the x86 architecture makes some memory
ordering promises that get broken if a store from a thread stays in
the store buffer of a different CPU when the thread gets migrated.)
And if we're going to start serializing when migrating a thread, then
we actually care about performance, in which case we should optimize
the crap out of this thing, which probably means using MFENCE on AMD
CPUs (AMD promises that MFENCE flushes the pipeline.  Intel seems to
be confused as to exactly what effect MFENCE has, or at least I'm
confused as to what Intel thinks MFENCE does.)  And we should make
sure that we only do the extra flush when we don't switch mms.

--Andy