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

Groups > linux.kernel > #1611600 > unrolled thread

[RFC][CFT][PATCHSET v1] uaccess unification

Back to article view | Back to linux.kernel

Contents

  [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-29 08:00 +0200
    Re: [RFC][CFT][PATCHSET v1] uaccess unification Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2017-03-29 22:20 +0200
      Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-29 22:40 +0200
        Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-29 23:10 +0200
          Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-29 23:30 +0200
            Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 01:20 +0200
              Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-30 01:50 +0200
                Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 17:40 +0200
      Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-29 22:40 +0200
        Re: [RFC][CFT][PATCHSET v1] uaccess unification Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2017-03-29 23:20 +0200
          Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 01:50 +0200
            Re: [RFC][CFT][PATCHSET v1] uaccess unification Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2017-03-30 02:10 +0200
              Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-30 02:40 +0200
                Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 03:20 +0200
                Re: [RFC][CFT][PATCHSET v1] uaccess unification Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2017-03-30 22:50 +0200
                  Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-30 23:10 +0200
                    Re: [RFC][CFT][PATCHSET v1] uaccess unification Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-31 01:30 +0200
    Re: [RFC][CFT][PATCHSET v1] uaccess unification Martin Schwidefsky <schwidefsky@de.ibm.com> - 2017-03-30 14:40 +0200
      Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 16:50 +0200
    Re: [RFC][CFT][PATCHSET v1] uaccess unification Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-30 18:30 +0200
      Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 18:50 +0200
        Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-30 19:20 +0200
          Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 20:50 +0200
            Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-30 21:00 +0200
            Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 21:00 +0200
              Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-30 21:00 +0200
                Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 21:20 +0200
                  Re: [RFC][CFT][PATCHSET v1] uaccess unification Linus Torvalds <torvalds@linux-foundation.org> - 2017-03-30 21:30 +0200
                    Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-03-30 23:20 +0200
    Re: [RFC][CFT][PATCHSET v1] uaccess unification Kees Cook <keescook@chromium.org> - 2017-03-31 02:30 +0200
      Re: [RFC][CFT][PATCHSET v1] uaccess unification James Hogan <james.hogan@imgtec.com> - 2017-03-31 15:40 +0200
    Re: [RFC][CFT][PATCHSET v1] uaccess unification James Morse <james.morse@arm.com> - 2017-04-03 18:30 +0200
    Re: [RFC][CFT][PATCHSET v1] uaccess unification Max Filippov <jcmvbkbc@gmail.com> - 2017-04-04 22:30 +0200
      Re: [RFC][CFT][PATCHSET v1] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-04 23:00 +0200
    ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-05 07:10 +0200
      Re: ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-05 10:10 +0200
        Re: ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) Tony Luck <tony.luck@gmail.com> - 2017-04-05 20:50 +0200
          Re: ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-05 22:40 +0200
    [RFC][CFT][PATCHSET v2] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-07 02:30 +0200
      Re: [RFC][CFT][PATCHSET v2] uaccess unification Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-07 02:40 +0200

Page 1 of 2  [1] 2  Next page →

#1611600 — [RFC][CFT][PATCHSET v1] uaccess unification

	We have several primitives for bulk kernel<->userland copying.
That stuff lives in various asm/uaccess.h, with serious code duplication
_and_ seriously inconsistent semantics.

	That code has grown a lot of cruft and more than a few bugs.
Some got caught and fixed last year, but some fairly unpleasant ones
still remain.  A large part of problem was that a lot of code used to
include <asm/uaccess.h> directly, so we had no single place to work
with.  That got finally fixed in 4.10-rc1, when everything had been
forcibly switched to including <linux/uaccess.h>.  At that point it
became possible to start getting rid of boilerplate; I hoped to deal
with that by 4.11-rc1, but the things didn't work out and that work
has slipped to this cycle.

	The patchset currently in vfs.git#work.uaccess is the result;
there's more work to do, but it takes care of a large part of the
problems.  About 2.8KLoc removed, a lot of cruft is gone and semantics
is hopefully in sync now.  All but two architectures (ia64 and metag)
had been switched to new mechanism; for these two I'm afraid that I'll
need serious help from maintainers.

	Currently we have 8 primitives - 6 on every architecture and 2 more
on biarch ones.  All of them have the same calling conventions: arguments
are the same as for memcpy() (void *to, const void *from, unsigned long size)
and the same rules for return value.
	If all loads and stores succeed, everything is obvious - the
'size' bytes starting at 'to' become equal to 'size' bytes starting at 'from'
and zero is returned.  If some loads or stores fail, non-zero value should
be returned.  If any of those primitives returns a positive value N,
	* N should be no greater than size
	* the values fetched out of from[0..size-N-1] should be stored into the
corresponding bytes of to[0..size-N-1]
	* N should not be equal to size unless not a single byte could have
been fetched or stored.  As long as that restriction is satisfied, these
primitives are not required to squeeze every possible byte in case some
loads or stores fail.

	1) copy_from_user() - 'to' points to kernel memory, 'from' is
normally a userland pointer.  This is used for copying structures from
userland in all kinds of ioctls, etc.  No faults on access to destination are
allowed, faults on access to source lead to zero-padding the rest of
destination.  Note that for architectures with the same address space split
between the kernel and userland (i.e. the ones that have non-trivial
access_ok()) passing a kernel address instead of a userland one should be
treated as 'every access would fail'.  In such cases the entire destination
should be zeroed (failure to do so was a fairly common bug).
	Note that all these functions, including copy_from_user(), are
affected by set_fs() - when called under set_fs(KERNEL_DS), they expect
kernel pointers where normally a userland one would be given.

	2) copy_to_user() - 'from' points to kernel memory, 'to' is
a userland pointer (subject to set_fs() effects, as usual).  Again.
this is used by all kinds of code in all kinds of drivers, syscalls, etc.
No faults on access to source, fault on access to destination terminates
copying.  No zero-padding, of course - the faults are going to be on store
here.  Does not assume that access_ok() had been checked by caller;
given 'to'/'size' that fails access_ok() returns "nothing copied".

	3) copy_in_user() - both 'from' and 'to' are in userland.  Used
only by compat code that needs to repack 32bit data structures into native
64bit counterparts.  As the result, provided only by biarch architectures.
Subject to set_fs(), but really should not be (and AFAICS isn't) used that way.
Some architectures tried to zero-pad, but did it inconsistently and it's
pointless anyway - destination is in userland memory, so no infoleaks would
happen.

	4) __copy_from_user_inatomic() - similar to copy_from_user(),
except that
	* the caller is presumed to have verified that the source range passes
access_ok() [note that this is does not guarantee the lack of faults]
	* most importantly, zero-padding SHOULD NOT happen on short copy.
If implementation fails to guarantee that, it's a bug and potentially
bad one[1].
	* it may be called with pagefaults disabled (of course, in
that case any pagefault results in a short copy).  That's what 'inatomic'
in the name refers to.  Note that actually disabling pagefaults is
up to the caller; blindly calling it e.g. from under a spinlock will just
get you a deadlock.  Even more precautions are needed to call it from
something like an interrupt handler - you must do that under set_fs(),
etc.   It's not "this variant is safe to call from atomic contexts", it's
"I know what I'm doing, don't worry if you see it in an atomic context".

	5) __copy_to_user_inatomic().  A counterpart of
