futex: Bugfixes and FUTEX_LOCK_PI2

PATCH Applied Thomas Gleixner 7 participants 22 messages Apr 22, 2021 – Jun 23, 2021

Subsystems (2)

Stable branch Futex

lore

Share & tools

lore

Thomas Gleixner

author

2021-04-22 20:29 UTC #0 lore raw

cover futex: Bugfixes and FUTEX_LOCK_PI2

The following series started off looking into supporting selectable clocks for FUTEX_LOCK_PI which is hardcoded to CLOCK_REALTIME and cannot be changed.

On the way I found two bugs related to the timeout handling:

- The allowance for FUTEX_WAIT to use FUTEX_CLOCK_REALTIME is broken and never worked.

- The recent time namespace support wreckaged FUTEX_LOCK_PI timeouts when the task belongs to a namespace which has an CLOCK_MONOTONIC offset.

Both should have been caught by that Gleixner dude when merging them, but it seems he's getting old.

Not having a selectable clock for PI futexes is inconsistent because all other interfaces have it. Unfortunately this was figured out by glibc folks quite some time ago, but nobody told us :(

The nasty hack to support it would be to treat FUTEX_CLOCK_REALTIME inverse for FUTEX_LOCK_PI, but that's a horrible idea. Adding a new flag to the futex op, i.e. FUTEX_CLOCK_MONOTONIC would be possible, but that's yet another variant which makes is harder for libraries to have a consistent clock selection handling.

So I went the way to let FUTEX_LOCK_PI alone and to add FUTEX_LOCK_PI2 which handles the clocks the same way as the other operands.

Thoughts?

The series is also available from git:

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git futex

Thanks,

tglx

---
 include/uapi/linux/futex.h |    1 
 kernel/futex.c             |   89 +++++++++++++++++++++++----------------------
 2 files changed, 47 insertions(+), 43 deletions(-)

Thomas Gleixner

author

2021-04-22 20:29 UTC #1 lore raw

PATCH 1/6 Revert 337f13046ff0 ("futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op")

The FUTEX_WAIT operand has historically a relative timeout which means that the clock id is irrelevant as relative timeouts on CLOCK_REALTIME are not subject to wall clock changes and therefore are mapped by the kernel to CLOCK_MONOTONIC for simplicity.

If a caller would set FUTEX_CLOCK_REALTIME for FUTEX_WAIT the timeout is still treated relative vs. CLOCK_MONOTONIC and then the wait arms that timeout based on CLOCK_REALTIME which is broken and obviously has never been used or even tested.

Reject any attempt to use FUTEX_CLOCK_REALTIME with FUTEX_WAIT again.

The desired functionality can be achieved with FUTEX_WAIT_BITSET and a FUTEX_BITSET_MATCH_ANY argument.

Fixes 337f13046ff0 ("futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op")

Signed-off-by Thomas Gleixner <[email protected]>

Cc [email protected]

Cc Darren Hart <[email protected]>

---
 kernel/futex.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

kernel/futex.c +1 -2

@@ -3711,8 +3711,7 @@ long do_futex(u32 __user *uaddr, int op,
 
 	if (op & FUTEX_CLOCK_REALTIME) {
 		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT && cmd != FUTEX_WAIT_BITSET && \
-		    cmd != FUTEX_WAIT_REQUEUE_PI)
+		if (cmd != FUTEX_WAIT_BITSET &&	cmd != FUTEX_WAIT_REQUEUE_PI)
 			return -ENOSYS;
 	}

tip-bot2 for Thomas Gleixner

merge bot Applied

2021-05-06 18:14 UTC #2 lore raw

[tip: locking/urgent] Revert 337f13046ff0 ("futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op")

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     4fbf5d6837bf81fd7a27d771358f4ee6c4f243f8
Gitweb:        https://git.kernel.org/tip/4fbf5d6837bf81fd7a27d771358f4ee6c4f243f8
Author:        Thomas Gleixner <[email protected]>
AuthorDate:    Thu, 22 Apr 2021 21:44:18 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Thu, 06 May 2021 20:12:40 +02:00

Revert 337f13046ff0 ("futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op")

Reject any attempt to use FUTEX_CLOCK_REALTIME with FUTEX_WAIT again.

The desired functionality can be achieved with FUTEX_WAIT_BITSET and a FUTEX_BITSET_MATCH_ANY argument.

Fixes 337f13046ff0 ("futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op")

Signed-off-by Thomas Gleixner <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

Cc [email protected]

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/futex.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

kernel/futex.c +1 -2

index c98b825..4740200 100644
@@ -3710,8 +3710,7 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 
 	if (op & FUTEX_CLOCK_REALTIME) {
 		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT && cmd != FUTEX_WAIT_BITSET && \
-		    cmd != FUTEX_WAIT_REQUEUE_PI)
+		if (cmd != FUTEX_WAIT_BITSET &&	cmd != FUTEX_WAIT_REQUEUE_PI)
 			return -ENOSYS;
 	}

Thomas Gleixner

author

2021-04-22 20:29 UTC #3 lore raw

PATCH 2/6 futex: Do not apply time namespace adjustment on FUTEX_LOCK_PI

In reply to Thomas Gleixner

FUTEX_LOCK_PI does not require to have the FUTEX_CLOCK_REALTIME bit set because it has been using CLOCK_REALTIME based absolute timeouts forever. Due to that, the time namespace adjustment which is applied when FUTEX_CLOCK_REALTIME is not set, will wrongly take place for FUTEX_LOCK_PI and wreckage the timeout.

Exclude it from that procedure.

Fixes c2f7d08cccf4 ("futex: Adjust absolute futex timeouts with per time namespace offset")

Signed-off-by Thomas Gleixner <[email protected]>

Cc Andrei Vagin <[email protected]>

Cc [email protected]

---
 kernel/futex.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/futex.c +2 -2

@@ -3781,7 +3781,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uad
 		t = timespec64_to_ktime(ts);
 		if (cmd == FUTEX_WAIT)
 			t = ktime_add_safe(ktime_get(), t);
