Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1611600 > unrolled thread
| Started by | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| First post | 2017-03-29 08:00 +0200 |
| Last post | 2017-04-07 02:40 +0200 |
| Articles | 20 on this page of 40 — 10 participants |
Back to article view | Back to linux.kernel
[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 2 of 2 — ← Prev page 1 [2]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-03-30 18:50 +0200 |
| Message-ID | <tqLqN-KI-7@gated-at.bofh.it> |
| In reply to | #1613269 |
On Thu, Mar 30, 2017 at 05:22:41PM +0100, Russell King - ARM Linux wrote:
> 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.
How would the following affect things?
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index e68604ae3ced..d24d338f0682 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -184,7 +184,7 @@ static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t b
kaddr = kmap(page);
from = kaddr + offset;
- left = __copy_to_user(buf, from, copy);
+ left = __copy_to_user_inatomic(buf, from, copy);
copy -= left;
skip += copy;
from += copy;
@@ -193,7 +193,7 @@ static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t b
iov++;
buf = iov->iov_base;
copy = min(bytes, iov->iov_len);
- left = __copy_to_user(buf, from, copy);
+ left = __copy_to_user_inatomic(buf, from, copy);
copy -= left;
skip = copy;
from += copy;
@@ -267,7 +267,7 @@ static size_t copy_page_from_iter_iovec(struct page *page, size_t offset, size_t
kaddr = kmap(page);
to = kaddr + offset;
- left = __copy_from_user(to, buf, copy);
+ left = __copy_from_user_inatomic(to, buf, copy);
copy -= left;
skip += copy;
to += copy;
@@ -276,7 +276,7 @@ static size_t copy_page_from_iter_iovec(struct page *page, size_t offset, size_t
iov++;
buf = iov->iov_base;
copy = min(bytes, iov->iov_len);
- left = __copy_from_user(to, buf, copy);
+ left = __copy_from_user_inatomic(to, buf, copy);
copy -= left;
skip = copy;
to += copy;
@@ -541,7 +541,7 @@ size_t copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
if (unlikely(i->type & ITER_PIPE))
return copy_pipe_to_iter(addr, bytes, i);
iterate_and_advance(i, bytes, v,
- __copy_to_user(v.iov_base, (from += v.iov_len) - v.iov_len,
+ __copy_to_user_inatomic(v.iov_base, (from += v.iov_len) - v.iov_len,
v.iov_len),
memcpy_to_page(v.bv_page, v.bv_offset,
(from += v.bv_len) - v.bv_len, v.bv_len),
@@ -560,7 +560,7 @@ size_t copy_from_iter(void *addr, size_t bytes, struct iov_iter *i)
return 0;
}
iterate_and_advance(i, bytes, v,
- __copy_from_user((to += v.iov_len) - v.iov_len, v.iov_base,
+ __copy_from_user_inatomic((to += v.iov_len) - v.iov_len, v.iov_base,
v.iov_len),
memcpy_from_page((to += v.bv_len) - v.bv_len, v.bv_page,
v.bv_offset, v.bv_len),
@@ -582,7 +582,7 @@ bool copy_from_iter_full(void *addr, size_t bytes, struct iov_iter *i)
return false;
iterate_all_kinds(i, bytes, v, ({
- if (__copy_from_user((to += v.iov_len) - v.iov_len,
+ if (__copy_from_user_inatomic((to += v.iov_len) - v.iov_len,
v.iov_base, v.iov_len))
return false;
0;}),
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-03-30 19:20 +0200 |
| Message-ID | <tqLTQ-1a6-7@gated-at.bofh.it> |
| In reply to | #1613289 |
On Thu, Mar 30, 2017 at 9:43 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Mar 30, 2017 at 05:22:41PM +0100, Russell King - ARM Linux wrote:
> How would the following affect things?
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index e68604ae3ced..d24d338f0682 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -184,7 +184,7 @@ static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t b
>
> kaddr = kmap(page);
> from = kaddr + offset;
> - left = __copy_to_user(buf, from, copy);
> + left = __copy_to_user_inatomic(buf, from, copy);
This is all going in the wrong direction entirely.
That "__copy_to_user()" code was bad from the beginning: it should
never have had the double underscores. I objected to it at the time.
Now you're making it go from bad to insane. You're apparently
mis-using "inatomic" because of subtle issues that have nothing to do
with "inatomic" - you want to get rid of a might_sleep() warning, but
you don't actuially want inatomic behavior, so the thing will still
sleep.
This all very subtle already depends on people having checked the
"struct iov_iter" beforehand. We should *remove* subtle stuff like
that, not add yet more layers of subtlety and possible future bugs
when somebody calls copy_page_to_iter() without having properly
validated the iter.
These are not theoretical issues. We've _had_ these exact bugs when
people didn't validate the stuff they created by hand and bypassed the
normal IO paths.
Trying to optimize away an access_ok() or a might_fault() is *not* a
valid reason to completely break our security model, and create code
that makes no sense (claiming it is atomic when it isn't).
Linus
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-03-30 20:50 +0200 |
| Message-ID | <tqNiW-27G-13@gated-at.bofh.it> |
| In reply to | #1613306 |
On Thu, Mar 30, 2017 at 10:18:23AM -0700, Linus Torvalds wrote: > This is all going in the wrong direction entirely. This is not going into the tree - it's just a "let's check your theory about might_fault() overhead being the source of slowdown you are seeing" quick-and-dirty patch. Speaking of the checks in there - if anything, might_fault() in those suckers belongs outside of the loop; note that even on the kmap_atomic() side of copy_page_to_iter_iovec() we do stuff like fault_in_pages_writeable(). BTW, ..._inatomic is a very unfortunate name, IMO - it's *not* safe to use in atomic contexts as-is, to start with; the caller needs to take care of pagefault_disable(). If anything, __copy_from_user_nofault() would probably be better... I really wonder about the low dispersion in those tests - IME on amd64 boxen it tends to be ~5% or so; what's normal for arm?
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-03-30 21:00 +0200 |
| Message-ID | <tqNsB-2b6-1@gated-at.bofh.it> |
| In reply to | #1613363 |
On Thu, Mar 30, 2017 at 11:48 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> This is not going into the tree - it's just a "let's check your
> theory about might_fault() overhead being the source of slowdown
> you are seeing" quick-and-dirty patch.
Note that for cached hdparm reads, I suspect a *much* bigger effects
than the fairly cheap might_fault() tests is just the random layout of
the data in the page cache.
Memory is just more expensive than CPU is.
The precise physical address that gets allocated for the page cache
entries ends up mattering, and is obviously fairly "sticky" within one
reboot (unless you have a huge working set and that flushes it, or you
use something like
echo 3 > /proc/sys/vm/drop_caches
to flush filesystem caches manually).
The reason things like page allocation matter for performance testing
is simply that the CPU caches are physically indexed (the L1 might not
be, but outer levels definitely are), and so page allocation ends up
impacting caching unless you have very high associativity.
And even if your workload doesn't fit in your CPU caches (I'd hope
that the "cached" hdparm is still doing a fairly big area), you'll
still see memory performance depend on physical addresses.
Doing kernel performance testing without rebooting several times is
generally very hard.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-03-30 21:00 +0200 |
| Message-ID | <tqNsC-2b6-13@gated-at.bofh.it> |
| In reply to | #1613363 |
On Thu, Mar 30, 2017 at 07:48:24PM +0100, Al Viro wrote: > BTW, ..._inatomic is a very unfortunate name, IMO - it's *not* safe > to use in atomic contexts as-is, to start with; the caller needs to take > care of pagefault_disable(). If anything, __copy_from_user_nofault() would > probably be better... Not even that - again, it will happily trigger page faults unless the caller disables those. __copy_from_user_I_know_what_I_am_doing()?
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-03-30 21:00 +0200 |
| Message-ID | <tqNsC-2b6-17@gated-at.bofh.it> |
| In reply to | #1613370 |
On Thu, Mar 30, 2017 at 11:54 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Not even that - again, it will happily trigger page faults unless the
> caller disables those. __copy_from_user_I_know_what_I_am_doing()?
That's a horrible name. Everybody always thinks they know what they are doing.
There's a reason I called the new odd user access functions
"unsafe_get/put_user()"
But regardless of that, I think you're being silly to even look at the
iovec code. That code simply *isn't* critical enough that one or two
extra instructions matter.
Show me profiles to the contrary. I dare you.
Those things shouldn't be using *anything* odd at all. They should be
using "copy_from_user()". Nothing else.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-03-30 21:20 +0200 |
| Message-ID | <tqNLX-2xj-1@gated-at.bofh.it> |
| In reply to | #1613376 |
On Thu, Mar 30, 2017 at 11:59:16AM -0700, Linus Torvalds wrote: > But regardless of that, I think you're being silly to even look at the > iovec code. That code simply *isn't* critical enough that one or two > extra instructions matter. > > Show me profiles to the contrary. I dare you. > > Those things shouldn't be using *anything* odd at all. They should be > using "copy_from_user()". Nothing else. That they very definitely should not. And not because of access_ok() or might_fault() - this is one place where zero-padding is absolutely wrong. So unless you are going to take it out of copy_from_user() and pray that random shit ioctls in random shit drivers check the return value properly, copy_from_user() is no-go here.
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-03-30 21:30 +0200 |
| Message-ID | <tqNVD-2Cg-9@gated-at.bofh.it> |
| In reply to | #1613382 |
On Thu, Mar 30, 2017 at 12:10 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> That they very definitely should not. And not because of access_ok() or
> might_fault() - this is one place where zero-padding is absolutely wrong.
> So unless you are going to take it out of copy_from_user() and pray
> that random shit ioctls in random shit drivers check the return value
> properly, copy_from_user() is no-go here.
Actually, that is a great example of why you should *not* use
__copy_from_user().
If the reason is lack of zero-padding, that doesn't mean that suddenly
we shouldn't check the range. And it doesn't mean that it shouldn't
document why it does it.
So dammit, just add something like this to lib/iovec.c:
static inline unsigned long copy_from_user_nozero(void *to, const
void __user *from, size_t len)
{
if (!access_ok(from, len))
return len;
return __copy_from_user(to, from, len);
}
which now isn't insecure, and also magically documents *why* you don't
just use the plain copy_from_user().
Linus
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-03-30 23:20 +0200 |
| Message-ID | <tqPE5-3Oq-3@gated-at.bofh.it> |
| In reply to | #1613390 |
On Thu, Mar 30, 2017 at 12:19:35PM -0700, Linus Torvalds wrote:
> On Thu, Mar 30, 2017 at 12:10 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > That they very definitely should not. And not because of access_ok() or
> > might_fault() - this is one place where zero-padding is absolutely wrong.
> > So unless you are going to take it out of copy_from_user() and pray
> > that random shit ioctls in random shit drivers check the return value
> > properly, copy_from_user() is no-go here.
>
> Actually, that is a great example of why you should *not* use
> __copy_from_user().
>
> If the reason is lack of zero-padding, that doesn't mean that suddenly
> we shouldn't check the range. And it doesn't mean that it shouldn't
> document why it does it.
>
> So dammit, just add something like this to lib/iovec.c:
>
> static inline unsigned long copy_from_user_nozero(void *to, const
> void __user *from, size_t len)
> {
> if (!access_ok(from, len))
> return len;
> return __copy_from_user(to, from, len);
> }
>
> which now isn't insecure, and also magically documents *why* you don't
> just use the plain copy_from_user().
Maybe... However, we *do* have places where it's done under kmap_atomic()
in there. Let's leave that one until this round of uaccess consolidation is
finished, OK? lib/iov_iter.c is special and isolated enough; we can figure
out what to do with those primitives later.
As far as I'm concerned, lib/*.c and mm/*.c are separate story; I would start
with getting rid of that stuff in random drivers. Here's what we have at the
moment:
there are only 3 irregular callers of __copy_to_user_inatomic():
arch/mips/kernel/unaligned.c:1276: res = __copy_to_user_inatomic(addr, fpr, sizeof(*fpr));
drivers/gpu/drm/i915/i915_gem.c:913: ret = __copy_to_user_inatomic(user_data, vaddr + offset, length);
drivers/gpu/drm/i915/i915_gem.c:983: unwritten = __copy_to_user_inatomic(user_data, vaddr + offset, length);
There are 32 irregular callers of __copy_from_user_inatomic(), majority in
perf/oprofile-related code. Leave those aside, only 8 are left:
arch/mips/kernel/unaligned.c:1242: res = __copy_from_user_inatomic(fpr, addr,
drivers/gpu/drm/i915/i915_gem.c:1324: ret = __copy_from_user_inatomic(vaddr + offset, user_data, len);
drivers/gpu/drm/i915/i915_gem_execbuffer.c:669: unwritten = __copy_from_user_inatomic(r, user_relocs, count*sizeo
f(r[0]));
drivers/gpu/drm/msm/msm_gem_submit.c:73: return __copy_from_user_inatomic(to, from, n);
kernel/trace/trace.c:5780: len = __copy_from_user_inatomic(&entry->buf, ubuf, cnt);
kernel/trace/trace.c:5851: len = __copy_from_user_inatomic(&entry->id, ubuf, cnt);
kernel/trace/trace_kprobe.c:216: ret = __copy_from_user_inatomic(&c, (u8 *)addr + len, 1);
virt/kvm/kvm_main.c:1832: r = __copy_from_user_inatomic(data, (void __user *)addr + offset, len);
Ones in perf and oprofile code really smell like a missing helper,
along the lines of probe_kernel_read(), but for userland pointers.
Incidentally, metag, mips, openrisc and xtensa instances of that lack
pagefault_disable() - might be a bug, need to check that. powerpc and
sparc ones also lack it, but those have pagefault_disable() done in
caller. tile ones open-code access_ok(), AFAICS. Sorting that pile
out would already about half the amount of callers.
Ho-hum... There's something odd about those - some of them seem to
assume that we are under set_fs(USER_DS), some do what access_ok()
would've done with USER_DS and proceed to __copy_from_user_inatomic().
And that includes the ones like sparc... Very strange.
Am I right assuming that perf_callchain_user() can't be called other than
with USER_DS, but oprofile ->backtrace() can? I'm not familiar enough
with oprofile guts... Folks?
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-03-31 02:30 +0200 |
| Message-ID | <tqSBX-5Fe-1@gated-at.bofh.it> |
| In reply to | #1611600 |
On Tue, Mar 28, 2017 at 10:57 PM, Al Viro <viro@zeniv.linux.org.uk> 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.
FWIW, I tested this on x86 and ARM with the LKDTM tests I built for
CONFIG_HARDENED_USERCOPY and this branch (which includes the earlier
fixes I suggested privately) tests fine for me.
> 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
> [...]
> 8) __copy_in_user(). Basically, copy_in_user() sans access_ok().
> Biarch-only, with the grand total of 6 callers...
It seems to me like everything above here should end up in comments
for these functions. I think even after the unification, it's valuable
to have this actually in the source.
> 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.
Bikeshed: I still prefer that the "raw_copy_*" functions be named
"arch_copy_*" or "__arch_copy_*" to match all the other arch-specific
functions in the kernel. This clearly marks them as arch-specific, and
in theory, the leading "__" would indicate that they're "internal" or
hint that they don't perform any of the checking done from the
standard interface functions.
Currently arm64 already uses the name __arch_copy_*, and arm's is
arm_copy_*. I just don't think "raw" is meaningful enough to avoid
people accidentally using it.
> * 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).
Yay! :)
> 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.
I would also call out lib/test_user_copy.c (CONFIG_TEST_USER_COPY) for
maintainers to see if things are working correctly. This tries to test
all the size-specific combinations of possible copies and checks for
zeroing, etc. (I'm sure the test could be improved, but it's already
caught tiny bugs in per-arch implementations in the past.)
> 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.
Thanks for working on this! I've wanted to see this done for a long
time; I'm glad you had the time for it!
-Kees
--
Kees Cook
Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | James Hogan <james.hogan@imgtec.com> |
|---|---|
| Date | 2017-03-31 15:40 +0200 |
| Message-ID | <tr4Wv-5cZ-31@gated-at.bofh.it> |
| In reply to | #1613561 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, Mar 30, 2017 at 05:21:32PM -0700, Kees Cook wrote: > On Tue, Mar 28, 2017 at 10:57 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > 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). > > Yay! :) In the mean time should ARCH_HAS_RAW_COPY_USER select HAVE_ARCH_HARDENED_USERCOPY? FWIW I already have patches for metag to enable RAW_COPY_USER that just need some more testing (mainly due to prerequisite user copy fixes), but I don't know about ia64 Cheers James
[toc] | [prev] | [next] | [standalone]
| From | James Morse <james.morse@arm.com> |
|---|---|
| Date | 2017-04-03 18:30 +0200 |
| Message-ID | <tsd1E-OH-17@gated-at.bofh.it> |
| In reply to | #1611600 |
On 29/03/17 06:57, Al Viro wrote: > 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>), > Comments, review, testing, replacement patches, etc. are very welcome. For the two "arm64: " patches: Reviewed-by: James Morse <james.morse@arm.com> Thanks, James
[toc] | [prev] | [next] | [standalone]
| From | Max Filippov <jcmvbkbc@gmail.com> |
|---|---|
| Date | 2017-04-04 22:30 +0200 |
| Message-ID | <tsDfs-1js-11@gated-at.bofh.it> |
| In reply to | #1611600 |
On Wed, Mar 29, 2017 at 06:57:06AM +0100, Al Viro wrote:
> 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.
For the xtensa part:
Tested-by: Max Filippov <jcmvbkbc@gmail.com>
I believe that the xtensa part needs the following correction:
---8<---
From 4505d69c3514fb12405409a7943e45831d037960 Mon Sep 17 00:00:00 2001
From: Max Filippov <jcmvbkbc@gmail.com>
Date: Tue, 4 Apr 2017 13:20:34 -0700
Subject: [PATCH] xtensa: fix prefetch in the raw_copy_to_user
'from' is the input buffer, it should be prefetched with prefetch, not
prefetchw.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
arch/xtensa/include/asm/uaccess.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/xtensa/include/asm/uaccess.h b/arch/xtensa/include/asm/uaccess.h
index 8e93ed8..2e7bac0 100644
--- a/arch/xtensa/include/asm/uaccess.h
+++ b/arch/xtensa/include/asm/uaccess.h
@@ -245,7 +245,7 @@ raw_copy_from_user(void *to, const void __user *from, unsigned long n)
static inline unsigned long
raw_copy_to_user(void __user *to, const void *from, unsigned long n)
{
- prefetchw(from);
+ prefetch(from);
return __xtensa_copy_user((__force void *)to, from, n);
}
#define INLINE_COPY_FROM_USER
---8<---
--
Thanks.
-- Max
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-04-04 23:00 +0200 |
| Message-ID | <tsDIu-1v1-5@gated-at.bofh.it> |
| In reply to | #1616371 |
On Tue, Apr 04, 2017 at 01:26:29PM -0700, Max Filippov wrote:
> On Wed, Mar 29, 2017 at 06:57:06AM +0100, Al Viro wrote:
> > 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.
>
> For the xtensa part:
> Tested-by: Max Filippov <jcmvbkbc@gmail.com>
>
> I believe that the xtensa part needs the following correction:
Applied.
> ---8<---
> >From 4505d69c3514fb12405409a7943e45831d037960 Mon Sep 17 00:00:00 2001
> From: Max Filippov <jcmvbkbc@gmail.com>
> Date: Tue, 4 Apr 2017 13:20:34 -0700
> Subject: [PATCH] xtensa: fix prefetch in the raw_copy_to_user
>
> 'from' is the input buffer, it should be prefetched with prefetch, not
> prefetchw.
>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
> arch/xtensa/include/asm/uaccess.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/xtensa/include/asm/uaccess.h b/arch/xtensa/include/asm/uaccess.h
> index 8e93ed8..2e7bac0 100644
> --- a/arch/xtensa/include/asm/uaccess.h
> +++ b/arch/xtensa/include/asm/uaccess.h
> @@ -245,7 +245,7 @@ raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> static inline unsigned long
> raw_copy_to_user(void __user *to, const void *from, unsigned long n)
> {
> - prefetchw(from);
> + prefetch(from);
> return __xtensa_copy_user((__force void *)to, from, n);
> }
> #define INLINE_COPY_FROM_USER
> ---8<---
>
> --
> Thanks.
> -- Max
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-04-05 07:10 +0200 |
| Subject | ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) |
| Message-ID | <tsLmF-6Gn-5@gated-at.bofh.it> |
| In reply to | #1611600 |
On Wed, Mar 29, 2017 at 06:57:06AM +0100, Al Viro wrote:
> 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.
Speaking of ia64: copy_user.S contains the following oddity:
2:
EX(.failure_in3,(p16) ld8 val1[0]=[src1],16)
(p16) ld8 val2[0]=[src2],16
src1 is 16-byte aligned, src2 is src1 + 8.
What guarantees that we can't race with e.g. TLB shootdown from a thread on
another CPU, ending up with the second insn taking a fault and oopsing?
AFAICS, other places where we have such pairs of loads or stores (e.g.
EX(.ex_handler, (p16) ld8 r34=[src0],16)
EK(.ex_handler, (p16) ld8 r38=[src1],16)
in the memcpy_mck.S counterpart of that code) both have exception table
entries associated with them.
Is that one intentional and correct for some subtle reason, or is it a very
narrow race on the hardware nobody gives a damn anymore? It is pre-mckinley
stuff, after all...
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-04-05 10:10 +0200 |
| Subject | Re: ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) |
| Message-ID | <tsOaS-5c-27@gated-at.bofh.it> |
| In reply to | #1616572 |
On Wed, Apr 05, 2017 at 06:05:08AM +0100, Al Viro wrote:
> Speaking of ia64: copy_user.S contains the following oddity:
> 2:
> EX(.failure_in3,(p16) ld8 val1[0]=[src1],16)
> (p16) ld8 val2[0]=[src2],16
>
> src1 is 16-byte aligned, src2 is src1 + 8.
>
> What guarantees that we can't race with e.g. TLB shootdown from a thread on
> another CPU, ending up with the second insn taking a fault and oopsing?
>
> AFAICS, other places where we have such pairs of loads or stores (e.g.
> EX(.ex_handler, (p16) ld8 r34=[src0],16)
> EK(.ex_handler, (p16) ld8 r38=[src1],16)
> in the memcpy_mck.S counterpart of that code) both have exception table
> entries associated with them.
>
> Is that one intentional and correct for some subtle reason, or is it a very
> narrow race on the hardware nobody gives a damn anymore? It is pre-mckinley
> stuff, after all...
Actually, the piece immediately after that one is worse. By that point,
we have
* checked that len is large enough to be worth bothering with word
copies. Fine.
* checked that src and dst have the same remainder modulo 8.
* copied until src is a multiple of 16, incrementing src and dst
by the same amount.
* prepared for copying in multiples of 16 bytes
* set src2 and dst2 8 bytes past src1 and dst1 resp.
and now we have a pipelined loop with
EX(.failure_in3,(p16) ld8 val1[0]=[src1],16)
(p16) ld8 val2[0]=[src2],16
EX(.failure_out, (EPI) st8 [dst1]=val1[PIPE_DEPTH-1],16)
(EPI) st8 [dst2]=val2[PIPE_DEPTH-1],16
for body. Now, consider the following case:
* to is 8 bytes before the end of user page, next page is unmapped
* from is at the beginning of kernel page
* len is simply PAGE_SIZE
and we call copy_to_user(). All the preparation work won't read or write
anything - all alignments are fine. src1 and src2 are kernel page and
kernel page + 8 resp.; dst1 is 8 bytes before the end of user page, dst2
is at the beginning of unmapped user page. No loads are going to fail;
the first store into dst1 won't fail either. The *second* store - one to
dst2 will not just fail, it'll oops.
<goes to test>
... and sure enough, on generic kernel (CONFIG_ITANIUM) that yields a nice
shiny oops at precisely that insn.
We really need tests for uaccess primitives. That's not a recent regression,
BTW - it had been that way since 2.3.48-pre2, as far as I can see.
[toc] | [prev] | [next] | [standalone]
| From | Tony Luck <tony.luck@gmail.com> |
|---|---|
| Date | 2017-04-05 20:50 +0200 |
| Subject | Re: ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) |
| Message-ID | <tsYad-6g1-9@gated-at.bofh.it> |
| In reply to | #1616674 |
On Wed, Apr 5, 2017 at 1:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > ... and sure enough, on generic kernel (CONFIG_ITANIUM) that yields a nice > shiny oops at precisely that insn. The right fix here might be to delete all the CONFIG_ITANIUM paths. I doubt that anyone is still running upstream kernels on Merced CPUs (and if they are, it might be a kindness to them to make them stop). > We really need tests for uaccess primitives. That's not a recent regression, > BTW - it had been that way since 2.3.48-pre2, as far as I can see. Probably be handy for new architectures to test all the corner cases. -Tony
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-04-05 22:40 +0200 |
| Subject | Re: ia64 exceptions (Re: [RFC][CFT][PATCHSET v1] uaccess unification) |
| Message-ID | <tsZSF-7nH-5@gated-at.bofh.it> |
| In reply to | #1617279 |
On Wed, Apr 05, 2017 at 11:44:23AM -0700, Tony Luck wrote:
> On Wed, Apr 5, 2017 at 1:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > ... and sure enough, on generic kernel (CONFIG_ITANIUM) that yields a nice
> > shiny oops at precisely that insn.
>
> The right fix here might be to delete all the CONFIG_ITANIUM paths. I
> doubt that anyone is still running upstream kernels on Merced CPUs
> (and if they are, it might be a kindness to them to make them stop).
Frankly, I would be surprised if it turned out that more Merced boxen are
running the current kernels than there had been 386 and 486DLC ones doing
the same in 2012. Granted, the latter bunch had been much older by that
point, but comparing the total amounts sold...
> > We really need tests for uaccess primitives. That's not a recent regression,
> > BTW - it had been that way since 2.3.48-pre2, as far as I can see.
>
> Probably be handy for new architectures to test all the corner cases.
I wouldn't be too optimistic about the existing ones, to be honest. Bitrot
happens, and slight modifications of exception table handling, etc. can
bugger some cases without anyone noticing.
FWIW, I'm running fairly exhaustive tests for mckinley __copy_user()
(after removing zero-padding part), giving it 0..4096 bytes available
until fault, asking to copy 0..4096 bytes and running it with all possible
(unsigned long)to % 16). The outermost loop is by number of bytes available,
so far got through 351 iterations, no problems found yet. Takes about 7s
per outer loop iteration; that'll go a bit slower as the distance to fault
increases, but not dramatically so - scaffolding includes 8Kb memcmp and a pair
of 8Kb memsets per combination, so the growing cost of __copy_user() (and
matching memcpy()) shouldn't increase it too much. That's just user-to-kernel
side, though...
For the record, the tests being run are as below (c_f_u() is a renamed
copy of __copy_user() with zero-padding taken out):
#define pr_fmt(fmt) "cfu test: %s " fmt, __func__
#include <linux/slab.h>
#include <linux/uaccess.h>
#include <linux/module.h>
extern unsigned long c_f_u(void *to, const void __user *from, unsigned long n);
static char pat[PAGE_SIZE];
static char cmp[PAGE_SIZE * 2];
static char *kp;
static char __user *up;
static int run_test(int avail, int asked, int off)
{
int copied;
char *p;
memset(kp, 1, 2 * PAGE_SIZE);
memset(cmp, 1, 2 * PAGE_SIZE);
copied = asked - c_f_u(kp + off, up + PAGE_SIZE - avail, asked);
if (copied < 0 || copied > asked) {
pr_err("impossible return value: %d not between 0 and %d\n",
copied, asked);
return -1;
}
if (avail && asked && !copied) {
pr_err("no progess (%d available, %d asked, nothing copied)\n",
avail, asked);
return -10;
}
if (asked <= avail && copied < asked) {
pr_err("bogus fault (%d available, %d asked, %d copied)\n",
avail, asked, copied);
return -2;
}
if (copied > avail) {
pr_err("claims to have copied %d with only %d avaialable\n",
copied, avail);
return -3;
}
memcpy(cmp + off, pat + PAGE_SIZE - avail, copied);
if (likely(!memcmp(kp, cmp, 2 * PAGE_SIZE)))
return 0;
if (memcmp(kp, cmp, off)) {
pr_err("modified memory _below_ 'to' (%d, %d, %d => %d)\n",
off, avail, asked, copied);
return -4;
}
if (memcmp(kp + off, cmp + off, copied)) {
char *p;
pr_err("crap in copy (%d, %d, %d => %d)",
off, avail, asked, copied);
p = memchr(kp + off, 1, copied);
if (p) {
int n = p - (kp + off);
memset(cmp + off + n, 1, copied - n);
if (!memcmp(kp, cmp, 2 * PAGE_SIZE)) {
pr_cont(" only %d copied\n", n);
return -5;
}
}
pr_cont("\n");
return -6;
}
/* must be after the copy... */
p = kp + off + copied;
if (!*p) {
int i, n;
n = 2 * PAGE_SIZE - off - copied;
for (i = 0; i < n && !p[i]; i++)
;
pr_err("crap after copy (%d, %d, %d => %d)",
off, avail, asked, copied);
pr_cont(" padded with %d zeroes\n", i);
return 0;
}
pr_err("crap after copy (%d, %d, %d => %d)\n",
off, avail, asked, copied);
return -8;
}
static int __init cfu_test(void)
{
int i;
kp = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
if (!kp)
return -EAGAIN;
up = (char __user *)vm_mmap(NULL, 0, 2 * PAGE_SIZE,
PROT_READ | PROT_WRITE | PROT_EXEC,
MAP_ANONYMOUS | MAP_PRIVATE, 0);
if (IS_ERR(up)) {
pr_err("Failed to allocate user memory\n");
kfree(kp);
return -EAGAIN;
}
vm_munmap((unsigned long)up + PAGE_SIZE, PAGE_SIZE);
for (i = 0; i < PAGE_SIZE; i++)
pat[i] = 128 | i;
if (copy_to_user(up, pat, PAGE_SIZE)) {
pr_err("failed to copy to user memory\n");
goto out;
}
for (i = 0; i <= 4096; i++) {
int j;
pr_err("trying %d\n", i);
for (j = 0; j <= 4096; j++) {
int k;
for (k = 0; k < 16; k++) {
if (run_test(i, j, k) < 0)
break;
}
}
}
out:
vm_munmap((unsigned long)up, PAGE_SIZE);
kfree(kp);
return -EAGAIN;
}
module_init(cfu_test);
MODULE_LICENSE("GPL");
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-04-07 02:30 +0200 |
| Subject | [RFC][CFT][PATCHSET v2] uaccess unification |
| Message-ID | <ttpWN-87i-5@gated-at.bofh.it> |
| In reply to | #1611600 |
Updates since v1:
* metag conversion (based on fixes from James Hogan) added. Result
tested by the aforementioned metag maintainer.
* xtensa fix added, result tested.
* arm, arm64, amd64 tested.
* s390 fix folded, result tested.
* arc fix added, result tested.
* parisc fix replaced with backmerge of the variant in mainline,
result tested.
* ia64 conversion for CONFIG_MCKINLEY added; appears to work.
CONFIG_ITANIUM *not* converted; the current mainline has all kinds
of bugs in that config, including a user-triggerable oops with one
hell of a DoS potential. That one needs to be fixed in -stable, at least
to the point where it wouldn't allow any user to leave the box in a state
when any lookup in /tmp hangs unkillably, but as for the mainline...
Frankly, I suspect that we have fewer Merced boxen running mainline
kernels now than we had 386 and 486DLC ones doing the same five years ago,
when CONFIG_M386 finally got killed. IOW, maybe it's time to put it
out of its misery.
* backmerges of mainline fixes (on ia64, mips, powerpc and parisc
branches) added.
* conversion made unconditional
* HAVE_ARCH_HARDENED_USERCOPY removed (universally true now)
* no object size checks remain in arch/*
* ibmvnet bugs spotted and fixed; that'll get fed into net-next
ASAP.
* balance is at -3KLoC now (OK, -2984LoC)
* the thing is included into #for-next.
The series lives in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
in #work.uaccess. It's still based at 4.11-rc1 and topology is unchanged,
except for backmerges into arch branches instead of cherry-picking the mainline
fixes into them + a couple of followup commits after the place where branches
converge (making stuff unconditional). Infrastructure part hadn't been
rebased or modified in any way since the previous version; if you are OK
with your architecture branch (uaccess.<arch>) you can say so and it'll
be put into never-rebased mode as well, making it safe to pull into your
tree. Alternatively, if you want to cherry-pick stuff from that branch,
just put it into never-rebased branch in your tree and tell me to pull
it.
As before, comments, review, testing, replacement patches, etc. are very
welcome. Folks, if you don't yell, it will get pushed come next cycle.
I don't believe that 104-piece mailbomb with total size at 0.5 megabyte is
a good idea for public lists, but if somebody wants one, just say so.
Or just use git...
FWIW, the current stats are:
Al Viro (100):
uaccess: move VERIFY_{READ,WRITE} definitions to linux/uaccess.h
uaccess: drop duplicate includes from asm/uaccess.h
uaccess: drop pointless ifdefs
add asm-generic/extable.h
new helper: uaccess_kernel()
asm-generic/uaccess.h: don't mess with __copy_{to,from}_user
asm-generic: zero in __get_user(), not __get_user_fn()
generic ...copy_..._user primitives
alpha: switch __copy_user() and __do_clean_user() to normal calling conventions
alpha: add asm/extable.h
alpha: get rid of 'segment' argument of __{get,put}_user_check()
alpha: don't bother with __access_ok() in traps.c
alpha: kill the 'segment' argument of __access_ok()
alpha: add a helper for emitting exception table entries
alpha: switch to RAW_COPY_USER
arc: get rid of unused declaration
arm: switch to generic extable.h
arm: switch to RAW_COPY_USER
arm64: add extable.h
avr32: switch to generic extable.h
arm64: switch to RAW_COPY_USER
avr32: switch to RAW_COPY_USER
blackfin: switch to generic extable.h
bfin: switch to RAW_COPY_USER
c6x: remove duplicate definition of __access_ok
c6x: switch to RAW_COPY_USER
cris: switch to generic extable.h
cris: don't rely upon __copy_user_zeroing() zeroing the tail
cris: get rid of zeroing in __asm_copy_from_user_N for N > 4
cris: get rid of zeroing
cris: rename __copy_user_zeroing to __copy_user_in
cris: switch to RAW_COPY_USER
frv: switch to use of fixup_exception()
frv: switch to RAW_COPY_USER
8300: switch to RAW_COPY_USER
hexagon: switch to RAW_COPY_USER
m32r: switch to generic extable.h
m32r: get rid of zeroing
m68k: switch to generic extable.h
m68k: get rid of zeroing
m68k: switch to RAW_COPY_USER
metag: switch to generic extable.h
metag: kill verify_area()
microblaze: switch to generic extable.h
microblaze: switch to RAW_COPY_USER
mn10300: switch to generic extable.h
mn10300: get rid of zeroing
mn10300: switch to RAW_COPY_USER
nios2: switch to generic extable.h
nios2: switch to RAW_COPY_USER
openrisc: switch to generic extable.h
openrisc: switch to RAW_COPY_USER
powerpc: switch to extable.h
s390: switch to extable.h
score: switch to generic extable.h
score: it's "VERIFY_WRITE", not "VERFITY_WRITE"...
score: switch to RAW_COPY_USER
sh: switch to extable.h
sh: switch to RAW_COPY_USER
sparc32: kill __ret_efault()
tile: switch to generic extable.h
tile: get rid of zeroing, switch to RAW_COPY_USER
um: switch to RAW_COPY_USER
amd64: get rid of zeroing
unicore32: get rid of zeroing and switch to RAW_COPY_USER
kill __copy_from_user_nocache()
xtensa: switch to generic extable.h
xtensa: get rid of zeroing, use RAW_COPY_USER
arc: switch to RAW_COPY_USER
m32r: switch to RAW_COPY_USER
x86: don't wank with magical size in __copy_in_user()
x86: switch to RAW_COPY_USER
s390: get rid of zeroing, switch to RAW_COPY_USER
Merge branch 'parisc-4.11-3' of git://git.kernel.org/.../deller/parisc-linux into uaccess.parisc
parisc: switch to RAW_COPY_USER
sparc: switch to RAW_COPY_USER
Merge branch 'fixes' of git://git.kernel.org/.../jhogan/metag into uaccess.metag
Merge commit 'fc69910f329d' into uaccess.mips
mips: sanitize __access_ok()
mips: consolidate __invoke_... wrappers
mips: clean and reorder the forest of macros...
mips: make copy_from_user() zero tail explicitly
mips: get rid of tail-zeroing in primitives
mips: switch to RAW_COPY_USER
don't open-code kernel_setsockopt()
alpha: fix stack smashing in old_adjtimex(2)
esas2r: don't open-code memdup_user()
ibmvnic: fix kstrtoul, copy_from_user and copy_to_user misuse
Merge commit 'a7d2475af7aedcb9b5c6343989a8bfadbf84429b' into uaccess.powerpc
powerpc: get rid of zeroing, switch to RAW_COPY_USER
Merge commit 'b4fb8f66f1ae2e167d06c12d018025a8d4d3ba7e' into uaccess.ia64
ia64: add extable.h
ia64: get rid of 'segment' argument of __{get,put}_user_check()
ia64: get rid of 'segment' argument of __do_{get,put}_user()
ia64: sanitize __access_ok()
ia64: get rid of copy_in_user()
get rid of padding, switch to RAW_COPY_USER
Merge branches 'uaccess.alpha', 'uaccess.arc', 'uaccess.arm', 'uaccess.arm64', 'uaccess.avr32', 'uaccess.bfin', 'uaccess.c6x', 'uaccess.cris', 'uaccess.frv', 'uaccess.h8300', 'uaccess.hexagon', 'uaccess.ia64', 'uaccess.m32r', 'uaccess.m68k', 'uaccess.metag', 'uaccess.microblaze', 'uaccess.mips', 'uaccess.mn10300', 'uaccess.nios2', 'uaccess.openrisc', 'uaccess.parisc', 'uaccess.powerpc', 'uaccess.s390', 'uaccess.score', 'uaccess.sh', 'uaccess.sparc', 'uaccess.tile', 'uaccess.um', 'uaccess.unicore32', 'uaccess.x86' and 'uaccess.xtensa' into work.uaccess
CONFIG_ARCH_HAS_RAW_COPY_USER is unconditional now
HAVE_ARCH_HARDENED_USERCOPY is unconditional now
James Hogan (8):
metag/usercopy: Drop unused macros
metag/usercopy: Fix alignment error checking
metag/usercopy: Add early abort to copy_to_user
metag/usercopy: Zero rest of buffer from copy_from_user
metag/usercopy: Set flags before ADDZ
metag/usercopy: Fix src fixup in from user rapf loops
metag/usercopy: Add missing fixups
metag/usercopy: Switch to RAW_COPY_USER
Max Filippov (1):
xtensa: fix prefetch in the raw_copy_to_user
Vineet Gupta (1):
ARC: uaccess: enable INLINE_COPY_{TO,FROM}_USER ...
arch/alpha/include/asm/extable.h | 55 ++++
arch/alpha/include/asm/futex.h | 16 +-
arch/alpha/include/asm/uaccess.h | 305 +++++---------------
arch/alpha/kernel/osf_sys.c | 2 +-
arch/alpha/kernel/traps.c | 152 +++-------
arch/alpha/lib/clear_user.S | 66 ++---
arch/alpha/lib/copy_user.S | 82 +++---
arch/alpha/lib/csum_partial_copy.c | 10 +-
arch/alpha/lib/ev6-clear_user.S | 84 +++---
arch/alpha/lib/ev6-copy_user.S | 104 +++----
arch/arc/include/asm/Kbuild | 1 +
arch/arc/include/asm/uaccess.h | 25 +-
arch/arc/mm/extable.c | 14 -
arch/arm/Kconfig | 1 -
arch/arm/include/asm/Kbuild | 1 +
arch/arm/include/asm/uaccess.h | 87 ++----
arch/arm/lib/uaccess_with_memcpy.c | 4 +-
arch/arm64/Kconfig | 1 -
arch/arm64/include/asm/extable.h | 25 ++
arch/arm64/include/asm/uaccess.h | 83 +-----
arch/arm64/kernel/arm64ksyms.c | 2 +-
arch/arm64/lib/copy_in_user.S | 4 +-
arch/avr32/include/asm/Kbuild | 1 +
arch/avr32/include/asm/uaccess.h | 39 +--
arch/avr32/kernel/avr32_ksyms.c | 2 -
arch/avr32/lib/copy_user.S | 15 -
arch/blackfin/include/asm/Kbuild | 1 +
arch/blackfin/include/asm/uaccess.h | 47 +---
arch/blackfin/kernel/process.c | 2 +-
arch/c6x/include/asm/Kbuild | 1 +
arch/c6x/include/asm/uaccess.h | 19 +-
arch/c6x/kernel/sys_c6x.c | 2 +-
arch/cris/arch-v10/lib/usercopy.c | 31 +--
arch/cris/arch-v32/lib/usercopy.c | 30 +-
arch/cris/include/arch-v10/arch/uaccess.h | 46 ++-
arch/cris/include/arch-v32/arch/uaccess.h | 54 ++--
arch/cris/include/asm/Kbuild | 1 +
arch/cris/include/asm/uaccess.h | 77 +----
arch/frv/include/asm/Kbuild | 1 +
arch/frv/include/asm/uaccess.h | 84 ++----
arch/frv/kernel/traps.c | 7 +-
arch/frv/mm/extable.c | 27 +-
arch/frv/mm/fault.c | 6 +-
arch/h8300/include/asm/Kbuild | 2 +-
arch/h8300/include/asm/uaccess.h | 54 ++++
arch/hexagon/include/asm/Kbuild | 1 +
arch/hexagon/include/asm/uaccess.h | 18 +-
arch/hexagon/kernel/hexagon_ksyms.c | 4 +-
arch/hexagon/mm/copy_from_user.S | 2 +-
arch/hexagon/mm/copy_to_user.S | 2 +-
arch/ia64/Kconfig | 1 -
arch/ia64/include/asm/extable.h | 11 +
arch/ia64/include/asm/uaccess.h | 102 ++-----
arch/ia64/lib/memcpy_mck.S | 13 +-
arch/ia64/mm/extable.c | 5 +-
arch/m32r/include/asm/Kbuild | 1 +
arch/m32r/include/asm/uaccess.h | 189 +------------
arch/m32r/kernel/m32r_ksyms.c | 2 -
arch/m32r/lib/usercopy.c | 21 --
arch/m68k/include/asm/Kbuild | 1 +
arch/m68k/include/asm/processor.h | 10 -
arch/m68k/include/asm/uaccess.h | 1 +
arch/m68k/include/asm/uaccess_mm.h | 103 ++++---
arch/m68k/include/asm/uaccess_no.h | 43 +--
arch/m68k/kernel/signal.c | 2 +-
arch/m68k/kernel/traps.c | 9 +-
arch/m68k/lib/uaccess.c | 12 +-
arch/m68k/mm/fault.c | 2 +-
arch/metag/include/asm/Kbuild | 1 +
arch/metag/include/asm/uaccess.h | 63 +----
arch/metag/lib/usercopy.c | 318 ++++++++-------------
arch/microblaze/include/asm/Kbuild | 1 +
arch/microblaze/include/asm/uaccess.h | 62 +----
arch/mips/Kconfig | 1 -
arch/mips/cavium-octeon/octeon-memcpy.S | 31 +--
arch/mips/include/asm/checksum.h | 4 +-
arch/mips/include/asm/r4kcache.h | 4 +-
arch/mips/include/asm/uaccess.h | 449 ++++--------------------------
arch/mips/kernel/mips-r2-to-r6-emul.c | 24 +-
arch/mips/kernel/syscall.c | 2 +-
arch/mips/kernel/unaligned.c | 10 +-
arch/mips/lib/memcpy.S | 49 ----
arch/mips/oprofile/backtrace.c | 2 +-
arch/mn10300/include/asm/Kbuild | 1 +
arch/mn10300/include/asm/uaccess.h | 187 +------------
arch/mn10300/kernel/mn10300_ksyms.c | 2 -
arch/mn10300/lib/usercopy.c | 18 --
arch/nios2/include/asm/Kbuild | 1 +
arch/nios2/include/asm/uaccess.h | 55 +---
arch/nios2/mm/uaccess.c | 16 +-
arch/openrisc/include/asm/Kbuild | 1 +
arch/openrisc/include/asm/uaccess.h | 53 +---
arch/parisc/Kconfig | 1 -
arch/parisc/include/asm/futex.h | 2 +-
arch/parisc/include/asm/uaccess.h | 69 +----
arch/parisc/lib/memcpy.c | 16 +-
arch/powerpc/Kconfig | 1 -
arch/powerpc/include/asm/extable.h | 29 ++
arch/powerpc/include/asm/uaccess.h | 96 +------
arch/powerpc/lib/Makefile | 2 +-
arch/powerpc/lib/copy_32.S | 14 -
arch/powerpc/lib/copyuser_64.S | 35 +--
arch/powerpc/lib/usercopy_64.c | 41 ---
arch/s390/Kconfig | 1 -
arch/s390/include/asm/extable.h | 28 ++
arch/s390/include/asm/uaccess.h | 153 +---------
arch/s390/lib/uaccess.c | 68 ++---
arch/score/include/asm/Kbuild | 1 +
arch/score/include/asm/extable.h | 11 -
arch/score/include/asm/uaccess.h | 59 +---
arch/sh/include/asm/extable.h | 10 +
arch/sh/include/asm/uaccess.h | 64 +----
arch/sparc/Kconfig | 1 -
arch/sparc/include/asm/uaccess.h | 2 +-
arch/sparc/include/asm/uaccess_32.h | 44 +--
arch/sparc/include/asm/uaccess_64.h | 44 +--
arch/sparc/kernel/head_32.S | 7 -
arch/sparc/lib/GENcopy_from_user.S | 2 +-
arch/sparc/lib/GENcopy_to_user.S | 2 +-
arch/sparc/lib/GENpatch.S | 4 +-
arch/sparc/lib/NG2copy_from_user.S | 2 +-
arch/sparc/lib/NG2copy_to_user.S | 2 +-
arch/sparc/lib/NG2patch.S | 4 +-
arch/sparc/lib/NG4copy_from_user.S | 2 +-
arch/sparc/lib/NG4copy_to_user.S | 2 +-
arch/sparc/lib/NG4patch.S | 4 +-
arch/sparc/lib/NGcopy_from_user.S | 2 +-
arch/sparc/lib/NGcopy_to_user.S | 2 +-
arch/sparc/lib/NGpatch.S | 4 +-
arch/sparc/lib/U1copy_from_user.S | 4 +-
arch/sparc/lib/U1copy_to_user.S | 4 +-
arch/sparc/lib/U3copy_to_user.S | 2 +-
arch/sparc/lib/U3patch.S | 4 +-
arch/sparc/lib/copy_in_user.S | 6 +-
arch/sparc/lib/copy_user.S | 16 +-
arch/tile/include/asm/Kbuild | 1 +
arch/tile/include/asm/uaccess.h | 166 +----------
arch/tile/lib/exports.c | 7 +-
arch/tile/lib/memcpy_32.S | 41 +--
arch/tile/lib/memcpy_user_64.c | 15 +-
arch/um/include/asm/Kbuild | 1 +
arch/um/include/asm/uaccess.h | 13 +-
arch/um/kernel/skas/uaccess.c | 18 +-
arch/unicore32/include/asm/Kbuild | 1 +
arch/unicore32/include/asm/uaccess.h | 15 +-
arch/unicore32/kernel/ksyms.c | 4 +-
arch/unicore32/kernel/process.c | 2 +-
arch/unicore32/lib/copy_from_user.S | 16 +-
arch/unicore32/lib/copy_to_user.S | 6 +-
arch/x86/Kconfig | 1 -
arch/x86/include/asm/uaccess.h | 70 +----
arch/x86/include/asm/uaccess_32.h | 127 +--------
arch/x86/include/asm/uaccess_64.h | 128 +--------
arch/x86/lib/usercopy.c | 54 +---
arch/x86/lib/usercopy_32.c | 288 +------------------
arch/x86/lib/usercopy_64.c | 13 -
arch/xtensa/include/asm/Kbuild | 1 +
arch/xtensa/include/asm/asm-uaccess.h | 3 -
arch/xtensa/include/asm/uaccess.h | 67 +----
arch/xtensa/lib/usercopy.S | 116 ++++----
block/bsg.c | 2 +-
drivers/net/ethernet/ibm/ibmvnic.c | 100 +++----
drivers/scsi/esas2r/esas2r_ioctl.c | 25 +-
drivers/scsi/sg.c | 2 +-
fs/ocfs2/cluster/tcp.c | 25 +-
include/asm-generic/extable.h | 26 ++
include/asm-generic/uaccess.h | 135 +--------
include/linux/uaccess.h | 197 ++++++++++++-
include/rdma/ib.h | 2 +-
kernel/trace/bpf_trace.c | 2 +-
lib/Makefile | 2 +-
lib/iov_iter.c | 6 +-
lib/usercopy.c | 26 ++
mm/memory.c | 2 +-
net/rds/tcp.c | 5 +-
net/rds/tcp_send.c | 8 +-
security/Kconfig | 9 -
security/tomoyo/network.c | 2 +-
178 files changed, 1608 insertions(+), 4592 deletions(-)
create mode 100644 arch/alpha/include/asm/extable.h
create mode 100644 arch/arm64/include/asm/extable.h
create mode 100644 arch/h8300/include/asm/uaccess.h
create mode 100644 arch/ia64/include/asm/extable.h
create mode 100644 arch/powerpc/include/asm/extable.h
delete mode 100644 arch/powerpc/lib/usercopy_64.c
create mode 100644 arch/s390/include/asm/extable.h
delete mode 100644 arch/score/include/asm/extable.h
create mode 100644 arch/sh/include/asm/extable.h
create mode 100644 include/asm-generic/extable.h
create mode 100644 lib/usercopy.c
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2017-04-07 02:40 +0200 |
| Subject | Re: [RFC][CFT][PATCHSET v2] uaccess unification |
| Message-ID | <ttq6t-8ax-3@gated-at.bofh.it> |
| In reply to | #1618422 |
On Fri, Apr 07, 2017 at 01:24:24AM +0100, Al Viro wrote: > * ibmvnet bugs spotted and fixed; that'll get fed into net-next > ASAP. ... except that net-next had them fixed in much better way - by removing the crap in question completely. Commit dropped.
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web