Re: [PATCH v3 00/23] Add generic vdso_base tracking

From: Dmitry Safonov <dima@arista.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
	LKML <linux-kernel@vger.kernel.org>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Andy Lutomirski <luto@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Borislav Petkov <bp@alien8.de>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Guo Ren <guoren@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
	Ingo Molnar <mingo@redhat.com>, Oleg Nesterov <oleg@redhat.com>,
	Russell King <linux@armlinux.org.uk>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Will Deacon <will@kernel.org>, X86 ML <x86@kernel.org>
Subject: Re: [PATCH v3 00/23] Add generic vdso_base tracking
Date: Mon, 21 Jun 2021 22:57:30 +0100	[thread overview]
Message-ID: <CAGrbwDRZ5Tgt68kwhRSFMwzfaup9HGU596X1GA2MWX4NEeFjYA@mail.gmail.com> (raw)
In-Reply-To: <c9d24f64-4c34-703f-2223-113076e07b46@csgroup.eu>

On 6/17/21 10:13 AM, Christophe Leroy wrote:
>
>
> Le 11/06/2021 à 20:02, Dmitry Safonov a écrit :
>> v3 Changes:
>> - Migrated arch/powerpc to vdso_base
>> - Added x86/selftest for unmapped vdso & no landing on fast syscall
>> - Review comments from Andy & Christophe (thanks!)
>> - Amended s/born process/execed process/ everywhere I noticed
>> - Build robot warning on cast from __user pointer
>>
>> I've tested it on x86, I would appreciate any help with
>> Tested-by on arm/arm64/mips/powerpc/s390/... platforms.
>
> I tried it on powerpc, normal use still works.

Thank you!

> What tests can be done exactly ?

Well, for x86 I've run all vdso tests from tools/testing/selftests/x86/
(with the new one from patch 23 here, which tests exactly forced
segfault on unmapped vdso).
I think normal use on other platforms sounds good to me.

