linux-kernel - Re: [PATCH v8 5/5] arm64: futex: support futex with FEAT

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: <aMqx67lZZEgYW-x5@J2N7QTR9R3>
Date: Wed, 17 Sep 2025 14:04:43 +0100
From: Mark Rutland <mark.rutland@....com>
To: Yeoreum Yun <yeoreum.yun@....com>
Cc: catalin.marinas@....com, will@...nel.org, broonie@...nel.org,
	maz@...nel.org, oliver.upton@...ux.dev, joey.gouly@....com,
	james.morse@....com, ardb@...nel.org, scott@...amperecomputing.com,
	suzuki.poulose@....com, yuzenghui@...wei.com,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 5/5] arm64: futex: support futex with FEAT_LSUI

On Wed, Sep 17, 2025 at 12:08:38PM +0100, Yeoreum Yun wrote:
> +static __always_inline int
> +__lsui_cmpxchg64(u64 __user *uaddr, u64 *oldval, u64 newval)
> +{
> +	int ret = 0;
> +
> +	asm volatile("// __lsui_cmpxchg64\n"
> +	__LSUI_PREAMBLE
> +"1:	casalt	%x2, %x3, %1\n"
> +"2:\n"
> +	_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %w0)
> +	: "+r" (ret), "+Q" (*uaddr), "+r" (*oldval)
> +	: "r" (newval)
> +	: "memory");
> +
> +	return ret;
> +}
> +
> +static __always_inline int
> +__lsui_cmpxchg32(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
> +{
> +	u64 __user *uaddr_al;

Please use 'uaddr64' to match the other 64-bit variables.

I assume that the '_al' suffix is meant to be short for 'aligned', but I
think using '64' is more consistent and clearer.

> +	u64 oval64, nval64, tmp;

Likewise, 'orig64' would be clearer than 'tmp' here.

> +	static const u64 hi_mask = IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ?
> +		GENMASK_U64(63, 32): GENMASK_U64(31, 0);
> +	static const u8 hi_shift = IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? 32 : 0;
> +	static const u8 lo_shift = IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? 0 : 32;
> +
> +	uaddr_al = (u64 __user *) PTR_ALIGN_DOWN(uaddr, sizeof(u64));
> +	if (get_user(oval64, uaddr_al))
> +		return -EFAULT;
> +
> +	if ((u32 __user *)uaddr_al != uaddr) {
> +		nval64 = ((oval64 & ~hi_mask) | ((u64)newval << hi_shift));
> +		oval64 = ((oval64 & ~hi_mask) | ((u64)oldval << hi_shift));
> +	} else {
> +		nval64 = ((oval64 & hi_mask) | ((u64)newval << lo_shift));
> +		oval64 = ((oval64 & hi_mask) | ((u64)oldval << lo_shift));
> +	}
> +
> +	tmp = oval64;
> +
> +	if (__lsui_cmpxchg64(uaddr_al, &oval64, nval64))
> +		return -EFAULT;
> +
> +	if (tmp != oval64)
> +		return -EAGAIN;

This means that we'll immediately return -EAGAIN upon a spurious failure
(where the adjacent 4 bytes have changed), whereas the LL/SC ops would
retry FUTEX_MAX_LOOPS before returning -EGAIN.

I suspect we want to retry here (or in the immediate caller).

> +
> +	*oval = oldval;
> +
> +	return 0;
> +}

Aside from the retry issue, I *think* you can simplify this to something
like:

static __always_inline int
__lsui_cmpxchg32(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
{
	uaddr64 = (u64 __user *)PTR_ALIGN_DOWN(uaddr, sizeof(u64));
	u64 oval64, nval64, orig64;

	if (get_user(oval64, uaddr64)
		return -EFAULT;
	
	if (IS_ALIGNED(addr, sizeof(u64)) == IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN))  {
		FIELD_MODIFY(GENMASK_U64(31, 0), &oval64, oldval);
		FIELD_MODIFY(GENMASK_U64(31, 0), &nval64, newval);
	} else {
		FIELD_MODIFY(GENMASK_U64(63, 32), &oval64, oldval);
		FIELD_MODIFY(GENMASK_U64(63, 32), &nval64, newval);
	}
	orig64 = oval64;

	if (__lsui_cmpxchg64(uaddr_al, &oval64, nval64))
		return -EFAULT;
	
	if (oval64 != orig64)
		return -EAGAIN;

	*oval = oldval;
	return 0;
}

Mark.

> +
> +static __always_inline int
> +__lsui_futex_atomic_and(int oparg, u32 __user *uaddr, int *oval)
> +{
> +	return __lsui_futex_atomic_andnot(~oparg, uaddr, oval);
> +}
> +
> +static __always_inline int
> +__lsui_futex_atomic_eor(int oparg, u32 __user *uaddr, int *oval)
> +{
> +	u32 oldval, newval;
> +
> +	/*
> +	 * there are no ldteor/stteor instructions...
> +	 */
> +	if (get_user(oldval, uaddr))
> +		return -EFAULT;
> +
> +	newval = oldval ^ oparg;
> +
> +	return __lsui_cmpxchg32(uaddr, oldval, newval, oval);
> +
> +}
> +
> +static __always_inline int
> +__lsui_futex_cmpxchg(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
> +{
> +	return __lsui_cmpxchg32(uaddr, oldval, newval, oval);
> +}
> +
> +#define __lsui_llsc_body(op, ...)					\
> +({									\
> +	alternative_has_cap_likely(ARM64_HAS_LSUI) ?			\
> +		__lsui_##op(__VA_ARGS__) : __llsc_##op(__VA_ARGS__);	\
> +})
> +
> +#else	/* CONFIG_AS_HAS_LSUI */
> +
> +#define __lsui_llsc_body(op, ...)	__llsc_##op(__VA_ARGS__)
> +
> +#endif	/* CONFIG_AS_HAS_LSUI */
> +
> +
>  #define FUTEX_ATOMIC_OP(op)						\
>  static __always_inline int						\
>  __futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval)		\
>  {									\
> -	return __llsc_futex_atomic_##op(oparg, uaddr, oval);		\
> +	return __lsui_llsc_body(futex_atomic_##op, oparg, uaddr, oval);	\
>  }
>  
>  FUTEX_ATOMIC_OP(add)
> -- 
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>