From: Hugh Dickins <[email protected]>
Since commit
d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
we use get_user_pages_unlocked() to pre-faulting user's memory if a
write generates a page fault while the handler is disabled.
This works in general and uncovered a bug as reported by Mike Rapoport.
It has been pointed out that this function may be fragile and a
simple pre-fault as in fault_in_pages_writeable() would be a better
solution. Better as in taste and simplicity: That write (as performed by
the alternative function) performs exactly the same faulting of memory
that we had before. This was suggested by Hugh Dickins and Andrew
Morton.
Use fault_in_pages_writeable() for pre-faulting of user's stack.
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: Hugh Dickins <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
[bigeasy: patch description]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
arch/x86/kernel/fpu/signal.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 5a8d118bc423e..060d6188b4533 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -5,6 +5,7 @@
#include <linux/compat.h>
#include <linux/cpu.h>
+#include <linux/pagemap.h>
#include <asm/fpu/internal.h>
#include <asm/fpu/signal.h>
@@ -189,15 +190,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
fpregs_unlock();
if (ret) {
- int aligned_size;
- int nr_pages;
-
- aligned_size = offset_in_page(buf_fx) + fpu_user_xstate_size;
- nr_pages = DIV_ROUND_UP(aligned_size, PAGE_SIZE);
-
- ret = get_user_pages_unlocked((unsigned long)buf_fx, nr_pages,
- NULL, FOLL_WRITE);
- if (ret == nr_pages)
+ if (!fault_in_pages_writeable(buf_fx, fpu_user_xstate_size))
goto retry;
return -EFAULT;
}
--
2.20.1
On 2019-05-26 19:33:25 [+0200], To Hugh Dickins wrote:
> From: Hugh Dickins <[email protected]>
…
> Signed-off-by: Hugh Dickins <[email protected]>
Hugh, I took your patch, slapped a signed-off-by line. Please say that
you are fine with it (or object otherwise).
Sebastian
On Sun, 26 May 2019, Sebastian Andrzej Siewior wrote:
> On 2019-05-26 19:33:25 [+0200], To Hugh Dickins wrote:
> From: Hugh Dickins <[email protected]>
> …
> > Signed-off-by: Hugh Dickins <[email protected]>
>
> Hugh, I took your patch, slapped a signed-off-by line. Please say that
> you are fine with it (or object otherwise).
I'm fine with it, thanks Sebastian. Sorry if I wasted your time by not
giving it my sign-off in the first place, but I was not comfortable to
dabble there without your sign-off too - which it now has. (And thought
you might already have your own version anyway: just provided mine as
illustration, so that we could be sure of exactly what I'd been testing.)
Hugh
Hi!
On Sun 2019-05-26 12:25:27, Hugh Dickins wrote:
> On Sun, 26 May 2019, Sebastian Andrzej Siewior wrote:
> > On 2019-05-26 19:33:25 [+0200], To Hugh Dickins wrote:
> > From: Hugh Dickins <[email protected]>
> > …
> > > Signed-off-by: Hugh Dickins <[email protected]>
> >
> > Hugh, I took your patch, slapped a signed-off-by line. Please say that
> > you are fine with it (or object otherwise).
>
> I'm fine with it, thanks Sebastian. Sorry if I wasted your time by not
> giving it my sign-off in the first place, but I was not comfortable to
> dabble there without your sign-off too - which it now has. (And thought
> you might already have your own version anyway: just provided mine as
> illustration, so that we could be sure of exactly what I'd been testing.)
I applied Hugh's patch on top of -rc2, but still get emacs problems:
But this time I'm not sure if it is same emacs problem or different
emacs problem....
X protocol error: BadValue (integer parameter out of range for
operation) on protocol request 139
When compiled with GTK, Emacs cannot recover from X disconnects.
This is a GTK bug: https://bugzilla.gnome.org/show_bug.cgi?id=85715
For details, see etc/PROBLEMS.
(emacs:8175): GLib-WARNING **: g_main_context_prepare() called
recursively from within a source's check() or prepare() member.
(emacs:8175): GLib-WARNING **: g_main_context_check() called
recursively from within a source's check() or prepare() member.
Fatal error 6: Aborted
Backtrace:
emacs[0x8138719]
emacs[0x8120446]
emacs[0x813875c]
emacs[0x80f54c0]
emacs[0x80f6f3f]
emacs[0x80f6fab]
/usr/lib/i386-linux-gnu/libX11.so.6(_XError+0x11a)[0xf6ea1b3a]
/usr/lib/i386-linux-gnu/libX11.so.6(+0x39b5b)[0xf6e9eb5b]
/usr/lib/i386-linux-gnu/libX11.so.6(+0x39c26)[0xf6e9ec26]
/usr/lib/i386-linux-gnu/libX11.so.6(_XEventsQueued+0x6e)[0xf6e9f4be]
/usr/lib/i386-linux-gnu/libX11.so.6(XPending+0x62)[0xf6e90752]
/usr/lib/i386-linux-gnu/libgdk-3.so.0(+0x48073)[0xf7566073]
/lib/i386-linux-gnu/libglib-2.0.so.0(g_main_context_prepare+0x17b)[0xf70244fb]
/lib/i386-linux-gnu/libglib-2.0.so.0(+0x46f74)[0xf7024f74]
/lib/i386-linux-gnu/libglib-2.0.so.0(g_main_context_pending+0x34)[0xf7025144]
/usr/lib/i386-linux-gnu/libgtk-3.so.0(gtk_events_pending+0x1f)[0xf77c9a8f]
emacs[0x80f55a9]
emacs[0x812714f]
emacs[0x8126a95]
emacs[0x8172db9]
emacs[0x8192bd7]
emacs[0x819312d]
emacs[0x8125634]
emacs[0x8125c6d]
emacs[0x812725b]
emacs[0x8129eaa]
emacs[0x81c7c90]
emacs[0x8127815]
emacs[0x812ada3]
emacs[0x812bdad]
emacs[0x812d838]
emacs[0x818b76c]
emacs[0x8120890]
emacs[0x818b66b]
emacs[0x8124b84]
emacs[0x8124e3f]
emacs[0x8059cb0]
/lib/i386-linux-gnu/i686/cmov/libc.so.6(__libc_start_main+0xf3)[0xf61a7a63]
emacs[0x805a76f]
Aborted (core dumped)
Best regards,
Pavel
commit 018c9da72adf920efd0ba250fcf433b836d3cfbc
Author: Hugh Dickins <[email protected]>
Date: Sun May 26 19:33:25 2019 +0200
x86/fpu: Use fault_in_pages_writeable() for pre-faulting
Since commit
d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
we use get_user_pages_unlocked() to pre-faulting user's memory if a
write generates a page fault while the handler is disabled.
This works in general and uncovered a bug as reported by Mike Rapoport.
It has been pointed out that this function may be fragile and a
simple pre-fault as in fault_in_pages_writeable() would be a better
solution. Better as in taste and simplicity: That write (as performed by
the alternative function) performs exactly the same faulting of memory
that we had before. This was suggested by Hugh Dickins and Andrew
Morton.
Use fault_in_pages_writeable() for pre-faulting of user's stack.
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: Hugh Dickins <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
[bigeasy: patch description]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 5a8d118..060d618 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -5,6 +5,7 @@
#include <linux/compat.h>
#include <linux/cpu.h>
+#include <linux/pagemap.h>
#include <asm/fpu/internal.h>
#include <asm/fpu/signal.h>
@@ -189,15 +190,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
fpregs_unlock();
if (ret) {
- int aligned_size;
- int nr_pages;
-
- aligned_size = offset_in_page(buf_fx) + fpu_user_xstate_size;
- nr_pages = DIV_ROUND_UP(aligned_size, PAGE_SIZE);
-
- ret = get_user_pages_unlocked((unsigned long)buf_fx, nr_pages,
- NULL, FOLL_WRITE);
- if (ret == nr_pages)
+ if (!fault_in_pages_writeable(buf_fx, fpu_user_xstate_size))
goto retry;
return -EFAULT;
}
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, 26 May 2019 19:33:25 +0200 Sebastian Andrzej Siewior <[email protected]> wrote:
> From: Hugh Dickins <[email protected]>
>
> Since commit
>
> d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
Please add this as a
Fixes: d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
line so that anyone who backports d9c9ce34ed5c8 has a chance of finding
this patch also.
From: Hugh Dickins <[email protected]>
Since commit
d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
we use get_user_pages_unlocked() to pre-faulting user's memory if a
write generates a pagefault while the handler is disabled.
This works in general and uncovered a bug as reported by Mike Rapoport.
It has been pointed out that this function may be fragile and a
simple pre-fault as in fault_in_pages_writeable() would be a better
solution. Better as in taste and simplicity: That write (as performed by
the alternative function) performs exactly the same faulting of memory
that we had before. This was suggested by Hugh Dickins and Andrew
Morton.
Use fault_in_pages_writeable() for pre-faulting of user's stack.
Fixes: d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: Hugh Dickins <[email protected]>
[bigeasy: patch description]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
v1…v2: Added a Fixes tag.
arch/x86/kernel/fpu/signal.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 5a8d118bc423e..060d6188b4533 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -5,6 +5,7 @@
#include <linux/compat.h>
#include <linux/cpu.h>
+#include <linux/pagemap.h>
#include <asm/fpu/internal.h>
#include <asm/fpu/signal.h>
@@ -189,15 +190,7 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
fpregs_unlock();
if (ret) {
- int aligned_size;
- int nr_pages;
-
- aligned_size = offset_in_page(buf_fx) + fpu_user_xstate_size;
- nr_pages = DIV_ROUND_UP(aligned_size, PAGE_SIZE);
-
- ret = get_user_pages_unlocked((unsigned long)buf_fx, nr_pages,
- NULL, FOLL_WRITE);
- if (ret == nr_pages)
+ if (!fault_in_pages_writeable(buf_fx, fpu_user_xstate_size))
goto retry;
return -EFAULT;
}
--
2.20.1
Quoting Sebastian Andrzej Siewior (2019-05-29 08:25:40)
> From: Hugh Dickins <[email protected]>
>
> Since commit
>
> d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
>
> we use get_user_pages_unlocked() to pre-faulting user's memory if a
> write generates a pagefault while the handler is disabled.
> This works in general and uncovered a bug as reported by Mike Rapoport.
> It has been pointed out that this function may be fragile and a
> simple pre-fault as in fault_in_pages_writeable() would be a better
> solution. Better as in taste and simplicity: That write (as performed by
> the alternative function) performs exactly the same faulting of memory
> that we had before. This was suggested by Hugh Dickins and Andrew
> Morton.
>
> Use fault_in_pages_writeable() for pre-faulting of user's stack.
>
> Fixes: d9c9ce34ed5c8 ("x86/fpu: Fault-in user stack if copy_fpstate_to_sigframe() fails")
> Suggested-by: Andrew Morton <[email protected]>
> Signed-off-by: Hugh Dickins <[email protected]>
> [bigeasy: patch description]
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
I am able to reliably hit the bug here by putting the system under
mempressure, and afterwards processes would die as the exit. This patch
also greatly reduces cycletest latencies while under that mempressure,
~320ms -> ~16ms (on a bxt while also spinning on i915.ko).
Tested-by: Chris Wilson <[email protected]>
-Chris