-		else if (!(op & FUTEX_CLOCK_REALTIME))
+		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}

@@ -3975,7 +3975,7 @@ SYSCALL_DEFINE6(futex_time32, u32 __user
 		t = timespec64_to_ktime(ts);
 		if (cmd == FUTEX_WAIT)
 			t = ktime_add_safe(ktime_get(), t);
-		else if (!(op & FUTEX_CLOCK_REALTIME))
+		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}

tip-bot2 for Thomas Gleixner

merge bot Applied

2021-05-06 18:14 UTC #4 lore raw

[tip: locking/urgent] futex: Do not apply time namespace adjustment on FUTEX_LOCK_PI

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     cdf78db4070967869e4d027c11f4dd825d8f815a
Gitweb:        https://git.kernel.org/tip/cdf78db4070967869e4d027c11f4dd825d8f815a
Author:        Thomas Gleixner <[email protected]>
AuthorDate:    Thu, 22 Apr 2021 21:44:19 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Thu, 06 May 2021 20:12:40 +02:00

futex: Do not apply time namespace adjustment on FUTEX_LOCK_PI

Exclude it from that procedure.

Fixes c2f7d08cccf4 ("futex: Adjust absolute futex timeouts with per time namespace offset")

Signed-off-by Thomas Gleixner <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

Cc [email protected]

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/futex.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/futex.c +2 -2

index 4740200..b0f5304 100644
@@ -3780,7 +3780,7 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 		t = timespec64_to_ktime(ts);
 		if (cmd == FUTEX_WAIT)
 			t = ktime_add_safe(ktime_get(), t);
-		else if (!(op & FUTEX_CLOCK_REALTIME))
+		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}

@@ -3974,7 +3974,7 @@ SYSCALL_DEFINE6(futex_time32, u32 __user *, uaddr, int, op, u32, val,
 		t = timespec64_to_ktime(ts);
 		if (cmd == FUTEX_WAIT)
 			t = ktime_add_safe(ktime_get(), t);
-		else if (!(op & FUTEX_CLOCK_REALTIME))
+		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}

Thomas Gleixner

author

2021-04-22 20:29 UTC #5 lore raw

PATCH 3/6 futex: Get rid of the val2 conditional dance

In reply to Thomas Gleixner

There is no point in checking which FUTEX operand treats the utime pointer as 'val2' argument because that argument to do_futex() is only used by exactly these operands.

So just handing it in unconditionally is not making any difference, but removes a lot of pointless gunk.

Signed-off-by Thomas Gleixner <[email protected]>

---
 kernel/futex.c |   16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

kernel/futex.c +2 ~~-14~~

@@ -3765,7 +3765,6 @@ SYSCALL_DEFINE6(futex, u32 __user *, uad
 {
 	struct timespec64 ts;
 	ktime_t t, *tp = NULL;
-	u32 val2 = 0;
 	int cmd = op & FUTEX_CMD_MASK;
 
 	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||

@@ -3785,15 +3784,8 @@ SYSCALL_DEFINE6(futex, u32 __user *, uad
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}
-	/*
-	 * requeue parameter in 'utime' if cmd == FUTEX_*_REQUEUE_*.
-	 * number of waiters to wake in 'utime' if cmd == FUTEX_WAKE_OP.
-	 */
-	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
-	    cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP)
-		val2 = (u32) (unsigned long) utime;
 
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
 }
 
 #ifdef CONFIG_COMPAT

@@ -3961,7 +3953,6 @@ SYSCALL_DEFINE6(futex_time32, u32 __user
 {
 	struct timespec64 ts;
 	ktime_t t, *tp = NULL;
-	int val2 = 0;
 	int cmd = op & FUTEX_CMD_MASK;
 
 	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||

@@ -3979,11 +3970,8 @@ SYSCALL_DEFINE6(futex_time32, u32 __user
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}
-	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
-	    cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP)
-		val2 = (int) (unsigned long) utime;
 
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
 }
 #endif /* CONFIG_COMPAT_32BIT_TIME */

André Almeida

2021-04-23 21:40 UTC #6 lore raw

Re: [patch 3/6] futex: Get rid of the val2 conditional dance

Hi Thomas,

Às 16:44 de 22/04/21, Thomas Gleixner escreveu:

Quoted 26 lines

There is no point in checking which FUTEX operand treats the utime pointer as 'val2' argument because that argument to do_futex() is only used by exactly these operands.
So just handing it in unconditionally is not making any difference, but removes a lot of pointless gunk.
Signed-off-by Thomas Gleixner <[email protected]>
---
  kernel/futex.c |   16 ++--------------
  1 file changed, 2 insertions(+), 14 deletions(-)
kernel/futex.c +0 -1
@@ -3765,7 +3765,6 @@ SYSCALL_DEFINE6(futex, u32 __user *, uad
  {
  	struct timespec64 ts;
  	ktime_t t, *tp = NULL;
-	u32 val2 = 0;
  	int cmd = op & FUTEX_CMD_MASK;
 
  	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
@@ -3785,15 +3784,8 @@ SYSCALL_DEFINE6(futex, u32 __user *, uad
  			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
  		tp = &t;
  	}

kernel/futex.c SYSCALL_DEFINE6(futex, u32 __user *, uad · line 3788

Quoted 10 lines

kernel/futex.c +1 -8

-	/*
-	 * requeue parameter in 'utime' if cmd == FUTEX_*_REQUEUE_*.
-	 * number of waiters to wake in 'utime' if cmd == FUTEX_WAKE_OP.
-	 */
-	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
-	    cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP)
-		val2 = (u32) (unsigned long) utime;
 
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);

Given do_futex()'s type signature, I think it makes more sense to cast utime to u32.

Quoted 12 lines

kernel/futex.c +0 -1

  }
 
  #ifdef CONFIG_COMPAT