__copy_from_user_inatomic(), except for the direction of copying.

	6) __copy_from_user().  Essentially the only difference from
__copy_from_user_inatomic() is that one isn't supposed to call it from
atomic contexts.  It may be marginally faster than copy_from_user() (due
to skipped access_ok()), but these days the main costs are not in doing
fairly light arithmetics.  In theory, you might do a single access_ok()
covering a large structure and then proceed to call __copy_from_user()
on various parts of that.  In practice doing many calls of that thing on
small chunks of data is going to cost a lot on current x86 boxen due to
STAC/CLAC pair inside each call.  Has fewer call sites than copy_from_user()
- copy_from_user() is in thousands, while this one has only 40 callers
outside of arch/, some fairly dubious.  In arch there's about 170 callers
total, mostly in sigreturn instances.

	7) __copy_to_user().  A counterpart of __copy_from_user(), with
pretty much the same considerations applied.

	8) __copy_in_user().  Basically, copy_in_user() sans access_ok().
Biarch-only, with the grand total of 6 callers...


	What this series does is:

* convert architectures to fewer primitives (raw_copy_{to,from,in}_user(),
the last one only on biarch ones), switching to generic implementations
of the 8 primitives aboves via raw_... ones.  Those generic implementations
are in linux/uaccess.h (and lib/usercopy.c).  Architecture provides
raw_... ones, selects ARCH_HAS_RAW_COPY_USER and it's done.

