Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466619 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-08-19 21:20 +0200
    Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-19 23:30 +0200
      Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-20 00:10 +0200
        Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-20 00:20 +0200
          Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-21 01:40 +0200
            Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-21 02:20 +0200
              Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-21 02:50 +0200
                Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-21 03:10 +0200
                  Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault "H. Peter Anvin" <hpa@zytor.com> - 2016-08-21 03:10 +0200
                    Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-21 03:50 +0200
                Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Jakub Jelinek <jakub@redhat.com> - 2016-08-21 07:00 +0200
                  Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-21 08:50 +0200
                    Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-21 20:00 +0200
                      Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-23 00:30 +0200
                        Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault "H. Peter Anvin" <hpa@zytor.com> - 2016-08-23 01:20 +0200
                          Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-23 01:50 +0200
                            Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of  fault David Miller <davem@davemloft.net> - 2016-08-23 02:00 +0200
                              Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault "H. Peter Anvin" <hpa@zytor.com> - 2016-08-23 02:10 +0200
                            Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault "H. Peter Anvin" <hpa@zytor.com> - 2016-08-23 02:00 +0200
                            Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault Al Viro <viro@ZenIV.linux.org.uk> - 2016-08-23 02:20 +0200
                        Re: [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault "H. Peter Anvin" <hpa@zytor.com> - 2016-08-23 01:20 +0200

Page 1 of 2  [1] 2  Next page →

#1466619 — [PATCH] ARC: uaccess: get_user to zero out dest in cause of fault

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.

Verified using following

| {
|  	u32 bogus1 = 0xdeadbeef;
|	u64 bogus2 = 0xdead;
|	int rc1, rc2;
|
|  	pr_info("Orig values %x %llx\n", bogus1, bogus2);
|	rc1 = get_user(bogus1, (u32 __user *)0x40000000);
|	rc2 = get_user(bogus2, (u64 __user *)0x50000000);
|	pr_info("access %d %d, new values %x %llx\n",
|		rc1, rc2, bogus1, bogus2);
| }

| [ARCLinux]# insmod /mnt/kernel-module/qtn.ko
| Orig values deadbeef dead
| access -14 -14, new values 0 0

Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-snps-arc@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/uaccess.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arc/include/asm/uaccess.h b/arch/arc/include/asm/uaccess.h
index a78d5670884f..41faf17cd28d 100644
--- a/arch/arc/include/asm/uaccess.h
+++ b/arch/arc/include/asm/uaccess.h
@@ -83,7 +83,10 @@
 	"2:	;nop\n"				\
 	"	.section .fixup, \"ax\"\n"	\
 	"	.align 4\n"			\
-	"3:	mov %0, %3\n"			\
+	"3:	# return -EFAULT\n"		\
+	"	mov %0, %3\n"			\
+	"	# zero out dst ptr\n"		\
+	"	mov %1,  0\n"			\
 	"	j   2b\n"			\
 	"	.previous\n"			\
 	"	.section __ex_table, \"a\"\n"	\
@@ -101,7 +104,11 @@
 	"2:	;nop\n"				\
 	"	.section .fixup, \"ax\"\n"	\
 	"	.align 4\n"			\
-	"3:	mov %0, %3\n"			\
+	"3:	# return -EFAULT\n"		\
+	"	mov %0, %3\n"			\
+	"	# zero out dst ptr\n"		\
+	"	mov %1,  0\n"			\
+	"	mov %R1, 0\n"			\
 	"	j   2b\n"			\
 	"	.previous\n"			\
 	"	.section __ex_table, \"a\"\n"	\
-- 
2.7.4

[toc] | [next] | [standalone]

#1466693

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?

[toc] | [prev] | [next] | [standalone]

#1466701

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

[toc] | [prev] | [next] | [standalone]

#1466702

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1466883

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1466884

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.

[toc] | [prev] | [next] | [standalone]

#1466887

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

[toc] | [prev] | [next] | [standalone]

#1466889

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

[toc] | [prev] | [next] | [standalone]

#1466890

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.

[toc] | [prev] | [next] | [standalone]

#1466892

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...

[toc] | [prev] | [next] | [standalone]

#1466915

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

[toc] | [prev] | [next] | [standalone]

#1466922

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...

[toc] | [prev] | [next] | [standalone]

#1467191

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

[toc] | [prev] | [next] | [standalone]

#1468108

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1468141

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.

[toc] | [prev] | [next] | [standalone]

#1468191

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1468196

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.

[toc] | [prev] | [next] | [standalone]

#1468194

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.

[toc] | [prev] | [next] | [standalone]

#1468200

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.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web