@@ -3961,7 +3953,6 @@ SYSCALL_DEFINE6(futex_time32, u32 __user
  {
  	struct timespec64 ts;
  	ktime_t t, *tp = NULL;
-	int val2 = 0;
  	int cmd = op & FUTEX_CMD_MASK;
 
  	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||

kernel/futex.c SYSCALL_DEFINE6(futex_time32, u32 __user · line 3974

Quoted 9 lines

kernel/futex.c +1 -4

@@ -3979,11 +3970,8 @@ SYSCALL_DEFINE6(futex_time32, u32 __user
  			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
  		tp = &t;
  	}
-	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
-	    cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP)
-		val2 = (int) (unsigned long) utime;
 
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);

Same here.

Quoted 4 lines

kernel/futex.c +0 -0

  }
  #endif /* CONFIG_COMPAT_32BIT_TIME */

Thomas Gleixner

author

2021-04-23 22:34 UTC #7 lore raw

Re: [patch 3/6] futex: Get rid of the val2 conditional dance

Quoted 6 lines · André Almeida

Review
Quoted 3 lines
(quoted diff) +1 -1
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
Given do_futex()'s type signature, I think it makes more sense to cast utime to u32.

It's a pointer which you better force cast to unsigned long first.

So the explicit thing would be '(u32)(unsigned long) utime' which is what the val2 dance stupidly did with 'int'

		val2 = (int) (unsigned long) utime;

But with doing it at function call argument it's implicit, because the

unsigned long to u32 conversion is well defined

while

(u32)ptr

is only well defined on 32bit.

Thanks,

tglx

André Almeida

2021-04-23 23:22 UTC #8 lore raw

Re: [patch 3/6] futex: Get rid of the val2 conditional dance

Às 19:34 de 23/04/21, Thomas Gleixner escreveu:

Quoted 17 lines · André Almeida

Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +1 -1
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
Given do_futex()'s type signature, I think it makes more sense to cast utime to u32.
It's a pointer which you better force cast to unsigned long first.
So the explicit thing would be '(u32)(unsigned long) utime' which is what the val2 dance stupidly did with 'int'
		val2 = (int) (unsigned long) utime;
But with doing it at function call argument it's implicit, because the

Quoted 8 lines

unsigned long to u32 conversion is well defined
while
   (u32)ptr
is only well defined on 32bit.

I see, thank you for the clarification!

Quoted 4 lines

Thanks,
tglx

tip-bot2 for Thomas Gleixner

merge bot Applied

2021-05-06 18:20 UTC #9 lore raw

[tip: locking/urgent] futex: Get rid of the val2 conditional dance

In reply to Thomas Gleixner

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     b097d5ed33561507eeffc77120a8c16c2f0f2c4c
Gitweb:        https://git.kernel.org/tip/b097d5ed33561507eeffc77120a8c16c2f0f2c4c
Author:        Thomas Gleixner <[email protected]>
AuthorDate:    Thu, 22 Apr 2021 21:44:20 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Thu, 06 May 2021 20:19:04 +02:00

futex: Get rid of the val2 conditional dance

There is no point in checking which FUTEX operand treats the utime pointer as 'val2' argument because that argument to do_futex() is only used by exactly these operands.

So just handing it in unconditionally is not making any difference, but removes a lot of pointless gunk.

Signed-off-by Thomas Gleixner <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/futex.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

kernel/futex.c +2 ~~-14~~

index b0f5304..4ddfdce 100644
@@ -3764,7 +3764,6 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 {
 	struct timespec64 ts;
 	ktime_t t, *tp = NULL;
-	u32 val2 = 0;
 	int cmd = op & FUTEX_CMD_MASK;
 
 	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||

@@ -3784,15 +3783,8 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}
-	/*
-	 * requeue parameter in 'utime' if cmd == FUTEX_*_REQUEUE_*.
-	 * number of waiters to wake in 'utime' if cmd == FUTEX_WAKE_OP.
-	 */
-	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
-	    cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP)
-		val2 = (u32) (unsigned long) utime;
 
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
 }
 
 #ifdef CONFIG_COMPAT

@@ -3960,7 +3952,6 @@ SYSCALL_DEFINE6(futex_time32, u32 __user *, uaddr, int, op, u32, val,
 {
 	struct timespec64 ts;
 	ktime_t t, *tp = NULL;
-	int val2 = 0;
 	int cmd = op & FUTEX_CMD_MASK;
 
 	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||

@@ -3978,11 +3969,8 @@ SYSCALL_DEFINE6(futex_time32, u32 __user *, uaddr, int, op, u32, val,
 			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
 		tp = &t;
 	}
-	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
-	    cmd == FUTEX_CMP_REQUEUE_PI || cmd == FUTEX_WAKE_OP)
-		val2 = (int) (unsigned long) utime;
 
-	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, tp, uaddr2, (unsigned long)utime, val3);
 }
 #endif /* CONFIG_COMPAT_32BIT_TIME */

Thomas Gleixner

author

2021-04-22 20:29 UTC #10 lore raw

PATCH 4/6 futex: Make syscall entry points less convoluted

In reply to Thomas Gleixner

The futex and the compat syscall entry points do pretty much the same except for the timespec data types and the corresponding copy from user function.

Split out the rest into inline functions and share the functionality.

Signed-off-by Thomas Gleixner <[email protected]>

---
 kernel/futex.c |   63 +++++++++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

kernel/futex.c +37 ~~-26~~

@@ -3758,30 +3758,48 @@ long do_futex(u32 __user *uaddr, int op,
 	return -ENOSYS;
 }
 
