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(-)
--- 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 */
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 <[email protected]>
> ---
> 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 */
>
>
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
À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
>
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]
---
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 */