Re: [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The inline assembly for 128-bit cmpxchg uses the unreliable `+A` constraint on x86-64, risking silent memory corruption or build failures.
- [Medium] The 32-bit stub for `__try_cmpxchg128_user_asm` relies on `BUILD_BUG_ON(1)`, which will break the 32-bit build for all callers of `unsafe_try_cmpxchg_user`.
--

commit 778924f40bf568ddfaeffc09239d3c0e2a09bba6
Author: Sairaj Kodilkar <sarunkod@amd.com>

x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands

This commit extends the existing user CMPXCHG helpers to support 16-byte
operands on x86-64 using the "cmpxchg16b" instruction. On 32-bit kernels,
it stubs the helper to fail, as cmpxchg16b requires 64-bit registers. KVM
uses this to atomically emulate guest cmpxchg16b on guest RAM mapped via
userspace addresses.

> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 3a0dd3c2b233e..39bcf327664ea 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -407,6 +407,25 @@ do {									\
[ ... ]
> +#else // !CONFIG_X86_32
> +#define __try_cmpxchg128_user_asm(_ptr, _pold, _new, label)	({	\
> +	bool success;							\
> +	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
> +	__typeof__(*(_ptr)) __old = *_old;				\
> +	__typeof__(*(_ptr)) __new = (_new);				\
> +	asm_goto_output("\n"						\
> +		     "1: " LOCK_PREFIX "cmpxchg16b %[ptr]\n"		\
> +		     _ASM_EXTABLE_UA(1b, %l[label])			\
> +		     : "=@ccz" (success),				\
> +		       "+A" (__old),					\

[Severity: High]
Does using the "+A" constraint for a 128-bit value provide the strict
rdx:rax register ordering required by the cmpxchg16b instruction?

The kernel documents in arch/x86/include/asm/asm.h that the "A" constraint
on x86-64 means rax or rdx, rather than the rdx:rax pair. If the compiler
does not guarantee the rdx:rax ordering for __int128/u128 with "+A", could
this lead to silent memory corruption with swapped operands, or outright
build failures on compilers like Clang?

The standard pattern used elsewhere (like __arch_cmpxchg128 in
arch/x86/include/asm/cmpxchg_64.h) splits the 128-bit value into a union
of two 64-bit halves and explicitly binds them to "+a" (rax) and "+d" (rdx).
Would it be safer to use that pattern here?

> +		       [ptr] "+m" (*_ptr)				\
> +		     : "b" ((u64)__new),				\
> +		       "c" ((u64)((u128)__new >> 64))			\
> +		     : "memory"						\
> +		     : label);						\
> +	if (unlikely(!success))						\
> +		*_old = __old;						\
> +	likely(success);					})
>  #endif // CONFIG_X86_32
>  #else  // !CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
[ ... ]
> +#else //!CONFIG_X86_32
> +#define __try_cmpxchg128_user_asm(_ptr, _pold, _new, label)	({	\
> +	int __result;							\
> +	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
> +	__typeof__(*(_ptr)) __old = *_old;				\
> +	__typeof__(*(_ptr)) __new = (_new);				\
> +	asm volatile("\n"						\
> +		     "1: " LOCK_PREFIX "cmpxchg16b %[ptr]\n"		\
> +		     "mov $0, %[result]\n\t"				\
> +		     "setz %b[result]\n"				\
> +		     "2:\n"						\
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG,	\
> +					   %[result])			\
> +		     : [result] "=q" (__result),			\
> +		       "+A" (__old),					\

[Severity: High]
Similar to the asm_goto_output block above, this fallback path also binds
the 128-bit __old variable to the "+A" constraint.

> +		       [ptr] "+m" (*_ptr)				\
> +		     : "b" ((u64)__new),				\
> +		       "c" ((u64)((u128)__new >> 64))			\
> +		     : "memory", "cc");					\
> +	if (unlikely(__result < 0))					\
> +		goto label;						\
> +	if (unlikely(!__result))					\
> +		*_old = __old;						\
> +	likely(__result);					})
>  #endif // CONFIG_X86_32
>  #endif // CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
[ ... ]
> @@ -551,11 +594,18 @@ do {										\
>  
>  extern void __try_cmpxchg_user_wrong_size(void);
>  
> -#ifndef CONFIG_X86_32
> +#ifdef CONFIG_X86_32
> +/* Always fail on 32 bit arch as it do not support 128 cmpxchg (i.e. cmpxchg16b
> + * instruction).
> + */
> +#define __try_cmpxchg128_user_asm(_ptr, _pold, _new, label) ({ BUILD_BUG_ON(1); 0; })

[Severity: Medium]
Will this BUILD_BUG_ON(1) break 32-bit x86 builds that call the
unsafe_try_cmpxchg_user() macro for perfectly valid 32-bit or 64-bit sizes?

Because unsafe_try_cmpxchg_user() dispatches via a switch(sizeof(*(_ptr)))
statement, compilers like Clang/LLVM eagerly evaluate all branches during
semantic analysis, including the 16-byte branch. If evaluated on 32-bit
builds, BUILD_BUG_ON(1) will trigger a compile-time error regardless of
whether the 16-byte path is actually reachable.

Since the commit message says "stub the helper to always return failure",
would it be better to return a runtime error code here instead of using
a build bug?

> +#else
>  #define __try_cmpxchg64_user_asm(_ptr, _oldp, _nval, _label)		\
>  	__try_cmpxchg_user_asm("q", "r", (_ptr), (_oldp), (_nval), _label)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826070004.8100-1-sarunkod@amd.com?part=1