[patch 0/6] futex: Bugfixes and FUTEX_LOCK_PI2

[patch 0/6] futex: Bugfixes and FUTEX_LOCK_PI2

[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.

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(-)

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

[Thomas Gleixner]

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 <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Cc: Darren Hart <dvhart@infradead.org>
---
 kernel/futex.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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: locking/urgent] Revert 337f13046ff0 ("futex: Allow FUTEX_CLOCK_REALTIME with FUTEX_WAIT op")

[tip-bot2 for Thomas Gleixner]

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 <tglx@linutronix.de>
AuthorDate:    Thu, 22 Apr 2021 21:44:18 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 06 May 2021 20:12:40 +02:00

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 <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20210422194704.834797921@linutronix.de

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

diff --git a/kernel/futex.c b/kernel/futex.c
index c98b825..4740200 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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;
 	}
 

[patch 2/6] futex: Do not apply time namespace adjustment on FUTEX_LOCK_PI

[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 <tglx@linutronix.de>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: stable@vger.kernel.org
---
 kernel/futex.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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: locking/urgent] futex: Do not apply time namespace adjustment on FUTEX_LOCK_PI

[tip-bot2 for Thomas Gleixner]

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 <tglx@linutronix.de>
AuthorDate:    Thu, 22 Apr 2021 21:44:19 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 06 May 2021 20:12:40 +02:00

futex: Do not apply time namespace adjustment on FUTEX_LOCK_PI

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 <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20210422194704.984540159@linutronix.de

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

diff --git a/kernel/futex.c b/kernel/futex.c
index 4740200..b0f5304 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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;
 	}

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

[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 <tglx@linutronix.de>
---
 kernel/futex.c |   16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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 */
 

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

[André Almeida]

Hi Thomas,

Às 16:44 de 22/04/21, Thomas Gleixner escreveu:
> 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 <tglx@linutronix.de>
> ---
>   kernel/futex.c |   16 ++--------------
>   1 file changed, 2 insertions(+), 14 deletions(-)
> 
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -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);

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

>   }
>   
>   #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);

Same here.

>   }
>   #endif /* CONFIG_COMPAT_32BIT_TIME */
>   
> 

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

[Thomas Gleixner]

On Fri, Apr 23 2021 at 18:40, André Almeida wrote:
>>   
>> -	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

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

[André Almeida]

Às 19:34 de 23/04/21, Thomas Gleixner escreveu:
> On Fri, Apr 23 2021 at 18:40, André Almeida wrote:
>>>    
>>> -	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.
> 

I see, thank you for the clarification!

> Thanks,
> 
>          tglx
> 

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

[tip-bot2 for 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 <tglx@linutronix.de>
AuthorDate:    Thu, 22 Apr 2021 21:44:20 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
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 <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20210422194705.125957049@linutronix.de

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

diff --git a/kernel/futex.c b/kernel/futex.c
index b0f5304..4ddfdce 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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 */
 

[patch 4/6] futex: Make syscall entry points less convoluted

[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 <tglx@linutronix.de>
---
 kernel/futex.c |   63 +++++++++++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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: locking/urgent] futex: Make syscall entry points less convoluted

[tip-bot2 for Thomas Gleixner]

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 <tglx@linutronix.de>
AuthorDate:    Thu, 22 Apr 2021 21:44:21 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
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 <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20210422194705.244476369@linutronix.de

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

diff --git a/kernel/futex.c b/kernel/futex.c
index 4ddfdce..4938a00 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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;
 	}
 

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

[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 <kurt@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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);

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

Hi Thomas,

> 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 <kurt@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/futex.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -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;

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

>  	}
>  
> @@ -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);
> 



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: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Thomas Gleixner]

On Fri, Apr 23 2021 at 11:34, Lukasz Majewski wrote:
>>  	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;
>
> 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: locking/core] futex: Prepare futex_lock_pi() for runtime clock selection

[tip-bot2 for 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 <tglx@linutronix.de>
AuthorDate:    Thu, 22 Apr 2021 21:44:22 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 22 Jun 2021 16:42:08 +02:00

futex: Prepare futex_lock_pi() for runtime clock selection

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 <kurt@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20210422194705.338657741@linutronix.de
---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 08008c2..f820439 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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);

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

[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 <kurt@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/uapi/linux/futex.h |    1 +
 kernel/futex.c             |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -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
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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;

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

[André Almeida]

Hi Thomas,

Às 16:44 de 22/04/21, Thomas Gleixner escreveu:
> 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 <kurt@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>   include/uapi/linux/futex.h |    1 +
>   kernel/futex.c             |    6 +++++-
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> --- a/include/uapi/linux/futex.h
> +++ b/include/uapi/linux/futex.h
> @@ -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?

> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -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;
  	}

>   
> @@ -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;
> 

Thanks,
	André

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

[Thomas Gleixner]

André!

On Fri, Apr 23 2021 at 19:20, André Almeida wrote:
>> @@ -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.

>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -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;
>   	}

Indeed. Forgot about that completely.

Thanks for spotting that!

       tglx

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

[tip-bot2 for 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 <tglx@linutronix.de>
AuthorDate:    Thu, 22 Apr 2021 21:44:23 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 22 Jun 2021 16:42:09 +02:00

futex: Provide FUTEX_LOCK_PI2 to support clock selection

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 <kurt@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20210422194705.440773992@linutronix.de
---
 include/uapi/linux/futex.h | 2 ++
 kernel/futex.c             | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index a89eb0a..235e5b2 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -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)
diff --git a/kernel/futex.c b/kernel/futex.c
index f820439..f832b64 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -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;

Re: [patch 0/6] futex: Bugfixes and FUTEX_LOCK_PI2

[Peter Zijlstra]

On Thu, Apr 22, 2021 at 09:44:17PM +0200, Thomas Gleixner wrote:
> 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?

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

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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 :/

Re: [patch 0/6] futex: Bugfixes and FUTEX_LOCK_PI2

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Wed May 05 2021, Peter Zijlstra wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]