Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Al Viro]

On Fri, Aug 19, 2016 at 12:10:02PM -0700, Vineet Gupta wrote:
> Al reported potential issue with ARC get_user() as it wasn't clearing
> out destination pointer in case of fault due to bad address etc.

Applied to my branch with other similar fixes.  FWIW, there's another
interesting question in the same general area - __get_user() callers
tend to be on hot paths and they clump a _lot_.  That's the original
reasoning behind the __-variants; doing access_ok() once for the entire
bunch rather then repeating it for every single call.

However, access_ok() is not the only problem.  Testing if an error has
happened and conditional branching can also be sensitive; moreover,
on recent x86 SMAP-related setup/teardown is costly as hell.  The latter
problem is solved by bracketing the entire series of accesses with
a single setup/teardown pair (uaccess_begin()/uaccess_end()) and using
unsafe_get_user()/unsafe_put_user() between those.  The former has spawned
a bunch of solutions:

	* pretty much arch-independent optimization - use err |= __get_user()
instead of if (__get_user() != 0) goto fail.  We drop branching, but we
still get a plenty of crap.

	* x86-only get_user_ex().  Does *not* return anything, uses per-process
flag to indicate errors, the entire sequence is bracketed by uaccess_try()
and uaccess_catch(err), the latter dumps the flag into err.  Pairing of
brackets is enforced - expansion of uaccess_try() contains do { and
uaccess_catch() - } while (0).  Can't mix any userland access other than
{get,put}_user_ex/unsafe_{get,put}_user into the series - AC flag will be
buggered.  In particular, any use of __copy_{to,from}_user() is a bug there.

	* somewhat similar, __get_user_err(v, p, err) on assorted architectures
that are less register-starved than x86 is.  Those are equivalent to
	if (__get_user(v, p))
		err = -EFAULT;
and translate into something along the lines of
in .text:
	1: reg = *p;
	2: v = (__typeof(*p))reg;
in .text.fixup:
fixup(1): reg = 0; err = -EFAULT; goto 2;
That gives a branch-free path in the normal case, with fixups done out-of-line.
get_user_ex() is similar, except that it uses a field in current_thread_info()
where those use a local variable.  No bracketing needed - only access_ok()
before going there.

About a half of __get_user() callers are in arch/*, mostly in sigreturn(2)
and friends.  For those the use of arch-specific primitives is OK.  However,
there's another big pile in assorted compat code, and that obviously isn't
OK with arch-specific stuff.

I realize that asking such questions can very easily devolve into bikeshedding,
with a bunch of "only x86 matters anyway" thrown in, but... it would be
nice to come up with a syntax that could be used in arch-independent places.
I toyed with things like
	uaccess_begin();
	...
	get_user_ex(v, p, err);
	...
	put_user_ex(v, q, err);
	...
	copy_from_user_ex(&s, r, err);
	...
	copy_to_user_ex(&s, r, err);
	...
	copy_in_user_ex(t, r, err);
	...
	uaccess_check(err);
	...
	err |= sanity_check(...);	// returns 0 or -EFAULT
	...
	uaccess_end(err);
with x86 basically ignoring err in ..._ex() primitives and doing
err |= current_thread_info()->flag; in uaccess_end()/uaccess_check(), while
something that currently has __get_user_err() et.al. mapping get_user_ex()
to it and making uaccess_{begin,end,check} no-ops, but that's pretty much
a mechanical merge of those variants and none too pretty, at that.

Suggestions?

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

On Fri, Aug 19, 2016 at 2:24 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>         * x86-only get_user_ex().  Does *not* return anything, uses per-process
> flag to indicate errors, the entire sequence is bracketed by uaccess_try()
> and uaccess_catch(err), the latter dumps the flag into err.

I really don't want people to copy that pattern.

It's absolutely horrendous if you start taking faults, and you'll take
fault after fault after fault - in kernel space too. Yes, faults are
very very rare, but still, the interface is bad.

I'd much rather other architectures used the "unsafe_get_user()"
model, which currently only "strncpy_from_user()" and friends use. If
gcc ever ends up supporting "asm goto" with outputs, that interface is
actually able to generate pretty much optimal code.

And "unsafe_put_user()" can actually generate "perfect" code *today*,
because it doesn't have outputs, so "asm goto" actually works right
now. You can make it do the branch-out directly from the exception
case. It's not used right now, but as a replacement for the nasty
"put_user_ex()" model, it's actually much much better.

(I have some experimental patches that actually use "asm goto" in
"unsafe_put_user()" to get that nice code generation, but they only
work if your gcc version supports "asm goto", which some older
versions of gcc does not)

> Suggestions?

Please see "unsafe_put_user(x, ptr, error_label)" as the future. No,
right now it ends up doing the same old thing, but that is _fixable_
unlike the other strange special cases.

Side note: the "error-label" form was introduced in this merge window,
exactly because I wanted to have an interface that is optimizable in
the future.

See commit 1bd4403d86a1 ("unsafe_[get|put]_user: change interface to
use a error target label")

            Linus

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

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

On Fri, Aug 19, 2016 at 3:00 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> (I have some experimental patches that actually use "asm goto" in
> "unsafe_put_user()" to get that nice code generation, but they only
> work if your gcc version supports "asm goto", which some older
> versions of gcc does not)

Since you actually are looking at the user access stuff, I'll just put
them here.

This is from an old branch of mine, based on commit f6c658df6385 just
because that happened to be my top-of-tree when I was playing around
with it. It probably doesn't even apply right now, and as mentioned,
it depends on "asm goto" (there is no case for !CC_HAVE_ASM_GOTO).

With this, you actually get almost perfect code generation if you then
replace all the "put_user_ex()" calls with

        if (access_ok(..))
            return -EFAULT;

        user_access_begin();
        unsafe_put_user(x,ptr, error_label);
        unsafe_put_user(y,ptr2, error_label);
        ...
        user_access_end();
        return 0;

    error_label:
        user_access_end();
        return -EFAULT;

or something similar. The exception handler will jump directly to
"error_label", and there will be no testing of anything at all in the
usual no-exception cases, nor will there be any extra registers for
error values etc.

           Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 4917 bytes --]

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 2982387ba817..849debe7aa10 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -204,19 +204,14 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
 
 #ifdef CONFIG_X86_32
-#define __put_user_asm_u64(x, addr, err, errret)			\
-	asm volatile("\n"						\
-		     "1:	movl %%eax,0(%2)\n"			\
-		     "2:	movl %%edx,4(%2)\n"			\
-		     "3:"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "4:	movl %3,%0\n"				\
-		     "	jmp 3b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 4b)				\
-		     _ASM_EXTABLE(2b, 4b)				\
-		     : "=r" (err)					\
-		     : "A" (x), "r" (addr), "i" (errret), "0" (err))
+#define __put_user_goto_u64(x, addr, label)			\
+	asm volatile("\n"					\
+		     "1:	movl %%eax,0(%2)\n"		\
+		     "2:	movl %%edx,4(%2)\n"		\
+		     _ASM_EXTABLE(1b, %2l)			\
+		     _ASM_EXTABLE(2b, %2l)			\
+		     : : "A" (x), "r" (addr)			\
+		     : : label)
 
 #define __put_user_asm_ex_u64(x, addr)					\
 	asm volatile("\n"						\
@@ -231,8 +226,8 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 	asm volatile("call __put_user_8" : "=a" (__ret_pu)	\
 		     : "A" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx")
 #else
-#define __put_user_asm_u64(x, ptr, retval, errret) \
-	__put_user_asm(x, ptr, retval, "q", "", "er", errret)
+#define __put_user_goto_u64(x, ptr, label) \
+	__put_user_goto(x, ptr, "q", "", "er", label)
 #define __put_user_asm_ex_u64(x, addr)	\
 	__put_user_asm_ex(x, addr, "q", "", "er")
 #define __put_user_x8(x, ptr, __ret_pu) __put_user_x(8, x, ptr, __ret_pu)
@@ -293,23 +288,21 @@ extern void __put_user_8(void);
 	__builtin_expect(__ret_pu, 0);				\
 })
 
-#define __put_user_size(x, ptr, size, retval, errret)			\
+#define __put_user_size(x, ptr, size, label)				\
 do {									\
-	retval = 0;							\
 	__chk_user_ptr(ptr);						\
 	switch (size) {							\
 	case 1:								\
-		__put_user_asm(x, ptr, retval, "b", "b", "iq", errret);	\
+		__put_user_goto(x, ptr, "b", "b", "iq", label);	\
 		break;							\
 	case 2:								\
-		__put_user_asm(x, ptr, retval, "w", "w", "ir", errret);	\
+		__put_user_goto(x, ptr, "w", "w", "ir", label);		\
 		break;							\
 	case 4:								\
-		__put_user_asm(x, ptr, retval, "l", "k", "ir", errret);	\
+		__put_user_goto(x, ptr, "l", "k", "ir", label);		\
 		break;							\
 	case 8:								\
-		__put_user_asm_u64((__typeof__(*ptr))(x), ptr, retval,	\
-				   errret);				\
+		__put_user_goto_u64((__typeof__(*ptr))(x), ptr, label);	\
 		break;							\
 	default:							\
 		__put_user_bad();					\
@@ -419,9 +412,12 @@ do {									\
 
 #define __put_user_nocheck(x, ptr, size)			\
 ({								\
-	int __pu_err;						\
+	__label__ __pu_label;					\
+	int __pu_err = -EFAULT;					\
 	__uaccess_begin();					\
-	__put_user_size((x), (ptr), (size), __pu_err, -EFAULT);	\
+	__put_user_size((x), (ptr), (size), __pu_label);	\
+	__pu_err = 0;						\
+__pu_label:							\
 	__uaccess_end();					\
 	__builtin_expect(__pu_err, 0);				\
 })
@@ -446,17 +442,23 @@ struct __large_struct { unsigned long buf[100]; };
  * we do not write to any memory gcc knows about, so there are no
  * aliasing issues.
  */
