Add cmpxchg functionality similar to that in cmpxchg.h except that the
target is a user space address and that the address' storage key is
matched with the access_key argument in order to honor key-controlled
protection.
The access is performed by changing to the secondary-spaces mode and
setting the PSW key for the duration of the compare and swap.
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
Possible variations:
* check the assumptions made in cmpxchg_user_key_size and error out
* call functions called by copy_to_user
* access_ok? is a nop
* should_fail_usercopy?
* instrument_copy_to_user? doesn't make sense IMO
* don't be overly strict in cmpxchg_user_key
arch/s390/include/asm/uaccess.h | 189 ++++++++++++++++++++++++++++++++
1 file changed, 189 insertions(+)
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index f7038b800cc3..f148f5a22c93 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -19,6 +19,8 @@
#include <asm/extable.h>
#include <asm/facility.h>
#include <asm-generic/access_ok.h>
+#include <asm/page.h>
+#include <linux/log2.h>
void debug_user_asce(int exit);
@@ -390,4 +392,191 @@ do { \
goto err_label; \
} while (0)
+static __always_inline int __cmpxchg_user_key_small(int size, u64 address,
+ unsigned __int128 *old_p,
+ unsigned __int128 new, u8 access_key)
+{
+ u32 shift, mask, old_word, new_word, align_mask, tmp;
+ u64 aligned;
+ int ret = -EFAULT;
+
+ switch (size) {
+ case 2:
+ align_mask = 2;
+ aligned = (address ^ (address & align_mask));
+ shift = (sizeof(u32) - (address & align_mask) - size) * 8;
+ mask = 0xffff << shift;
+ old_word = ((u16)*old_p) << shift;
+ new_word = ((u16)new) << shift;
+ break;
+ case 1:
+ align_mask = 3;
+ aligned = (address ^ (address & align_mask));
+ shift = (sizeof(u32) - (address & align_mask) - size) * 8;
+ mask = 0xff << shift;
+ old_word = ((u8)*old_p) << shift;
+ new_word = ((u8)new) << shift;
+ break;
+ }
+ tmp = old_word; /* don't modify *old_p on fault */
+ asm volatile(
+ "spka 0(%[access_key])\n"
+ " sacf 256\n"
+ "0: l %[tmp],%[aligned]\n"
+ "1: nr %[tmp],%[mask]\n"
+ " xilf %[mask],0xffffffff\n"
+ " or %[new_word],%[tmp]\n"
+ " or %[tmp],%[old_word]\n"
+ "2: lr %[old_word],%[tmp]\n"
+ "3: cs %[tmp],%[new_word],%[aligned]\n"
+ "4: jnl 5f\n"
+ /* We'll restore old_word before the cs, use reg for the diff */
+ " xr %[old_word],%[tmp]\n"
+ /* Apply diff assuming only bits outside target byte(s) changed */
+ " xr %[new_word],%[old_word]\n"
+ /* If prior assumption false we exit loop, so not an issue */
+ " nr %[old_word],%[mask]\n"
+ " jz 2b\n"
+ "5: ipm %[ret]\n"
+ " srl %[ret],28\n"
+ "6: sacf 768\n"
+ " spka %[default_key]\n"
+ EX_TABLE(0b, 6b) EX_TABLE(1b, 6b)
+ EX_TABLE(3b, 6b) EX_TABLE(4b, 6b)
+ : [old_word] "+&d" (old_word),
+ [new_word] "+&d" (new_word),
+ [tmp] "+&d" (tmp),
+ [aligned] "+Q" (*(u32 *)aligned),
+ [ret] "+d" (ret)
+ : [access_key] "a" (access_key << 4),
+ [mask] "d" (~mask),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "cc"
+ );
+ *old_p = (tmp & mask) >> shift;
+ return ret;
+}
+
+/**
+ * cmpxchg_user_key_size() - cmpxchg with user space target, honoring storage keys
+ * @size: Size of the value being cmpxchg'ed, one of 1,2,4,8,16.
+ * @address: User space address of value to compare to *@old_p and exchange with
+ * @new. Must be aligned to @size.
+ * @old_p: Pointer to old value. Interpreted as a @size byte integer and compared
+ * to the content pointed to by @address in order to determine if the
+ * exchange occurs. The value read from @address is written back to *@old_p.
+ * @new: New value to place at @address, interpreted as a @size byte integer.
+ * @access_key: Access key to use for checking storage key protection.
+ *
+ * Perform a cmpxchg on a user space target, honoring storage key protection.
+ * @access_key alone determines how key checking is performed, neither
+ * storage-protection-override nor fetch-protection-override apply.
+ *
+ * Return: 0: successful exchange
+ * 1: exchange failed
+ * -EFAULT: @address not accessible or not naturally aligned
+ * -EINVAL: invalid @size
+ */
+static __always_inline int cmpxchg_user_key_size(int size, void __user *address,
+ unsigned __int128 *old_p,
+ unsigned __int128 new, u8 access_key)
+{
+ union {
+ u32 word;
+ u64 doubleword;
+ } old;
+ int ret = -EFAULT;
+
+ /*
+ * The following assumes that:
+ * * the current psw key is the default key
+ * * no storage protection overrides are in effect
+ */
+ might_fault();
+ switch (size) {
+ case 16:
+ asm volatile(
+ "spka 0(%[access_key])\n"
+ " sacf 256\n"
+ "0: cdsg %[old],%[new],%[target]\n"
+ "1: ipm %[ret]\n"
+ " srl %[ret],28\n"
+ "2: sacf 768\n"
+ " spka %[default_key]\n"
+ EX_TABLE(0b, 2b) EX_TABLE(1b, 2b)
+ : [old] "+d" (*old_p),
+ [target] "+Q" (*(unsigned __int128 __user *)address),
+ [ret] "+d" (ret)
+ : [access_key] "a" (access_key << 4),
+ [new] "d" (new),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "cc"
+ );
+ return ret;
+ case 8:
+ old.doubleword = *old_p;
+ asm volatile(
+ "spka 0(%[access_key])\n"
+ " sacf 256\n"
+ "0: csg %[old],%[new],%[target]\n"
+ "1: ipm %[ret]\n"
+ " srl %[ret],28\n"
+ "2: sacf 768\n"
+ " spka %[default_key]\n"
+ EX_TABLE(0b, 2b) EX_TABLE(1b, 2b)
+ : [old] "+d" (old.doubleword),
+ [target] "+Q" (*(u64 __user *)address),
+ [ret] "+d" (ret)
+ : [access_key] "a" (access_key << 4),
+ [new] "d" ((u64)new),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "cc"
+ );
+ *old_p = old.doubleword;
+ return ret;
+ case 4:
+ old.word = *old_p;
+ asm volatile(
+ "spka 0(%[access_key])\n"
+ " sacf 256\n"
+ "0: cs %[old],%[new],%[target]\n"
+ "1: ipm %[ret]\n"
+ " srl %[ret],28\n"
+ "2: sacf 768\n"
+ " spka %[default_key]\n"
+ EX_TABLE(0b, 2b) EX_TABLE(1b, 2b)
+ : [old] "+d" (old.word),
+ [target] "+Q" (*(u32 __user *)address),
+ [ret] "+d" (ret)
+ : [access_key] "a" (access_key << 4),
+ [new] "d" ((u32)new),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "cc"
+ );
+ *old_p = old.word;
+ return ret;
+ case 2:
+ case 1:
+ return __cmpxchg_user_key_small(size, (u64)address, old_p, new, access_key);
+ default:
+ return -EINVAL;
+ }
+}
+
+#define cmpxchg_user_key(target_p, old_p, new, access_key) \
+({ \
+ __typeof__(old_p) __old_p = (old_p); \
+ unsigned __int128 __old = *__old_p; \
+ size_t __size = sizeof(*(target_p)); \
+ int __ret; \
+ \
+ BUILD_BUG_ON(__size != sizeof(*__old_p)); \
+ BUILD_BUG_ON(__size != sizeof(new)); \
+ BUILD_BUG_ON(__size > 16 || !is_power_of_2(__size)); \
+ __ret = cmpxchg_user_key_size(__size, (target_p), &__old, (new), \
+ (access_key)); \
+ *__old_p = __old; \
+ __ret; \
+})
+
#endif /* __S390_UACCESS_H */
--
2.34.1Hi Janis, On Wed, Oct 12, 2022 at 10:56:01PM +0200, Janis Schoetterl-Glausch wrote: > Add cmpxchg functionality similar to that in cmpxchg.h except that the > target is a user space address and that the address' storage key is > matched with the access_key argument in order to honor key-controlled > protection. > The access is performed by changing to the secondary-spaces mode and > setting the PSW key for the duration of the compare and swap. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > > > Possible variations: > * check the assumptions made in cmpxchg_user_key_size and error out > * call functions called by copy_to_user > * access_ok? is a nop > * should_fail_usercopy? > * instrument_copy_to_user? doesn't make sense IMO > * don't be overly strict in cmpxchg_user_key > > > arch/s390/include/asm/uaccess.h | 189 ++++++++++++++++++++++++++++++++ > 1 file changed, 189 insertions(+) So finally I send the uaccess/cmpxchg patches in reply to this mail. Sorry for the long delay! The first three patches are not required for the functionality you need, but given that I always stress that the code should be consistent I include them anyway. The changes are probably quite obvious: - Keep uaccess cmpxchg code more or less identical to regular cmpxchg code. I wasn't able to come up with a readable code base which could be used for both variants. - Users may only use the cmpxchg_user_key() macro - _not_ the inline function, which is an internal API. This will require that you need to add a switch statement and couple of casts within the KVM code, but shouldn't have much of an impact on the generated code. - Cause link error for non-integral sizes, similar to other uaccess functions. - cmpxchg_user_key() has now a simple return value: 0 or -EFAULT, and writes the old value to a location provided by a pointer. This is quite similar to the futex code. Users must compare the old and expected value to figure out if something was exchanged. Note that this is in most cases more efficient than extracting the condition code from the PSW with ipm, since nowadays we have instructions like compare and branch relative on condition, etc. - Couple of other minor changes which I forgot. Code is untested (of course :) ). Please give it a try and let me know if this is good enough for your purposes. I also did not limit the number of retries for the one and two byte scenarion. Before doing that we need to have proof that there really is a problem. Maybe Nico or you will give this a try. Thanks, Heiko
On Wed, 2022-11-02 at 15:12 +0100, Heiko Carstens wrote: > [...] > I also did not limit the number of retries for the one and two byte > scenarion. Before doing that we need to have proof that there really is a > problem. Maybe Nico or you will give this a try. I wrote a memop selftest testcase where the main thread uses the one byte cmpxchg while n vcpus flip adjacent bits. The time the test case runs increases superlinearly with n. With 248 vcpus, 1000 one byte cmpxchgs take 25s. I'm not sure how meaningful the test is since the worst case would be if the threads hammering the word would run on a cpu dedicated to them. In any case, why not err on the side of caution and limit the iterations? I'll send an rfc patch. > > Thanks, > Heiko
Quoting Janis Schoetterl-Glausch (2022-11-16 20:36:46) > On Wed, 2022-11-02 at 15:12 +0100, Heiko Carstens wrote: > > > [...] > > > I also did not limit the number of retries for the one and two byte > > scenarion. Before doing that we need to have proof that there really is a > > problem. Maybe Nico or you will give this a try. > > I wrote a memop selftest testcase where the main thread uses the one byte cmpxchg > while n vcpus flip adjacent bits. The time the test case runs increases superlinearly with n. > With 248 vcpus, 1000 one byte cmpxchgs take 25s. > I'm not sure how meaningful the test is since the worst case would be if the threads hammering > the word would run on a cpu dedicated to them. > > In any case, why not err on the side of caution and limit the iterations? > I'll send an rfc patch. I agree, limiting sounds like the safe choice.
cmpxchg_user_key for byte and short values is implemented via a one word
cmpxchg loop. Give up trying to perform the cmpxchg if it fails too
often because of contention on the cache line. This ensures that the
thread cannot become stuck in the kernel.
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
128 might seem like a small number, but it actually seems to be plenty.
I could not get it to return EAGAIN with MAX_LOOP being 8 while 248
vcpus/threads are hammering the same word.
This could mean that we don't actually need to limit the number of
retries, but then, I didn't simulate the absolute worst case, where
the competing threads are running on dedicated cpus.
arch/s390/include/asm/uaccess.h | 35 +++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index d028ee59e941..f2d3a4e27963 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -392,6 +392,8 @@ do { \
void __cmpxchg_user_key_called_with_bad_pointer(void);
+#define CMPXCHG_USER_KEY_MAX_LOOPS 128
+
static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
__uint128_t old, __uint128_t new,
unsigned long key, int size)
@@ -400,7 +402,7 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
switch (size) {
case 1: {
- unsigned int prev, shift, mask, _old, _new;
+ unsigned int prev, shift, mask, _old, _new, count;
shift = (3 ^ (address & 3)) << 3;
address ^= address & 3;
@@ -410,6 +412,7 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
asm volatile(
" spka 0(%[key])\n"
" sacf 256\n"
+ " llill %[count],%[max_loops]\n"
"0: l %[prev],%[address]\n"
"1: nr %[prev],%[mask]\n"
" xilf %[mask],0xffffffff\n"
@@ -421,7 +424,8 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
" xr %[tmp],%[prev]\n"
" xr %[new],%[tmp]\n"
" nr %[tmp],%[mask]\n"
- " jz 2b\n"
+ " jnz 5f\n"
+ " brct %[count],2b\n"
"5: sacf 768\n"
" spka %[default_key]\n"
EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
@@ -433,15 +437,19 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
[address] "+Q" (*(int *)address),
[tmp] "+&d" (_old),
[new] "+&d" (_new),
- [mask] "+&d" (mask)
- : [key] "a" (key << 4),
- [default_key] "J" (PAGE_DEFAULT_KEY)
+ [mask] "+&d" (mask),
+ [count] "=a" (count)
+ : [key] "%[count]" (key << 4),
+ [default_key] "J" (PAGE_DEFAULT_KEY),
+ [max_loops] "J" (CMPXCHG_USER_KEY_MAX_LOOPS)
: "memory", "cc");
*(unsigned char *)uval = prev >> shift;
+ if (!count)
+ rc = -EAGAIN;
return rc;
}
case 2: {
- unsigned int prev, shift, mask, _old, _new;
+ unsigned int prev, shift, mask, _old, _new, count;
shift = (2 ^ (address & 2)) << 3;
address ^= address & 2;
@@ -451,6 +459,7 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
asm volatile(
" spka 0(%[key])\n"
" sacf 256\n"
+ " llill %[count],%[max_loops]\n"
"0: l %[prev],%[address]\n"
"1: nr %[prev],%[mask]\n"
" xilf %[mask],0xffffffff\n"
@@ -462,7 +471,8 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
" xr %[tmp],%[prev]\n"
" xr %[new],%[tmp]\n"
" nr %[tmp],%[mask]\n"
- " jz 2b\n"
+ " jnz 5f\n"
+ " brct %[count],2b\n"
"5: sacf 768\n"
" spka %[default_key]\n"
EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
@@ -474,11 +484,15 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
[address] "+Q" (*(int *)address),
[tmp] "+&d" (_old),
[new] "+&d" (_new),
- [mask] "+&d" (mask)
- : [key] "a" (key << 4),
- [default_key] "J" (PAGE_DEFAULT_KEY)
+ [mask] "+&d" (mask),
+ [count] "=a" (count)
+ : [key] "%[count]" (key << 4),
+ [default_key] "J" (PAGE_DEFAULT_KEY),
+ [max_loops] "J" (CMPXCHG_USER_KEY_MAX_LOOPS)
: "memory", "cc");
*(unsigned short *)uval = prev >> shift;
+ if (!count)
+ rc = -EAGAIN;
return rc;
}
case 4: {
@@ -568,6 +582,7 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
*
* Return: 0: cmpxchg executed
* -EFAULT: an exception happened when trying to access *@ptr
+ * -EAGAIN: maxed out number of retries (byte and short only)
*/
#define cmpxchg_user_key(ptr, uval, old, new, key) \
({ \
base-commit: b23ddf9d5a30f64a1a51a85f0d9e2553210b21a2
prerequisite-patch-id: c5cdc3ce7cdffc18c5e56abfb657c84141fb623a
--
2.34.1On Thu, Nov 17, 2022 at 11:07:45AM +0100, Janis Schoetterl-Glausch wrote: > cmpxchg_user_key for byte and short values is implemented via a one word > cmpxchg loop. Give up trying to perform the cmpxchg if it fails too > often because of contention on the cache line. This ensures that the > thread cannot become stuck in the kernel. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > > > 128 might seem like a small number, but it actually seems to be plenty. > I could not get it to return EAGAIN with MAX_LOOP being 8 while 248 > vcpus/threads are hammering the same word. > This could mean that we don't actually need to limit the number of > retries, but then, I didn't simulate the absolute worst case, where > the competing threads are running on dedicated cpus. > > > arch/s390/include/asm/uaccess.h | 35 +++++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 10 deletions(-) Looks good, also applied to wip/cmpxchg_user_branch. Thanks!
Make cmpxchg() inline assemblies more readable by using symbolic names
for operands.
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
arch/s390/include/asm/cmpxchg.h | 76 ++++++++++++++++++---------------
1 file changed, 42 insertions(+), 34 deletions(-)
diff --git a/arch/s390/include/asm/cmpxchg.h b/arch/s390/include/asm/cmpxchg.h
index 84c3f0d576c5..56fb8aa08945 100644
--- a/arch/s390/include/asm/cmpxchg.h
+++ b/arch/s390/include/asm/cmpxchg.h
@@ -96,56 +96,64 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
shift = (3 ^ (address & 3)) << 3;
address ^= address & 3;
asm volatile(
- " l %0,%2\n"
- "0: nr %0,%5\n"
- " lr %1,%0\n"
- " or %0,%3\n"
- " or %1,%4\n"
- " cs %0,%1,%2\n"
- " jnl 1f\n"
- " xr %1,%0\n"
- " nr %1,%5\n"
- " jnz 0b\n"
+ " l %[prev],%[address]\n"
+ "0: nr %[prev],%[mask]\n"
+ " lr %[tmp],%[prev]\n"
+ " or %[prev],%[old]\n"
+ " or %[tmp],%[new]\n"
+ " cs %[prev],%[tmp],%[address]\n"
+ " jnl 1f\n"
+ " xr %[tmp],%[prev]\n"
+ " nr %[tmp],%[mask]\n"
+ " jnz 0b\n"
"1:"
- : "=&d" (prev), "=&d" (tmp), "+Q" (*(int *) address)
- : "d" ((old & 0xff) << shift),
- "d" ((new & 0xff) << shift),
- "d" (~(0xff << shift))
+ : [prev] "=&d" (prev),
+ [tmp] "=&d" (tmp),
+ [address] "+Q" (*(int *)address)
+ : [old] "d" ((old & 0xff) << shift),
+ [new] "d" ((new & 0xff) << shift),
+ [mask] "d" (~(0xff << shift))
: "memory", "cc");
return prev >> shift;
case 2:
shift = (2 ^ (address & 2)) << 3;
address ^= address & 2;
asm volatile(
- " l %0,%2\n"
- "0: nr %0,%5\n"
- " lr %1,%0\n"
- " or %0,%3\n"
- " or %1,%4\n"
- " cs %0,%1,%2\n"
- " jnl 1f\n"
- " xr %1,%0\n"
- " nr %1,%5\n"
- " jnz 0b\n"
+ " l %[prev],%[address]\n"
+ "0: nr %[prev],%[mask]\n"
+ " lr %[tmp],%[prev]\n"
+ " or %[prev],%[old]\n"
+ " or %[tmp],%[new]\n"
+ " cs %[prev],%[tmp],%[address]\n"
+ " jnl 1f\n"
+ " xr %[tmp],%[prev]\n"
+ " nr %[tmp],%[mask]\n"
+ " jnz 0b\n"
"1:"
- : "=&d" (prev), "=&d" (tmp), "+Q" (*(int *) address)
- : "d" ((old & 0xffff) << shift),
- "d" ((new & 0xffff) << shift),
- "d" (~(0xffff << shift))
+ : [prev] "=&d" (prev),
+ [tmp] "=&d" (tmp),
+ [address] "+Q" (*(int *)address)
+ : [old] "d" ((old & 0xffff) << shift),
+ [new] "d" ((new & 0xffff) << shift),
+ [mask] "d" (~(0xffff << shift))
: "memory", "cc");
return prev >> shift;
case 4:
asm volatile(
- " cs %0,%3,%1\n"
- : "=&d" (prev), "+Q" (*(int *) address)
- : "0" (old), "d" (new)
+ " cs %[prev],%[new],%[address]\n"
+ : [prev] "=&d" (prev),
+ [address] "+Q" (*(int *)address)
+ : "0" (old),
+ [new] "d" (new)
: "memory", "cc");
return prev;
case 8:
asm volatile(
- " csg %0,%3,%1\n"
- : "=&d" (prev), "+QS" (*(long *) address)
- : "0" (old), "d" (new)
+ " csg %[prev],%[new],%[address]\n"
+ : [prev] "=&d" (prev),
+ [address] "+QS" (*(long *)address)
+ : "0" (old),
+ [new] "d" (new)
: "memory", "cc");
return prev;
}
--
2.34.1Make variables local to each case label. This limits the scope of
variables and allows to use proper types everywhere.
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
arch/s390/include/asm/cmpxchg.h | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/arch/s390/include/asm/cmpxchg.h b/arch/s390/include/asm/cmpxchg.h
index 56fb8aa08945..2ad057b94481 100644
--- a/arch/s390/include/asm/cmpxchg.h
+++ b/arch/s390/include/asm/cmpxchg.h
@@ -88,11 +88,10 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
unsigned long old,
unsigned long new, int size)
{
- unsigned long prev, tmp;
- int shift;
-
switch (size) {
- case 1:
+ case 1: {
+ unsigned int prev, tmp, shift;
+
shift = (3 ^ (address & 3)) << 3;
address ^= address & 3;
asm volatile(
@@ -115,7 +114,10 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
[mask] "d" (~(0xff << shift))
: "memory", "cc");
return prev >> shift;
- case 2:
+ }
+ case 2: {
+ unsigned int prev, tmp, shift;
+
shift = (2 ^ (address & 2)) << 3;
address ^= address & 2;
asm volatile(
@@ -138,7 +140,10 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
[mask] "d" (~(0xffff << shift))
: "memory", "cc");
return prev >> shift;
- case 4:
+ }
+ case 4: {
+ unsigned int prev;
+
asm volatile(
" cs %[prev],%[new],%[address]\n"
: [prev] "=&d" (prev),
@@ -147,7 +152,10 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
[new] "d" (new)
: "memory", "cc");
return prev;
- case 8:
+ }
+ case 8: {
+ unsigned long prev;
+
asm volatile(
" csg %[prev],%[new],%[address]\n"
: [prev] "=&d" (prev),
@@ -157,6 +165,7 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
: "memory", "cc");
return prev;
}
+ }
__cmpxchg_called_with_bad_pointer();
return old;
}
--
2.34.1Instead of using a digit for input constraints simply initialize the
corresponding output operand in C code and use a "+" constraint
modifier.
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
arch/s390/include/asm/cmpxchg.h | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/arch/s390/include/asm/cmpxchg.h b/arch/s390/include/asm/cmpxchg.h
index 2ad057b94481..1c5785b851ec 100644
--- a/arch/s390/include/asm/cmpxchg.h
+++ b/arch/s390/include/asm/cmpxchg.h
@@ -142,26 +142,24 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
return prev >> shift;
}
case 4: {
- unsigned int prev;
+ unsigned int prev = old;
asm volatile(
" cs %[prev],%[new],%[address]\n"
- : [prev] "=&d" (prev),
+ : [prev] "+&d" (prev),
[address] "+Q" (*(int *)address)
- : "0" (old),
- [new] "d" (new)
+ : [new] "d" (new)
: "memory", "cc");
return prev;
}
case 8: {
- unsigned long prev;
+ unsigned long prev = old;
asm volatile(
" csg %[prev],%[new],%[address]\n"
- : [prev] "=&d" (prev),
+ : [prev] "+&d" (prev),
[address] "+QS" (*(long *)address)
- : "0" (old),
- [new] "d" (new)
+ : [new] "d" (new)
: "memory", "cc");
return prev;
}
--
2.34.1Add new exception table type which is able to handle register
pairs. If an exception is recognized on such an instruction the
specified register pair will be zeroed, and the specified error
register will be modified so it contains -EFAULT, similar to the
existing EX_TABLE_UA_LOAD_REG() macro.
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
arch/s390/include/asm/asm-extable.h | 4 ++++
arch/s390/mm/extable.c | 9 +++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/s390/include/asm/asm-extable.h b/arch/s390/include/asm/asm-extable.h
index b74f1070ddb2..55a02a153dfc 100644
--- a/arch/s390/include/asm/asm-extable.h
+++ b/arch/s390/include/asm/asm-extable.h
@@ -12,6 +12,7 @@
#define EX_TYPE_UA_STORE 3
#define EX_TYPE_UA_LOAD_MEM 4
#define EX_TYPE_UA_LOAD_REG 5
+#define EX_TYPE_UA_LOAD_REGPAIR 6
#define EX_DATA_REG_ERR_SHIFT 0
#define EX_DATA_REG_ERR GENMASK(3, 0)
@@ -85,4 +86,7 @@
#define EX_TABLE_UA_LOAD_REG(_fault, _target, _regerr, _regzero) \
__EX_TABLE_UA(__ex_table, _fault, _target, EX_TYPE_UA_LOAD_REG, _regerr, _regzero, 0)
+#define EX_TABLE_UA_LOAD_REGPAIR(_fault, _target, _regerr, _regzero) \
+ __EX_TABLE_UA(__ex_table, _fault, _target, EX_TYPE_UA_LOAD_REGPAIR, _regerr, _regzero, 0)
+
#endif /* __ASM_EXTABLE_H */
diff --git a/arch/s390/mm/extable.c b/arch/s390/mm/extable.c
index 1e4d2187541a..fe87291df95d 100644
--- a/arch/s390/mm/extable.c
+++ b/arch/s390/mm/extable.c
@@ -47,13 +47,16 @@ static bool ex_handler_ua_load_mem(const struct exception_table_entry *ex, struc
return true;
}
-static bool ex_handler_ua_load_reg(const struct exception_table_entry *ex, struct pt_regs *regs)
+static bool ex_handler_ua_load_reg(const struct exception_table_entry *ex,
+ bool pair, struct pt_regs *regs)
{
unsigned int reg_zero = FIELD_GET(EX_DATA_REG_ADDR, ex->data);
unsigned int reg_err = FIELD_GET(EX_DATA_REG_ERR, ex->data);
regs->gprs[reg_err] = -EFAULT;
regs->gprs[reg_zero] = 0;
+ if (pair)
+ regs->gprs[reg_zero + 1] = 0;
regs->psw.addr = extable_fixup(ex);
return true;
}
@@ -75,7 +78,9 @@ bool fixup_exception(struct pt_regs *regs)
case EX_TYPE_UA_LOAD_MEM:
return ex_handler_ua_load_mem(ex, regs);
case EX_TYPE_UA_LOAD_REG:
- return ex_handler_ua_load_reg(ex, regs);
+ return ex_handler_ua_load_reg(ex, false, regs);
+ case EX_TYPE_UA_LOAD_REGPAIR:
+ return ex_handler_ua_load_reg(ex, true, regs);
}
panic("invalid exception table entry");
}
--
2.34.1Add 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"
+ " 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),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "memory", "cc");
+ *(unsigned char *)uval = prev >> shift;
+ return rc;
+ }
+ case 2: {
+ unsigned int prev, tmp, shift;
+
+ shift = (2 ^ (address & 2)) << 3;
+ address ^= address & 2;
+ 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"
+ " 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 & 0xffff) << shift),
+ [new] "d" (((unsigned int)new & 0xffff) << shift),
+ [mask] "d" (~(0xffff << shift)),
+ [key] "a" (key),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "memory", "cc");
+ *(unsigned short *)uval = prev >> shift;
+ return rc;
+ }
+ case 4: {
+ unsigned int prev = old;
+
+ asm volatile(
+ " spka 0(%[key])\n"
+ " sacf 256\n"
+ "0: cs %[prev],%[new],%[address]\n"
+ "1: sacf 768\n"
+ " spka %[default_key]\n"
+ EX_TABLE_UA_LOAD_REG(0b, 1b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(1b, 1b, %[rc], %[prev])
+ : [rc] "+&d" (rc),
+ [prev] "+&d" (prev),
+ [address] "+Q" (*(int *)address)
+ : [new] "d" ((unsigned int)new),
+ [key] "a" (key),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "memory", "cc");
+ *(unsigned int *)uval = prev;
+ return rc;
+ }
+ case 8: {
+ unsigned long prev = old;
+
+ asm volatile(
+ " spka 0(%[key])\n"
+ " sacf 256\n"
+ "0: csg %[prev],%[new],%[address]\n"
+ "1: sacf 768\n"
+ " spka %[default_key]\n"
+ EX_TABLE_UA_LOAD_REG(0b, 1b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(1b, 1b, %[rc], %[prev])
+ : [rc] "+&d" (rc),
+ [prev] "+&d" (prev),
+ [address] "+QS" (*(long *)address)
+ : [new] "d" ((unsigned long)new),
+ [key] "a" (key),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "memory", "cc");
+ *(unsigned long *)uval = prev;
+ return rc;
+ }
+ case 16: {
+ __uint128_t prev = old;
+
+ asm volatile(
+ " spka 0(%[key])\n"
+ " sacf 256\n"
+ "0: cdsg %[prev],%[new],%[address]\n"
+ "1: sacf 768\n"
+ " spka %[default_key]\n"
+ EX_TABLE_UA_LOAD_REGPAIR(0b, 1b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REGPAIR(1b, 1b, %[rc], %[prev])
+ : [rc] "+&d" (rc),
+ [prev] "+&d" (prev),
+ [address] "+QS" (*(__int128_t *)address)
+ : [new] "d" (new),
+ [key] "a" (key),
+ [default_key] "J" (PAGE_DEFAULT_KEY)
+ : "memory", "cc");
+ *(__uint128_t *)uval = prev;
+ return rc;
+ }
+ }
+ __cmpxchg_user_key_called_with_bad_pointer();
+ return rc;
+}
+
+/**
+ * cmpxchg_user_key() - cmpxchg with user space target, honoring storage keys
+ * @ptr: User space address of value to compare to @old and exchange with
+ * @new. Must be aligned to sizeof(*@size).
+ * @uval: Address where the old value of *@ptr is written to.
+ * @old: Old value. Compared to the content pointed to by @ptr in order to
+ * determine if the exchange occurs. The old value read from *@ptr is
+ * written to *@uval.
+ * @new: New value to place at *@ptr.
+ * @key: Access key to use for checking storage key protection.
+ *
+ * Perform a cmpxchg on a user space target, honoring storage key protection.
+ * @key alone determines how key checking is performed, neither
+ * storage-protection-override nor fetch-protection-override apply.
+ * The caller must compare *@uval and @old to determine if values have been
+ * exchanged. In case of an exception *@uval is set to zero.
+ *
+ * Return: 0: cmpxchg executed
+ * -EFAULT: an exception happened when trying to access *@ptr
+ */
+#define cmpxchg_user_key(ptr, uval, old, new, key) \
+({ \
+ __typeof__(ptr) __ptr = (ptr); \
+ __typeof__(uval) __uval = (uval); \
+ \
+ BUILD_BUG_ON(sizeof(*(__ptr)) != sizeof(*(__uval))); \
+ might_fault(); \
+ __chk_user_ptr(__ptr); \
+ __cmpxchg_user_key((unsigned long)(__ptr), (void *)(__uval), \
+ (old), (new), (key), sizeof(*(__ptr))); \
+})
+
#endif /* __S390_UACCESS_H */
--
2.34.1On 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;
> + }
[...]
On Wed, Nov 09, 2022 at 04:46:29PM +0100, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-11-02 at 15:19 +0100, Heiko Carstens wrote:
> > + 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?
I fail to parse what you are trying to say. Please elaborate.
> The loop condition being imprecise seems non-ideal.
What exactly is imprecise?
> > + [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.
That's not only inconsistent, but also a bug.
Thank you for pointing this out. Will be fixed.
On Wed, 2022-11-09 at 23:24 +0100, Heiko Carstens wrote:
> On Wed, Nov 09, 2022 at 04:46:29PM +0100, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-11-02 at 15:19 +0100, Heiko Carstens wrote:
> > > + 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?
>
> I fail to parse what you are trying to say. Please elaborate.
>
> > The loop condition being imprecise seems non-ideal.
>
> What exactly is imprecise?
The loop retries the CS if bits outside the target byte changed instead
of retrying until the target byte differs from the old value.
So if you attempt to exchange (prev_left_0 old_byte prev_right_0) and
that fails because the word at the address is (prev_left_1 x prev_right_1)
where both x != old_byte and one of the prev_*_1 values differs from the respective
prev_*_0 value, the CS is retried. If there were a native 1 byte compare and swap,
the exchange would just fail here. Instead the loop retries the CS until the margin
values are stable and it can infer from that that the CS failed because of the target value.
(Assuming that doesn't change to the old_byte value.)
It's not a problem, but it struck me as non-ideal, which is why for v2 I inverted the mask
after using it to punch the hole for the old/new values.
Then you can use it to test if bits inside the target byte differ.
That's why I asked about cmpxchg.h. If you don't want non-cosmetic changes to the existing
cmpxchg function and consistency of the new key checked function, then obviously the loop
condition needs to be the same.
>
> > > + [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.
>
> That's not only inconsistent, but also a bug.
> Thank you for pointing this out. Will be fixed.
Well, you could pass in the shifted key as argument, but yeah.
On Thu, Nov 10, 2022 at 12:01:23PM +0100, Janis Schoetterl-Glausch wrote: > On Wed, 2022-11-09 at 23:24 +0100, Heiko Carstens wrote: > > On Wed, Nov 09, 2022 at 04:46:29PM +0100, Janis Schoetterl-Glausch wrote: > > > Are you only entertaining cosmetic changes to cmpxchg.h? > > > > I fail to parse what you are trying to say. Please elaborate. > > > > > The loop condition being imprecise seems non-ideal. > > > > What exactly is imprecise? > > The loop retries the CS if bits outside the target byte changed instead > of retrying until the target byte differs from the old value. > So if you attempt to exchange (prev_left_0 old_byte prev_right_0) and > that fails because the word at the address is (prev_left_1 x prev_right_1) > where both x != old_byte and one of the prev_*_1 values differs from the respective > prev_*_0 value, the CS is retried. If there were a native 1 byte compare and swap, > the exchange would just fail here. Instead the loop retries the CS until the margin > values are stable and it can infer from that that the CS failed because of the target value. > (Assuming that doesn't change to the old_byte value.) > > It's not a problem, but it struck me as non-ideal, which is why for v2 I inverted the mask > after using it to punch the hole for the old/new values. > Then you can use it to test if bits inside the target byte differ. > > That's why I asked about cmpxchg.h. If you don't want non-cosmetic changes to the existing > cmpxchg function and consistency of the new key checked function, then obviously the loop > condition needs to be the same. Such a change is fine of course, even though compare-and-swap for one and two byte patterns don't really matter. I would appreciate if you could send one or two patches on-top of this series which adds the improved logic to (now) both variants. And, since the question will come up anyway: as soon as we agreed on a complete patch series, I think we should go for a features branch on s390's kernel.org tree which would contain the first five patches sent by me plus potential addon patches provided by you. This tree can then be pulled in by the kvms390 tree where your kvm specific patches can then be applied on top.
On Thu, Nov 10, 2022 at 12:32:06PM +0100, Heiko Carstens wrote: > > That's why I asked about cmpxchg.h. If you don't want non-cosmetic changes to the existing > > cmpxchg function and consistency of the new key checked function, then obviously the loop > > condition needs to be the same. > > Such a change is fine of course, even though compare-and-swap for one and > two byte patterns don't really matter. I would appreciate if you could send > one or two patches on-top of this series which adds the improved logic to > (now) both variants. > > And, since the question will come up anyway: as soon as we agreed on a > complete patch series, I think we should go for a features branch on s390's > kernel.org tree which would contain the first five patches sent by me plus > potential addon patches provided by you. > This tree can then be pulled in by the kvms390 tree where your kvm specific > patches can then be applied on top. FWIW, pushed a non-stable work-in-progress branch to git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git wip/cmpxchg_user_key This includes also an updated patch, which fixes the missing shift of the access key.
The cmpxchg implementation for 1 and 2 bytes consists of a 4 byte
cmpxchg loop. Currently, the decision to retry is imprecise, looping if
bits outside the target byte(s) change instead of retrying until the
target byte(s) differ from the old value.
E.g. if an attempt to exchange (prev_left_0 old_bytes prev_right_0) is
made and it fails because the word at the address is
(prev_left_1 x prev_right_1) where both x != old_bytes and one of the
prev_*_1 values differs from the respective prev_*_0 value, the cmpxchg
is retried, even if by a semantic equivalent to a normal cmpxchg, the
exchange would fail.
Instead exit the loop if x != old_bytes and retry otherwise.
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
Unfortunately the diff got blown up quite a bit, even tho the asm
changes are not that complex. This is mostly because of in arguments
becoming (in)out arguments.
I don't think all the '&' constraints are necessary, but I don't see how
they could affect code generation.
I don't see why we would need the memory clobber, however.
I tested the cmpxchg_user_key changes via the kvm memop selftest that is
part of the KVM cmpxchg memop series.
I looked for an existing way to test the cmpxchg changes, but didn't
find anything.
arch/s390/include/asm/cmpxchg.h | 60 ++++++++++++++-----------
arch/s390/include/asm/uaccess.h | 80 ++++++++++++++++++---------------
2 files changed, 78 insertions(+), 62 deletions(-)
diff --git a/arch/s390/include/asm/cmpxchg.h b/arch/s390/include/asm/cmpxchg.h
index 1c5785b851ec..3f26416c2ad8 100644
--- a/arch/s390/include/asm/cmpxchg.h
+++ b/arch/s390/include/asm/cmpxchg.h
@@ -90,55 +90,63 @@ static __always_inline unsigned long __cmpxchg(unsigned long address,
{
switch (size) {
case 1: {
- unsigned int prev, tmp, shift;
+ unsigned int prev, shift, mask;
shift = (3 ^ (address & 3)) << 3;
address ^= address & 3;
+ old = (old & 0xff) << shift;
+ new = (new & 0xff) << shift;
+ mask = ~(0xff << shift);
asm volatile(
" l %[prev],%[address]\n"
- "0: nr %[prev],%[mask]\n"
- " lr %[tmp],%[prev]\n"
- " or %[prev],%[old]\n"
- " or %[tmp],%[new]\n"
- " cs %[prev],%[tmp],%[address]\n"
+ " nr %[prev],%[mask]\n"
+ " xilf %[mask],0xffffffff\n"
+ " or %[new],%[prev]\n"
+ " or %[prev],%[tmp]\n"
+ "0: lr %[tmp],%[prev]\n"
+ " cs %[prev],%[new],%[address]\n"
" jnl 1f\n"
" xr %[tmp],%[prev]\n"
+ " xr %[new],%[tmp]\n"
" nr %[tmp],%[mask]\n"
- " jnz 0b\n"
+ " jz 0b\n"
"1:"
: [prev] "=&d" (prev),
- [tmp] "=&d" (tmp),
- [address] "+Q" (*(int *)address)
- : [old] "d" ((old & 0xff) << shift),
- [new] "d" ((new & 0xff) << shift),
- [mask] "d" (~(0xff << shift))
- : "memory", "cc");
+ [address] "+Q" (*(int *)address),
+ [tmp] "+&d" (old),
+ [new] "+&d" (new),
+ [mask] "+&d" (mask)
+ :: "memory", "cc");
return prev >> shift;
}
case 2: {
- unsigned int prev, tmp, shift;
+ unsigned int prev, shift, mask;
shift = (2 ^ (address & 2)) << 3;
address ^= address & 2;
+ old = (old & 0xffff) << shift;
+ new = (new & 0xffff) << shift;
+ mask = ~(0xffff << shift);
asm volatile(
" l %[prev],%[address]\n"
- "0: nr %[prev],%[mask]\n"
- " lr %[tmp],%[prev]\n"
- " or %[prev],%[old]\n"
- " or %[tmp],%[new]\n"
- " cs %[prev],%[tmp],%[address]\n"
+ " nr %[prev],%[mask]\n"
+ " xilf %[mask],0xffffffff\n"
+ " or %[new],%[prev]\n"
+ " or %[prev],%[tmp]\n"
+ "0: lr %[tmp],%[prev]\n"
+ " cs %[prev],%[new],%[address]\n"
" jnl 1f\n"
" xr %[tmp],%[prev]\n"
+ " xr %[new],%[tmp]\n"
" nr %[tmp],%[mask]\n"
- " jnz 0b\n"
+ " jz 0b\n"
"1:"
: [prev] "=&d" (prev),
- [tmp] "=&d" (tmp),
- [address] "+Q" (*(int *)address)
- : [old] "d" ((old & 0xffff) << shift),
- [new] "d" ((new & 0xffff) << shift),
- [mask] "d" (~(0xffff << shift))
- : "memory", "cc");
+ [address] "+Q" (*(int *)address),
+ [tmp] "+&d" (old),
+ [new] "+&d" (new),
+ [mask] "+&d" (mask)
+ :: "memory", "cc");
return prev >> shift;
}
case 4: {
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index a125e60a1521..d028ee59e941 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -400,74 +400,82 @@ static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
switch (size) {
case 1: {
- unsigned int prev, tmp, shift;
+ unsigned int prev, shift, mask, _old, _new;
shift = (3 ^ (address & 3)) << 3;
address ^= address & 3;
+ _old = (old & 0xff) << shift;
+ _new = (new & 0xff) << shift;
+ mask = ~(0xff << shift);
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"
+ " xilf %[mask],0xffffffff\n"
+ " or %[new],%[prev]\n"
+ " or %[prev],%[tmp]\n"
+ "2: lr %[tmp],%[prev]\n"
+ "3: cs %[prev],%[new],%[address]\n"
+ "4: jnl 5f\n"
" xr %[tmp],%[prev]\n"
+ " xr %[new],%[tmp]\n"
" nr %[tmp],%[mask]\n"
- " jnz 1b\n"
- "4: sacf 768\n"
+ " jz 2b\n"
+ "5: 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])
+ EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(1b, 5b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(3b, 5b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(4b, 5b, %[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 << 4),
+ [address] "+Q" (*(int *)address),
+ [tmp] "+&d" (_old),
+ [new] "+&d" (_new),
+ [mask] "+&d" (mask)
+ : [key] "a" (key << 4),
[default_key] "J" (PAGE_DEFAULT_KEY)
: "memory", "cc");
*(unsigned char *)uval = prev >> shift;
return rc;
}
case 2: {
- unsigned int prev, tmp, shift;
+ unsigned int prev, shift, mask, _old, _new;
shift = (2 ^ (address & 2)) << 3;
address ^= address & 2;
+ _old = (old & 0xffff) << shift;
+ _new = (new & 0xffff) << shift;
+ mask = ~(0xffff << shift);
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"
+ " xilf %[mask],0xffffffff\n"
+ " or %[new],%[prev]\n"
+ " or %[prev],%[tmp]\n"
+ "2: lr %[tmp],%[prev]\n"
+ "3: cs %[prev],%[new],%[address]\n"
+ "4: jnl 5f\n"
" xr %[tmp],%[prev]\n"
+ " xr %[new],%[tmp]\n"
" nr %[tmp],%[mask]\n"
- " jnz 1b\n"
- "4: sacf 768\n"
+ " jz 2b\n"
+ "5: 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])
+ EX_TABLE_UA_LOAD_REG(0b, 5b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(1b, 5b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(3b, 5b, %[rc], %[prev])
+ EX_TABLE_UA_LOAD_REG(4b, 5b, %[rc], %[prev])
: [rc] "+&d" (rc),
[prev] "=&d" (prev),
- [tmp] "=&d" (tmp),
- [address] "+Q" (*(int *)address)
- : [old] "d" (((unsigned int)old & 0xffff) << shift),
- [new] "d" (((unsigned int)new & 0xffff) << shift),
- [mask] "d" (~(0xffff << shift)),
- [key] "a" (key << 4),
+ [address] "+Q" (*(int *)address),
+ [tmp] "+&d" (_old),
+ [new] "+&d" (_new),
+ [mask] "+&d" (mask)
+ : [key] "a" (key << 4),
[default_key] "J" (PAGE_DEFAULT_KEY)
: "memory", "cc");
*(unsigned short *)uval = prev >> shift;
base-commit: b23ddf9d5a30f64a1a51a85f0d9e2553210b21a2
--
2.34.1On Wed, Nov 16, 2022 at 03:47:11PM +0100, Janis Schoetterl-Glausch wrote: > The cmpxchg implementation for 1 and 2 bytes consists of a 4 byte > cmpxchg loop. Currently, the decision to retry is imprecise, looping if > bits outside the target byte(s) change instead of retrying until the > target byte(s) differ from the old value. > E.g. if an attempt to exchange (prev_left_0 old_bytes prev_right_0) is > made and it fails because the word at the address is > (prev_left_1 x prev_right_1) where both x != old_bytes and one of the > prev_*_1 values differs from the respective prev_*_0 value, the cmpxchg > is retried, even if by a semantic equivalent to a normal cmpxchg, the > exchange would fail. > Instead exit the loop if x != old_bytes and retry otherwise. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > > > Unfortunately the diff got blown up quite a bit, even tho the asm > changes are not that complex. This is mostly because of in arguments > becoming (in)out arguments. > > I don't think all the '&' constraints are necessary, but I don't see how > they could affect code generation. For cmpxchg() it wouldn't make any difference. For cmpxchg_user_key() it might lead to a small improvement, since the register that is allocated for the key variable might be reused. But I haven't looked into that in detail. None of the early clobbers is necessary anymore after your changes, but let's leave it as it is. > I don't see why we would need the memory clobber, however. The memory clobber (aka memory barrier) is necessary because it may be used to implement e.g. some custom locking. For that it is required the compiler does reorder read or write accesses behind/before the inline assembly. > I tested the cmpxchg_user_key changes via the kvm memop selftest that is > part of the KVM cmpxchg memop series. > I looked for an existing way to test the cmpxchg changes, but didn't > find anything. Yeah, guess having a test for this would be a nice to have :) > arch/s390/include/asm/cmpxchg.h | 60 ++++++++++++++----------- > arch/s390/include/asm/uaccess.h | 80 ++++++++++++++++++--------------- > 2 files changed, 78 insertions(+), 62 deletions(-) The patch looks good - applied to the wip/cmpxchg_user_key branch.
Quoting Janis Schoetterl-Glausch (2022-10-12 22:56:01)
[...]
> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
> index f7038b800cc3..f148f5a22c93 100644
[...]
> +static __always_inline int __cmpxchg_user_key_small(int size, u64 address,
> + unsigned __int128 *old_p,
> + unsigned __int128 new, u8 access_key)
> +{
This function is quite hard to understand for me without some context. I have a
few suggestions for some comments and one small question below.
> + u32 shift, mask, old_word, new_word, align_mask, tmp;
> + u64 aligned;
> + int ret = -EFAULT;
> +
something like this:
/*
* There is no instruction for 2 and 1 byte compare swap, hence emulate it with
* a 4-byte compare swap.
* When the 4-bytes compare swap fails, it can be because the actual value user
* space wanted to exchange mismatched. In this case, return to user space.
* Or it can be because something outside of the value user space wanted to
* access mismatched (the "remainder" of the word). In this case, retry in
* kernel space.
*/
> + switch (size) {
> + case 2:
/* assume address is 2-byte-aligned - cannot cross word boundary */
> + align_mask = 2;
/* fancy way of saying aligned = address & ~align_mask */
> + aligned = (address ^ (address & align_mask));
/* generate mask to extract value to xchg from the word */
> + shift = (sizeof(u32) - (address & align_mask) - size) * 8;
> + mask = 0xffff << shift;
> + old_word = ((u16)*old_p) << shift;
> + new_word = ((u16)new) << shift;
> + break;
> + case 1:
> + align_mask = 3;
> + aligned = (address ^ (address & align_mask));
> + shift = (sizeof(u32) - (address & align_mask) - size) * 8;
> + mask = 0xff << shift;
> + old_word = ((u8)*old_p) << shift;
> + new_word = ((u8)new) << shift;
> + break;
> + }
> + tmp = old_word; /* don't modify *old_p on fault */
> + asm volatile(
> + "spka 0(%[access_key])\n"
/* secondary space has user asce loaded */
> + " sacf 256\n"
> + "0: l %[tmp],%[aligned]\n"
> + "1: nr %[tmp],%[mask]\n"
/* invert mask to generate mask for the remainder */
> + " xilf %[mask],0xffffffff\n"
> + " or %[new_word],%[tmp]\n"
> + " or %[tmp],%[old_word]\n"
> + "2: lr %[old_word],%[tmp]\n"
> + "3: cs %[tmp],%[new_word],%[aligned]\n"
> + "4: jnl 5f\n"
> + /* We'll restore old_word before the cs, use reg for the diff */
> + " xr %[old_word],%[tmp]\n"
> + /* Apply diff assuming only bits outside target byte(s) changed */
> + " xr %[new_word],%[old_word]\n"
> + /* If prior assumption false we exit loop, so not an issue */
> + " nr %[old_word],%[mask]\n"
> + " jz 2b\n"
So if the remainder changed but the actual value to exchange stays the same, we
loop in the kernel. Does it maybe make sense to limit the number of iterations
we spend retrying? I think while looping here the calling process can't be
killed, can it?
On Thu, Oct 20, 2022 at 03:40:56PM +0200, Nico Boehr wrote:
> Quoting Janis Schoetterl-Glausch (2022-10-12 22:56:01)
> > + "2: lr %[old_word],%[tmp]\n"
> > + "3: cs %[tmp],%[new_word],%[aligned]\n"
> > + "4: jnl 5f\n"
> > + /* We'll restore old_word before the cs, use reg for the diff */
> > + " xr %[old_word],%[tmp]\n"
> > + /* Apply diff assuming only bits outside target byte(s) changed */
> > + " xr %[new_word],%[old_word]\n"
> > + /* If prior assumption false we exit loop, so not an issue */
> > + " nr %[old_word],%[mask]\n"
> > + " jz 2b\n"
>
> So if the remainder changed but the actual value to exchange stays the same, we
> loop in the kernel. Does it maybe make sense to limit the number of iterations
> we spend retrying? I think while looping here the calling process can't be
> killed, can it?
Yes, the number of loops should be limited; quite similar what arm64
implemented with commit 03110a5cb216 ("arm64: futex: Bound number of
LDXR/STXR loops in FUTEX_WAKE_OP").
Hi Janis, On Wed, Oct 12, 2022 at 10:56:01PM +0200, Janis Schoetterl-Glausch wrote: > Add cmpxchg functionality similar to that in cmpxchg.h except that the > target is a user space address and that the address' storage key is > matched with the access_key argument in order to honor key-controlled > protection. > The access is performed by changing to the secondary-spaces mode and > setting the PSW key for the duration of the compare and swap. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > > > Possible variations: > * check the assumptions made in cmpxchg_user_key_size and error out > * call functions called by copy_to_user > * access_ok? is a nop > * should_fail_usercopy? > * instrument_copy_to_user? doesn't make sense IMO > * don't be overly strict in cmpxchg_user_key > > > arch/s390/include/asm/uaccess.h | 189 ++++++++++++++++++++++++++++++++ > 1 file changed, 189 insertions(+) This not how I would have expected something that would be symmetrical/consistent with what we already have. However instead of spending several iterations I'll send something for this. This might take a bit due to my limited time. So please be patient.
© 2016 - 2026 Red Hat, Inc.