Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAHk-=wjc-ktbOr7ZHMY8gfAmHxUK+aMdDsQjeh+BvmQwnQfN_g@mail.gmail.com>
In-Reply-To: <CAHk-=wjc-ktbOr7ZHMY8gfAmHxUK+aMdDsQjeh+BvmQwnQfN_g@mail.gmail.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Wed, 1 Jul 2020 11:29:26 -0700
Message-ID: <CALCETrXLcyuVT2JCrb0d-5Oq8aRLrERUdhRfGLxVZyYYo3BoBQ@mail.gmail.com>
Subject: Re: objtool clac/stac handling change..
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Josh Poimboeuf <jpoimboe@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jul 1, 2020 at 11:22 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Josh / PeterZ,
>  it turns out that clang seems to now have fixed the last known
> nagging details with "asm goto" with outputs, so I'm looking at
> actually trying to merge the support for that in the kernel.
>
> The main annoyance isn't actually using "asm goto" at all, the main
> annoyance is just that it will all have to be conditional on whether
> the compiler supports it or not. We have the config option for that
> already, but it will just end up with two copies of the code depending
> on that option.
>
> It's not a huge deal: the recent cleanups wrt the x86 uaccess code
> have made the code _much_ more straightforward and legible, and I'm
> not so worried about it all.
>
> Except that when I looked at this, I realized that I really had picked
> the wrong model for how exceptions are handled wrt stac/clac. In
> particular user access exceptions return with stac set, so we end up
> having a code pattern where the error case will also have to do the
> user_access_end() to finish the STAC region.
>
> Adding a user_access_end() to the user exception fault handler is trivial.
>
> But the thing I'm asking you for is how nasty it would be to change
> objtool to have those rules?
>
> IOW, right now we have
>
>     if (!user_acces_begin(...))
>            goto efault;
>     unsafe_get/put_user(ptr, val, label);
>     user_access_end();
>     return 0;
>
> label:
>      user_access_end();
> efaulr:
>      return -EFAULT;
>
> and I'd like to make the "label" case just go to "efault", with
> objtool knowing that the exception handling already did the
> user_access_end().

Do we really want the exception handling to do the CLAC?  Having
unsafe_get_user() do CLAC seems surprising to me, and it will break
use cases like:

if (!user_access_begin(...)
  goto out;

ret = unsafe_get_user(...);

user_access_end();

check ret;

I have no problem with a special ex_handler_uaccess_clac, but I don't
think it should be the default.

--Andy