-#define __put_user_asm(x, addr, err, itype, rtype, ltype, errret)	\
-	asm volatile("\n"						\
-		     "1:	mov"itype" %"rtype"1,%2\n"		\
-		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:	mov %3,%0\n"				\
-		     "	jmp 2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
-		     : "=r"(err)					\
-		     : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
+#define __put_user_goto(x, addr, itype, rtype, ltype, label)	\
+	asm volatile goto("\n"						\
+		"1:	mov"itype" %"rtype"0,%1\n"			\
+		_ASM_EXTABLE(1b, %l2)					\
+		: : ltype(x), "m" (__m(addr))				\
+		: : label)
+
+#define __put_user_failed(x, addr, itype, rtype, ltype, errret)		\
+	({	__label__ __puflab;					\
+		int __pufret = errret;					\
+		__put_user_goto(x,addr,itype,rtype,ltype,__puflab);	\
+		__pufret = 0;						\
+	__puflab: __pufret; })
+
+#define __put_user_asm(x, addr, retval, itype, rtype, ltype, errret)	do {	\
+	retval = __put_user_failed(x, addr, itype, rtype, ltype, errret);	\
+} while (0)
 
 #define __put_user_asm_ex(x, addr, itype, rtype, ltype)			\
 	asm volatile("1:	mov"itype" %"rtype"0,%1\n"		\
@@ -793,12 +795,9 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
 #define user_access_begin()	__uaccess_begin()
 #define user_access_end()	__uaccess_end()
 
-#define unsafe_put_user(x, ptr)						\
-({										\
-	int __pu_err;								\
-	__put_user_size((x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);		\
-	__builtin_expect(__pu_err, 0);						\
-})
+#define unsafe_put_user(x, ptr, label)	\
+	__put_user_size((x), (ptr), sizeof(*(ptr)), label)
+
 
 #define unsafe_get_user(x, ptr)						\
 ({										\

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

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

On Fri, Aug 19, 2016 at 3:11 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>>
>> (I have some experimental patches that actually use "asm goto" in
>> "unsafe_put_user()" to get that nice code generation, but they only
>> work if your gcc version supports "asm goto", which some older
>> versions of gcc does not)
>
> Since you actually are looking at the user access stuff, I'll just put
> them here.

Here's an updated patch that applies on current git and that actually
uses this for filldir() (but not signal handling).

It turns out that on Skylake, which supports SMAP, the clac/stac
instructions are quite slow, and doing them for each access makes
things insanely much slower than it could be. And "filldir" does the
user accesses one by one (except for the name copying), and is
actually somewhat common under some loads (ie the "find . -name XYZ"
kind of thing).

Anyway, the asm coming out of gcc looks nasty, because it has all the
ugly section stuiff and fixups for SMAP not existing on some CPU's
etc. So the resulting fs/readdir.s file is hard to read. But if you
look at the disassembly at the object file that hides all that (and
shows what the end result actually is), the actual filldir user
accesses end up looking beautiful, with no extra code anywhere. An
exception just goes to the EFAULT handling directly.

Sadly, unsafe_get_user() looking as good does require gcc improvements
that aren't imminent.

This patch is untested, although the earlier original pre-rebased
version of it actually got a fair amount of testing on my machine
(including the filldir use)

                    Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 6423 bytes --]

 arch/x86/include/asm/uaccess.h | 80 ++++++++++++++++++++----------------------
 fs/readdir.c                   | 22 ++++++------
 2 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a0ae610b9280..8d6b299522f1 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -204,19 +204,14 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
 
 #ifdef CONFIG_X86_32
-#define __put_user_asm_u64(x, addr, err, errret)			\
-	asm volatile("\n"						\
-		     "1:	movl %%eax,0(%2)\n"			\
-		     "2:	movl %%edx,4(%2)\n"			\
-		     "3:"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "4:	movl %3,%0\n"				\
-		     "	jmp 3b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 4b)				\
-		     _ASM_EXTABLE(2b, 4b)				\
-		     : "=r" (err)					\
-		     : "A" (x), "r" (addr), "i" (errret), "0" (err))
+#define __put_user_goto_u64(x, addr, label)			\
+	asm volatile("\n"					\
+		     "1:	movl %%eax,0(%2)\n"		\
+		     "2:	movl %%edx,4(%2)\n"		\
+		     _ASM_EXTABLE(1b, %2l)			\
+		     _ASM_EXTABLE(2b, %2l)			\
+		     : : "A" (x), "r" (addr)			\
+		     : : label)
 
 #define __put_user_asm_ex_u64(x, addr)					\
 	asm volatile("\n"						\
@@ -231,8 +226,8 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 	asm volatile("call __put_user_8" : "=a" (__ret_pu)	\
 		     : "A" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx")
 #else
-#define __put_user_asm_u64(x, ptr, retval, errret) \
-	__put_user_asm(x, ptr, retval, "q", "", "er", errret)
+#define __put_user_goto_u64(x, ptr, label) \
+	__put_user_goto(x, ptr, "q", "", "er", label)
 #define __put_user_asm_ex_u64(x, addr)	\
 	__put_user_asm_ex(x, addr, "q", "", "er")
 #define __put_user_x8(x, ptr, __ret_pu) __put_user_x(8, x, ptr, __ret_pu)
@@ -293,23 +288,21 @@ extern void __put_user_8(void);
 	__builtin_expect(__ret_pu, 0);				\
 })
 
-#define __put_user_size(x, ptr, size, retval, errret)			\
+#define __put_user_size(x, ptr, size, label)				\
 do {									\
-	retval = 0;							\
 	__chk_user_ptr(ptr);						\
 	switch (size) {							\
 	case 1:								\
-		__put_user_asm(x, ptr, retval, "b", "b", "iq", errret);	\
+		__put_user_goto(x, ptr, "b", "b", "iq", label);	\
 		break;							\
 	case 2:								\
-		__put_user_asm(x, ptr, retval, "w", "w", "ir", errret);	\
+		__put_user_goto(x, ptr, "w", "w", "ir", label);		\
 		break;							\
 	case 4:								\
-		__put_user_asm(x, ptr, retval, "l", "k", "ir", errret);	\
+		__put_user_goto(x, ptr, "l", "k", "ir", label);		\
 		break;							\
 	case 8:								\
-		__put_user_asm_u64((__typeof__(*ptr))(x), ptr, retval,	\
-				   errret);				\
+		__put_user_goto_u64((__typeof__(*ptr))(x), ptr, label);	\
 		break;							\
 	default:							\
 		__put_user_bad();					\
@@ -438,9 +431,12 @@ do {									\
 
 #define __put_user_nocheck(x, ptr, size)			\
 ({								\
-	int __pu_err;						\
+	__label__ __pu_label;					\
+	int __pu_err = -EFAULT;					\
 	__uaccess_begin();					\
-	__put_user_size((x), (ptr), (size), __pu_err, -EFAULT);	\
+	__put_user_size((x), (ptr), (size), __pu_label);	\
+	__pu_err = 0;						\
+__pu_label:							\
 	__uaccess_end();					\
 	__builtin_expect(__pu_err, 0);				\
 })
@@ -465,17 +461,23 @@ struct __large_struct { unsigned long buf[100]; };
  * we do not write to any memory gcc knows about, so there are no
  * aliasing issues.
  */
-#define __put_user_asm(x, addr, err, itype, rtype, ltype, errret)	\
-	asm volatile("\n"						\
-		     "1:	mov"itype" %"rtype"1,%2\n"		\
-		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:	mov %3,%0\n"				\
-		     "	jmp 2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
-		     : "=r"(err)					\
-		     : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
+#define __put_user_goto(x, addr, itype, rtype, ltype, label)	\
+	asm volatile goto("\n"						\
+		"1:	mov"itype" %"rtype"0,%1\n"			\
+		_ASM_EXTABLE(1b, %l2)					\
+		: : ltype(x), "m" (__m(addr))				\
+		: : label)
+
+#define __put_user_failed(x, addr, itype, rtype, ltype, errret)		\
+	({	__label__ __puflab;					\
+		int __pufret = errret;					\
+		__put_user_goto(x,addr,itype,rtype,ltype,__puflab);	\
+		__pufret = 0;						\
+	__puflab: __pufret; })
+
+#define __put_user_asm(x, addr, retval, itype, rtype, ltype, errret)	do {	\
+	retval = __put_user_failed(x, addr, itype, rtype, ltype, errret);	\
+} while (0)
 
 #define __put_user_asm_ex(x, addr, itype, rtype, ltype)			\
 	asm volatile("1:	mov"itype" %"rtype"0,%1\n"		\
@@ -814,12 +816,8 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
 #define user_access_begin()	__uaccess_begin()
 #define user_access_end()	__uaccess_end()
 
-#define unsafe_put_user(x, ptr, err_label)					\
-do {										\
-	int __pu_err;								\
-	__put_user_size((x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);		\
-	if (unlikely(__pu_err)) goto err_label;					\
-} while (0)
+#define unsafe_put_user(x, ptr, label)	\
+	__put_user_size((x), (ptr), sizeof(*(ptr)), label)
 
 #define unsafe_get_user(x, ptr, err_label)					\
 do {										\
diff --git a/fs/readdir.c b/fs/readdir.c
index 9d0212c374d6..03324f54c0e9 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -184,25 +184,27 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
 	if (dirent) {
 		if (signal_pending(current))
 			return -EINTR;
-		if (__put_user(offset, &dirent->d_off))
-			goto efault;
 	}
+
+	user_access_begin();
+	if (dirent)
+		unsafe_put_user(offset, &dirent->d_off, efault_end);
 	dirent = buf->current_dir;
-	if (__put_user(d_ino, &dirent->d_ino))
-		goto efault;
-	if (__put_user(reclen, &dirent->d_reclen))
-		goto efault;
+	unsafe_put_user(d_ino, &dirent->d_ino, efault_end);
+	unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
+	unsafe_put_user(0, dirent->d_name + namlen, efault_end);
+	unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
+	user_access_end();
+
 	if (copy_to_user(dirent->d_name, name, namlen))
 		goto efault;
-	if (__put_user(0, dirent->d_name + namlen))
-		goto efault;
-	if (__put_user(d_type, (char __user *) dirent + reclen - 1))
-		goto efault;
 	buf->previous = dirent;
 	dirent = (void __user *)dirent + reclen;
 	buf->current_dir = dirent;
 	buf->count -= reclen;
 	return 0;
+efault_end:
+	user_access_end();
 efault:
 	buf->error = -EFAULT;
 	return -EFAULT;

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

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

On Fri, Aug 19, 2016 at 3:11 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>>
>> (I have some experimental patches that actually use "asm goto" in
>> "unsafe_put_user()" to get that nice code generation, but they only
>> work if your gcc version supports "asm goto", which some older
>> versions of gcc does not)
>
> Since you actually are looking at the user access stuff, I'll just put
> them here.

Here's an updated patch that applies on current git and that actually
uses this for filldir() (but not signal handling).

It turns out that on Skylake, which supports SMAP, the clac/stac
instructions are quite slow, and doing them for each access makes
things insanely much slower than it could be. And "filldir" does the
user accesses one by one (except for the name copying), and is
actually somewhat common under some loads (ie the "find . -name XYZ"
kind of thing).

Anyway, the asm coming out of gcc looks nasty, because it has all the
ugly section stuiff and fixups for SMAP not existing on some CPU's
etc. So the resulting fs/readdir.s file is hard to read. But if you
look at the disassembly at the object file that hides all that (and
shows what the end result actually is), the actual filldir user
accesses end up looking beautiful, with no extra code anywhere. An
exception just goes to the EFAULT handling directly.

Sadly, unsafe_get_user() looking as good does require gcc improvements
that aren't imminent.

This patch is untested, although the earlier original pre-rebased
version of it actually got a fair amount of testing on my machine
(including the filldir use)

                    Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 6423 bytes --]

 arch/x86/include/asm/uaccess.h | 80 ++++++++++++++++++++----------------------
 fs/readdir.c                   | 22 ++++++------
 2 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a0ae610b9280..8d6b299522f1 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -204,19 +204,14 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
 
 #ifdef CONFIG_X86_32
-#define __put_user_asm_u64(x, addr, err, errret)			\
-	asm volatile("\n"						\
-		     "1:	movl %%eax,0(%2)\n"			\
-		     "2:	movl %%edx,4(%2)\n"			\
-		     "3:"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "4:	movl %3,%0\n"				\
-		     "	jmp 3b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 4b)				\
-		     _ASM_EXTABLE(2b, 4b)				\
-		     : "=r" (err)					\
-		     : "A" (x), "r" (addr), "i" (errret), "0" (err))
+#define __put_user_goto_u64(x, addr, label)			\
+	asm volatile("\n"					\
+		     "1:	movl %%eax,0(%2)\n"		\
+		     "2:	movl %%edx,4(%2)\n"		\
+		     _ASM_EXTABLE(1b, %2l)			\
+		     _ASM_EXTABLE(2b, %2l)			\
+		     : : "A" (x), "r" (addr)			\
+		     : : label)
 
 #define __put_user_asm_ex_u64(x, addr)					\
 	asm volatile("\n"						\
@@ -231,8 +226,8 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 	asm volatile("call __put_user_8" : "=a" (__ret_pu)	\
 		     : "A" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx")
 #else
-#define __put_user_asm_u64(x, ptr, retval, errret) \
-	__put_user_asm(x, ptr, retval, "q", "", "er", errret)
+#define __put_user_goto_u64(x, ptr, label) \
+	__put_user_goto(x, ptr, "q", "", "er", label)
 #define __put_user_asm_ex_u64(x, addr)	\
 	__put_user_asm_ex(x, addr, "q", "", "er")
 #define __put_user_x8(x, ptr, __ret_pu) __put_user_x(8, x, ptr, __ret_pu)
@@ -293,23 +288,21 @@ extern void __put_user_8(void);
 	__builtin_expect(__ret_pu, 0);				\
 })
 
-#define __put_user_size(x, ptr, size, retval, errret)			\
+#define __put_user_size(x, ptr, size, label)				\
 do {									\
-	retval = 0;							\
 	__chk_user_ptr(ptr);						\
 	switch (size) {							\
 	case 1:								\
-		__put_user_asm(x, ptr, retval, "b", "b", "iq", errret);	\
+		__put_user_goto(x, ptr, "b", "b", "iq", label);	\
 		break;							\
 	case 2:								\
-		__put_user_asm(x, ptr, retval, "w", "w", "ir", errret);	\
+		__put_user_goto(x, ptr, "w", "w", "ir", label);		\
 		break;							\
 	case 4:								\
-		__put_user_asm(x, ptr, retval, "l", "k", "ir", errret);	\
+		__put_user_goto(x, ptr, "l", "k", "ir", label);		\
 		break;							\
 	case 8:								\
-		__put_user_asm_u64((__typeof__(*ptr))(x), ptr, retval,	\
-				   errret);				\
+		__put_user_goto_u64((__typeof__(*ptr))(x), ptr, label);	\
 		break;							\
 	default:							\
 		__put_user_bad();					\
@@ -438,9 +431,12 @@ do {									\
 
 #define __put_user_nocheck(x, ptr, size)			\
 ({								\
-	int __pu_err;						\
+	__label__ __pu_label;					\
+	int __pu_err = -EFAULT;					\
 	__uaccess_begin();					\
-	__put_user_size((x), (ptr), (size), __pu_err, -EFAULT);	\
+	__put_user_size((x), (ptr), (size), __pu_label);	\
+	__pu_err = 0;						\
+__pu_label:							\
 	__uaccess_end();					\
 	__builtin_expect(__pu_err, 0);				\
 })
@@ -465,17 +461,23 @@ struct __large_struct { unsigned long buf[100]; };
  * we do not write to any memory gcc knows about, so there are no
  * aliasing issues.
  */
-#define __put_user_asm(x, addr, err, itype, rtype, ltype, errret)	\
-	asm volatile("\n"						\
-		     "1:	mov"itype" %"rtype"1,%2\n"		\
-		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:	mov %3,%0\n"				\
-		     "	jmp 2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
-		     : "=r"(err)					\
-		     : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
+#define __put_user_goto(x, addr, itype, rtype, ltype, label)	\
+	asm volatile goto("\n"						\
+		"1:	mov"itype" %"rtype"0,%1\n"			\
+		_ASM_EXTABLE(1b, %l2)					\
+		: : ltype(x), "m" (__m(addr))				\
+		: : label)
+
+#define __put_user_failed(x, addr, itype, rtype, ltype, errret)		\
+	({	__label__ __puflab;					\
+		int __pufret = errret;					\
+		__put_user_goto(x,addr,itype,rtype,ltype,__puflab);	\
+		__pufret = 0;						\
+	__puflab: __pufret; })
+
+#define __put_user_asm(x, addr, retval, itype, rtype, ltype, errret)	do {	\
+	retval = __put_user_failed(x, addr, itype, rtype, ltype, errret);	\
+} while (0)
 
 #define __put_user_asm_ex(x, addr, itype, rtype, ltype)			\
 	asm volatile("1:	mov"itype" %"rtype"0,%1\n"		\
@@ -814,12 +816,8 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
 #define user_access_begin()	__uaccess_begin()
 #define user_access_end()	__uaccess_end()
 
-#define unsafe_put_user(x, ptr, err_label)					\
-do {										\
-	int __pu_err;								\
-	__put_user_size((x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);		\
-	if (unlikely(__pu_err)) goto err_label;					\
-} while (0)
+#define unsafe_put_user(x, ptr, label)	\
+	__put_user_size((x), (ptr), sizeof(*(ptr)), label)
 
 #define unsafe_get_user(x, ptr, err_label)					\
 do {										\
diff --git a/fs/readdir.c b/fs/readdir.c
index 9d0212c374d6..03324f54c0e9 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -184,25 +184,27 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
 	if (dirent) {
 		if (signal_pending(current))
 			return -EINTR;
-		if (__put_user(offset, &dirent->d_off))
-			goto efault;
 	}
+
+	user_access_begin();
+	if (dirent)
+		unsafe_put_user(offset, &dirent->d_off, efault_end);
 	dirent = buf->current_dir;
-	if (__put_user(d_ino, &dirent->d_ino))
-		goto efault;
-	if (__put_user(reclen, &dirent->d_reclen))
-		goto efault;
+	unsafe_put_user(d_ino, &dirent->d_ino, efault_end);
+	unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
+	unsafe_put_user(0, dirent->d_name + namlen, efault_end);
+	unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
+	user_access_end();
+
 	if (copy_to_user(dirent->d_name, name, namlen))
 		goto efault;
-	if (__put_user(0, dirent->d_name + namlen))
-		goto efault;
-	if (__put_user(d_type, (char __user *) dirent + reclen - 1))
-		goto efault;
 	buf->previous = dirent;
 	dirent = (void __user *)dirent + reclen;
 	buf->current_dir = dirent;
 	buf->count -= reclen;
 	return 0;
+efault_end:
+	user_access_end();
 efault:
 	buf->error = -EFAULT;
 	return -EFAULT;

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Al Viro]

On Sat, Aug 20, 2016 at 04:32:57PM -0700, Linus Torvalds wrote:

> Anyway, the asm coming out of gcc looks nasty, because it has all the
> ugly section stuiff and fixups for SMAP not existing on some CPU's
> etc. So the resulting fs/readdir.s file is hard to read. But if you
> look at the disassembly at the object file that hides all that (and
> shows what the end result actually is), the actual filldir user
> accesses end up looking beautiful, with no extra code anywhere. An
> exception just goes to the EFAULT handling directly.
> 
> Sadly, unsafe_get_user() looking as good does require gcc improvements
> that aren't imminent.
> 
> This patch is untested, although the earlier original pre-rebased
> version of it actually got a fair amount of testing on my machine
> (including the filldir use)

Interesting...  BTW, how's this in the "really vile tricks" department?

	if (!uaccess_begin())
		goto fail;
	unsafe_...
	...
	uacess_end();

with uaccess_begin() along the lines of
	p = &current_thread_info()->foo;
	asm
		.text:
			STAC
			*p = 1f
			res = true;
			2:;
		.fixups:
			1:res = false;
			CLAC
			jmp 2;
	if (unlikely(res))
		asm
			clobber everything
	res;
and exception handlers in unsafe_... jumping to the address found
in current_thread_info()->foo.  AFAICS, it should avoid the problems
with asm goto, right?  The branch target is tied to the entry into the
damn series, so it's not as if it could disappear; and path to a branch
cc(1) doesn't see passes through the chunk produced by that asm block
in uaccess_begin(), so if it looks unreachable without taking those branches
into account, it _is_ unreachable.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

On Sat, Aug 20, 2016 at 5:11 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Interesting...  BTW, how's this in the "really vile tricks" department?
>
>         if (!uaccess_begin())
>                 goto fail;

So I slightly considered it, because gcc actually has support for that
kind of behavior thanks to setjmp/longjmp (and yes, the compiler
actually needs to know about the magic "this code can be entered a
second time from elsewhere" - it _used_ to be purely a library thing
back in the days of stupid compilers, but no more).

And I'm not saying it's wrong, but I'm not a huge fan of
setjmp/longjmp. Afaik it tends to make gcc generate potentially much
worse code in the function that uses setjmp.

That said, you have a really strong argument that I hadn't even thought about:

> AFAICS, it should avoid the problems with asm goto, right?

Yes. That was something I never even thought about. I just thought
"asm goto has some limitations, but they aren't _fundamental_, so
hopefully they get fixed". But they may not be fundamental, but it
will take a long time. If ever.

And you're right, using setjmp semantics would avoid all that and
"just work". Even for "get_user()" that needs to return a value.

Hmm.

You have to save the stack pointer at the setjmp point too. And there
might be other architecture-specific ABI rules for that. But you're
right, it might be worth it.

I *would* be a bit worried about code generation issues.
setjmp/longjmp is so seldom used that it's one of those things where
it might be best to verify with some gcc person that it doesn't cause
huge code-gen problems.

Adding Jakub just to check: Jakub, would a setjump/longjump kind of
interface for exception handling going to cause us problems
(performance or correctness) with gcc?

            Linus

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

On Sat, Aug 20, 2016 at 5:45 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I slightly considered it, because gcc actually has support for that
> kind of behavior thanks to setjmp/longjmp (and yes, the compiler
> actually needs to know about the magic "this code can be entered a
> second time from elsewhere" - it _used_ to be purely a library thing
> back in the days of stupid compilers, but no more).

Hmm. I may just be full of sh*t.

I was pretty sure that there used to be a "setjmp" attribute that gcc
used to make sure that "setjmp()" really could return twice, without
bad things happening on the stack.

But looking at the normal user space headers, I see nothing like that. It's just

    extern int setjmp (jmp_buf __env) __THROWNL;

where __THROWNL just sets the __nothrow__ attribute, which shouldn't
even matter in the kernel since we use -fno-exceptions.

So my "setjmp does potentially bad things to the optimization of the
function calling it" seems to have been just some drug-induced fever
dream of mine.

Sorry for the bogus noise. I don't know why I was so convinced setjmp
needed special gcc semantics.

             Linus

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

On Sat, Aug 20, 2016 at 5:45 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I slightly considered it, because gcc actually has support for that
> kind of behavior thanks to setjmp/longjmp (and yes, the compiler
> actually needs to know about the magic "this code can be entered a
> second time from elsewhere" - it _used_ to be purely a library thing
> back in the days of stupid compilers, but no more).

Hmm. I may just be full of sh*t.

I was pretty sure that there used to be a "setjmp" attribute that gcc
used to make sure that "setjmp()" really could return twice, without
bad things happening on the stack.

But looking at the normal user space headers, I see nothing like that. It's just

    extern int setjmp (jmp_buf __env) __THROWNL;

where __THROWNL just sets the __nothrow__ attribute, which shouldn't
even matter in the kernel since we use -fno-exceptions.

So my "setjmp does potentially bad things to the optimization of the
function calling it" seems to have been just some drug-induced fever
dream of mine.

Sorry for the bogus noise. I don't know why I was so convinced setjmp
needed special gcc semantics.

             Linus

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[H. Peter Anvin]

On August 20, 2016 6:00:17 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Sat, Aug 20, 2016 at 5:45 PM, Linus Torvalds
><torvalds@linux-foundation.org> wrote:
>>
>> So I slightly considered it, because gcc actually has support for
>that
>> kind of behavior thanks to setjmp/longjmp (and yes, the compiler
>> actually needs to know about the magic "this code can be entered a
>> second time from elsewhere" - it _used_ to be purely a library thing
>> back in the days of stupid compilers, but no more).
>
>Hmm. I may just be full of sh*t.
>
>I was pretty sure that there used to be a "setjmp" attribute that gcc
>used to make sure that "setjmp()" really could return twice, without
>bad things happening on the stack.
>
>But looking at the normal user space headers, I see nothing like that.
>It's just
>
>    extern int setjmp (jmp_buf __env) __THROWNL;
>
>where __THROWNL just sets the __nothrow__ attribute, which shouldn't
>even matter in the kernel since we use -fno-exceptions.
>
>So my "setjmp does potentially bad things to the optimization of the
>function calling it" seems to have been just some drug-induced fever
>dream of mine.
>
>Sorry for the bogus noise. I don't know why I was so convinced setjmp
>needed special gcc semantics.
>
>             Linus

I think the specific name setjmp() is magic in gcc.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[H. Peter Anvin]

On August 20, 2016 6:00:17 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Sat, Aug 20, 2016 at 5:45 PM, Linus Torvalds
><torvalds@linux-foundation.org> wrote:
>>
>> So I slightly considered it, because gcc actually has support for
>that
>> kind of behavior thanks to setjmp/longjmp (and yes, the compiler
>> actually needs to know about the magic "this code can be entered a
>> second time from elsewhere" - it _used_ to be purely a library thing
>> back in the days of stupid compilers, but no more).
>
>Hmm. I may just be full of sh*t.
>
>I was pretty sure that there used to be a "setjmp" attribute that gcc
>used to make sure that "setjmp()" really could return twice, without
>bad things happening on the stack.
>
>But looking at the normal user space headers, I see nothing like that.
>It's just
>
>    extern int setjmp (jmp_buf __env) __THROWNL;
>
>where __THROWNL just sets the __nothrow__ attribute, which shouldn't
>even matter in the kernel since we use -fno-exceptions.
>
>So my "setjmp does potentially bad things to the optimization of the
>function calling it" seems to have been just some drug-induced fever
>dream of mine.
>
>Sorry for the bogus noise. I don't know why I was so convinced setjmp
>needed special gcc semantics.
>
>             Linus

I think the specific name setjmp() is magic in gcc.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Al Viro]

On Sat, Aug 20, 2016 at 06:09:15PM -0700, H. Peter Anvin wrote:

> >Sorry for the bogus noise. I don't know why I was so convinced setjmp
> >needed special gcc semantics.
> >
> >             Linus
> 
> I think the specific name setjmp() is magic in gcc.

It is; attribute equivalent is returns_twice.  I wonder if "explicitly
clobber everything if we got false" + asm volatile to prevent reordering
would suffice for our purposes, but that's really a question for gcc
folks...

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Jakub Jelinek]

On Sat, Aug 20, 2016 at 05:45:00PM -0700, Linus Torvalds wrote:
> You have to save the stack pointer at the setjmp point too. And there
> might be other architecture-specific ABI rules for that. But you're
> right, it might be worth it.
> 
> I *would* be a bit worried about code generation issues.
> setjmp/longjmp is so seldom used that it's one of those things where
> it might be best to verify with some gcc person that it doesn't cause
> huge code-gen problems.
> 
> Adding Jakub just to check: Jakub, would a setjump/longjump kind of
> interface for exception handling going to cause us problems
> (performance or correctness) with gcc?

If you plan to use setjmp/longjmp a lot, then it is certainly a major
performance and compile time/memory problem.
Older versions don't model it properly, and newer gccs emit abnormal edges
from every longjmp or call that might longjmp to an artificial basic block
and from there to every setjmp.
Also note that gcc has/supports two setjmp kind of APIs, normal setjmp and
slightly more lightweight __builtin_setjmp which saves fewer registers, and
on some targets is/used to be used for EH instead of DWARF based ones.

	Jakub

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Al Viro]

On Sun, Aug 21, 2016 at 06:54:02AM +0200, Jakub Jelinek wrote:
> On Sat, Aug 20, 2016 at 05:45:00PM -0700, Linus Torvalds wrote:

> If you plan to use setjmp/longjmp a lot, then it is certainly a major
> performance and compile time/memory problem.
> Older versions don't model it properly, and newer gccs emit abnormal edges
> from every longjmp or call that might longjmp to an artificial basic block
> and from there to every setjmp.
> Also note that gcc has/supports two setjmp kind of APIs, normal setjmp and
> slightly more lightweight __builtin_setjmp which saves fewer registers, and
> on some targets is/used to be used for EH instead of DWARF based ones.

It's not exactly setjmp/longjmp; what I had in mind was along the lines of

static inline bool start(void)
{
	asm(
		save enough state into current_thread_info()->something, with
			1f for saved %rip
		stac
		res = true
	2:
	.section .text.fixup
	1:	res = false
		clac
		jmp 2b
	.previous
	)
	if (unlikely(!res))
		asm clobber everything
	return res;
}

and in unsafe_get_user() exception fixup (again, in .text.fixup section,
and invisible to gcc) jumping to common code that would pick saved state
from current_thread_info() and jump to saved location.

	The uses would be along the lines of
	if (!start())
		goto fail;
	unsafe_get_user(foo, &p1->foo);
	unsafe_get_user(bar, &p1->bar);
	...
	asm clac

IOW, a bunch of branches hidden from gcc, with destination (in the same
function) dominating the source of each (via visible branches as well).

Originally I hoped to get away with saving just the %rip; Linus has pointed
out that stack pointer is also needed.  It's obviously much less generic
than setjmp/longjmp is.  Single per-thread jmp_buf rudiment, all "longjmp"
calls in the same function as "setjmp" one, pretty much not giving a damn
about any local variables we might've changed if the "longjmp" is taken,
etc.

The point of the exercise is to have the normal execution path containing
no error checks - just the data copying, with all exception handling happening
out-of-line...

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Al Viro]

On Sun, Aug 21, 2016 at 06:54:02AM +0200, Jakub Jelinek wrote:
> On Sat, Aug 20, 2016 at 05:45:00PM -0700, Linus Torvalds wrote:

> If you plan to use setjmp/longjmp a lot, then it is certainly a major
> performance and compile time/memory problem.
> Older versions don't model it properly, and newer gccs emit abnormal edges
> from every longjmp or call that might longjmp to an artificial basic block
> and from there to every setjmp.
> Also note that gcc has/supports two setjmp kind of APIs, normal setjmp and
> slightly more lightweight __builtin_setjmp which saves fewer registers, and
> on some targets is/used to be used for EH instead of DWARF based ones.

It's not exactly setjmp/longjmp; what I had in mind was along the lines of

static inline bool start(void)
{
	asm(
		save enough state into current_thread_info()->something, with
			1f for saved %rip
		stac
		res = true
	2:
	.section .text.fixup
	1:	res = false
		clac
		jmp 2b
	.previous
	)
	if (unlikely(!res))
		asm clobber everything
	return res;
}

and in unsafe_get_user() exception fixup (again, in .text.fixup section,
and invisible to gcc) jumping to common code that would pick saved state
from current_thread_info() and jump to saved location.

	The uses would be along the lines of
	if (!start())
		goto fail;
	unsafe_get_user(foo, &p1->foo);
	unsafe_get_user(bar, &p1->bar);
	...
	asm clac

IOW, a bunch of branches hidden from gcc, with destination (in the same
function) dominating the source of each (via visible branches as well).

Originally I hoped to get away with saving just the %rip; Linus has pointed
out that stack pointer is also needed.  It's obviously much less generic
than setjmp/longjmp is.  Single per-thread jmp_buf rudiment, all "longjmp"
calls in the same function as "setjmp" one, pretty much not giving a damn
about any local variables we might've changed if the "longjmp" is taken,
etc.

The point of the exercise is to have the normal execution path containing
no error checks - just the data copying, with all exception handling happening
out-of-line...

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

On Sat, Aug 20, 2016 at 11:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> It's not exactly setjmp/longjmp; what I had in mind was along the lines of

That ends up having all the exact same issues as setjmp, and generally
you *do* want the compiler to know about it.

For example, let's say that you have something like

    if (start())
        return -EFAULT;

    ... do things that can fault and trigger an exception ..

    stop();
    return 0;

then it doesn't matter that "start" clobbers all memory and registers,
if it returns twice the code generation by a compiler that doesn't
know about the magical setjmp-like behavior can trigger bugs.

For example, the most common case is that lots of compilers try to
share the final return-point - sometimes because of instrumentation,
sometimes just because there's a big stack frame and the return is a
lot of pop instructions and stack undo code.

And *particularly* if your magical 'start()' function has an inline
asm that clobbers memory and registers, the compiler will have to
spill state to stack around it - but part of the spill might be the
return value that it had in a register.

So the compiler might end up generating code like this:

         movl $-EFAULT,8(%rbp)    # retval
         call start
         testl %eax,%eax
         jne return_point;

         ...

         movl $0,8(%rbp)    # retval

         ....

    return_point:
        .. pop-pop-pop-whatever ..
        movl 4(%rbp),%eax
        .. more stack frame cleanup ..
        ret

and notice how if "start()" returns a second time - even if it
restored all registers including the stack pointer - the function
might return the wrong error value if the exception that caused
longjmp happened after the code that had updated the return.

There are lots of other ways a setjmp() point is special. Some
compilers might push/pop values just temporarily around a call, so you
might have sequences like

     pushq %rdx
     call fn
     popq %rdx

where the compiler wanted to save register %rdx around the call (I've
never actually seen gcc generate that code, the exact same thing may
happen with just random register spills).

Again, that fails completely in the presence of a function that
returns twice - even it the stack pointer itself gets reset, the stack
*contents* that the code pops the saved value of %rdx might have been
re-used for something else (and for a register spill, the frame slot
might have been re-used). So now you're restoring garbage.

So the interface you propose is in fact *exactly* the same as setjmp,
and we'd need to make sure that the compiler knows that.

                       Linus

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

On Sat, Aug 20, 2016 at 11:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> It's not exactly setjmp/longjmp; what I had in mind was along the lines of

That ends up having all the exact same issues as setjmp, and generally
you *do* want the compiler to know about it.

For example, let's say that you have something like

    if (start())
        return -EFAULT;

    ... do things that can fault and trigger an exception ..

    stop();
    return 0;

then it doesn't matter that "start" clobbers all memory and registers,
if it returns twice the code generation by a compiler that doesn't
know about the magical setjmp-like behavior can trigger bugs.

For example, the most common case is that lots of compilers try to
share the final return-point - sometimes because of instrumentation,
sometimes just because there's a big stack frame and the return is a
lot of pop instructions and stack undo code.

And *particularly* if your magical 'start()' function has an inline
asm that clobbers memory and registers, the compiler will have to
spill state to stack around it - but part of the spill might be the
return value that it had in a register.

So the compiler might end up generating code like this:

         movl $-EFAULT,8(%rbp)    # retval
         call start
         testl %eax,%eax
         jne return_point;

         ...

         movl $0,8(%rbp)    # retval

         ....

    return_point:
        .. pop-pop-pop-whatever ..
        movl 4(%rbp),%eax
        .. more stack frame cleanup ..
        ret

and notice how if "start()" returns a second time - even if it
restored all registers including the stack pointer - the function
might return the wrong error value if the exception that caused
longjmp happened after the code that had updated the return.

There are lots of other ways a setjmp() point is special. Some
compilers might push/pop values just temporarily around a call, so you
might have sequences like

     pushq %rdx
     call fn
     popq %rdx

where the compiler wanted to save register %rdx around the call (I've
never actually seen gcc generate that code, the exact same thing may
happen with just random register spills).

Again, that fails completely in the presence of a function that
returns twice - even it the stack pointer itself gets reset, the stack
*contents* that the code pops the saved value of %rdx might have been
re-used for something else (and for a register spill, the frame slot
might have been re-used). So now you're restoring garbage.

So the interface you propose is in fact *exactly* the same as setjmp,
and we'd need to make sure that the compiler knows that.

                       Linus

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

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

On Sun, Aug 21, 2016 at 10:52 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Aug 20, 2016 at 11:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> It's not exactly setjmp/longjmp; what I had in mind was along the lines of
>
> That ends up having all the exact same issues as setjmp, and generally
> you *do* want the compiler to know about it.

So just in case you wanted to play around with it, here's a kernel
implementation of 'setjmp/longjmp' for x86.

It's very lightly tested (and I'll admit to editing it for some
cleanups after that light testing), but it does look largely sane.

The whole interface choice may be debatable: maybe it would be better
to allocate the register buffer on the stack, and just hide a pointer
to it in the task struct. Things like that could be changed fairly
easily. But if you want to play around with this, this patch should
get you started.

Of course, you'd want to wrap things up somehow, and I would *not*
want to see naked setjmp() calls in the kernel.

And we'd need this for all other architectures too, but it's usually
not hard to do. It needs to save all the callee-saved registers and
the stack pointer and return address. That should generally be it.

The 32-bit version has not been tested at all, but it compiled at some
point, and the code looks mostly sane. The 64-bit code I actually had
a stupid non-user-access test-case for.

                Linus

[-- Attachment #2: setjmp.patch --]
[-- Type: text/x-patch, Size: 5950 bytes --]

commit a8062ecb780bed81eaec10bd9fea60bf595a9c40
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Aug 22 13:15:51 2016 -0700

    x86: add basic setjmp/longjmp implementation
    
    To make the compiler happy, we have to actually call it setjmp/longjmp
    too.  Even if the exact semantics aren't the same - we keep the register
    buffer in the thread structure, for example, rather than pass it in as
    an argument.
    
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 arch/x86/include/asm/processor.h |  2 ++
 arch/x86/include/asm/setjmp.h    | 31 ++++++++++++++++++++++++++++++
 arch/x86/kernel/asm-offsets.c    |  1 +
 arch/x86/lib/Makefile            |  1 +
 arch/x86/lib/setjmp_32.S         | 37 ++++++++++++++++++++++++++++++++++++
 arch/x86/lib/setjmp_64.S         | 41 ++++++++++++++++++++++++++++++++++++++++
 include/linux/setjmp.h           | 19 +++++++++++++++++++
 7 files changed, 132 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 63def9537a2d..1af2c7025d51 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -22,6 +22,7 @@ struct vm86;
 #include <asm/nops.h>
 #include <asm/special_insns.h>
 #include <asm/fpu/types.h>
+#include <asm/setjmp.h>
 
 #include <linux/personality.h>
 #include <linux/cache.h>
@@ -425,6 +426,7 @@ struct thread_struct {
 	unsigned		io_bitmap_max;
 
 	mm_segment_t		addr_limit;
+	struct setjmp		setjmp;
 
 	unsigned int		sig_on_uaccess_err:1;
 	unsigned int		uaccess_err:1;	/* uaccess failed */
diff --git a/arch/x86/include/asm/setjmp.h b/arch/x86/include/asm/setjmp.h
new file mode 100644
index 000000000000..6cda8608ce04
--- /dev/null
+++ b/arch/x86/include/asm/setjmp.h
@@ -0,0 +1,31 @@
+#ifndef _ASM_SETJMP_H
+#define _ASM_SETJMP_H
+
+/*
+ * setjmp needs to save the callee-saved registers and
+ * the stack setup, so that it looks like a normal call.
+ *
+ * In addition, gcc needs to know that it's setjmp, but
+ * that seems to literally just trigger on the name.
+ *
+ * Unlike the legacy C implementation, we just have the
+ * save area in the task structure.
+ */
+
+#ifdef CONFIG_X86_64
+
+struct setjmp {
+	unsigned long rbx, r12, r13, r14, r15;
+	unsigned long rbp, rsp, rip;
+};
+
+#else
+
+struct setjmp {
+	unsigned long ebx, esi, edi;
+	unsigned long ebp, esp, eip;
+};
+
+#endif
+
+#endif // _ASM_SETJMP_H
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 2bd5c6ff7ee7..78a10eb048e8 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -34,6 +34,7 @@ void common(void) {
 
 	BLANK();
 	OFFSET(TASK_addr_limit, task_struct, thread.addr_limit);
+	OFFSET(TASK_setjmp, task_struct, thread.setjmp);
 
 	BLANK();
 	OFFSET(crypto_tfm_ctx_offset, crypto_tfm, __crt_ctx);
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 34a74131a12c..bb7a34648c2c 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SMP) += msr-smp.o cache-smp.o
 lib-y := delay.o misc.o cmdline.o cpu.o
 lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o
 lib-y += memcpy_$(BITS).o
+lib-y += setjmp_$(BITS).o
 lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
 lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o
 lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
diff --git a/arch/x86/lib/setjmp_32.S b/arch/x86/lib/setjmp_32.S
new file mode 100644
index 000000000000..44f3d52ee40c
--- /dev/null
+++ b/arch/x86/lib/setjmp_32.S
@@ -0,0 +1,37 @@
+#include <linux/linkage.h>
+#include <asm/asm-offsets.h>
+#include <asm/asm.h>
+#include <asm/percpu.h>
+
+// The explicit add of TASK_setjmp keeps the
+// following offset 8-bit values, and shrinks
+// the modrm bytes in the instructions that
+// follow.
+
+ENTRY(setjmp)
+	movl PER_CPU_VAR(current_task),%eax
+	addl $TASK_setjmp,%eax
+
+	movl %ebx,(%eax)
+	movl %esi,4(%eax)
+	movl %edi,8(%eax)
+	movl %ebp,12(%eax)
+	lea 4(%esp),%edx
+	movl %edx,16(%eax)
+	movl (%esp),%edx
+	movl %edx,20(%eax)
+	xorl %eax,%eax
+	ret
+
+ENTRY(longjmp)
+	movl PER_CPU_VAR(current_task),%eax
+	addl $TASK_setjmp,%eax
+
+	movl (%eax),%ebx
+	movl 4(%eax),%esi
+	movl 8(%eax),%edi
+	movl 12(%eax),%ebp
+	movl 16(%eax),%esp
+	movl 20(%eax),%edx
+	movl $1,%eax
+	jmp *%edx
diff --git a/arch/x86/lib/setjmp_64.S b/arch/x86/lib/setjmp_64.S
new file mode 100644
index 000000000000..c113e132e4d8
--- /dev/null
+++ b/arch/x86/lib/setjmp_64.S
@@ -0,0 +1,41 @@
+#include <linux/linkage.h>
+#include <asm/asm-offsets.h>
+#include <asm/asm.h>
+#include <asm/percpu.h>
+
+// The explicit add of TASK_setjmp keeps the
+// following offset 8-bit values, and shrinks
+// the modrm bytes in the instructions that
+// follow.
+
+ENTRY(setjmp)
+	movq PER_CPU_VAR(current_task),%rax
+	addq $TASK_setjmp,%rax
+
+	movq %rbx,(%rax)
+	movq %r12,8(%rax)
+	movq %r13,16(%rax)
+	movq %r14,24(%rax)
+	movq %r15,32(%rax)
+	movq %rbp,40(%rax)
+	lea 8(%rsp),%rdx
+	movq %rdx,48(%rax)
+	movq (%rsp),%rdx
+	movq %rdx,56(%rax)
+	xorl %eax,%eax
+	ret
+
+ENTRY(longjmp)
+	movq PER_CPU_VAR(current_task),%rax
+	addq $TASK_setjmp,%rax
+
+	movq (%rax),%rbx
+	movq 8(%rax),%r12
+	movq 16(%rax),%r13
+	movq 24(%rax),%r14
+	movq 32(%rax),%r15
+	movq 40(%rax),%rbp
+	movq 48(%rax),%rsp
+	movq 56(%rax),%rdx
+	movl $1,%eax
+	jmp *%rdx
diff --git a/include/linux/setjmp.h b/include/linux/setjmp.h
new file mode 100644
index 000000000000..42d4674791e8
--- /dev/null
+++ b/include/linux/setjmp.h
@@ -0,0 +1,19 @@
+#ifndef _LINUX_SETJMP_H
+#define _LINUX_SETJMP_H
+
+#include <linux/compiler.h>
+
+//
+// NOTE! We call it 'setjmp' to make gcc treat it specially,
+// but the calling conventions are different from the regular
+// user-space setjmp.
+//
+// So setjmp() always returns 0/1, and the  register buffer
+// is always in the task struct rather than being passed in
+// as an argument.
+//
+
+extern int setjmp(void);
+extern void __noreturn longjmp(void);
+
+#endif // _LINUX_SETJMP_H

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[H. Peter Anvin]

On August 22, 2016 3:23:06 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Sun, Aug 21, 2016 at 10:52 AM, Linus Torvalds
><torvalds@linux-foundation.org> wrote:
>> On Sat, Aug 20, 2016 at 11:42 PM, Al Viro <viro@zeniv.linux.org.uk>
>wrote:
>>>
>>> It's not exactly setjmp/longjmp; what I had in mind was along the
>lines of
>>
>> That ends up having all the exact same issues as setjmp, and
>generally
>> you *do* want the compiler to know about it.
>
>So just in case you wanted to play around with it, here's a kernel
>implementation of 'setjmp/longjmp' for x86.
>
>It's very lightly tested (and I'll admit to editing it for some
>cleanups after that light testing), but it does look largely sane.
>
>The whole interface choice may be debatable: maybe it would be better
>to allocate the register buffer on the stack, and just hide a pointer
>to it in the task struct. Things like that could be changed fairly
>easily. But if you want to play around with this, this patch should
>get you started.
>
>Of course, you'd want to wrap things up somehow, and I would *not*
>want to see naked setjmp() calls in the kernel.
>
>And we'd need this for all other architectures too, but it's usually
>not hard to do. It needs to save all the callee-saved registers and
>the stack pointer and return address. That should generally be it.
>
>The 32-bit version has not been tested at all, but it compiled at some
>point, and the code looks mostly sane. The 64-bit code I actually had
>a stupid non-user-access test-case for.
>
>                Linus

How about the gcc native __builtin_setjmp stuff which is supposedly better?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Linus Torvalds]

On Mon, Aug 22, 2016 at 4:12 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> How about the gcc native __builtin_setjmp stuff which is supposedly better?

How new is it? The whole point was that we'd not have to worry and
wait for gcc features..

glibc doesn't use it, which worries me a bit. Has it ever gotten any
use/testing?

But yes, the compiler could do better. If we can rely on it, and it
doesn't do stupid things (like have signal state etc crap - glibc
seems to just alias "setjmp" to "sigsetjmp" with a "didn't save
signals" flag)

             Linus

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[H. Peter Anvin]

On August 22, 2016 4:48:00 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Mon, Aug 22, 2016 at 4:12 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> How about the gcc native __builtin_setjmp stuff which is supposedly
>better?
>
>How new is it? The whole point was that we'd not have to worry and
>wait for gcc features..
>
>glibc doesn't use it, which worries me a bit. Has it ever gotten any
>use/testing?
>
>But yes, the compiler could do better. If we can rely on it, and it
>doesn't do stupid things (like have signal state etc crap - glibc
>seems to just alias "setjmp" to "sigsetjmp" with a "didn't save
>signals" flag)
>
>             Linus

We can always fall back on the classic implementation for older gcc, just like we do for so many other features.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 22 Aug 2016 16:48:00 -0700

> How new is it? The whole point was that we'd not have to worry and
> wait for gcc features..

It's been around for a long time.

I'd say at least gcc-3.0 and later support it.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 22 Aug 2016 16:48:00 -0700

> How new is it? The whole point was that we'd not have to worry and
> wait for gcc features..

It's been around for a long time.

I'd say at least gcc-3.0 and later support it.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[H. Peter Anvin]

On August 22, 2016 4:57:10 PM PDT, David Miller <davem@davemloft.net> wrote:
>From: Linus Torvalds <torvalds@linux-foundation.org>
>Date: Mon, 22 Aug 2016 16:48:00 -0700
>
>> How new is it? The whole point was that we'd not have to worry and
>> wait for gcc features..
>
>It's been around for a long time.
>
>I'd say at least gcc-3.0 and later support it.

I think on some architectures it simply calls the library function, but that's okay.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[Al Viro]

On Mon, Aug 22, 2016 at 04:48:00PM -0700, Linus Torvalds wrote:
> On Mon, Aug 22, 2016 at 4:12 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> >
> > How about the gcc native __builtin_setjmp stuff which is supposedly better?
> 
> How new is it? The whole point was that we'd not have to worry and
> wait for gcc features..

Sat Jan 27 07:59:25 1996  Richard Kenner  (kenner@vlsi1.ultra.nyu.edu)

        * tree.h (enum built_in_function): Add BUILT_IN_{SET,LONG}JMP.
        * expr.c: Include hard-reg-set.h.
        (arg_pointer_save_area): New declaration.
        (expand_builtin, case BUILT_IN_{SET,LONG}JMP): New cases.
        * Makefile.in (expr.o): Includes hard-reg-set.h.
        * c-decl.c (init_decl_processing): Add definitions for
        __builtin_setjmp and __builtin_longjmp.
        * cccp.c (initialize_builtins): Add def of __HAVE_BUILTIN_SETJMP__.

        * expr.c (expand_expr, case COMPONENT_REF): Pass EXPAND_INITIALIZER
        to recursive call.

IOW - old; the last branch lacking it is 2.7.2.*  Both 2.8 and egcs (and thus
2.95 and everything subsequent) have it.

Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

[H. Peter Anvin]

On August 22, 2016 3:23:06 PM PDT, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Sun, Aug 21, 2016 at 10:52 AM, Linus Torvalds
><torvalds@linux-foundation.org> wrote:
>> On Sat, Aug 20, 2016 at 11:42 PM, Al Viro <viro@zeniv.linux.org.uk>
>wrote:
>>>
>>> It's not exactly setjmp/longjmp; what I had in mind was along the
>lines of
>>
>> That ends up having all the exact same issues as setjmp, and
>generally
>> you *do* want the compiler to know about it.
>
>So just in case you wanted to play around with it, here's a kernel
>implementation of 'setjmp/longjmp' for x86.
>
>It's very lightly tested (and I'll admit to editing it for some
>cleanups after that light testing), but it does look largely sane.
>
>The whole interface choice may be debatable: maybe it would be better
>to allocate the register buffer on the stack, and just hide a pointer
>to it in the task struct. Things like that could be changed fairly
>easily. But if you want to play around with this, this patch should
>get you started.
>
>Of course, you'd want to wrap things up somehow, and I would *not*
>want to see naked setjmp() calls in the kernel.
>
>And we'd need this for all other architectures too, but it's usually
>not hard to do. It needs to save all the callee-saved registers and
>the stack pointer and return address. That should generally be it.
>
>The 32-bit version has not been tested at all, but it compiled at some
>point, and the code looks mostly sane. The 64-bit code I actually had
>a stupid non-user-access test-case for.
>
>                Linus

The nice thing about using __builtin_ is that I believe gcc is aware of which registers need saving, and also know that the common path doesn't clobber registers at all.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.