[v4] Fix tst-pkey expectations on pkey_get [BZ #23202]

Message ID 20200217120952.5219-1-lamm@linux.ibm.com
State Committed
Delegated to: Florian Weimer
Headers

Commit Message

Lucas A. M. Magalhaes Feb. 17, 2020, 12:09 p.m. UTC
  From the GNU C Library manual the pkey_set can receive a combination of
PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS.  However PKEY_DISABLE_ACCESS
is more restrictive than PKEY_DISABLE_WRITE and includes its behavior.

The test expects that after setting
(PKEY_DISABLE_WRITE|PKEY_DISABLE_ACCESS) pkey_get should return the
same.  This may not be true as PKEY_DISABLE_ACCESS will succeed in
describe the state of the key in this case.

The pkey behavior during signal handling is different between x86 and
POWER.  This change make the test compatible with both architectures.

---

Hi,

in V4:
	- Fix comments.

in V3:
	- Commit message changes.
	- Add comments.

in V2:
	- Fix signal handling expectations to accept x86 and POWER behavior.

 sysdeps/unix/sysv/linux/tst-pkey.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
  

Comments

Florian Weimer Feb. 17, 2020, 12:50 p.m. UTC | #1
* Lucas A. M. Magalhaes:

> diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c b/sysdeps/unix/sysv/linux/tst-pkey.c
> index 4ea1bc4f9a..cba40c73de 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];
>  
> @@ -111,14 +111,16 @@ check_page_access (int page, bool write)
>  }
>  
>  static volatile sig_atomic_t sigusr1_handler_ran;
> -
> -/* Used to check that access is revoked in signal handlers.  */
> +/* Used to check the behavior in signal handlers.  In x86 all access are
> +   revoked during signal handling.  In PowerPC the key permissions are
> +   inherited by the interrupted thread. This test accept both approaches.  */
>  static void
>  sigusr1_handler (int signum)
>  {
>    TEST_COMPARE (signum, SIGUSR1);
>    for (int i = 0; i < key_count; ++i)
> -    TEST_COMPARE (pkey_get (keys[i]), PKEY_DISABLE_ACCESS);
> +    TEST_VERIFY (pkey_get (keys[i]) == PKEY_DISABLE_ACCESS
> +                 || pkey_get (keys[i]) == i);
>    sigusr1_handler_ran = 1;
>  }

This version looks okay to me.  Thanks.

Florian
  
Tulio Magno Quites Machado Filho Feb. 19, 2020, 2:41 p.m. UTC | #2
"Lucas A. M. Magalhaes" <lamm@linux.ibm.com> writes:

> From the GNU C Library manual the pkey_set can receive a combination of
> PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS.  However PKEY_DISABLE_ACCESS
> is more restrictive than PKEY_DISABLE_WRITE and includes its behavior.
>
> The test expects that after setting
> (PKEY_DISABLE_WRITE|PKEY_DISABLE_ACCESS) pkey_get should return the
> same.  This may not be true as PKEY_DISABLE_ACCESS will succeed in
> describe the state of the key in this case.
>
> The pkey behavior during signal handling is different between x86 and
> POWER.  This change make the test compatible with both architectures.

The patch LGTM.
This test is now passing on Linux >= 5.0, but it still fails on Linux <= 4.18
because of issues in the kernel.
I'm not sure if 4.19 should work.

Old kernels do return valid data:

[pid 56757] pkey_alloc(0, 0 <unfinished ...>
[pid 56758] set_robust_list(0x7fff8994f260, 24 <unfinished ...>
[pid 56757] <... pkey_alloc resumed> )  = 2
[pid 56758] <... set_robust_list resumed> ) = 0
[pid 56757] pkey_alloc(0, PKEY_DISABLE_ACCESS <unfinished ...>
[pid 56758] futex(0x100208a4, FUTEX_WAIT_PRIVATE, 0, NULL <unfinished ...>
[pid 56757] <... pkey_alloc resumed> )  = 3
[pid 56757] pkey_alloc(0, PKEY_DISABLE_WRITE) = 4
[pid 56757] mmap(NULL, 65536, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fff89130000
[pid 56757] pkey_mprotect(0x7fff89130000, 65536, PROT_READ|PROT_WRITE, 2) = 0
[pid 56757] mmap(NULL, 65536, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fff89120000
[pid 56757] pkey_mprotect(0x7fff89120000, 65536, PROT_READ|PROT_WRITE, 3) = 0
[pid 56757] mmap(NULL, 65536, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fff89110000
[pid 56757] pkey_mprotect(0x7fff89110000, 65536, PROT_READ|PROT_WRITE, 4) = 0
[pid 56757] futex(0x100208a4, FUTEX_WAKE_PRIVATE, 2147483647) = 1
[pid 56758] <... futex resumed> )       = 0
[pid 56757] futex(0x7fff8994f250, FUTEX_WAIT, 56758, NULL <unfinished ...>
[pid 56758] mmap(NULL, 134217728, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0x7fff81110000
[pid 56758] munmap(0x7fff81110000, 49217536) = 0
[pid 56758] munmap(0x7fff88000000, 17891328) = 0
[pid 56758] mprotect(0x7fff84000000, 196608, PROT_READ|PROT_WRITE) = 0
[pid 56758] rt_sigprocmask(SIG_BLOCK, NULL, [], 8) = 0
[pid 56758] rt_sigaction(SIGSEGV, {sa_handler=0x1001fca8, sa_mask=[], sa_flags=SA_RESETHAND|SA_SIGINFO}, NULL, 8) = 0
[pid 56758] rt_sigaction(SIGSEGV, {sa_handler=SIG_DFL, sa_mask=[SEGV], sa_flags=SA_RESTART}, {sa_handler=0x1001fca8, sa_mask=[], sa_flags=SA_RESETHAND|SA_SIGINFO}, 8) = 0
[pid 56758] write(1, "error: ../sysdeps/unix/sysv/linu"..., 90error: ../sysdeps/unix/sysv/linux/tst-pkey.c:161: not true: !check_page_access (i, false)
) = 90

Anyway, pushed as 8d42bf859a289944749d9f978c076cd318119867.

Thanks!
  

Patch

diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c b/sysdeps/unix/sysv/linux/tst-pkey.c
index 4ea1bc4f9a..cba40c73de 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];
 
@@ -111,14 +111,16 @@  check_page_access (int page, bool write)
 }
 
 static volatile sig_atomic_t sigusr1_handler_ran;
-
-/* Used to check that access is revoked in signal handlers.  */
+/* Used to check the behavior in signal handlers.  In x86 all access are
+   revoked during signal handling.  In PowerPC the key permissions are
+   inherited by the interrupted thread. This test accept both approaches.  */
 static void
 sigusr1_handler (int signum)
 {
   TEST_COMPARE (signum, SIGUSR1);
   for (int i = 0; i < key_count; ++i)
-    TEST_COMPARE (pkey_get (keys[i]), PKEY_DISABLE_ACCESS);
+    TEST_VERIFY (pkey_get (keys[i]) == PKEY_DISABLE_ACCESS
+                 || pkey_get (keys[i]) == i);
   sigusr1_handler_ran = 1;
 }