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 for Android: free password hash cracker in your pocket

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

Message-ID: <aKCBlVP084tZnfqH@e129823.arm.com>
Date: Sat, 16 Aug 2025 14:03:17 +0100
From: Yeoreum Yun <yeoreum.yun@....com>
To: Catalin Marinas <catalin.marinas@....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

Hi Catalin,

[...]
> > 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.

Sorry for bothering you. I'll restore indentation to make it clear.
and yes. the only difference is to change you mention it.

>
> >  	_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?)

Not really. I keep the "oldval" as it is.
Thanks.

>
> >  	  "+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.
>

Yes. It's a small optimisation -- not use "mov" instruction.
I'll separate that part.

Thanks!

--
Sincerely,
Yeoreum Yun