[PATCH 1/1] futex: futex_wake_op, fix sign_extend32 sign bits

[PATCH 1/1] futex: futex_wake_op, fix sign_extend32 sign bits

[Jiri Slaby]

sign_extend32 counts the sign bit parameter from 0, not from 1. So we
have to use "11" for 12th bit, not "12".

This mistake means we have not allowed negative op and cmp args since
commit 30d6e0a4190d ("futex: Remove duplicated code and fix undefined
behaviour") till now.

Fixes: 30d6e0a4190d ("futex: Remove duplicated code and fix undefined behaviour")
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 kernel/futex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 4205ce8b8a7a..56712560fb63 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1564,8 +1564,8 @@ static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
 {
 	unsigned int op =	  (encoded_op & 0x70000000) >> 28;
 	unsigned int cmp =	  (encoded_op & 0x0f000000) >> 24;
-	int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
-	int cmparg = sign_extend32(encoded_op & 0x00000fff, 12);
+	int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 11);
+	int cmparg = sign_extend32(encoded_op & 0x00000fff, 11);
 	int oldval, ret;
 
 	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
-- 
2.15.0

Re: [PATCH 1/1] futex: futex_wake_op, fix sign_extend32 sign bits

[Linus Torvalds]

On Thu, Nov 30, 2017 at 6:35 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> sign_extend32 counts the sign bit parameter from 0, not from 1. So we
> have to use "11" for 12th bit, not "12".

This interface is crap. It really doesn't make much sense. I wonder
how many people have gotten this wrong, but it's hard to tell.

I'm applying this directly to my tree since I didn't see anybody else
react to it, but the whole pattern worries me.

Also, clearly nobody actually uses the odder corners of futex ops
anyway. Maybe we should deprecate them entirely?

Jiri, did you notice by testing, or what?

      Linus

Re: [PATCH 1/1] futex: futex_wake_op, fix sign_extend32 sign bits

[Jiri Slaby]

On 12/10/2017, 09:50 PM, Linus Torvalds wrote:
> On Thu, Nov 30, 2017 at 6:35 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>> sign_extend32 counts the sign bit parameter from 0, not from 1. So we
>> have to use "11" for 12th bit, not "12".
> 
> This interface is crap. It really doesn't make much sense. I wonder
> how many people have gotten this wrong, but it's hard to tell.

I tend to agree, because it really surprised me. So at that time I
searched for most (all?) uses of the interface, checked them and all of
them *seem* to be fine.

> I'm applying this directly to my tree since I didn't see anybody else
> react to it, but the whole pattern worries me.
> 
> Also, clearly nobody actually uses the odder corners of futex ops
> anyway. Maybe we should deprecate them entirely?
> 
> Jiri, did you notice by testing, or what?

I noticed it by coincidence while fixing the strace build test failures
-- e78c38f6bdd9 (futex: futex_wake_op, do not fail on invalid op). I
compiled a bit modified futex_atomic_op_inuser in userspace to test the
conversion and the added check and it did not work.

And yes, somebody (tglx?) noted already that this interface is old and
perhaps unused.

thanks,
-- 
js
suse labs

Re: [PATCH 1/1] futex: futex_wake_op, fix sign_extend32 sign bits

[Darren Hart]

On Mon, Dec 11, 2017 at 08:37:11AM +0100, Jiri Slaby wrote:
> On 12/10/2017, 09:50 PM, Linus Torvalds wrote:
> > On Thu, Nov 30, 2017 at 6:35 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> >> sign_extend32 counts the sign bit parameter from 0, not from 1. So we
> >> have to use "11" for 12th bit, not "12".
> > 
> > This interface is crap. It really doesn't make much sense. I wonder
> > how many people have gotten this wrong, but it's hard to tell.
> 
> I tend to agree, because it really surprised me. So at that time I
> searched for most (all?) uses of the interface, checked them and all of
> them *seem* to be fine.
> 
> > I'm applying this directly to my tree since I didn't see anybody else
> > react to it, but the whole pattern worries me.
> > 
> > Also, clearly nobody actually uses the odder corners of futex ops
> > anyway. Maybe we should deprecate them entirely?
> > 
> > Jiri, did you notice by testing, or what?
> 
> I noticed it by coincidence while fixing the strace build test failures
> -- e78c38f6bdd9 (futex: futex_wake_op, do not fail on invalid op). I
> compiled a bit modified futex_atomic_op_inuser in userspace to test the
> conversion and the added check and it did not work.
> 
> And yes, somebody (tglx?) noted already that this interface is old and
> perhaps unused.

The only use I know of for FUTEX_WAKE_OP is glibc
lll_futex_wake_unlock(). and that is limited to a single operation.

At the very least, we need to add a futex_wake_op test to the
kselftests, something that's been nagging me for a very long time.
There are some 120 different combinations of op and cmp and condition
value.

Assuming this isn't urgent, I've added it to my projects list.

-- 
Darren Hart
VMware Open Source Technology Center