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

Janis Schoetterl-Glausch
2022-11-09T15:46:45Z

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;
> + } …