Re: [PATCH 5/5] s390/uaccess: add cmpxchg_user_key()

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On Wed, 2022-11-02 at 15:19 +0100, Heiko Carstens wrote:
> Add cmpxchg_user_key() which allows to execute a compare and exchange
> on a user space address. This allows also to specify a storage key
> which makes sure that key-controlled protection is considered.
> 
> This is based on a patch written by Janis Schoetterl-Glausch.
> 
> Link: https://lore.kernel.org/all/20220930210751.225873-2-scgl@linux.ibm.com
> Cc: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/include/asm/uaccess.h | 183 ++++++++++++++++++++++++++++++++
>  1 file changed, 183 insertions(+)
> 
> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
> index f7038b800cc3..9bbdecb80e06 100644
> --- a/arch/s390/include/asm/uaccess.h
> +++ b/arch/s390/include/asm/uaccess.h
> @@ -390,4 +390,187 @@ do {									\
>  		goto err_label;						\
>  } while (0)
>  
> +void __cmpxchg_user_key_called_with_bad_pointer(void);
> +
> +static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
> +					      __uint128_t old, __uint128_t new,
> +					      unsigned long key, int size)
> +{
> +	int rc = 0;
> +
> +	switch (size) {
> +	case 1: {
> +		unsigned int prev, tmp, shift;
> +
> +		shift = (3 ^ (address & 3)) << 3;
> +		address ^= address & 3;
> +		asm volatile(
> +			"	spka	0(%[key])\n"
> +			"	sacf	256\n"
> +			"0:	l	%[prev],%[address]\n"
> +			"1:	nr	%[prev],%[mask]\n"
> +			"	lr	%[tmp],%[prev]\n"
> +			"	or	%[prev],%[old]\n"
> +			"	or	%[tmp],%[new]\n"
> +			"2:	cs	%[prev],%[tmp],%[address]\n"
> +			"3:	jnl	4f\n"
> +			"	xr	%[tmp],%[prev]\n"
> +			"	nr	%[tmp],%[mask]\n"

Are you only entertaining cosmetic changes to cmpxchg.h?
The loop condition being imprecise seems non-ideal.

> +			"	jnz	1b\n"
> +			"4:	sacf	768\n"
> +			"	spka	%[default_key]\n"
> +			EX_TABLE_UA_LOAD_REG(0b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(1b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(2b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(3b, 4b, %[rc], %[prev])
> +			: [rc] "+&d" (rc),
> +			  [prev] "=&d" (prev),
> +			  [tmp] "=&d" (tmp),
> +			  [address] "+Q" (*(int *)address)
> +			: [old] "d" (((unsigned int)old & 0xff) << shift),
> +			  [new] "d" (((unsigned int)new & 0xff) << shift),
> +			  [mask] "d" (~(0xff << shift)),
> +			  [key] "a" (key),

Why did you get rid of the << 4 shift?
That's inconsistent with the other uaccess functions that take an access key.

> +			  [default_key] "J" (PAGE_DEFAULT_KEY)
> +			: "memory", "cc");
> +		*(unsigned char *)uval = prev >> shift;
> +		return rc;
> +	}

[...]