+static __always_inline bool futex_cmd_has_timeout(u32 cmd)
+{
+	switch (cmd) {
+	case FUTEX_WAIT:
+	case FUTEX_LOCK_PI:
+	case FUTEX_WAIT_BITSET:
+	case FUTEX_WAIT_REQUEUE_PI:
+		return true;
+	}
+	return false;
+}
+
+static __always_inline int
+futex_init_timeout(u32 cmd, u32 op, struct timespec64 *ts, ktime_t *t)
+{
+	if (!timespec64_valid(ts))
+		return -EINVAL;
+
+	*t = timespec64_to_ktime(*ts);
+	if (cmd == FUTEX_WAIT)
+		*t = ktime_add_safe(ktime_get(), *t);
+	else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
+		*t = timens_ktime_to_host(CLOCK_MONOTONIC, *t);
+	return 0;
+}
 
 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 		const struct __kernel_timespec __user *, utime,
 		u32 __user *, uaddr2, u32, val3)
 {
-	struct timespec64 ts;
+	int ret, cmd = op & FUTEX_CMD_MASK;
 	ktime_t t, *tp = NULL;
-	int cmd = op & FUTEX_CMD_MASK;
+	struct timespec64 ts;
 
-	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
-		      cmd == FUTEX_WAIT_BITSET ||
-		      cmd == FUTEX_WAIT_REQUEUE_PI)) {
+	if (utime && futex_cmd_has_timeout(cmd)) {
 		if (unlikely(should_fail_futex(!(op & FUTEX_PRIVATE_FLAG))))
 			return -EFAULT;
 		if (get_timespec64(&ts, utime))
 			return -EFAULT;
-		if (!timespec64_valid(&ts))
-			return -EINVAL;
-
-		t = timespec64_to_ktime(ts);
-		if (cmd == FUTEX_WAIT)
-			t = ktime_add_safe(ktime_get(), t);
-		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
-			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
+		ret = futex_init_timeout(cmd, op, &ts, &t);
+		if (ret)
+			return ret;
 		tp = &t;
 	}

@@ -3951,23 +3969,16 @@ SYSCALL_DEFINE6(futex_time32, u32 __user
 		const struct old_timespec32 __user *, utime, u32 __user *, uaddr2,
 		u32, val3)
 {
-	struct timespec64 ts;
+	int ret, cmd = op & FUTEX_CMD_MASK;
 	ktime_t t, *tp = NULL;
-	int cmd = op & FUTEX_CMD_MASK;
+	struct timespec64 ts;
 
-	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
-		      cmd == FUTEX_WAIT_BITSET ||
-		      cmd == FUTEX_WAIT_REQUEUE_PI)) {
+	if (utime && futex_cmd_has_timeout(cmd)) {
 		if (get_old_timespec32(&ts, utime))
 			return -EFAULT;
-		if (!timespec64_valid(&ts))
-			return -EINVAL;
-
-		t = timespec64_to_ktime(ts);
-		if (cmd == FUTEX_WAIT)
-			t = ktime_add_safe(ktime_get(), t);
-		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
-			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
+		ret = futex_init_timeout(cmd, op, &ts, &t);
+		if (ret)
+			return ret;
 		tp = &t;
 	}

tip-bot2 for Thomas Gleixner

merge bot Applied

2021-05-06 18:20 UTC #11 lore raw

[tip: locking/urgent] futex: Make syscall entry points less convoluted

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     51cf94d16860a324e97d1b670d88f1f2b643bc32
Gitweb:        https://git.kernel.org/tip/51cf94d16860a324e97d1b670d88f1f2b643bc32
Author:        Thomas Gleixner <[email protected]>
AuthorDate:    Thu, 22 Apr 2021 21:44:21 +02:00
Committer:     Thomas Gleixner <[email protected]>
CommitterDate: Thu, 06 May 2021 20:19:04 +02:00

futex: Make syscall entry points less convoluted

The futex and the compat syscall entry points do pretty much the same except for the timespec data types and the corresponding copy from user function.

Split out the rest into inline functions and share the functionality.

Signed-off-by Thomas Gleixner <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/futex.c | 63 ++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

kernel/futex.c +37 ~~-26~~

index 4ddfdce..4938a00 100644
@@ -3757,30 +3757,48 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 	return -ENOSYS;
 }
 
