Message ID | 20200207134604.29046-1-lamm@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | Fix tst-pkey expectations on pkey_get | expand |
* Lucas A. M. Magalhaes: > Florian, Your patch including pkey_set and pkey_get looks good to me. > Can you merge it? This one > https://sourceware.org/ml/libc-alpha/2018-05/msg00760.html. Thanks. Is the patch really correct for 32-bit userspace? > With this there will be one failure on this test on powerpc machines. > The test expects that during a signal handling the pkey_get returns > PKEY_DISABLE_ACCESS for all keys. In my tests it returns the same > permissions as before the signal. I couldn't find where this is done for > x86. Is this kernel implementation? POWER has read-disable and write-disable flags which work independently. The kernel-defined flags cannot represent the read-disable configuration. At the time, there was no read-disable flag allocated in the kernel. Has this changed? (See the ”Translate” comments in my patch.) On x86, the hardware has write-disable and read-write-disable flags instead, which matches the original UAPI interfaces. This is why no translation is necessary. Thanks, Florian
Quoting Florian Weimer (2020-02-07 15:22:16) > * Lucas A. M. Magalhaes: > > > Florian, Your patch including pkey_set and pkey_get looks good to me. > > Can you merge it? This one > > https://sourceware.org/ml/libc-alpha/2018-05/msg00760.html. > > Thanks. Is the patch really correct for 32-bit userspace? > Thanks for pointing it out. Even in 32-bit mode the mtspr will effect all 64 bits and there is no 32 bit limitation for AMR. I will try to setup a 32 bit userspace machine with pkeys enabled to test this out. > > With this there will be one failure on this test on powerpc machines. > > The test expects that during a signal handling the pkey_get returns > > PKEY_DISABLE_ACCESS for all keys. In my tests it returns the same > > permissions as before the signal. I couldn't find where this is done for > > x86. Is this kernel implementation? > > POWER has read-disable and write-disable flags which work independently. > The kernel-defined flags cannot represent the read-disable > configuration. At the time, there was no read-disable flag allocated in > the kernel. Has this changed? (See the ”Translate” comments in my > patch.) > I think we are talking about this patch https://marc.info/?l=linux-api&m=154390462121345&w=2 that you were discussing a PKEY_DISABLE_READ flag for pkey_alloc. Unfortunately It was not merged. I agree that this work should be done. But this should not block the pkey_get and pkey_set implementation for power to be merged for the actual api. > On x86, the hardware has write-disable and read-write-disable flags > instead, which matches the original UAPI interfaces. This is why no > translation is necessary. Excuse my ignorance. How this translation problem influences the signal handling behaviour?
* Lucas A. M. Magalhaes: > Quoting Florian Weimer (2020-02-07 15:22:16) >> * Lucas A. M. Magalhaes: >> >> > Florian, Your patch including pkey_set and pkey_get looks good to me. >> > Can you merge it? This one >> > https://sourceware.org/ml/libc-alpha/2018-05/msg00760.html. >> >> Thanks. Is the patch really correct for 32-bit userspace? >> > > Thanks for pointing it out. Even in 32-bit mode the mtspr will effect all > 64 bits and there is no 32 bit limitation for AMR. I will try to setup > a 32 bit userspace machine with pkeys enabled to test this out. Thanks. We may have to tweak the constraints a little bit, though. >> > With this there will be one failure on this test on powerpc machines. >> > The test expects that during a signal handling the pkey_get returns >> > PKEY_DISABLE_ACCESS for all keys. In my tests it returns the same >> > permissions as before the signal. I couldn't find where this is done for >> > x86. Is this kernel implementation? >> >> POWER has read-disable and write-disable flags which work independently. >> The kernel-defined flags cannot represent the read-disable >> configuration. At the time, there was no read-disable flag allocated in >> the kernel. Has this changed? (See the ”Translate” comments in my >> patch.) >> > > I think we are talking about this patch > https://marc.info/?l=linux-api&m=154390462121345&w=2 that you were > discussing a PKEY_DISABLE_READ flag for pkey_alloc. Unfortunately It > was not merged. Right. > I agree that this work should be done. But this should not block the > pkey_get and pkey_set implementation for power to be merged for the > actual api. That sounds about right. >> On x86, the hardware has write-disable and read-write-disable flags >> instead, which matches the original UAPI interfaces. This is why no >> translation is necessary. > > Excuse my ignorance. How this translation problem influences the signal > handling behaviour? The signal handling behavior is just different. If I recall correctly, x86 always resets PKRU to a mostly-disable value, while POWER inherits the AMR value from the interrupted thread. The POWER behavior seems more useful to me, but that depends on what the programmer tries to do. I think I have posted x86 patches for changing the behavior, too. Thanks, Florian
> >> Thanks. Is the patch really correct for 32-bit userspace? > >> > > > > Thanks for pointing it out. Even in 32-bit mode the mtspr will effect all > > 64 bits and there is no 32 bit limitation for AMR. I will try to setup > > a 32 bit userspace machine with pkeys enabled to test this out. > > Thanks. We may have to tweak the constraints a little bit, though. > Actually there is a problem in this regard we may encounter. We cannot ensure that in a context change the upper bits of the GPR will remain the same. We could either 1. Restrict the usage of the upper pkeys on 32bits. Maybe the kernel already restrict already on pkey_alloc. 2. Make it ppc64 specific. > >> On x86, the hardware has write-disable and read-write-disable flags > >> instead, which matches the original UAPI interfaces. This is why no > >> translation is necessary. > > > > Excuse my ignorance. How this translation problem influences the signal > > handling behaviour? > > The signal handling behavior is just different. If I recall correctly, > x86 always resets PKRU to a mostly-disable value, while POWER inherits > the AMR value from the interrupted thread. The POWER behavior seems > more useful to me, but that depends on what the programmer tries to do. > What do you suggest for this. To change x86 behavior or too change the test to accept both behaviors? > I think I have posted x86 patches for changing the behavior, too. > I could not find it.
* Lucas A. M. Magalhaes: >> >> Thanks. Is the patch really correct for 32-bit userspace? >> >> >> > >> > Thanks for pointing it out. Even in 32-bit mode the mtspr will effect all >> > 64 bits and there is no 32 bit limitation for AMR. I will try to setup >> > a 32 bit userspace machine with pkeys enabled to test this out. >> >> Thanks. We may have to tweak the constraints a little bit, though. >> > > Actually there is a problem in this regard we may encounter. We cannot > ensure that in a context change the upper bits of the GPR will remain > the same. We could either > > 1. Restrict the usage of the upper pkeys on 32bits. Maybe the kernel > already restrict already on pkey_alloc. > > 2. Make it ppc64 specific. Option 2 is fine with me. >> >> On x86, the hardware has write-disable and read-write-disable flags >> >> instead, which matches the original UAPI interfaces. This is why no >> >> translation is necessary. >> > >> > Excuse my ignorance. How this translation problem influences the signal >> > handling behaviour? >> >> The signal handling behavior is just different. If I recall correctly, >> x86 always resets PKRU to a mostly-disable value, while POWER inherits >> the AMR value from the interrupted thread. The POWER behavior seems >> more useful to me, but that depends on what the programmer tries to do. >> > > What do you suggest for this. To change x86 behavior or too change the > test to accept both behaviors? For now: Change the test to accept both behaviors. >> I think I have posted x86 patches for changing the behavior, too. >> > > I could not find it. It was a kernel patch: <https://marc.info/?l=linux-mm&m=152526767625176> <https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/172420.html> Thanks, Florian
diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c b/sysdeps/unix/sysv/linux/tst-pkey.c index 4ea1bc4f9a..11084520b3 100644 --- a/sysdeps/unix/sysv/linux/tst-pkey.c +++ b/sysdeps/unix/sysv/linux/tst-pkey.c @@ -37,7 +37,7 @@ static pthread_barrier_t barrier; /* The keys used for testing. These have been allocated with access rights set based on their array index. */ -enum { key_count = 4 }; +enum { key_count = 3 }; static int keys[key_count]; static volatile int *pages[key_count];