Re: [PATCH] arm64: futex: Consolidate 'old == new' check in __lsui_cmpxchg32()

[Yeoreum Yun <yeoreum.yun@arm.com>]

Thanks.

At the time of writing the code, expected the upper layer to handle
changes to the futex value for cmpxchg. However, it now seems that it
simply returns -EAGAIN there as well.

Reviewed-by: Yeoreum Yun <yeoreum.yun@arm.com>

> The LSUI futex implementation relies on a cmpxchg() loop to implement
> FUTEX_OP_XOR, as the architecture doesn't provide unprivileged *EOR
> atomics. Since the unprivileged 'CAST' instructions used to implement
> the cmpxchg() can only operate on 64-bit memory locations, the
> __lsui_cmpxchg32() helper function performs a song and dance to marshall
> the 32-bit futex value into the correct part of a 64-bit register and
> fill the remaining bytes with the neighbouring data.
> 
> A consequence of this structure is that the 'CAST' failure/success
> condition ends up being split into two separate 32-bit checks across
> __lsui_cmpxchg32() and its caller. This is a little fiddly to read and
> introduces some additional local variables which can be avoided if the
> check is done in one place.
> 
> Tweak __lsui_cmpxchg32() so that it performs the full 64-bit check on
> the value returned from the 'CAST' instruction and returns success to
> its caller only in the case that the cmpxchg() operation has succeeded.
> With that in place, simplify the outer loop in __lsui_futex_atomic_eor()
> to pass 'oldval' by reference and return unless the cmpxchg() operation
> returns -EAGAIN.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Yeoreum Yun <yeoreum.yun@arm.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/futex.h | 53 ++++++++++++----------------------
>  1 file changed, 18 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> index d1d2ff9d323a..db84a7b2de74 100644
> --- a/arch/arm64/include/asm/futex.h
> +++ b/arch/arm64/include/asm/futex.h
> @@ -151,42 +151,31 @@ __lsui_cmpxchg64(u64 __user *uaddr, u64 *oldval, u64 newval)
>  }
>  
>  static __always_inline int
> -__lsui_cmpxchg32(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
> +__lsui_cmpxchg32(u32 __user *uaddr, u32 *oldval, u32 newval)
>  {
>  	u64 __user *uaddr64;
>  	bool futex_pos, other_pos;
> -	u32 other, orig_other;
>  	union {
>  		u32 futex[2];
>  		u64 raw;
> -	} oval64, orig64, nval64;
> +	} orig64, oval64, nval64;
>  
>  	uaddr64 = (u64 __user *)PTR_ALIGN_DOWN(uaddr, sizeof(u64));
>  	futex_pos = !IS_ALIGNED((unsigned long)uaddr, sizeof(u64));
>  	other_pos = !futex_pos;
>  
> -	oval64.futex[futex_pos] = oldval;
> -	if (get_user(oval64.futex[other_pos], (u32 __user *)uaddr64 + other_pos))
> +	orig64.futex[futex_pos] = *oldval;
> +	if (get_user(orig64.futex[other_pos], (u32 __user *)uaddr64 + other_pos))
>  		return -EFAULT;
>  
> -	orig64.raw = oval64.raw;
> -
> +	nval64 = oval64 = orig64;
>  	nval64.futex[futex_pos] = newval;
> -	nval64.futex[other_pos] = oval64.futex[other_pos];
>  
>  	if (__lsui_cmpxchg64(uaddr64, &oval64.raw, nval64.raw))
>  		return -EFAULT;
>  
> -	oldval = oval64.futex[futex_pos];
> -	other = oval64.futex[other_pos];
> -	orig_other = orig64.futex[other_pos];
> -
> -	if (other != orig_other)
> -		return -EAGAIN;
> -
> -	*oval = oldval;
> -
> -	return 0;
> +	*oldval = oval64.futex[futex_pos];
> +	return oval64.raw == orig64.raw ? 0 : -EAGAIN;
>  }
>  
>  static __always_inline int
> @@ -202,7 +191,7 @@ __lsui_futex_atomic_and(int oparg, u32 __user *uaddr, int *oval)
>  static __always_inline int
>  __lsui_futex_atomic_eor(int oparg, u32 __user *uaddr, int *oval)
>  {
> -	u32 oldval, newval, val;
> +	u32 oldval, newval;
>  	int ret, i;
>  
>  	if (get_user(oldval, uaddr))
> @@ -214,33 +203,27 @@ __lsui_futex_atomic_eor(int oparg, u32 __user *uaddr, int *oval)
>  	for (i = 0; i < FUTEX_MAX_LOOPS; i++) {
>  		newval = oldval ^ oparg;
>  
> -		ret = __lsui_cmpxchg32(uaddr, oldval, newval, &val);
> -		switch (ret) {
> -		case -EFAULT:
> -			return ret;
> -		case -EAGAIN:
> -			continue;
> -		}
> -
> -		if (val == oldval) {
> -			*oval = val;
> -			return 0;
> -		}
> -
> -		oldval = val;
> +		ret = __lsui_cmpxchg32(uaddr, &oldval, newval);
> +		if (ret != -EAGAIN)
> +			break;
>  	}
>  
> -	return -EAGAIN;
> +	*oval = oldval;
> +	return ret;
>  }
>  
>  static __always_inline int
>  __lsui_futex_cmpxchg(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
>  {
> +	int ret;
> +
>  	/*
>  	 * Callers of futex_atomic_cmpxchg_inatomic() already retry on
>  	 * -EAGAIN, no need for another loop of max retries.
>  	 */
> -	return __lsui_cmpxchg32(uaddr, oldval, newval, oval);
> +	ret = __lsui_cmpxchg32(uaddr, &oldval, newval);
> +	*oval = oldval;
> +	return ret;
>  }
>  #endif	/* CONFIG_ARM64_LSUI */
>  
> -- 
> 2.54.0.563.g4f69b47b94-goog
> 

-- 
Sincerely,
Yeoreum Yun