+static __always_inline bool futex_cmd_has_timeout(u32 cmd)
+{
+	switch (cmd) {
+	case FUTEX_WAIT:
+	case FUTEX_LOCK_PI:
+	case FUTEX_WAIT_BITSET:
+	case FUTEX_WAIT_REQUEUE_PI:
+		return true;
+	}
+	return false;
+}
+
+static __always_inline int
+futex_init_timeout(u32 cmd, u32 op, struct timespec64 *ts, ktime_t *t)
+{
+	if (!timespec64_valid(ts))
+		return -EINVAL;
+
+	*t = timespec64_to_ktime(*ts);
+	if (cmd == FUTEX_WAIT)
+		*t = ktime_add_safe(ktime_get(), *t);
+	else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
+		*t = timens_ktime_to_host(CLOCK_MONOTONIC, *t);
+	return 0;
+}
 
 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
 		const struct __kernel_timespec __user *, utime,
 		u32 __user *, uaddr2, u32, val3)
 {
-	struct timespec64 ts;
+	int ret, cmd = op & FUTEX_CMD_MASK;
 	ktime_t t, *tp = NULL;
-	int cmd = op & FUTEX_CMD_MASK;
+	struct timespec64 ts;
 
-	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
-		      cmd == FUTEX_WAIT_BITSET ||
-		      cmd == FUTEX_WAIT_REQUEUE_PI)) {
+	if (utime && futex_cmd_has_timeout(cmd)) {
 		if (unlikely(should_fail_futex(!(op & FUTEX_PRIVATE_FLAG))))
 			return -EFAULT;
 		if (get_timespec64(&ts, utime))
 			return -EFAULT;
-		if (!timespec64_valid(&ts))
-			return -EINVAL;
-
-		t = timespec64_to_ktime(ts);
-		if (cmd == FUTEX_WAIT)
-			t = ktime_add_safe(ktime_get(), t);
-		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
-			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
+		ret = futex_init_timeout(cmd, op, &ts, &t);
+		if (ret)
+			return ret;
 		tp = &t;
 	}

@@ -3950,23 +3968,16 @@ SYSCALL_DEFINE6(futex_time32, u32 __user *, uaddr, int, op, u32, val,
 		const struct old_timespec32 __user *, utime, u32 __user *, uaddr2,
 		u32, val3)
 {
-	struct timespec64 ts;
+	int ret, cmd = op & FUTEX_CMD_MASK;
 	ktime_t t, *tp = NULL;
-	int cmd = op & FUTEX_CMD_MASK;
+	struct timespec64 ts;
 
-	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
-		      cmd == FUTEX_WAIT_BITSET ||
-		      cmd == FUTEX_WAIT_REQUEUE_PI)) {
+	if (utime && futex_cmd_has_timeout(cmd)) {
 		if (get_old_timespec32(&ts, utime))
 			return -EFAULT;
-		if (!timespec64_valid(&ts))
-			return -EINVAL;
-
-		t = timespec64_to_ktime(ts);
-		if (cmd == FUTEX_WAIT)
-			t = ktime_add_safe(ktime_get(), t);
-		else if (cmd != FUTEX_LOCK_PI && !(op & FUTEX_CLOCK_REALTIME))
-			t = timens_ktime_to_host(CLOCK_MONOTONIC, t);
+		ret = futex_init_timeout(cmd, op, &ts, &t);
+		if (ret)
+			return ret;
 		tp = &t;
 	}

Thomas Gleixner

author

2021-04-22 20:29 UTC #12 lore raw

PATCH 5/6 futex: Prepare futex_lock_pi() for runtime clock selection

In reply to Thomas Gleixner

futex_lock_pi() is the only futex operation which cannot select the clock for timeouts (CLOCK_MONOTONIC/CLOCK_REALTIME). That's inconsistent and there is no particular reason why this cannot be supported.

This was overlooked when CLOCK_REALTIME_FLAG was introduced and unfortunately not reported when the inconsistency was discovered in glibc.

Prepare the function and enforce the CLOCK_REALTIME_FLAG on FUTEX_LOCK_PI so that a new FUTEX_LOCK_PI2 can implement it correctly.

Reported-by Kurt Kanzenbach <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 kernel/futex.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

kernel/futex.c +3 -2

@@ -2786,7 +2786,7 @@ static int futex_lock_pi(u32 __user *uad
 	if (refill_pi_state_cache())
 		return -ENOMEM;
 
-	to = futex_setup_timer(time, &timeout, FLAGS_CLOCKRT, 0);
+	to = futex_setup_timer(time, &timeout, flags, 0);
 
 retry:
 	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key, FUTEX_WRITE);

@@ -3711,7 +3711,7 @@ long do_futex(u32 __user *uaddr, int op,
 
 	if (op & FUTEX_CLOCK_REALTIME) {
 		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET &&	cmd != FUTEX_WAIT_REQUEUE_PI)
+		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
 			return -ENOSYS;
 	}

@@ -3743,6 +3743,7 @@ long do_futex(u32 __user *uaddr, int op,
 	case FUTEX_WAKE_OP:
 		return futex_wake_op(uaddr, flags, uaddr2, val, val2, val3);
 	case FUTEX_LOCK_PI:
+		flags |= FLAGS_CLOCKRT;
 		return futex_lock_pi(uaddr, flags, timeout, 0);
 	case FUTEX_UNLOCK_PI:
 		return futex_unlock_pi(uaddr, flags);

Lukasz Majewski

2021-04-23 09:43 UTC #13 lore raw

Re: [patch 5/6] futex: Prepare futex_lock_pi() for runtime clock selection

Hi Thomas,

Quoted 36 lines

futex_lock_pi() is the only futex operation which cannot select the clock for timeouts (CLOCK_MONOTONIC/CLOCK_REALTIME). That's inconsistent and there is no particular reason why this cannot be supported.
This was overlooked when CLOCK_REALTIME_FLAG was introduced and unfortunately not reported when the inconsistency was discovered in glibc.
Prepare the function and enforce the CLOCK_REALTIME_FLAG on FUTEX_LOCK_PI so that a new FUTEX_LOCK_PI2 can implement it correctly.
Reported-by Kurt Kanzenbach <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
---
 kernel/futex.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
kernel/futex.c +1 -1
@@ -2786,7 +2786,7 @@ static int futex_lock_pi(u32 __user *uad
 	if (refill_pi_state_cache())
 		return -ENOMEM;
 
-	to = futex_setup_timer(time, &timeout, FLAGS_CLOCKRT, 0);
+	to = futex_setup_timer(time, &timeout, flags, 0);
 
 retry:
 	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key,
FUTEX_WRITE); @@ -3711,7 +3711,7 @@ long do_futex(u32 __user *uaddr, int op,
kernel/futex.c +0 -1
 	if (op & FUTEX_CLOCK_REALTIME) {
 		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET &&	cmd !=
FUTEX_WAIT_REQUEUE_PI)
kernel/futex.c +1 -0
+		if (cmd != FUTEX_WAIT_BITSET && cmd !=

Quoted 1 line

FUTEX_WAIT_REQUEUE_PI) return -ENOSYS;

What is the exact change for those two lines above? Looks like some different tab/spaces...

kernel/futex.c long do_futex(u32 __user *uaddr, int op, · line 3749

Quoted 11 lines

kernel/futex.c +0 -0

@@ -3743,6 +3743,7 @@ long do_futex(u32 __user *uaddr, int op,
 	case FUTEX_WAKE_OP:
 		return futex_wake_op(uaddr, flags, uaddr2, val,

val2, val3); case FUTEX_LOCK_PI:

kernel/futex.c +1 -0

+		flags |= FLAGS_CLOCKRT;
 		return futex_lock_pi(uaddr, flags, timeout, 0);
 	case FUTEX_UNLOCK_PI:
 		return futex_unlock_pi(uaddr, flags);

Best regards,

Lukasz Majewski

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]

Thomas Gleixner

author

2021-04-23 10:08 UTC #14 lore raw

Re: [patch 5/6] futex: Prepare futex_lock_pi() for runtime clock selection

Quoted 4 lines · Lukasz Majewski

Quoted 4 lines
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key,
FUTEX_WRITE); @@ -3711,7 +3711,7 @@ long do_futex(u32 __user *uaddr, int op,
if (op & FUTEX_CLOCK_REALTIME) {

Quoted 8 lines

Quoted 5 lines
(quoted diff) +0 -1
 		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET &&	cmd !=
FUTEX_WAIT_REQUEUE_PI)
(quoted diff) +1 -0
+		if (cmd != FUTEX_WAIT_BITSET && cmd !=
FUTEX_WAIT_REQUEUE_PI) return -ENOSYS;
What is the exact change for those two lines above? Looks like some different tab/spaces...

Oops. Yes. I surely stared at every hunk more than once...

tip-bot2 for Thomas Gleixner

merge bot Applied

2021-06-23 08:19 UTC #15 lore raw

[tip: locking/core] futex: Prepare futex_lock_pi() for runtime clock selection

In reply to Thomas Gleixner

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     e112c41341c03d9224a9fc522bdb3539bc849b56
Gitweb:        https://git.kernel.org/tip/e112c41341c03d9224a9fc522bdb3539bc849b56
Author:        Thomas Gleixner <[email protected]>
AuthorDate:    Thu, 22 Apr 2021 21:44:22 +02:00
Committer:     Peter Zijlstra <[email protected]>
CommitterDate: Tue, 22 Jun 2021 16:42:08 +02:00

futex: Prepare futex_lock_pi() for runtime clock selection

This was overlooked when CLOCK_REALTIME_FLAG was introduced and unfortunately not reported when the inconsistency was discovered in glibc.

Prepare the function and enforce the CLOCK_REALTIME_FLAG on FUTEX_LOCK_PI so that a new FUTEX_LOCK_PI2 can implement it correctly.

Reported-by Kurt Kanzenbach <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

kernel/futex.c +2 -1

index 08008c2..f820439 100644
@@ -2783,7 +2783,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags,
 	if (refill_pi_state_cache())
 		return -ENOMEM;
 
-	to = futex_setup_timer(time, &timeout, FLAGS_CLOCKRT, 0);
+	to = futex_setup_timer(time, &timeout, flags, 0);
 
 retry:
 	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key, FUTEX_WRITE);

@@ -3739,6 +3739,7 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 	case FUTEX_WAKE_OP:
 		return futex_wake_op(uaddr, flags, uaddr2, val, val2, val3);
 	case FUTEX_LOCK_PI:
+		flags |= FLAGS_CLOCKRT;
 		return futex_lock_pi(uaddr, flags, timeout, 0);
 	case FUTEX_UNLOCK_PI:
 		return futex_unlock_pi(uaddr, flags);

Thomas Gleixner

author

2021-04-22 20:29 UTC #16 lore raw

PATCH 6/6 futex: Provide FUTEX_LOCK_PI2 to support clock selection

In reply to Thomas Gleixner

The FUTEX_LOCK_PI futex operand uses a CLOCK_REALTIME based absolute timeout since it was implemented, but it does not require that the FUTEX_CLOCK_REALTIME flag is set, because that was introduced later.

In theory as none of the user space implementations can set the FUTEX_CLOCK_REALTIME flag on this operand, it would be possible to creatively abuse it and make the meaning invers, i.e. select CLOCK_REALTIME when not set and CLOCK_MONOTONIC when set. But that's a nasty hackery.

Another option would be to have a new FUTEX_CLOCK_MONOTONIC flag only for FUTEX_LOCK_PI, but that's also awkward because it does not allow libraries to handle the timeout clock selection consistently.

So provide a new FUTEX_LOCK_PI2 operand which implements the timeout semantics which the other operands use and leave FUTEX_LOCK_PI alone.

Reported-by Kurt Kanzenbach <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 include/uapi/linux/futex.h |    1 +
 kernel/futex.c             |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

include/uapi/linux/futex.h +1 -0

@@ -21,6 +21,7 @@
 #define FUTEX_WAKE_BITSET	10
 #define FUTEX_WAIT_REQUEUE_PI	11
 #define FUTEX_CMP_REQUEUE_PI	12
+#define FUTEX_LOCK_PI2		13
 
 #define FUTEX_PRIVATE_FLAG	128
 #define FUTEX_CLOCK_REALTIME	256

kernel/futex.c +5 -1

@@ -3711,7 +3711,8 @@ long do_futex(u32 __user *uaddr, int op,
 
 	if (op & FUTEX_CLOCK_REALTIME) {
 		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
+		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI &&
+		    cmd != FUTEX_LOCK_PI2)
 			return -ENOSYS;
 	}

@@ -3744,6 +3745,8 @@ long do_futex(u32 __user *uaddr, int op,
 		return futex_wake_op(uaddr, flags, uaddr2, val, val2, val3);
 	case FUTEX_LOCK_PI:
 		flags |= FLAGS_CLOCKRT;
+		fallthrough;
+	case FUTEX_LOCK_PI2:
 		return futex_lock_pi(uaddr, flags, timeout, 0);
 	case FUTEX_UNLOCK_PI:
 		return futex_unlock_pi(uaddr, flags);

@@ -3764,6 +3767,7 @@ static inline bool futex_cmd_has_timeout
 	switch (cmd) {
 	case FUTEX_WAIT:
 	case FUTEX_LOCK_PI:
+	case FUTEX_LOCK_PI2:
 	case FUTEX_WAIT_BITSET:
 	case FUTEX_WAIT_REQUEUE_PI:
 		return true;

André Almeida

2021-04-23 22:27 UTC #17 lore raw

Re: [patch 6/6] futex: Provide FUTEX_LOCK_PI2 to support clock selection

Hi Thomas,

Às 16:44 de 22/04/21, Thomas Gleixner escreveu:

Quoted 26 lines

The FUTEX_LOCK_PI futex operand uses a CLOCK_REALTIME based absolute timeout since it was implemented, but it does not require that the FUTEX_CLOCK_REALTIME flag is set, because that was introduced later.
In theory as none of the user space implementations can set the FUTEX_CLOCK_REALTIME flag on this operand, it would be possible to creatively abuse it and make the meaning invers, i.e. select CLOCK_REALTIME when not set and CLOCK_MONOTONIC when set. But that's a nasty hackery.
Another option would be to have a new FUTEX_CLOCK_MONOTONIC flag only for FUTEX_LOCK_PI, but that's also awkward because it does not allow libraries to handle the timeout clock selection consistently.
So provide a new FUTEX_LOCK_PI2 operand which implements the timeout semantics which the other operands use and leave FUTEX_LOCK_PI alone.
Reported-by Kurt Kanzenbach <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
---
  include/uapi/linux/futex.h |    1 +
  kernel/futex.c             |    6 +++++-
  2 files changed, 6 insertions(+), 1 deletion(-)

include/uapi/linux/futex.h line 27

Quoted 7 lines

include/uapi/linux/futex.h +1 -0

@@ -21,6 +21,7 @@
  #define FUTEX_WAKE_BITSET	10
  #define FUTEX_WAIT_REQUEUE_PI	11
  #define FUTEX_CMP_REQUEUE_PI	12
+#define FUTEX_LOCK_PI2		13
 
  #define FUTEX_PRIVATE_FLAG	128
  #define FUTEX_CLOCK_REALTIME	256

To keep consistency with other operations, maybe add a FUTEX_LOCK_PI2_PRIVATE?

kernel/futex.c long do_futex(u32 __user *uaddr, int op, · line 3717

Quoted 11 lines

kernel/futex.c +2 -1

@@ -3711,7 +3711,8 @@ long do_futex(u32 __user *uaddr, int op,
 
  	if (op & FUTEX_CLOCK_REALTIME) {
  		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
+		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI &&
+		    cmd != FUTEX_LOCK_PI2)
  			return -ENOSYS;
  	}

As FUTEX_LOCK_PI, FUTEX_LOCK_PI2 also requires FUTEX_CMPXCHG right? Then, add it here:

	switch (cmd) {
	case FUTEX_LOCK_PI:

+ case FUTEX_LOCK_PI2:
case FUTEX_UNLOCK_PI:
case FUTEX_TRYLOCK_PI:
case FUTEX_WAIT_REQUEUE_PI:
case FUTEX_CMP_REQUEUE_PI:
if (!futex_cmpxchg_enabled)
return -ENOSYS;
}

Quoted 12 lines

kernel/futex.c +2 -0

@@ -3744,6 +3745,8 @@ long do_futex(u32 __user *uaddr, int op,
  		return futex_wake_op(uaddr, flags, uaddr2, val, val2, val3);
  	case FUTEX_LOCK_PI:
  		flags |= FLAGS_CLOCKRT;
+		fallthrough;
+	case FUTEX_LOCK_PI2:
  		return futex_lock_pi(uaddr, flags, timeout, 0);
  	case FUTEX_UNLOCK_PI:
  		return futex_unlock_pi(uaddr, flags);

kernel/futex.c static inline bool futex_cmd_has_timeout · line 3773

Quoted 7 lines

kernel/futex.c +1 -0

@@ -3764,6 +3767,7 @@ static inline bool futex_cmd_has_timeout
  	switch (cmd) {
  	case FUTEX_WAIT:
  	case FUTEX_LOCK_PI:
+	case FUTEX_LOCK_PI2:
  	case FUTEX_WAIT_BITSET:
  	case FUTEX_WAIT_REQUEUE_PI:
  		return true;

Thanks,
André

Thomas Gleixner

author

2021-04-23 22:36 UTC #18 lore raw

Re: [patch 6/6] futex: Provide FUTEX_LOCK_PI2 to support clock selection

André!

Quoted 11 lines · André Almeida

include/uapi/linux/futex.h line 27
Quoted 8 lines
include/uapi/linux/futex.h +1 -0
@@ -21,6 +21,7 @@
  #define FUTEX_WAKE_BITSET	10
  #define FUTEX_WAIT_REQUEUE_PI	11
  #define FUTEX_CMP_REQUEUE_PI	12
+#define FUTEX_LOCK_PI2		13
 
  #define FUTEX_PRIVATE_FLAG	128
  #define FUTEX_CLOCK_REALTIME	256
To keep consistency with other operations, maybe add a FUTEX_LOCK_PI2_PRIVATE?

Good point! Missed that.

Quoted 18 lines

kernel/futex.c long do_futex(u32 __user *uaddr, int op, · line 3717

Quoted 11 lines

kernel/futex.c +2 -1

@@ -3711,7 +3711,8 @@ long do_futex(u32 __user *uaddr, int op,
 
  	if (op & FUTEX_CLOCK_REALTIME) {
  		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
+		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI &&
+		    cmd != FUTEX_LOCK_PI2)
  			return -ENOSYS;
  	}

As FUTEX_LOCK_PI, FUTEX_LOCK_PI2 also requires FUTEX_CMPXCHG right? Then, add it here:

	switch (cmd) {
	case FUTEX_LOCK_PI:

(quoted diff) +1 -0

+	case FUTEX_LOCK_PI2:

Quoted 7 lines

case FUTEX_UNLOCK_PI:
case FUTEX_TRYLOCK_PI:
case FUTEX_WAIT_REQUEUE_PI:
case FUTEX_CMP_REQUEUE_PI:
if (!futex_cmpxchg_enabled)
return -ENOSYS;
}

Indeed. Forgot about that completely.

Thanks for spotting that!

tglx

tip-bot2 for Thomas Gleixner

merge bot Applied

2021-06-23 08:19 UTC #19 lore raw

[tip: locking/core] futex: Provide FUTEX_LOCK_PI2 to support clock selection

In reply to Thomas Gleixner

The following commit has been merged into the locking/core branch of tip:

Commit-ID:     bf22a6976897977b0a3f1aeba6823c959fc4fdae
Gitweb:        https://git.kernel.org/tip/bf22a6976897977b0a3f1aeba6823c959fc4fdae
Author:        Thomas Gleixner <[email protected]>
AuthorDate:    Thu, 22 Apr 2021 21:44:23 +02:00
Committer:     Peter Zijlstra <[email protected]>
CommitterDate: Tue, 22 Jun 2021 16:42:09 +02:00

futex: Provide FUTEX_LOCK_PI2 to support clock selection

So provide a new FUTEX_LOCK_PI2 operand which implements the timeout semantics which the other operands use and leave FUTEX_LOCK_PI alone.

Reported-by Kurt Kanzenbach <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 include/uapi/linux/futex.h | 2 ++
 kernel/futex.c             | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

include/uapi/linux/futex.h +2 -0

index a89eb0a..235e5b2 100644
@@ -21,6 +21,7 @@
 #define FUTEX_WAKE_BITSET	10
 #define FUTEX_WAIT_REQUEUE_PI	11
 #define FUTEX_CMP_REQUEUE_PI	12
+#define FUTEX_LOCK_PI2		13
 
 #define FUTEX_PRIVATE_FLAG	128
 #define FUTEX_CLOCK_REALTIME	256

@@ -32,6 +33,7 @@
 #define FUTEX_CMP_REQUEUE_PRIVATE (FUTEX_CMP_REQUEUE | FUTEX_PRIVATE_FLAG)
 #define FUTEX_WAKE_OP_PRIVATE	(FUTEX_WAKE_OP | FUTEX_PRIVATE_FLAG)
 #define FUTEX_LOCK_PI_PRIVATE	(FUTEX_LOCK_PI | FUTEX_PRIVATE_FLAG)
+#define FUTEX_LOCK_PI2_PRIVATE	(FUTEX_LOCK_PI2 | FUTEX_PRIVATE_FLAG)
 #define FUTEX_UNLOCK_PI_PRIVATE	(FUTEX_UNLOCK_PI | FUTEX_PRIVATE_FLAG)
 #define FUTEX_TRYLOCK_PI_PRIVATE (FUTEX_TRYLOCK_PI | FUTEX_PRIVATE_FLAG)
 #define FUTEX_WAIT_BITSET_PRIVATE	(FUTEX_WAIT_BITSET | FUTEX_PRIVATE_FLAG)

kernel/futex.c +6 -1

index f820439..f832b64 100644
@@ -3707,12 +3707,14 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 
 	if (op & FUTEX_CLOCK_REALTIME) {
 		flags |= FLAGS_CLOCKRT;
-		if (cmd != FUTEX_WAIT_BITSET &&	cmd != FUTEX_WAIT_REQUEUE_PI)
+		if (cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI &&
+		    cmd != FUTEX_LOCK_PI2)
 			return -ENOSYS;
 	}
 
 	switch (cmd) {
 	case FUTEX_LOCK_PI:
+	case FUTEX_LOCK_PI2:
 	case FUTEX_UNLOCK_PI:
 	case FUTEX_TRYLOCK_PI:
 	case FUTEX_WAIT_REQUEUE_PI:

@@ -3740,6 +3742,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 		return futex_wake_op(uaddr, flags, uaddr2, val, val2, val3);
 	case FUTEX_LOCK_PI:
 		flags |= FLAGS_CLOCKRT;
+		fallthrough;
+	case FUTEX_LOCK_PI2:
 		return futex_lock_pi(uaddr, flags, timeout, 0);
 	case FUTEX_UNLOCK_PI:
 		return futex_unlock_pi(uaddr, flags);

@@ -3760,6 +3764,7 @@ static __always_inline bool futex_cmd_has_timeout(u32 cmd)
 	switch (cmd) {
 	case FUTEX_WAIT:
 	case FUTEX_LOCK_PI:
+	case FUTEX_LOCK_PI2:
 	case FUTEX_WAIT_BITSET:
 	case FUTEX_WAIT_REQUEUE_PI:
 		return true;

Peter Zijlstra

reviewer Acked-by on the v1 series

2021-05-05 12:59 UTC #20 lore raw

In reply to Thomas Gleixner

Quoted 25 lines · Thomas Gleixner

The following series started off looking into supporting selectable clocks for FUTEX_LOCK_PI which is hardcoded to CLOCK_REALTIME and cannot be changed.
On the way I found two bugs related to the timeout handling:
- The allowance for FUTEX_WAIT to use FUTEX_CLOCK_REALTIME is broken and never worked.
- The recent time namespace support wreckaged FUTEX_LOCK_PI timeouts when the task belongs to a namespace which has an CLOCK_MONOTONIC offset.
Both should have been caught by that Gleixner dude when merging them, but it seems he's getting old.
Not having a selectable clock for PI futexes is inconsistent because all other interfaces have it. Unfortunately this was figured out by glibc folks quite some time ago, but nobody told us :(
The nasty hack to support it would be to treat FUTEX_CLOCK_REALTIME inverse for FUTEX_LOCK_PI, but that's a horrible idea. Adding a new flag to the futex op, i.e. FUTEX_CLOCK_MONOTONIC would be possible, but that's yet another variant which makes is harder for libraries to have a consistent clock selection handling.

Quoted 4 lines

So I went the way to let FUTEX_LOCK_PI alone and to add FUTEX_LOCK_PI2 which handles the clocks the same way as the other operands.
Thoughts?

With the missing FUTEX_LOCK_PI2 in #6, as spotted by André Almeida, fixed:

Acked-by Peter Zijlstra (Intel) <[email protected]>

It's all somewhat sad, but I don't see any other way out of this. Using LOCK_PI2 will be a fairly horrible pile of hacks on the userspace side of things given they need to first detect it's presence etc., but that seems unavoidable whatever we do :/

Kurt Kanzenbach

2021-05-05 13:51 UTC #21 lore raw

Quoted 4 lines · Peter Zijlstra

It's all somewhat sad, but I don't see any other way out of this. Using LOCK_PI2 will be a fairly horrible pile of hacks on the userspace side of things given they need to first detect it's presence etc., but that seems unavoidable whatever we do :/

Well, that's the interesting point. I do have such horrible hacks for glibc, which detect the presence of LOCK_PI2 at run time. However, the glibc has also the notion of kernel features based on the Linux kernel version. Then, it could be detected at compile time. At the end of the day it depends on how this patch set is merged. I was hoping for the glibc folks to share their opinion on this :-).

Thanks,
Kurt