* all object size check, kasan, etc. instrumentation is taken care of
in linux/uaccess.h; no need to touch it in arch/*

* consistent semantics wrt zero-padding - none of the raw_... do any of
that, copy_from_user() does (outside of fast path).

At the moment I have that conversion done for everything except ia64 and
metag.  Once everything is converted, I'll remove ARCH_HAS_RAW_COPY_USER
and make generic stuff unconditional; at the same point
HAVE_ARCH_HARDENED_USERCOPY will be gone (becoming unconditionally true).

The series lives in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
in #work.uaccess.  It's based at 4.11-rc1.  Infrastructure is in
#uaccess.stem, then it splits into per-architecture branches (uaccess.<arch>),
eventually merged into #work.uaccess.  Some stuff (including a cherry-picked
mips build fix) is in #uaccess.misc, also merged into the final.

I hope that infrastructure part is stable enough to put it into never-rebased
state.  Some of per-architecture branches might be even done right; however,
most of them got no testing whatsoever, so any help with testing (as well
as "Al, for fuck sake, dump that garbage of yours, here's the correct patch"
from maintainers) would be very welcome.  So would the review, of course.

In particular, the fix in uaccess.parisc should be replaced with the stuff
Helge posted on parisc list, probably along with the get_user/put_user
patches.  I've put my variant of fix there as a stopgap; switch of pa_memcpy()
to assembler is clearly the right way to solve it and I'll be happy to
switch to that as soon as parisc folks settle on the final version of that
stuff.

For most of the oddball architectures I have no way to test that stuff, so
please treat the asm-affecting patches in there as a starting point for
doing it right.  Some might even work as is - stranger things had happened,
but don't count ont it.

And again, metag and ia64 parts are simply not there - both architectures
zero-pad in __copy_from_user_inatomic() and that really needs fixing.
In case of metag there's __copy_to_user() breakage as well, AFAICS, and
I've been unable to find any documentation describing the architecture
wrt exceptions, and that part is apparently fairly weird.  In case of
ia64...  I can test mckinley side of things, but not the generic __copy_user()
and ia64 is about as weird as it gets.  With no reliable emulator, at that...
So these two are up to respective maintainers.

Other things not there:
	* unification of strncpy_from_user() and friends.  Probably next
cycle.
	* anything to do with uaccess_begin/unsafe accesses/uaccess_end
stuff.  Definitely next cycle.

I'm not sure if mailbombing linux-arch would be a good idea; there are
90 patches in that pile, with total size nearly half a megabyte.  If anyone
wants that posted, I'll do so, but it might be more convenient to just
use git.

Comments, review, testing, replacement patches, etc. are very welcome.

				Al "hates assembers, dozens of them" Viro


[1]  Nick Piggin has spotted that bug back in early 2000s, fixed it for
i386 and hadn't bothered to do anything about other architectures (including
amd64, for crying out loud!).  Since then we had inconsistent behaviour
between the architectures.  Results of those bugs range from transient bogus
values observed in mmap() if you get memory pressure combined with bad timing
to outright fs corruption, if the timing is *really* bad.  All architectures
used to have it, hopefully this series will take care of the last stragglers.

[toc] | [next] | [standalone]

#1612293

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

On 03/28/2017 10:57 PM, Al Viro wrote:
> 	We have several primitives for bulk kernel<->userland copying.
> That stuff lives in various asm/uaccess.h, with serious code duplication
> _and_ seriously inconsistent semantics.
>
> 	That code has grown a lot of cruft and more than a few bugs.
> Some got caught and fixed last year, but some fairly unpleasant ones
> still remain.  A large part of problem was that a lot of code used to
> include <asm/uaccess.h> directly, so we had no single place to work
> with.  That got finally fixed in 4.10-rc1, when everything had been
> forcibly switched to including <linux/uaccess.h>.  At that point it
> became possible to start getting rid of boilerplate; I hoped to deal
> with that by 4.11-rc1, but the things didn't work out and that work
> has slipped to this cycle.
>
> 	The patchset currently in vfs.git#work.uaccess is the result;
> there's more work to do, but it takes care of a large part of the
> problems.  About 2.8KLoc removed, a lot of cruft is gone and semantics
> is hopefully in sync now.  All but two architectures (ia64 and metag)
> had been switched to new mechanism; for these two I'm afraid that I'll
> need serious help from maintainers.
>
> 	Currently we have 8 primitives - 6 on every architecture and 2 more
> on biarch ones.  All of them have the same calling conventions: arguments
> are the same as for memcpy() (void *to, const void *from, unsigned long size)
> and the same rules for return value.
> 	If all loads and stores succeed, everything is obvious - the
> 'size' bytes starting at 'to' become equal to 'size' bytes starting at 'from'
> and zero is returned.  If some loads or stores fail, non-zero value should
> be returned.  If any of those primitives returns a positive value N,
> 	* N should be no greater than size
> 	* the values fetched out of from[0..size-N-1] should be stored into the
> corresponding bytes of to[0..size-N-1]
> 	* N should not be equal to size unless not a single byte could have
> been fetched or stored.  As long as that restriction is satisfied, these
> primitives are not required to squeeze every possible byte in case some
> loads or stores fail.
>
> 	1) copy_from_user() - 'to' points to kernel memory, 'from' is
> normally a userland pointer.  This is used for copying structures from
> userland in all kinds of ioctls, etc.  No faults on access to destination are
> allowed, faults on access to source lead to zero-padding the rest of
> destination.  Note that for architectures with the same address space split
> between the kernel and userland (i.e. the ones that have non-trivial
> access_ok()) passing a kernel address instead of a userland one should be
> treated as 'every access would fail'.  In such cases the entire destination
> should be zeroed (failure to do so was a fairly common bug).
> 	Note that all these functions, including copy_from_user(), are
> affected by set_fs() - when called under set_fs(KERNEL_DS), they expect
> kernel pointers where normally a userland one would be given.
>
> 	2) copy_to_user() - 'from' points to kernel memory, 'to' is
> a userland pointer (subject to set_fs() effects, as usual).  Again.
> this is used by all kinds of code in all kinds of drivers, syscalls, etc.
> No faults on access to source, fault on access to destination terminates
> copying.  No zero-padding, of course - the faults are going to be on store
> here.  Does not assume that access_ok() had been checked by caller;
> given 'to'/'size' that fails access_ok() returns "nothing copied".
>
> 	3) copy_in_user() - both 'from' and 'to' are in userland.  Used
> only by compat code that needs to repack 32bit data structures into native
> 64bit counterparts.  As the result, provided only by biarch architectures.
> Subject to set_fs(), but really should not be (and AFAICS isn't) used that way.
> Some architectures tried to zero-pad, but did it inconsistently and it's
> pointless anyway - destination is in userland memory, so no infoleaks would
> happen.
>
> 	4) __copy_from_user_inatomic() - similar to copy_from_user(),
> except that
> 	* the caller is presumed to have verified that the source range passes
> access_ok() [note that this is does not guarantee the lack of faults]
> 	* most importantly, zero-padding SHOULD NOT happen on short copy.
> If implementation fails to guarantee that, it's a bug and potentially
> bad one[1].
> 	* it may be called with pagefaults disabled (of course, in
> that case any pagefault results in a short copy).  That's what 'inatomic'
> in the name refers to.  Note that actually disabling pagefaults is
> up to the caller; blindly calling it e.g. from under a spinlock will just
> get you a deadlock.  Even more precautions are needed to call it from
> something like an interrupt handler - you must do that under set_fs(),
> etc.   It's not "this variant is safe to call from atomic contexts", it's
> "I know what I'm doing, don't worry if you see it in an atomic context".
>
> 	5) __copy_to_user_inatomic().  A counterpart of
> __copy_from_user_inatomic(), except for the direction of copying.
>
> 	6) __copy_from_user().  Essentially the only difference from
> __copy_from_user_inatomic() is that one isn't supposed to call it from
> atomic contexts.  It may be marginally faster than copy_from_user() (due
> to skipped access_ok()), but these days the main costs are not in doing
> fairly light arithmetics.  In theory, you might do a single access_ok()
> covering a large structure and then proceed to call __copy_from_user()
> on various parts of that.  In practice doing many calls of that thing on
> small chunks of data is going to cost a lot on current x86 boxen due to
> STAC/CLAC pair inside each call.  Has fewer call sites than copy_from_user()
> - copy_from_user() is in thousands, while this one has only 40 callers
> outside of arch/, some fairly dubious.  In arch there's about 170 callers
> total, mostly in sigreturn instances.
>
> 	7) __copy_to_user().  A counterpart of __copy_from_user(), with
> pretty much the same considerations applied.
>
> 	8) __copy_in_user().  Basically, copy_in_user() sans access_ok().
> Biarch-only, with the grand total of 6 callers...
>
>
> 	What this series does is:
>
> * convert architectures to fewer primitives (raw_copy_{to,from,in}_user(),
> the last one only on biarch ones), switching to generic implementations
> of the 8 primitives aboves via raw_... ones.  Those generic implementations
> are in linux/uaccess.h (and lib/usercopy.c).  Architecture provides
> raw_... ones, selects ARCH_HAS_RAW_COPY_USER and it's done.
>
> * all object size check, kasan, etc. instrumentation is taken care of
> in linux/uaccess.h; no need to touch it in arch/*
>
> * consistent semantics wrt zero-padding - none of the raw_... do any of
> that, copy_from_user() does (outside of fast path).
>
> At the moment I have that conversion done for everything except ia64 and
> metag.  Once everything is converted, I'll remove ARCH_HAS_RAW_COPY_USER
> and make generic stuff unconditional; at the same point
> HAVE_ARCH_HARDENED_USERCOPY will be gone (becoming unconditionally true).
>
> The series lives in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
> in #work.uaccess.  It's based at 4.11-rc1.  Infrastructure is in
> #uaccess.stem, then it splits into per-architecture branches (uaccess.<arch>),
> eventually merged into #work.uaccess.  Some stuff (including a cherry-picked
> mips build fix) is in #uaccess.misc, also merged into the final.
>
> I hope that infrastructure part is stable enough to put it into never-rebased
> state.  Some of per-architecture branches might be even done right; however,
> most of them got no testing whatsoever, so any help with testing (as well
> as "Al, for fuck sake, dump that garbage of yours, here's the correct patch"
> from maintainers) would be very welcome.  So would the review, of course.
>
> In particular, the fix in uaccess.parisc should be replaced with the stuff
> Helge posted on parisc list, probably along with the get_user/put_user
> patches.  I've put my variant of fix there as a stopgap; switch of pa_memcpy()
> to assembler is clearly the right way to solve it and I'll be happy to
> switch to that as soon as parisc folks settle on the final version of that
> stuff.
>
> For most of the oddball architectures I have no way to test that stuff, so
> please treat the asm-affecting patches in there as a starting point for
> doing it right.  Some might even work as is - stranger things had happened,
> but don't count ont it.
>
> And again, metag and ia64 parts are simply not there - both architectures
> zero-pad in __copy_from_user_inatomic() and that really needs fixing.
> In case of metag there's __copy_to_user() breakage as well, AFAICS, and
> I've been unable to find any documentation describing the architecture
> wrt exceptions, and that part is apparently fairly weird.  In case of
> ia64...  I can test mckinley side of things, but not the generic __copy_user()
> and ia64 is about as weird as it gets.  With no reliable emulator, at that...
> So these two are up to respective maintainers.
>
> Other things not there:
> 	* unification of strncpy_from_user() and friends.  Probably next
> cycle.
> 	* anything to do with uaccess_begin/unsafe accesses/uaccess_end
> stuff.  Definitely next cycle.
>
> I'm not sure if mailbombing linux-arch would be a good idea; there are
> 90 patches in that pile, with total size nearly half a megabyte.  If anyone
> wants that posted, I'll do so, but it might be more convenient to just
> use git.
>
> Comments, review, testing, replacement patches, etc. are very welcome.
>
> 				Al "hates assembers, dozens of them" Viro

Hi Al,

Thx for taking this up. It seems ARC was missing INLINE_COPY* switch likely due to
existing 2 variants (inline/out-of-line) we already have.
I've added a patch for that (attached too) - boot tested the series on ARC.

------->
From 29205ba126468986fcee0d12dba6b5f831506803 Mon Sep 17 00:00:00 2001
From: Vineet Gupta <vgupta@synopsys.com>
Date: Wed, 29 Mar 2017 11:53:33 -0700
Subject: [PATCH] ARC: uaccess: enable INLINE_COPY_{TO,FROM}_USER ...

... and switch to generic out of line version in lib/usercopy.c

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/uaccess.h | 16 ++++++----------
 arch/arc/mm/extable.c          | 14 --------------
 2 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/arch/arc/include/asm/uaccess.h b/arch/arc/include/asm/uaccess.h
index c4d26e8a21b3..f35974ee7264 100644
--- a/arch/arc/include/asm/uaccess.h
+++ b/arch/arc/include/asm/uaccess.h
@@ -168,7 +168,7 @@
 
 
 static inline unsigned long
-__arc_copy_from_user(void *to, const void __user *from, unsigned long n)
+raw_copy_from_user(void *to, const void __user *from, unsigned long n)
 {
     long res = 0;
     char val;
@@ -395,7 +395,7 @@ __arc_copy_from_user(void *to, const void __user *from,
unsigned long n)
 }
 
 static inline unsigned long
-__arc_copy_to_user(void __user *to, const void *from, unsigned long n)
+raw_copy_to_user(void __user *to, const void *from, unsigned long n)
 {
     long res = 0;
     char val;
@@ -721,24 +721,20 @@ static inline long __arc_strnlen_user(const char __user *s,
long n)
 }
 
 #ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
-#define raw_copy_from_user        __arc_copy_from_user
-#define raw_copy_to_user        __arc_copy_to_user
+
+#define INLINE_COPY_TO_USER
+#define INLINE_COPY_FROM_USER
+
 #define __clear_user(d, n)        __arc_clear_user(d, n)
 #define __strncpy_from_user(d, s, n)    __arc_strncpy_from_user(d, s, n)
 #define __strnlen_user(s, n)        __arc_strnlen_user(s, n)
 #else
-extern long arc_copy_from_user_noinline(void *to, const void __user * from,
-        unsigned long n);
-extern long arc_copy_to_user_noinline(void __user *to, const void *from,
-        unsigned long n);
 extern unsigned long arc_clear_user_noinline(void __user *to,
         unsigned long n);
 extern long arc_strncpy_from_user_noinline (char *dst, const char __user *src,
         long count);
 extern long arc_strnlen_user_noinline(const char __user *src, long n);
 
-#define raw_copy_from_user        arc_copy_from_user_noinline
-#define raw_copy_to_user        arc_copy_to_user_noinline
 #define __clear_user(d, n)        arc_clear_user_noinline(d, n)
 #define __strncpy_from_user(d, s, n)    arc_strncpy_from_user_noinline(d, s, n)
 #define __strnlen_user(s, n)        arc_strnlen_user_noinline(s, n)
diff --git a/arch/arc/mm/extable.c b/arch/arc/mm/extable.c
index c86906b41bfe..72125a34e780 100644
--- a/arch/arc/mm/extable.c
+++ b/arch/arc/mm/extable.c
@@ -28,20 +28,6 @@ int fixup_exception(struct pt_regs *regs)
 
 #ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 
-long arc_copy_from_user_noinline(void *to, const void __user *from,
-        unsigned long n)
-{
-    return __arc_copy_from_user(to, from, n);
-}
-EXPORT_SYMBOL(arc_copy_from_user_noinline);
-
-long arc_copy_to_user_noinline(void __user *to, const void *from,
-        unsigned long n)
-{
-    return __arc_copy_to_user(to, from, n);
-}
-EXPORT_SYMBOL(arc_copy_to_user_noinline);
-
 unsigned long arc_clear_user_noinline(void __user *to,
         unsigned long n)
 {
-- 
2.7.4

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

#1612303

On Wed, Mar 29, 2017 at 1:29 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> BTW, I wonder if inlining all of the copy_{to,from}_user() is actually a win.

I would suggest against it.

The only part I think is worth inlining is the compile time size
checks for kasan - and that only because of the obvious "sizes are
constant only when inlining" issue.

We used to inline a *lot* of user accesses historically, pretty much
all of them were bogus.

The only ones that really want inlining are the non-checking ones that
people should never use directly, but that are just helper things used
by other routines (ie the "unsafe_copy_from_user()" kind of things
that are designed for strncpy_from_user()).

Once you start checking access ranges, and have might_fault debugging
etc, it shouldn't be inlined.

              Linus

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

#1612345

On Wed, Mar 29, 2017 at 01:37:30PM -0700, Linus Torvalds wrote:
> On Wed, Mar 29, 2017 at 1:29 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > BTW, I wonder if inlining all of the copy_{to,from}_user() is actually a win.
> 
> I would suggest against it.
> 
> The only part I think is worth inlining is the compile time size
> checks for kasan - and that only because of the obvious "sizes are
> constant only when inlining" issue.
> 
> We used to inline a *lot* of user accesses historically, pretty much
> all of them were bogus.
> 
> The only ones that really want inlining are the non-checking ones that
> people should never use directly, but that are just helper things used
> by other routines (ie the "unsafe_copy_from_user()" kind of things
> that are designed for strncpy_from_user()).
> 
> Once you start checking access ranges, and have might_fault debugging
> etc, it shouldn't be inlined.

FWIW, that's why I'd put those knobs (INLINE_COPY_{TO,FROM}_USER) in there;
if for some architectures making those inlined is really a win, they can
request the inlining; for now I'd mostly set them to match what architectures
had been doing, but I also strongly suspect that in a lot of cases that
inlining is counterproductive.  Building it both ways is simply a matter of
deleting those two lines in asm/uaccess.h in question, and if testing
shows that out-of-line works better on given architecture, well... 

I would expect that the final variant will have those remain only on a few
architectures.  IMO decision whether to inline them or not is up to
architecture - it's not as if having the possibility to inline them
would really complicate the generic side of things...

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

#1612372

On Wed, Mar 29, 2017 at 2:03 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> it's not as if having the possibility to inline them
> would really complicate the generic side of things...

I disagree.

I think one of the biggest problems with our current uaccess.h mess is
just how illegible the header files are, and the
INLINE_COPY_{TO,FROM}_USER thing is not helping.

I think it would be much better if the header file just had

 extern unsigned long _copy_from_user(void *, const void __user *,
unsigned long);

and nothing else. No unnecessary noise.

The same goes for things like [__]copy_in_user() - why is that thing
still inlined? If it was a *macro*, it might be useful due to the
might_fault() thing giving the caller information, but that's not even
the case here, so we'd actually be much better off without any of that
inlining stuff. Do it all in lib/usercopy.c, and move the
might_fault() in there too.

                Linus

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

#1612438

On Wed, Mar 29, 2017 at 02:24:37PM -0700, Linus Torvalds wrote:

> I think one of the biggest problems with our current uaccess.h mess is
> just how illegible the header files are, and the
> INLINE_COPY_{TO,FROM}_USER thing is not helping.
> 
> I think it would be much better if the header file just had
> 
>  extern unsigned long _copy_from_user(void *, const void __user *,
> unsigned long);
> 
> and nothing else. No unnecessary noise.
> 
> The same goes for things like [__]copy_in_user() - why is that thing
> still inlined? If it was a *macro*, it might be useful due to the
> might_fault() thing giving the caller information, but that's not even
> the case here, so we'd actually be much better off without any of that
> inlining stuff. Do it all in lib/usercopy.c, and move the
> might_fault() in there too.

IMO that's a separate series.  For now I would be bloody happy if we got
	* arch-dependent asm fixes out of the way
	* everything consolidated outside of arch/*
	* arch/*/include/uaccess*.h simplified.

