Re: [PATCH 15/15] x86: use non-set_fs based maccess routines

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Christoph Hellwig <hch@lst.de>
Cc: "the arch/x86 maintainers" <x86@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-parisc@vger.kernel.org,
	linux-um <linux-um@lists.infradead.org>,
	Netdev <netdev@vger.kernel.org>,
	bpf@vger.kernel.org, Linux-MM <linux-mm@kvack.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 15/15] x86: use non-set_fs based maccess routines
Date: Wed, 6 May 2020 12:01:32 -0700	[thread overview]
Message-ID: <CAHk-=wghKpGdTmD4EDfwX2uyppwxksU+nFyS1B--kbopcQAgwg@mail.gmail.com> (raw)
In-Reply-To: <20200506181543.GA7873@lst.de>

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

On Wed, May 6, 2020 at 11:15 AM Christoph Hellwig <hch@lst.de> wrote:
>
> That was the first prototype, and or x86 it works great, just the
> __user cases in maccess.c are a little ugly.  And they point to
> the real problem - for architectures like sparc and s390 that use
> an entirely separate address space for the kernel vs userspace
> I dont think just use unsafe_{get,put}_user will work, as they need
> different instructions.

Oh, absolutely. I did *NOT* mean that you'd use "unsafe_get_user()" as
the actual interface. I just meant that as an implementation detail on
x86, using "unsafe_get_user()" instead of "__get_user_size()"
internally both simplifies the implementation, and means that it
doesn't clash horribly with my local changes.

Btw, that brings up another issue: so that people can't mis-use those
kernel accessors and use them for user addresses, they probably should
actually do something like

        if ((long)addr >= 0)
                goto error_label;

on x86. IOW, have the "strict" kernel pointer behavior.

Otherwise somebody will start using them for user pointers, and it
will happen to work on old x86 without CLAC/STAC support.

Of course, maybe CLAC/STAC is so common these days (at least with
developers) that we don't have to worry about it.

> Btw, where is you magic private tree and what is the plan for it?

I don't want to make it a public branch, but here's a private bundle.

It's based on top of my current -git tree - I just maintain a separate
tree that I keep up-to-date locally for testing. My "normal" tree I do
build tests on (allmodconfig etc), this separate tree I keep around to
actually do boot tests on, and I end up using "current Linus' tree
plus this" as the code I actually run om my main desktop.

But this *ONLY* works with clang, and only with current HEAD of the
clang development tree. So it's almosty entirely useless to anybody
else right now. You literally have to clone the llvm tree, build your
own clang, and install it to even _build_ this.

I'm not planning on going any further than my local testing until the
whole thing calms down. The llvm tree still has some known bugs in the
asm goto with output area, and I want there to be an actual release of
it before I actually merge anything like this (and I need to do the
small extra work to then have that conditional "does the compiler
support asm goto with outputs" so that it works with gcc too).

But here you see what it is, if you want to. __get_user_size()
technically still exists, but it has the "target branch" semantics in
here, so your patch clashes badly with it. (Well, those are the
semantics you want, so "badly" may not be the right word, but
basically it means that if you _had_ used unsafe_get_user(), there
wouldn't have been those kinds of semantic conflicts).

                Linus

[-- Attachment #2: asm-goto-outputs.bundle --]
[-- Type: application/octet-stream, Size: 6251 bytes --]