>
> We have a selftest in powerpc
> (https://github.com/linuxppc/linux/blob/master/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c)
> but it doesn't work anymore since the split of VDSO into VDSO+VVAR.

Well, it doesn't sound very hard to fix, see the sample diff inline.

Thank you,
Dmitry

--->8---

diff --git a/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
b/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
index e282fff0fe25..a4f85ee13c4a 100644
--- a/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
+++ b/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
@@ -13,6 +13,7 @@
 #include <signal.h>
 #include <stdlib.h>
 #include <string.h>
+#include <stdbool.h>
 #include <sys/mman.h>
 #include <sys/types.h>
 #include <unistd.h>
@@ -23,7 +24,7 @@

 #include "utils.h"

-static int search_proc_maps(char *needle, unsigned long *low,
unsigned long *high)
+static int search_proc_maps(char *needle, unsigned long *low,
unsigned long *high, unsigned long *size)
 {
     unsigned long start, end;
     static char buf[4096];
@@ -52,6 +53,7 @@ static int search_proc_maps(char *needle, unsigned
long *low, unsigned long *hig
         if (strstr(name, needle)) {
             *low = start;
             *high = end - 1;
+            *size = end - start;
             rc = 0;
             break;
         }
@@ -71,9 +73,12 @@ static void sigusr1_handler(int sig)

 int test_sigreturn_vdso(void)
 {
-    unsigned long low, high, size;
+    unsigned long stack_start, stack_end, stack_size;
+    unsigned long vdso_start, vdso_end, vdso_size;
+    unsigned long vvar_start, vvar_end, vvar_size;
+    char *vdso_parking, *vvar_parking;
     struct sigaction act;
-    char *p;
+    bool vvar_present;

     act.sa_handler = sigusr1_handler;
     act.sa_flags = 0;
@@ -82,36 +87,56 @@ int test_sigreturn_vdso(void)
     assert(sigaction(SIGUSR1, &act, NULL) == 0);

     // Confirm the VDSO is mapped, and work out where it is
-    assert(search_proc_maps("[vdso]", &low, &high) == 0);
-    size = high - low + 1;
-    printf("VDSO is at 0x%lx-0x%lx (%lu bytes)\n", low, high, size);
+    assert(search_proc_maps("[vdso]", &vdso_start, &vdso_end,
&vdso_size) == 0);
+    printf("VDSO is at 0x%lx-0x%lx (%lu bytes)\n", vdso_start,
vdso_end, vdso_size);
+    // On older kernels there's only vdso, on newer vdso/vvar pair
+    if (search_proc_maps("[vvar]", &vvar_start, &vvar_end, &vvar_size) == 0) {
+        vvar_present = true;
+        printf("VVAR is at 0x%lx-0x%lx (%lu bytes)\n",
+                vvar_start, vvar_end, vvar_size);
+    } else {
+        vvar_present = false;
+        vvar_size = 0;
+    }

     kill(getpid(), SIGUSR1);
     assert(took_signal == 1);
     printf("Signal delivered OK with VDSO mapped\n");

-    // Remap the VDSO somewhere else
-    p = mmap(NULL, size, PROT_READ|PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
-    assert(p != MAP_FAILED);
-    assert(mremap((void *)low, size, size,
MREMAP_MAYMOVE|MREMAP_FIXED, p) != MAP_FAILED);
-    assert(search_proc_maps("[vdso]", &low, &high) == 0);
-    size = high - low + 1;
-    printf("VDSO moved to 0x%lx-0x%lx (%lu bytes)\n", low, high, size);
+    // Remap the VDSO and VVAR somewhere else
+    vdso_parking = mmap(NULL, vdso_size + vvar_size,
PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
+    assert(vdso_parking != MAP_FAILED);
+
+    if (vvar_present) {
+        // The relative position of vdso/vvar must always stay the same
+        if (vvar_start > vdso_start) {
+            vvar_parking = vdso_parking + vdso_size;
+        } else {
+            vvar_parking = vdso_parking;
+            vdso_parking = vvar_parking + vvar_size;
+        }
+        assert(mremap((void *)vvar_start, vvar_size, vvar_size,
MREMAP_MAYMOVE|MREMAP_FIXED, vvar_parking) != MAP_FAILED);
+    }
+    assert(mremap((void *)vdso_start, vdso_size, vdso_size,
MREMAP_MAYMOVE|MREMAP_FIXED, vdso_parking) != MAP_FAILED);
+
+    assert(search_proc_maps("[vdso]", &vdso_start, &vdso_end,
&vdso_size) == 0);
+    printf("VDSO moved to 0x%lx-0x%lx (%lu bytes)\n", vdso_start,
vdso_end, vdso_size);
+    assert(search_proc_maps("[vvar]", &vvar_start, &vvar_end,
&vvar_size) == 0);
+    printf("VVAR moved to 0x%lx-0x%lx (%lu bytes)\n", vvar_start,
vvar_end, vvar_size);

     kill(getpid(), SIGUSR1);
     assert(took_signal == 2);
     printf("Signal delivered OK with VDSO moved\n");

-    assert(munmap((void *)low, size) == 0);
+    assert(munmap((void *)vdso_start, vdso_size) == 0);
     printf("Unmapped VDSO\n");

     // Confirm the VDSO is not mapped anymore
-    assert(search_proc_maps("[vdso]", &low, &high) != 0);
+    assert(search_proc_maps("[vdso]", &vdso_start, &vdso_end,
&vdso_size) != 0);

     // Make the stack executable
-    assert(search_proc_maps("[stack]", &low, &high) == 0);
-    size = high - low + 1;
-    mprotect((void *)low, size, PROT_READ|PROT_WRITE|PROT_EXEC);
+    assert(search_proc_maps("[stack]", &stack_start, &stack_end,
&stack_size) == 0);
+    mprotect((void *)stack_start, stack_size, PROT_READ|PROT_WRITE|PROT_EXEC);
     printf("Remapped the stack executable\n");

     kill(getpid(), SIGUSR1);
