[PATCH] futex: fix shared futex operations on nommu

[PATCH] futex: fix shared futex operations on nommu

[Rich Felker]

The shared get_futex_key code does not work on nommu, but is not
needed anyway because it's impossible for a given backing to have
multiple distinct virtual addresses on nommu. Simply disable these
code paths by refraining from setting FLAG_SHARED when CONFIG_MMU is
not enabled.

Signed-off-by: Rich Felker <dalias@libc.org>
---
 kernel/futex.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/futex.c b/kernel/futex.c
index a5d2e74..ed6f475 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3131,8 +3131,10 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 	int cmd = op & FUTEX_CMD_MASK;
 	unsigned int flags = 0;
 
+#ifdef CONFIG_MMU
 	if (!(op & FUTEX_PRIVATE_FLAG))
 		flags |= FLAGS_SHARED;
+#endif
 
 	if (op & FUTEX_CLOCK_REALTIME) {
 		flags |= FLAGS_CLOCKRT;
-- 
2.8.1

Re: [PATCH] futex: fix shared futex operations on nommu

[Sebastian Andrzej Siewior]

* Rich Felker | 2016-04-25 21:03:03 [-0400]:

>The shared get_futex_key code does not work on nommu, but is not
>needed anyway because it's impossible for a given backing to have
>multiple distinct virtual addresses on nommu. Simply disable these
>code paths by refraining from setting FLAG_SHARED when CONFIG_MMU is
>not enabled.

by disabling it you are not fixing it.
Why isn't it working? Why does it pop up now and not earlier?

Sebastian

Re: [PATCH] futex: fix shared futex operations on nommu

[Rich Felker]

On Tue, Apr 26, 2016 at 11:55:52AM +0200, Sebastian Andrzej Siewior wrote:
> * Rich Felker | 2016-04-25 21:03:03 [-0400]:
> 
> >The shared get_futex_key code does not work on nommu, but is not
> >needed anyway because it's impossible for a given backing to have
> >multiple distinct virtual addresses on nommu. Simply disable these
> >code paths by refraining from setting FLAG_SHARED when CONFIG_MMU is
> >not enabled.
> 
> by disabling it you are not fixing it.
> Why isn't it working? Why does it pop up now and not earlier?

The whole shared futex logic is meaningless for nommu. Perhaps I
should have written a better message, though.

With MMU, shared futex keys need to identify the physical backing for
a memory address because it may be mapped at different addresses in
different processes (or even multiple times in the same process).
Without MMU this cannot happen. You only have physical addresses. So
the "private futex" behavior of using the virtual address as the key
is always correct (for both shared and private cases) on nommu
systems.

Rich

Re: [PATCH] futex: fix shared futex operations on nommu

[Sebastian Andrzej Siewior]

* Rich Felker | 2016-04-26 11:53:44 [-0400]:

>The whole shared futex logic is meaningless for nommu. Perhaps I
>should have written a better message, though.
>
>With MMU, shared futex keys need to identify the physical backing for
>a memory address because it may be mapped at different addresses in
>different processes (or even multiple times in the same process).
>Without MMU this cannot happen. You only have physical addresses. So
>the "private futex" behavior of using the virtual address as the key
>is always correct (for both shared and private cases) on nommu
>systems.

So using a shared futex on NOMMU does work but it would be more
efficient to always use a private futex instead.
Is this what you are saying?

>Rich

Sebastian

Re: [PATCH] futex: fix shared futex operations on nommu

[Rich Felker]

On Tue, Apr 26, 2016 at 06:11:07PM +0200, Sebastian Andrzej Siewior wrote:
> * Rich Felker | 2016-04-26 11:53:44 [-0400]:
> 
> >The whole shared futex logic is meaningless for nommu. Perhaps I
> >should have written a better message, though.
> >
> >With MMU, shared futex keys need to identify the physical backing for
> >a memory address because it may be mapped at different addresses in
> >different processes (or even multiple times in the same process).
> >Without MMU this cannot happen. You only have physical addresses. So
> >the "private futex" behavior of using the virtual address as the key
> >is always correct (for both shared and private cases) on nommu
> >systems.
> 
> So using a shared futex on NOMMU does work but it would be more
> efficient to always use a private futex instead.
> Is this what you are saying?

No. What I'm saying is that the current code paths for shared futex
are mmu-specific. They neither work (due to different mm internals, I
think) nor make sense (due to lack of virtual addresses that map to
the same physical address) on nommu.

The private futex code paths are correct for either private or shared
futexes on nommu. This is both the natural theoretical prediction, and
confirmed by testing the patch.

Rich

Re: [PATCH] futex: fix shared futex operations on nommu

[Andrew Morton]

On Tue, 26 Apr 2016 12:27:39 -0400 Rich Felker <dalias@libc.org> wrote:

> On Tue, Apr 26, 2016 at 06:11:07PM +0200, Sebastian Andrzej Siewior wrote:
> > * Rich Felker | 2016-04-26 11:53:44 [-0400]:
> > 
> > >The whole shared futex logic is meaningless for nommu. Perhaps I
> > >should have written a better message, though.
> > >
> > >With MMU, shared futex keys need to identify the physical backing for
> > >a memory address because it may be mapped at different addresses in
> > >different processes (or even multiple times in the same process).
> > >Without MMU this cannot happen. You only have physical addresses. So
> > >the "private futex" behavior of using the virtual address as the key
> > >is always correct (for both shared and private cases) on nommu
> > >systems.
> > 
> > So using a shared futex on NOMMU does work but it would be more
> > efficient to always use a private futex instead.
> > Is this what you are saying?
> 
> No. What I'm saying is that the current code paths for shared futex
> are mmu-specific. They neither work (due to different mm internals, I
> think) nor make sense (due to lack of virtual addresses that map to
> the same physical address) on nommu.
> 
> The private futex code paths are correct for either private or shared
> futexes on nommu. This is both the natural theoretical prediction, and
> confirmed by testing the patch.

It is apparent from Sebastian's questioning that a code comment will be
needed, please.

Also, what specifically is the runtime effect of the patch?  Does the
futex code presently misbehave on NOMMU when FUTEX_PRIVATE_FLAG is
unset?

Re: [PATCH] futex: fix shared futex operations on nommu

[Rich Felker]

On Tue, Apr 26, 2016 at 03:27:10PM -0700, Andrew Morton wrote:
> On Tue, 26 Apr 2016 12:27:39 -0400 Rich Felker <dalias@libc.org> wrote:
> 
> > On Tue, Apr 26, 2016 at 06:11:07PM +0200, Sebastian Andrzej Siewior wrote:
> > > * Rich Felker | 2016-04-26 11:53:44 [-0400]:
> > > 
> > > >The whole shared futex logic is meaningless for nommu. Perhaps I
> > > >should have written a better message, though.
> > > >
> > > >With MMU, shared futex keys need to identify the physical backing for
> > > >a memory address because it may be mapped at different addresses in
> > > >different processes (or even multiple times in the same process).
> > > >Without MMU this cannot happen. You only have physical addresses. So
> > > >the "private futex" behavior of using the virtual address as the key
> > > >is always correct (for both shared and private cases) on nommu
> > > >systems.
> > > 
> > > So using a shared futex on NOMMU does work but it would be more
> > > efficient to always use a private futex instead.
> > > Is this what you are saying?
> > 
> > No. What I'm saying is that the current code paths for shared futex
> > are mmu-specific. They neither work (due to different mm internals, I
> > think) nor make sense (due to lack of virtual addresses that map to
> > the same physical address) on nommu.
> > 
> > The private futex code paths are correct for either private or shared
> > futexes on nommu. This is both the natural theoretical prediction, and
> > confirmed by testing the patch.
> 
> It is apparent from Sebastian's questioning that a code comment will be
> needed, please.

Indeed, I agree. I'll work on a better patch. At least this sufficed
to get discussion started.

> Also, what specifically is the runtime effect of the patch?  Does the
> futex code presently misbehave on NOMMU when FUTEX_PRIVATE_FLAG is
> unset?

Without this patch, all futex ops without FUTEX_PRIVATE_FLAG fail with
EFAULT. It's been a while since I tracked down where the EFAULT is
generated but it's somewhere in the shared get-key vm logic.

If userspace treats this as an error, the corresponding pthread, etc.
functions fail. Otherwise, userspace just spins at 100% cpu retrying
FUTEX_WAIT and FUTEX_WAKE "works" fine as a nop against such a wait,
etc. (In a sense an always-failing implementation of futex is a
working implementation for the basic ops, just a highly suboptimal
one.)

Rich

Re: [PATCH] futex: fix shared futex operations on nommu

[Thomas Gleixner]

On Mon, 25 Apr 2016, Rich Felker wrote:

> The shared get_futex_key code does not work on nommu, but is not
> needed anyway because it's impossible for a given backing to have
> multiple distinct virtual addresses on nommu. Simply disable these
> code paths by refraining from setting FLAG_SHARED when CONFIG_MMU is
> not enabled.
> 
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  kernel/futex.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index a5d2e74..ed6f475 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3131,8 +3131,10 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
>  	int cmd = op & FUTEX_CMD_MASK;
>  	unsigned int flags = 0;
>  
> +#ifdef CONFIG_MMU
>  	if (!(op & FUTEX_PRIVATE_FLAG))
>  		flags |= FLAGS_SHARED;
> +#endif

Well, that's lame. The proper solution is to disable the code which is
involved and let the compiler throw it away. Saves quite some text.

Thanks,

	tglx

8<-------------------

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -179,7 +179,15 @@ int __read_mostly futex_cmpxchg_enabled;
  * Futex flags used to encode options to functions and preserve them across
  * restarts.
  */
-#define FLAGS_SHARED		0x01
+#ifdef CONFIG_MMU
+# define FLAGS_SHARED		0x01
+#else
+/*
+ * NOMMU does not have per process address space. Let the compiler optimize
+ * code away.
+ */
+# define FLAGS_SHARED		0x00
+#endif
 #define FLAGS_CLOCKRT		0x02
 #define FLAGS_HAS_TIMEOUT	0x04
 
@@ -405,6 +413,16 @@ static void get_futex_key_refs(union fut
 	if (!key->both.ptr)
 		return;
 
+	/*
+	 * On MMU less systems futexes are always "private" as there is no per
+	 * process address space. We need the smp wmb nevertheless - yes,
+	 * arch/blackfin has MMU less SMP ...
+	 */
+	if (!IS_ENABLED(CONFIG_MMU)) {
+		smp_mb(); /* explicit smp_mb(); (B) */
+		return;
+	}
+
 	switch (key->both.offset & (FUT_OFF_INODE|FUT_OFF_MMSHARED)) {
 	case FUT_OFF_INODE:
 		ihold(key->shared.inode); /* implies smp_mb(); (B) */
@@ -436,6 +454,9 @@ static void drop_futex_key_refs(union fu
 		return;
 	}
 
+	if (!IS_ENABLED(CONFIG_MMU))
+		return;
+
 	switch (key->both.offset & (FUT_OFF_INODE|FUT_OFF_MMSHARED)) {
 	case FUT_OFF_INODE:
 		iput(key->shared.inode);