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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 8/26/2026 12:53 PM, sashiko-bot@kernel.org wrote:
> 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?

It is easy to misread/misunderstand the official gcc documentations [1].
It specifically mentions that
"Single word values will be allocated either in ax or dx".

Here on x86_64 arch, the __old is 128 bit (i.e. it cannot fit in single
register) so the compiler will always split it in rdx:rax pair.


>> +		       [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?
> 

The BUILD_BUG_ON(1) will only cause compile failure if there the code is
getting used. As sean mentioned in the previous version, compiler
elimiates this code if it is not used.

Thanks
Sairaj

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