As for __copy_in_user()... I'm not sure we want to keep it in the long run -
drivers/gpu/drm/drm_ioc32.c:390:        if (__copy_in_user(buf, argp, offsetof(drm_buf_desc32_t, agp_start))
drivers/gpu/drm/drm_ioc32.c:399:        if (__copy_in_user(argp, buf, offsetof(drm_buf_desc32_t, agp_start))
drivers/gpu/drm/drm_ioc32.c:475:                        if (__copy_in_user(&to[i], &list[i],
drivers/gpu/drm/drm_ioc32.c:536:                        if (__copy_in_user(&list32[i], &list[i],
fs/compat_ioctl.c:753:  if (__copy_in_user(&tdata->read_write, &udata->read_write, 2 * sizeof(u8)))
fs/compat_ioctl.c:755:  if (__copy_in_user(&tdata->size, &udata->size, 2 * sizeof(u32)))

are all callers out there.  And looking at those callers...  fs/compat_ioctl.c
ones are ridiculous - they translate
struct i2c_smbus_ioctl_data {
        __u8 read_write;
        __u8 command;
        __u32 size;
        union i2c_smbus_data __user *data;
};
into
struct i2c_smbus_ioctl_data32 {
        u8 read_write;
        u8 command;
        u32 size;
        compat_caddr_t data; /* union i2c_smbus_data *data */
};
by doing
	* 2 byte copy (read_write + command -> read_write + command)
	* 8 byte copy (size + data -> size + half of data; WTF 8 and not 4?)
	* 4 byte load (data)
	* 8 byte store (data)
That gem went into the tree in 2003, apparently as a quick hack from
benh, and never had been touched since then.  IMO inlining is very far
down the list of, er, deficiencies there.  If anything, it would be
better off with a single copy_from_user() into a local union, followed by
something like foo.native.data = compat_ptr(foo.compat.data) and
copy_to_user() into tdata.  And that's assuming we won't be better off
with proper ->compat_ioctl() for that sucker - AFAICS, there's a bunch
of I2C_... stuff understood by fs/compat_ioctl.c, all for the sake of
one driver.  I'll look into that tonight...

As for the drm ones, I don't see any reasons for them not to be copy_in_user().
If any of that is the hot path (the last two are in loops), we have worse
problems with STAC/CLAC anyway.

So I'm not sure if __copy_in_user() shouldn't just die.  copy_in_user()
might be a good candidate for move to lib/usercopy.c; I'm somewhat worried
about sparc64, though.  access_ok() is a no-op there, so on the builds
without lockdep where might_fault() is a no-op we get pointless extra
jump for no good reason.  I would like to see comments from davem on that
one...

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

#1612448

On Wed, Mar 29, 2017 at 4:09 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> IMO that's a separate series.  For now I would be bloody happy if we got
>         * arch-dependent asm fixes out of the way
>         * everything consolidated outside of arch/*
>         * arch/*/include/uaccess*.h simplified.

Sure, I agree.

At the same time, I just think that we really *should* aim for a
simpler uaccess.h in the long term, so I would prefer we not encourage
architectures to do things that simply won't matter.

> As for __copy_in_user()... I'm not sure we want to keep it in the long run -

I agree, it's probably not worth it at all.

In fact, I suspect none of the "__copy_.*_user()" versions are worth
it, and we should strive to remove them.

There aren't even that many users, and they _have_ caused security
issues when people have had some path that hasn't checked the range.

                Linus

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

#1613201

On Wed, Mar 29, 2017 at 04:43:05PM -0700, Linus Torvalds wrote:

> > As for __copy_in_user()... I'm not sure we want to keep it in the long run -
> 
> I agree, it's probably not worth it at all.
> 
> In fact, I suspect none of the "__copy_.*_user()" versions are worth
> it, and we should strive to remove them.
> 
> There aren't even that many users, and they _have_ caused security
> issues when people have had some path that hasn't checked the range.

Actually, looking through those users shows some very odd places: for example,
sctp_setsockopt_bindx() does
        /* Check the user passed a healthy pointer.  */
        if (unlikely(!access_ok(VERIFY_READ, addrs, addrs_size)))
                return -EFAULT;

        /* Alloc space for the address array in kernel memory.  */
        kaddrs = kmalloc(addrs_size, GFP_USER | __GFP_NOWARN);
        if (unlikely(!kaddrs))
                return -ENOMEM;

        if (__copy_from_user(kaddrs, addrs, addrs_size)) {
                kfree(kaddrs);
                return -EFAULT;
        }
The obvious question is "why not memdup_user()?" and rationale looks fishy:
 * We don't use copy_from_user() for optimization: we first do the
 * sanity checks (buffer size -fast- and access check-healthy
 * pointer); if all of those succeed, then we can alloc the memory
 * (expensive operation) needed to copy the data to kernel. Then we do
 * the copying without checking the user space area
 * (__copy_from_user()).
plus that:
    sctp: use GFP_USER for user-controlled kmalloc
    
    Dmitry Vyukov reported that the user could trigger a kernel warning by
    using a large len value for getsockopt SCTP_GET_LOCAL_ADDRS, as that
    value directly affects the value used as a kmalloc() parameter.
    
    This patch thus switches the allocation flags from all user-controllable
    kmalloc size to GFP_USER to put some more restrictions on it and also
    disables the warn, as they are not necessary.

First of all, access_ok() for sanity checks on size is BS - on some
architectures it's constant 1 and on *all* architectures it allows a lot
more than what kmalloc() will.  So it won't stop a malicious program from
getting to kmalloc() and wasting its cycles and it's not much help with
buggy ones.  __GFP_NOWARN part is more interesting, but... the quoted
commit misses memdup_user() calls in other setsockopt cases on the same
sctp.  So if we care about that one, we probably should care about the
rest of them as well, and I doubt that open-coding each is a good solution.
Something like kvmemdup_user(), perhaps?  I.e. quiet fallback to vmalloc
on large sizes/in case when kmalloc barfs, with kvfree on the freeing side?
Or just a flat-out check for some reasonably upper limit on optlen in
the very beginning?

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

#1612309

On Wed, Mar 29, 2017 at 01:08:12PM -0700, Vineet Gupta wrote:

> Hi Al,
> 
> Thx for taking this up. It seems ARC was missing INLINE_COPY* switch likely due to
> existing 2 variants (inline/out-of-line) we already have.
> I've added a patch for that (attached too) - boot tested the series on ARC.

BTW, I wonder if inlining all of the copy_{to,from}_user() is actually a win.
It's probably arch-dependent and it would be nice if somebody compared
performance with and without inlining those...  ARC, in particular, has
__arc_copy_{to,from}_user() inlining a whole lot, even in case of non-constant
size and your patch, AFAICS, will inline all of it in *all* cases.  It might
end up being a win, but that's not apriori obvious...  Do you have any
profiling results in that area?

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

#1612355

On 03/29/2017 01:29 PM, Al Viro wrote:
> On Wed, Mar 29, 2017 at 01:08:12PM -0700, Vineet Gupta wrote:
> 
>> Hi Al,
>>
>> Thx for taking this up. It seems ARC was missing INLINE_COPY* switch likely due to
>> existing 2 variants (inline/out-of-line) we already have.
>> I've added a patch for that (attached too) - boot tested the series on ARC.
> 
> BTW, I wonder if inlining all of the copy_{to,from}_user() is actually a win.

Just to be clear, your series was doing this for everyone.

> It's probably arch-dependent and it would be nice if somebody compared
> performance with and without inlining those...  ARC, in particular, has
> __arc_copy_{to,from}_user() inlining a whole lot, even in case of non-constant
> size and your patch, AFAICS, will inline all of it in *all* cases. 

Yes we do inline all of it: the non-constant case is actually simpler, it is a
simple byte loop.

		"	mov.f   lp_count, %0		\n"
		"	lpnz 3f				\n"
		"	ldb.ab  %1, [%3, 1]		\n"
		"1:	stb.ab  %1, [%2, 1]		\n"
		"	sub     %0, %0, 1		\n"

Doing it out of line (3 args) will be 4 instructions anyways.

For constant size, there's laddered copy for blocks of 16 bytes + stragglers 1-15.
We do "manual" constant propagation there to compile time optimize away the
straggler part. But yes all of this is emitted inline.


> It might
> end up being a win, but that's not apriori obvious...  Do you have any
> profiling results in that area?

Unfortunately not at the moment. The reason for adding out-of-line variant was not
so much as performance but to improve the footprint for -Os case (some customer I
think).

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

#1612449

On Wed, Mar 29, 2017 at 02:14:22PM -0700, Vineet Gupta wrote:

> > BTW, I wonder if inlining all of the copy_{to,from}_user() is actually a win.
> 
> Just to be clear, your series was doing this for everyone.

Huh?  It's just that most of architectures *were* inlining that;
arc change was unintentional (copy_from_user/copy_to_user went
uninlined, which your patch deals with), but it's not that I'm forcing
inlining on every architecture out there.

> > It might
> > end up being a win, but that's not apriori obvious...  Do you have any
> > profiling results in that area?
> 
> Unfortunately not at the moment. The reason for adding out-of-line variant was not
> so much as performance but to improve the footprint for -Os case (some customer I
> think).

Just to make it clear - I'm less certain than Linus that uninlined is uniformly
better, but I have a strong suspicion that on most architectures it *is*.
And not just in terms of kernel size - I would expect better speed as well.
The only reason why these knobs are there is that I want to separate the
"who should switch to uninlined" from this series and allow for the possibility
that for some architectures inlined will really turn out to be better.
I do _not_ expect that there'll be many of those; if it turns out that there's
none, I'll be only glad to make the guts of copy_{to,from}_user() always
out of line.

IOW your patch reverts an unintentional change of behaviour, but I really
wonder if that (out-of-line guts of copy_{to,from}_user) isn't an overall
win for arc.  I've applied your patch, but it would be nice if you could
arrange for testing with and without inlining and post the results.  The
same goes for all architectures; again, I would expect out-of-line to end up
a win on most of them.

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

#1612454

On 03/29/2017 04:42 PM, Al Viro wrote:
> On Wed, Mar 29, 2017 at 02:14:22PM -0700, Vineet Gupta wrote:
> 
>>> BTW, I wonder if inlining all of the copy_{to,from}_user() is actually a win.
>>
>> Just to be clear, your series was doing this for everyone.
> 
> Huh?  It's just that most of architectures *were* inlining that;
> arc change was unintentional (copy_from_user/copy_to_user went
> uninlined, which your patch deals with), but it's not that I'm forcing
> inlining on every architecture out there.

That is correct - I didn't mean to say you changed it per-se , but that I saw
INLINE_COPY* all over the place but not for ARC :-)


>>> It might
>>> end up being a win, but that's not apriori obvious...  Do you have any
>>> profiling results in that area?
>>
>> Unfortunately not at the moment. The reason for adding out-of-line variant was not
>> so much as performance but to improve the footprint for -Os case (some customer I
>> think).
> 
> Just to make it clear - I'm less certain than Linus that uninlined is uniformly
> better, but I have a strong suspicion that on most architectures it *is*.
> And not just in terms of kernel size - I would expect better speed as well.
> The only reason why these knobs are there is that I want to separate the
> "who should switch to uninlined" from this series and allow for the possibility
> that for some architectures inlined will really turn out to be better.
> I do _not_ expect that there'll be many of those; if it turns out that there's
> none, I'll be only glad to make the guts of copy_{to,from}_user() always
> out of line.
> 
> IOW your patch reverts an unintentional change of behaviour, but I really
> wonder if that (out-of-line guts of copy_{to,from}_user) isn't an overall
> win for arc.  I've applied your patch, but it would be nice if you could
> arrange for testing with and without inlining and post the results.  The
> same goes for all architectures; again, I would expect out-of-line to end up
> a win on most of them.

I guess I can in next day or two - but mind you the inline version for ARC is kind
of special vs. other arches. We have this "manual" constant propagation to elide
the unrolled LD/ST for 1-15 byte stragglers, when @sz is constant. In the
out-of-line version, we loose all of that and the code needs to fall thru all the
cases. We can possibly improve that by re-arranging the checks - so exit early if
no stragglers etc ...

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

#1612465

On Wed, Mar 29, 2017 at 5:02 PM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
>
> I guess I can in next day or two - but mind you the inline version for ARC is kind
> of special vs. other arches. We have this "manual" constant propagation to elide
> the unrolled LD/ST for 1-15 byte stragglers, when @sz is constant.

I don't think that's special. We do that on x86 too, and I suspect ARC
copied it from there (or from somebody else who did it).

But at least on x86 is is limited entirely to the "__" versions, and
it's almost entirely pointless. We actually removed some of that kind
of code because it was *do* pointless, and it had just been copied
around into the "atomic" versions too.

See for example commit bd28b14591b9 ("x86: remove more uaccess_32.h
complexity"), which did that.

The basic "__" versions still do that constant-size thing, but they
really are questionable. Exactly because it's just the "__" versions -
the *regular* "copy_to/from_user()" is an unconditional function call,
because inlining it isn't just the access operations, it's the size
check, and on modern x86 it's also the "set AC to mark the user access
as safe".

.. and many distros enable some of the might_sleep() debugging code
etc. With any of that, inlining is simply a *bad* choice.

                  Linus

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

#1612482

On Wed, Mar 29, 2017 at 05:27:40PM -0700, Linus Torvalds wrote:

> The basic "__" versions still do that constant-size thing, but they
> really are questionable. Exactly because it's just the "__" versions -
> the *regular* "copy_to/from_user()" is an unconditional function call,
> because inlining it isn't just the access operations, it's the size
> check, and on modern x86 it's also the "set AC to mark the user access
> as safe".

Keep in mind that come architectures have __copy_from_user() (well,
raw_copy_from_user(), now) used in __get_user().  This is a bad idea
for a lot of reasons, and it needs to be taken care of, but I really
don't want to mix __get_user()/__put_user() stuff (there's a lot
of boilerplate in that area as well) into this series.

Infrastructure for that would have to go into the uaccess.stem, and that
would pretty much guarantee that it wouldn't get into no-rebase mode for
extra couple of weeks.  As it is, uaccess.<arch> are on top of no-rebase
branch, so once architecture maintainers are happy with what's in it,
we can put it in no-rebase mode and have it pulled into that architecture's
tree.  That way we can avoid any merge conflicts; fighting the conflicts
between vfs.git and random growing set of architecture trees, all the way
through -next into the merge window... <shudder>

For even more fun, there's VFS (well, fs, actually - it's in ->write_end()
instances) work depending on the __copy_from_user_inatomic() not zero-padding
anything on short copy.  With the set of potential conflicts of its own,
with individual fs trees... ;-/

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

#1613464

On 03/29/2017 05:27 PM, Linus Torvalds wrote:
> On Wed, Mar 29, 2017 at 5:02 PM, Vineet Gupta
> <Vineet.Gupta1@synopsys.com> wrote:
>>
>> I guess I can in next day or two - but mind you the inline version for ARC is kind
>> of special vs. other arches. We have this "manual" constant propagation to elide
>> the unrolled LD/ST for 1-15 byte stragglers, when @sz is constant.
> 
> I don't think that's special. We do that on x86 too, and I suspect ARC
> copied it from there (or from somebody else who did it).

No, I (re)wrote that code and AFAIKR didn't copy from anyone and AFAICS it is
certainly different from others if not special. If you look closely at
arc:access.h it is not the trivial check for 1-2-4 conversion as in the commit you
referred to. It actually tries to compile time eliminate hunks from inline
assembly, for constant @sz (so is designed purely for inlined variants, whether
that matters or  not is a different story). Thing is from the hardware POV, 4
LD/ST in flight is good (atleast for ARC700 cores) so we wrap it up in a Zero
delay loop. This takes care of multiples of 16 bytes, the last 15 bytes are the
killer which requires bunch of conditionals which is what I try to eliminate.

FWIW, I experimented with uaccess inlining on ARC
1. pristine 4.11-rc1 (all inline)
2. Inline + disabling the "smart" const propagation
3. Out of line only variants (which already existed/default on ARC for -Os, but
hacked for current -O3)

Numbers for LMBench FS latency (off of tmpfs to avoid any device related
perturbation). Note that LMBench already runs them several times itself and each
of below is obviously with a fresh reboot since kernels were different.

So it seems 0k file create/del gets worse without the smart inline, while 10k gets
better. mmap (16k) got worse as well. With out of line some got better while some
worse.


   File & VM system latencies in microseconds - smaller is better
   -------------------------------------------------------------------------------
   Host                 OS   0K File      10K File     Mmap    Prot   Page   100fd
                           Create Delete Create Delete Latency Fault  Fault  selct
   --------- ------------- ------ ------ ------ ------ ------- ----- ------- -----
   170329-v4 Linux 4.11.0-  124.3   75.3  734.2  147.8  2200.0 6.205    10.9  87.6
   170330-v4 Linux 4.11.0-  154.9   88.3  709.2  131.2  2494.0 4.056    11.0  91.1
   170330-v4 Linux 4.11.0-  157.7   69.8  622.7  140.8  2168.0 5.654    10.8  91.0

Compare that to data against

1. pristine 4.11-rc1 (all inline)
2. Al's series + ARC forced inline
3. Al's series + ARC forced NOT inline

   File & VM system latencies in microseconds - smaller is better
   -------------------------------------------------------------------------------
   Host                 OS   0K File      10K File     Mmap    Prot   Page   100fd
                           Create Delete Create Delete Latency Fault  Fault  selct
   --------- ------------- ------ ------ ------ ------ ------- ----- ------- -----
   170329-v4 Linux 4.11.0-  124.3   75.3  734.2  147.8  2200.0 6.205    10.9  87.6
   170329-v4 Linux 4.11.0-  141.2   63.4  629.7  130.0  2172.0 5.796    10.8  90.0
   170329-v4 Linux 4.11.0-  154.9   89.2  691.6  147.7  2323.0 4.922    10.8  92.3

So it's a mix bag really. Maybe we need some better directed test to really drill
it down.


> But at least on x86 is is limited entirely to the "__" versions, and
> it's almost entirely pointless. We actually removed some of that kind
> of code because it was *do* pointless, and it had just been copied
> around into the "atomic" versions too.
> 
> See for example commit bd28b14591b9 ("x86: remove more uaccess_32.h
> complexity"), which did that.
> 
> The basic "__" versions still do that constant-size thing, but they
> really are questionable. 

Perhaps because the scope of constant usage was pretty narrow - it would only
benefit if *copy_from_user() were called with 1,2,4 which is relatively unlikely
as we have __get_user and friends for that already.

> Exactly because it's just the "__" versions -
> the *regular* "copy_to/from_user()" is an unconditional function call,
> because inlining it isn't just the access operations, it's the size
> check, and on modern x86 it's also the "set AC to mark the user access
> as safe".

So what you are saying is it is relatively costly on x86 because of SMAP which may
not be true for arches w/o hardware support.
Note that I'm not arguing for/against inlining per-se, it seems it doesn't matter

-Vineet

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

#1613469

On Thu, Mar 30, 2017 at 1:40 PM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
>
> So it's a mix bag really. Maybe we need some better directed test to really drill
> it down.

As mentioned inn the discussion about ARM, I seriously doubt that the
inlining will even be noticeable compared to other effects here.

The cases where I've seen this matter have been the small
constant-sized copies, and in every case the right fix was to use
"get_user()" instead of "copy_from_user()".

There are a couple of really special cases that can show up where
there's a slightly bigger and more complex case that still is
meaningful: the most noticeable of those being "stat()".

> So what you are saying is it is relatively costly on x86 because of SMAP which may
> not be true for arches w/o hardware support.
> Note that I'm not arguing for/against inlining per-se, it seems it doesn't matter

So on x86 (and ARM) we have the SMAP/UAO issue, and on other
architectures we have another expense entirely: maintenance and
testing.

(On ARM, hopefully the UAO bit is faster to set, but it's still
"another instruction before and after", so even if it's not as
expensive as clac/stac are on current x86 chips, it's an argument
against inlining)

And on the maintenance and testing side, it means that unless some
header organization makes sense on x86 or ARM (or power), it likely
doesn't make sense to try to tweak for any other architecture.

Or at the very least it would have to be a _really_ big and noticeable
issue, not something that might be hard/impossible to even measure.

                Linus

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

#1613546

On Thu, Mar 30, 2017 at 01:59:58PM -0700, Linus Torvalds wrote:
> On Thu, Mar 30, 2017 at 1:40 PM, Vineet Gupta
> <Vineet.Gupta1@synopsys.com> wrote:
> >
> > So it's a mix bag really. Maybe we need some better directed test to really drill
> > it down.
> 
> As mentioned inn the discussion about ARM, I seriously doubt that the
> inlining will even be noticeable compared to other effects here.

(Sorry to switch sub-threads.)

I'm running tests on that point, concentrating on hdparm -T and perfing
that.  You're right in so far as perf identifies the hotspot as the
copy_to_user() function for that workload, rather than the inlined bits
- the top hits in perf of hdparm -T are:

+   66.52%  hdparm  [k] __copy_to_user_std
+    8.49%  hdparm  [k] generic_file_read_iter
+    3.82%  hdparm  [k] lock_acquire
+    2.80%  hdparm  [k] copy_page_to_iter
+    2.49%  hdparm  [k] find_get_entry
+    1.19%  hdparm  [k] lock_release

Note: perf on ARM does is affected by IRQ-disabled regions, so hotspots
can be off.

The generic_file_read_iter() one is definitely affected by an IRQ-
disabled region in there.

Here's the average hdparm -T transfer rates and standard deviation over
20 samples:

Unpatched:        Average=320.42 MB/s sigma=0.878657
Uaccess+inline:   Average=318.77 MB/s sigma=1.003332
Uaccess+noinline: Average=319.40 MB/s sigma=1.088354

This pattern - where the noinline version sits between the inlined
version and unpatched version seems to be a pattern in all the
measurements I've done so far, and it points to inlining that code
having a slight detrimental effect.  What we don't know is whether
uninlining the code without Al's patch would see a slight boost,
but I'm not about to go there.

However, this all points towards there being a very slight advantage
to dropping the INLINE_COPY_TO_USER and INLINE_COPY_FROM_USER for
ARM, but I'd say it's really down in the noise - I'm not concerned.

> (On ARM, hopefully the UAO bit is faster to set, but it's still
> "another instruction before and after", so even if it's not as
> expensive as clac/stac are on current x86 chips, it's an argument
> against inlining)

The UAO set/clear does show up as a hotspot within copy_page_to_iter(),
but as we can see, overall its about 3% of the workload.  Within
copy_page_to_iter(), it's the __put_user() based loop inside
fault_in_pages_writeable() which has the hotspot, due to the repeated
enable+disable sequence (more the instruction barriers that we need.)

Perf reports that the barriers account for 8.33 and 17.59% of the
time spent within that function, so we're actually talking about
maybe .25% and .5% of this workload spent doing the UAO thing.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

#1613026

On Wed, 29 Mar 2017 06:57:06 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> 	The patchset currently in vfs.git#work.uaccess is the result;
> there's more work to do, but it takes care of a large part of the
> problems.  About 2.8KLoc removed, a lot of cruft is gone and semantics
> is hopefully in sync now.  All but two architectures (ia64 and metag)
> had been switched to new mechanism; for these two I'm afraid that I'll
> need serious help from maintainers.

I have tested the code in vfs.git#work.uaccess and in principle it works
for s390. I found one bug which would return an incorrect result
for copy_from_user if the access faults on the last page of the copy.
In that case the new code would return 0 instead of the remaining bytes.

This patch snippet should fix it, please just merge it into commit
"s390: get rid of zeroing, switch to RAW_COPY_USER"

diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
index b55172c..1e5bb2b 100644
--- a/arch/s390/lib/uaccess.c
+++ b/arch/s390/lib/uaccess.c
@@ -35,7 +35,7 @@ static inline unsigned long copy_from_user_mvcos(void *x, const void __user *ptr
 		"   nr    %4,%3\n"	/* %4 = (ptr + 4095) & -4096 */
 		"   slgr  %4,%1\n"
 		"   clgr  %0,%4\n"	/* copy crosses next page boundary? */
-		"   jnh   4f\n"
+		"   jnh   5f\n"
 		"3: .insn ss,0xc80000000000,0(%4,%2),0(%1),0\n"
 		"7: slgr  %0,%4\n"
 		"   j     5f\n"

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

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

#1613130

On Thu, Mar 30, 2017 at 02:32:12PM +0200, Martin Schwidefsky wrote:
> On Wed, 29 Mar 2017 06:57:06 +0100
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > 	The patchset currently in vfs.git#work.uaccess is the result;
> > there's more work to do, but it takes care of a large part of the
> > problems.  About 2.8KLoc removed, a lot of cruft is gone and semantics
> > is hopefully in sync now.  All but two architectures (ia64 and metag)
> > had been switched to new mechanism; for these two I'm afraid that I'll
> > need serious help from maintainers.
> 
> I have tested the code in vfs.git#work.uaccess and in principle it works
> for s390. I found one bug which would return an incorrect result
> for copy_from_user if the access faults on the last page of the copy.
> In that case the new code would return 0 instead of the remaining bytes.
> 
> This patch snippet should fix it, please just merge it into commit
> "s390: get rid of zeroing, switch to RAW_COPY_USER"

Done.

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

#1613269

On Wed, Mar 29, 2017 at 06:57:06AM +0100, Al Viro wrote:
> Comments, review, testing, replacement patches, etc. are very welcome.

I've given this a spin, and it appears to work (in that the box boots).

Kernel size wise:

   text    data      bss      dec     hex filename
8020229 3014220 10243276 21277725 144ac1d vmlinux.orig
8034741 3014388 10243276 21292405 144e575 vmlinux.uaccess
7976719 3014324 10243276 21234319 144028f vmlinux.noinline

Performance using hdparm -T (cached reads) to evaluate against a SSD
gives me the following results:

* original:
 Timing cached reads:   580 MB in  2.00 seconds = 289.64 MB/sec
 Timing cached reads:   580 MB in  2.00 seconds = 290.06 MB/sec
 Timing cached reads:   580 MB in  2.00 seconds = 289.65 MB/sec
 Timing cached reads:   582 MB in  2.00 seconds = 290.82 MB/sec
 Timing cached reads:   578 MB in  2.00 seconds = 289.07 MB/sec

 Average = 289.85MB/s

* uaccess:
 Timing cached reads:   578 MB in  2.00 seconds = 288.36 MB/sec
 Timing cached reads:   534 MB in  2.00 seconds = 266.68 MB/sec
 Timing cached reads:   534 MB in  2.00 seconds = 267.07 MB/sec
 Timing cached reads:   552 MB in  2.00 seconds = 275.45 MB/sec
 Timing cached reads:   532 MB in  2.00 seconds = 266.08 MB/sec

 Average = 272.73 MB/sec

* noinline:
 Timing cached reads:   548 MB in  2.00 seconds = 274.16 MB/sec
 Timing cached reads:   574 MB in  2.00 seconds = 287.19 MB/sec
 Timing cached reads:   574 MB in  2.00 seconds = 286.47 MB/sec
 Timing cached reads:   572 MB in  2.00 seconds = 286.20 MB/sec
 Timing cached reads:   578 MB in  2.00 seconds = 288.86 MB/sec

 Average = 284.58 MB/sec

I've run the test twice, and there's definitely a reproducable drop in
performance for some reason when switching between current and Al's
uaccess patches, which is partly recovered by switching to the out of
line versions.

The only difference that I can identify that could explain this are
the extra might_fault() checks in Al's version but which are missing
from the ARM version.

I'd suggest that we immediately switch to the uninlined versions on
ARM so that the impact of that change is reduced.  We end up with
a 1.9% performance reduction rather than a 6% reduction with the
inlined versions.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web