linux-kernel - Re: [PATCH v6 4/5] arm64: futex: refactor futex atomic operation

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: <aJ9in0fUI01J3a4S@arm.com>
Date: Fri, 15 Aug 2025 17:38:55 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Yeoreum Yun <yeoreum.yun@....com>
Cc: will@...nel.org, broonie@...nel.org, maz@...nel.org,
	oliver.upton@...ux.dev, shameerali.kolothum.thodi@...wei.com,
	joey.gouly@....com, james.morse@....com, ardb@...nel.org,
	scott@...amperecomputing.com, suzuki.poulose@....com,
	yuzenghui@...wei.com, mark.rutland@....com,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 4/5] arm64: futex: refactor futex atomic operation

On Mon, Aug 11, 2025 at 05:36:34PM +0100, Yeoreum Yun wrote:
> Refactor futex atomic operations using ll/sc method with
> clearing PSTATE.PAN to prepare to apply FEAT_LSUI on them.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>
> ---
>  arch/arm64/include/asm/futex.h | 183 ++++++++++++++++++++++-----------
>  1 file changed, 124 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> index bc06691d2062..fdec4f3f2b15 100644
> --- a/arch/arm64/include/asm/futex.h
> +++ b/arch/arm64/include/asm/futex.h
> @@ -7,73 +7,164 @@
>  
>  #include <linux/futex.h>
>  #include <linux/uaccess.h>
> +#include <linux/stringify.h>
>  
>  #include <asm/errno.h>
>  
> -#define FUTEX_MAX_LOOPS	128 /* What's the largest number you can think of? */
> +#define LLSC_MAX_LOOPS	128 /* What's the largest number you can think of? */
>  
> -#define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg)		\
> -do {									\
> -	unsigned int loops = FUTEX_MAX_LOOPS;				\
> +#define LLSC_FUTEX_ATOMIC_OP(op, asm_op)				\
> +static __always_inline int						\
> +__llsc_futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval)	\
> +{									\
> +	unsigned int loops = LLSC_MAX_LOOPS;				\
> +	int ret, val, tmp;						\
>  									\
>  	uaccess_enable_privileged();					\
> -	asm volatile(							\
> -"	prfm	pstl1strm, %2\n"					\
> -"1:	ldxr	%w1, %2\n"						\
> -	insn "\n"							\
> -"2:	stlxr	%w0, %w3, %2\n"						\
> -"	cbz	%w0, 3f\n"						\
> -"	sub	%w4, %w4, %w0\n"					\
> -"	cbnz	%w4, 1b\n"						\
> -"	mov	%w0, %w6\n"						\
> -"3:\n"									\
> -"	dmb	ish\n"							\
> +	asm volatile("// __llsc_futex_atomic_" #op "\n"		\
> +	"	prfm	pstl1strm, %2\n"				\
> +	"1:	ldxr	%w1, %2\n"					\
> +	"	" #asm_op "	%w3, %w1, %w5\n"			\
> +	"2:	stlxr	%w0, %w3, %2\n"					\
> +	"	cbz	%w0, 3f\n"					\
> +	"	sub	%w4, %w4, %w0\n"				\
> +	"	cbnz	%w4, 1b\n"					\
> +	"	mov	%w0, %w6\n"					\
> +	"3:\n"								\
> +	"	dmb	ish\n"						\

Don't change indentation and code in the same patch, it makes it harder
to follow what you actually changed. I guess the only difference is
asm_op instead of insn.

>  	_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0)				\
>  	_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0)				\
> -	: "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp),	\
> +	: "=&r" (ret), "=&r" (val), "+Q" (*uaddr), "=&r" (tmp),		\

And here you changed oldval to val (was this necessary?)

>  	  "+r" (loops)							\
>  	: "r" (oparg), "Ir" (-EAGAIN)					\
>  	: "memory");							\
>  	uaccess_disable_privileged();					\
> -} while (0)
> +									\
> +	if (!ret)							\
> +		*oval = val;						\
> +									\
> +	return ret;							\
> +}
> +
> +LLSC_FUTEX_ATOMIC_OP(add, add)
> +LLSC_FUTEX_ATOMIC_OP(or, orr)
> +LLSC_FUTEX_ATOMIC_OP(and, and)
> +LLSC_FUTEX_ATOMIC_OP(eor, eor)
> +
> +static __always_inline int
> +__llsc_futex_atomic_set(int oparg, u32 __user *uaddr, int *oval)
> +{
> +	unsigned int loops = LLSC_MAX_LOOPS;
> +	int ret, val;
> +
> +	uaccess_enable_privileged();
> +	asm volatile("//__llsc_futex_xchg\n"
> +	"	prfm	pstl1strm, %2\n"
> +	"1:	ldxr	%w1, %2\n"
> +	"2:	stlxr	%w0, %w4, %2\n"
> +	"	cbz	%w3, 3f\n"
> +	"	sub	%w3, %w3, %w0\n"
> +	"	cbnz	%w3, 1b\n"
> +	"	mov	%w0, %w5\n"
> +	"3:\n"
> +	"	dmb	ish\n"
> +	_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0)
> +	_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0)
> +	: "=&r" (ret), "=&r" (val), "+Q" (*uaddr), "+r" (loops)
> +	: "r" (oparg), "Ir" (-EAGAIN)
> +	: "memory");
> +	uaccess_disable_privileged();

Was this separate function just to avoid the "mov" instruction for the
"set" case? The patch description states that the reworking is necessary
for the FEAT_LSUI use but it looks to me like it does more. Please split
it in separate patches, though I'd leave any potential optimisation for
a separate series and keep the current code as close as possible to the
original one.

-- 
Catalin