linux-next.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Heiko Carstens <heiko.carstens@de.ibm.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: Tuxist <tuxist@tuxist.de>,
	Patrick McCarthy <patrickjmc@gmail.com>,
	Andreas Schwab <schwab@linux-m68k.org>,
	Finn Thain <fthain@telegraphics.com.au>,
	Rusty Russell <rusty@rustcorp.com.au>,
	Thomas Gleixner <tglx@linutronix.de>,
	Darren Hart <dvhart@linux.intel.com>,
	Martin Schwidefsky <schwidefsky@de.ibm.com>,
	linux-next@vger.kernel.org
Subject: [BUG -next] "futex: switch to USER_DS for futex test" breaks s390
Date: Fri, 3 Jan 2014 15:19:43 +0100	[thread overview]
Message-ID: <20140103141943.GA4219@osiris> (raw)

Hi Geert,

your patch "futex: switch to USER_DS for futex test" in linux-next now causes
s390 to die on boot. As reference your patch:

    futex: switch to USER_DS for futex test
    
    Since e4f2dfbb5e92b ("m68k: implement futex.h to support userspace robust
    futexes and PI mutexes"), the kernel crashes during boot up on MC68030:
    
    Data read fault at 0x00000000 in Super Data (pc=0x3afec)
    BAD KERNEL BUSERR

[...]

     [<000020ac>] do_one_initcall+0xa4/0x144
     [<00001000>] kernel_pg_dir+0x0/0x1000
     [<00002008>] do_one_initcall+0x0/0x144
     [<002b3a56>] kernel_init_freeable+0xca/0x152
     [<002b8ca4>] futex_init+0x0/0x54
     [<001e316a>] kernel_init+0x0/0xc8
     [<001e3172>] kernel_init+0x8/0xc8
     [<001e316a>] kernel_init+0x0/0xc8
     [<000025d4>] ret_from_kernel_thread+0xc/0x14
    
    This happens because the futex test in futex_init() lacks a switch to the
    USER_DS address space, while cmpxchg_futex_value_locked() and
    futex_atomic_cmpxchg_inatomic() operate on userspace pointers (albeit NULL
    for this particular test).
    
    Fix this by switching to USER_DS before running the test, and restoring
    the old address space afterwards.

diff --git a/kernel/futex.c b/kernel/futex.c
index f6ff0191ecf7..66d23727c6ab 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -63,6 +63,7 @@
 #include <linux/sched/rt.h>
 #include <linux/hugetlb.h>
 #include <linux/freezer.h>
+#include <linux/uaccess.h>
 
 #include <asm/futex.h>
 
@@ -2733,6 +2734,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 
 static int __init futex_init(void)
 {
+	mm_segment_t fs;
	u32 curval;
	int i;
 
@@ -2746,8 +2748,11 @@ static int __init futex_init(void)
	 * implementation, the non-functional ones will return
	 * -ENOSYS.
	 */
+	fs = get_fs();
+	set_fs(USER_DS);
	if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
		futex_cmpxchg_enabled = 1;
+	set_fs(fs);
 
	for (i = 0; i < ARRAY_SIZE(futex_queues); i++) {
		plist_head_init(&futex_queues[i].chain);

Now, this seems to be wrong. It was intended to cause a fault while in kernel
space. When accessing user space current->mm must not be NULL, but it is, since
this is early code and we have no user space context to operate with.

Hence at least s390's page tables aren't setup yet to handle this correctly.
Actually our code dies when walking page tables and trying to acquire current's
mm->page_table_lock, which points to nowhere.

I'm wondering why m68k's exception handling for put/get_user doesn't fixup
the instruction pointers and these functions simply return -EFAULT?

Also m68k's futex_atomic_cmpxchg_inatomic() implementation seems to miss a
page_fault_disable()/page_fault_enable() pair.

Since this is already the second or third time this specific futex code causes
problems on s390, it would be nice if we could get rid of it. E.g. with the
following patch:

>From d3b5585ebc302a7b94edff7267f9ec7f63b57141 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Fri, 3 Jan 2014 14:16:19 +0100
Subject: [PATCH] futex: allow architectures to skip
 futex_atomic_cmpxchg_inatomic() test

If an architecture has futex_atomic_cmpxchg_inatomic() implemented and there
is no runtume check necessary if it is working allow to skip the test within
futex_init().
This allows to get rid of some code which would always give the same result.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/Kconfig     |  1 +
 include/linux/futex.h |  4 ++++
 init/Kconfig          |  6 ++++++
 kernel/futex.c        | 14 ++++++++++++--
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 1e1a03d2d19f..e00a76224537 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -117,6 +117,7 @@ config S390
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_TRACE_MCOUNT_TEST
+	select HAVE_FUTEX_CMPXCHG if FUTEX
 	select HAVE_KERNEL_BZIP2
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_LZ4
diff --git a/include/linux/futex.h b/include/linux/futex.h
index b0d95cac826e..6435f46d6e13 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -55,7 +55,11 @@ union futex_key {
 #ifdef CONFIG_FUTEX
 extern void exit_robust_list(struct task_struct *curr);
 extern void exit_pi_state_list(struct task_struct *curr);
+#ifdef CONFIG_HAVE_FUTEX_CMPXCHG
+#define futex_cmpxchg_enabled 1
+#else
 extern int futex_cmpxchg_enabled;
+#endif
 #else
 static inline void exit_robust_list(struct task_struct *curr)
 {
diff --git a/init/Kconfig b/init/Kconfig
index 8d402e33b7fc..5699a638e1ca 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1398,6 +1398,12 @@ config FUTEX
 	  support for "fast userspace mutexes".  The resulting kernel may not
 	  run glibc-based applications correctly.
 
+config HAVE_FUTEX_CMPXCHG
+	bool
+	help
+	  Architectures should select this if futex_atomic_cmpxchg_inatomic()
+	  is implemented and always working.
+
 config EPOLL
 	bool "Enable eventpoll support" if EXPERT
 	default y
diff --git a/kernel/futex.c b/kernel/futex.c
index 66d23727c6ab..7c7dfb233515 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -69,7 +69,9 @@
 
 #include "locking/rtmutex_common.h"
 
+#ifndef CONFIG_HAVE_FUTEX_CMPXCHG
 int __read_mostly futex_cmpxchg_enabled;
+#endif
 
 #define FUTEX_HASHBITS (CONFIG_BASE_SMALL ? 4 : 8)
 
@@ -2732,11 +2734,11 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
 }
 
-static int __init futex_init(void)
+static void __init futex_detect_cmpxchg(void)
 {
+#ifndef CONFIG_HAVE_FUTEX_CMPXCHG
 	mm_segment_t fs;
 	u32 curval;
-	int i;
 
 	/*
 	 * This will fail and we want it. Some arch implementations do
@@ -2753,6 +2755,14 @@ static int __init futex_init(void)
 	if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
 		futex_cmpxchg_enabled = 1;
 	set_fs(fs);
+#endif /* CONFIG_HAVE_FUTEX_CMPXCHG */
+}
+
+static int __init futex_init(void)
+{
+	int i;
+
+	futex_detect_cmpxchg();
 
 	for (i = 0; i < ARRAY_SIZE(futex_queues); i++) {
 		plist_head_init(&futex_queues[i].chain);
-- 
1.8.4.5

             reply	other threads:[~2014-01-03 14:19 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-03 14:19 Heiko Carstens [this message]
2014-01-03 14:42 ` [BUG -next] "futex: switch to USER_DS for futex test" breaks s390 Geert Uytterhoeven
2014-01-03 15:36   ` Heiko Carstens
2014-01-03 15:41     ` Andreas Schwab
2014-01-03 16:09       ` Heiko Carstens
2014-01-07  8:47         ` Heiko Carstens

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140103141943.GA4219@osiris \
    --to=heiko.carstens@de.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=dvhart@linux.intel.com \
    --cc=fthain@telegraphics.com.au \
    --cc=geert@linux-m68k.org \
    --cc=linux-next@vger.kernel.org \
    --cc=patrickjmc@gmail.com \
    --cc=rusty@rustcorp.com.au \
    --cc=schwab@linux-m68k.org \
    --cc=schwidefsky@de.ibm.com \
    --cc=tglx@linutronix.de \
    --cc=tuxist@tuxist.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).