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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f8cda7fa-46e9-5add-ccb6-441323ba2042@collabora.com>
Date:   Fri, 23 Apr 2021 18:40:13 -0300
From:   André Almeida <andrealmeid@...labora.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Adhemerval Zanella <adhemerval.zanella@...aro.org>,
        Lukasz Majewski <lukma@...x.de>,
        Florian Weimer <fweimer@...hat.com>,
        Carlos O'Donell <carlos@...hat.com>,
        "Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Ingo Molnar <mingo@...nel.org>,
        Darren Hart <dvhart@...radead.org>,
        Andrei Vagin <avagin@...il.com>,
        Kurt Kanzenbach <kurt@...utronix.de>, kernel@...labora.com
Subject: Re: [patch 3/6] futex: Get rid of the val2 conditional dance

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@...utronix.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 */
>   
>