From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stable-owner@vger.kernel.org>
Received: from mail-io0-f196.google.com ([209.85.223.196]:46057 "EHLO
        mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S934919AbeFGRGn (ORCPT
        <rfc822;stable@vger.kernel.org>); Thu, 7 Jun 2018 13:06:43 -0400
MIME-Version: 1.0
References: <alpine.DEB.2.21.1806061223560.1579@nanos.tec.linutronix.de> <152838798950.14521.4893346294059739135.stgit@dwillia2-desk3.amr.corp.intel.com>
In-Reply-To: <152838798950.14521.4893346294059739135.stgit@dwillia2-desk3.amr.corp.intel.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 7 Jun 2018 10:06:31 -0700
Message-ID: <CA+55aFyp1xYWomHUHhSv0habEVDXLMOKB_uJnRUSmoVt6SitRQ@mail.gmail.com>
Subject: Re: [PATCH v2] x86/spectre_v1: Disable compiler optimizations over array_index_mask_nospec()
To: Dan Williams <dan.j.williams@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
        stable <stable@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: stable-owner@vger.kernel.org
List-ID: <stable.vger.kernel.org>

On Thu, Jun 7, 2018 at 9:23 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Mark notes that gcc optimization passes have the potential to elide
> necessary invocations of this instruction sequence, so mark the asm
> volatile.

Ack. I'm not entirely sure this matters much, but it certainly isn't
wrong either.

The reason I'm not 100% convinced this matters is that gcc can *still*
mess things up for us by simply adding conditionals elsewhere.

For example, let's say we write this:

    if (idx < foo) {
        idx = array_idx_nospec(idx, foo);
        do_something(idx);
    } else {
        do_something_else();
    }

then everything is obviously fine, right? With the volatile on the
array_idx_nospec(), we're guaranteed to use the right reduced idx, and
there's only one user, so we're all good.

Except maybe do_something(idx) looks something like this:

    do_something(int idx)
    {
        do_something_else()
        access(idx);
    }

and gcc decides that "hey, I can combine the two do_something_else()
cases", and then generates code that is basically

    if (idx < foo)
        idx = array_idx_nospec(idx, foo);
    do_something_else();
    if (idx < foo)
        access(idx);

instead. And now we're back to the "first branch can be predicted
correctly, second branch can be mis-predicted".

Honestly, I don't really care, and I don't think the kernel _should_
care. I don't think this is a problem in practice. I'm just saying
that adding a "volatile" on array_idx_nospec() doesn't really
guarantee anything, since it's not a volatile over the whole relevant
sequence, only over that small part.

So I think the volatile is fine, but I really think it doesn't matter
either. We're not going to plug every theoretical hole, and I think
the hole that the volatile plugs is theoretical, not practical.

                      Linus