Re: [PATCH] selftests/powerpc: Add a test of sigreturn vs VDSO
Le 26/03/2020 à 13:06, Michael Ellerman a écrit : On Wed, 2020-03-04 at 11:04:02 UTC, Michael Ellerman wrote: There's two different paths through the sigreturn code, depending on whether the VDSO is mapped or not. We recently discovered a bug in the unmapped case, because it's not commonly used these days. So add a test that sends itself a signal, then moves the VDSO, takes another signal and finally unmaps the VDSO before sending itself another signal. That tests the standard signal path, the code that handles the VDSO being moved, and also the signal path in the case where the VDSO is unmapped. Signed-off-by: Michael Ellerman Applied to powerpc next. https://git.kernel.org/powerpc/c/a0968a025c04702427a4aee2c618f451a5098cd8 cheers Doesn't work anymore since the split of VDSO and VVAR. Christophe
Re: [PATCH] selftests/powerpc: Add a test of sigreturn vs VDSO
On Wed, 2020-03-04 at 11:04:02 UTC, Michael Ellerman wrote: > There's two different paths through the sigreturn code, depending on > whether the VDSO is mapped or not. We recently discovered a bug in the > unmapped case, because it's not commonly used these days. > > So add a test that sends itself a signal, then moves the VDSO, takes > another signal and finally unmaps the VDSO before sending itself > another signal. That tests the standard signal path, the code that > handles the VDSO being moved, and also the signal path in the case > where the VDSO is unmapped. > > Signed-off-by: Michael Ellerman Applied to powerpc next. https://git.kernel.org/powerpc/c/a0968a025c04702427a4aee2c618f451a5098cd8 cheers
Re: [PATCH] selftests/powerpc: Add a test of sigreturn vs VDSO
Nathan Lynch writes:
> Nathan Lynch writes:
>> Michael Ellerman writes:
>>> +static int search_proc_maps(char *needle, unsigned long *low, unsigned
>>> long *high)
>>
>>^^ const?
Sorry I meant to do this but then forgot.
>>> +{
>>> + unsigned long start, end;
>>> + static char buf[4096];
>>> + char name[128];
>>> + FILE *f;
>>> + int rc = -1;
>>> +
>>> + f = fopen("/proc/self/maps", "r");
>>> + if (!f) {
>>> + perror("fopen");
>>> + return -1;
>>> + }
>>> +
>>> + while (fgets(buf, sizeof(buf), f)) {
>>> + rc = sscanf(buf, "%lx-%lx %*c%*c%*c%*c %*x %*d:%*d %*d %127s\n",
>>> + &start, &end, name);
>>
>> I suspect it doesn't matter in practice for this particular test, but
>> since this looks like a generally useful function that could gain users
>> in the future: does this spuriously fail if the matching line straddles
>> a 4096-byte boundary? Maybe fscanf(3) should be used instead?
>
> Or maybe I should read the fgets man page more closely :-)
>
> "Reading stops after an EOF or a newline."
>
> Sorry for the noise.
No worries, thanks for reviewing.
cheers
Re: [PATCH] selftests/powerpc: Add a test of sigreturn vs VDSO
Nathan Lynch writes:
> Michael Ellerman writes:
>
>> +static int search_proc_maps(char *needle, unsigned long *low, unsigned long
>> *high)
>
>^^ const?
>
>> +{
>> +unsigned long start, end;
>> +static char buf[4096];
>> +char name[128];
>> +FILE *f;
>> +int rc = -1;
>> +
>> +f = fopen("/proc/self/maps", "r");
>> +if (!f) {
>> +perror("fopen");
>> +return -1;
>> +}
>> +
>> +while (fgets(buf, sizeof(buf), f)) {
>> +rc = sscanf(buf, "%lx-%lx %*c%*c%*c%*c %*x %*d:%*d %*d %127s\n",
>> +&start, &end, name);
>
> I suspect it doesn't matter in practice for this particular test, but
> since this looks like a generally useful function that could gain users
> in the future: does this spuriously fail if the matching line straddles
> a 4096-byte boundary? Maybe fscanf(3) should be used instead?
Or maybe I should read the fgets man page more closely :-)
"Reading stops after an EOF or a newline."
Sorry for the noise.
Re: [PATCH] selftests/powerpc: Add a test of sigreturn vs VDSO
Michael Ellerman writes:
> +static int search_proc_maps(char *needle, unsigned long *low, unsigned long
> *high)
^^ const?
> +{
> + unsigned long start, end;
> + static char buf[4096];
> + char name[128];
> + FILE *f;
> + int rc = -1;
> +
> + f = fopen("/proc/self/maps", "r");
> + if (!f) {
> + perror("fopen");
> + return -1;
> + }
> +
> + while (fgets(buf, sizeof(buf), f)) {
> + rc = sscanf(buf, "%lx-%lx %*c%*c%*c%*c %*x %*d:%*d %*d %127s\n",
> + &start, &end, name);
I suspect it doesn't matter in practice for this particular test, but
since this looks like a generally useful function that could gain users
in the future: does this spuriously fail if the matching line straddles
a 4096-byte boundary? Maybe fscanf(3) should be used instead?
> + if (rc == 2)
> + continue;
> +
> + if (rc != 3) {
> + printf("sscanf errored\n");
> + rc = -1;
> + break;
> + }
> +
> + if (strstr(name, needle)) {
> + *low = start;
> + *high = end - 1;
> + rc = 0;
> + break;
> + }
> + }
> +
> + fclose(f);
> +
> + return rc;
> +}
> +
> +static volatile sig_atomic_t took_signal = 0;
> +
> +static void sigusr1_handler(int sig)
> +{
> + took_signal++;
> +}
> +
> +int test_sigreturn_vdso(void)
> +{
> + unsigned long low, high, size;
> + struct sigaction act;
> + char *p;
> +
> + act.sa_handler = sigusr1_handler;
> + act.sa_flags = 0;
> + sigemptyset(&act.sa_mask);
> +
> + 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);
> +
> + kill(getpid(), SIGUSR1);
> + assert(took_signal == 1);
> + printf("Signal delivered OK with VDSO mapped\n");
I haven't looked at the test harness in detail but this should be
reliable if the program is a single thread - lgtm.
[PATCH] selftests/powerpc: Add a test of sigreturn vs VDSO
There's two different paths through the sigreturn code, depending on
whether the VDSO is mapped or not. We recently discovered a bug in the
unmapped case, because it's not commonly used these days.
So add a test that sends itself a signal, then moves the VDSO, takes
another signal and finally unmaps the VDSO before sending itself
another signal. That tests the standard signal path, the code that
handles the VDSO being moved, and also the signal path in the case
where the VDSO is unmapped.
Signed-off-by: Michael Ellerman
---
.../selftests/powerpc/signal/.gitignore | 1 +
.../testing/selftests/powerpc/signal/Makefile | 2 +-
.../selftests/powerpc/signal/sigreturn_vdso.c | 127 ++
3 files changed, 129 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
diff --git a/tools/testing/selftests/powerpc/signal/.gitignore
b/tools/testing/selftests/powerpc/signal/.gitignore
index dca5852a1546..03dafa795255 100644
--- a/tools/testing/selftests/powerpc/signal/.gitignore
+++ b/tools/testing/selftests/powerpc/signal/.gitignore
@@ -1,3 +1,4 @@
signal
signal_tm
sigfuz
+sigreturn_vdso
diff --git a/tools/testing/selftests/powerpc/signal/Makefile
b/tools/testing/selftests/powerpc/signal/Makefile
index 113838fbbe7f..63b57583e07d 100644
--- a/tools/testing/selftests/powerpc/signal/Makefile
+++ b/tools/testing/selftests/powerpc/signal/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-TEST_GEN_PROGS := signal signal_tm sigfuz
+TEST_GEN_PROGS := signal signal_tm sigfuz sigreturn_vdso
CFLAGS += -maltivec
$(OUTPUT)/signal_tm: CFLAGS += -mhtm
diff --git a/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
b/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
new file mode 100644
index ..e282fff0fe25
--- /dev/null
+++ b/tools/testing/selftests/powerpc/signal/sigreturn_vdso.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test that we can take signals with and without the VDSO mapped, which
trigger
+ * different paths in the signal handling code.
+ *
+ * See handle_rt_signal64() and setup_trampoline() in signal_64.c
+ */
+
+#define _GNU_SOURCE
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+// Ensure assert() is not compiled out
+#undef NDEBUG
+#include
+
+#include "utils.h"
+
+static int search_proc_maps(char *needle, unsigned long *low, unsigned long
*high)
+{
+ unsigned long start, end;
+ static char buf[4096];
+ char name[128];
+ FILE *f;
+ int rc = -1;
+
+ f = fopen("/proc/self/maps", "r");
+ if (!f) {
+ perror("fopen");
+ return -1;
+ }
+
+ while (fgets(buf, sizeof(buf), f)) {
+ rc = sscanf(buf, "%lx-%lx %*c%*c%*c%*c %*x %*d:%*d %*d %127s\n",
+ &start, &end, name);
+ if (rc == 2)
+ continue;
+
+ if (rc != 3) {
+ printf("sscanf errored\n");
+ rc = -1;
+ break;
+ }
+
+ if (strstr(name, needle)) {
+ *low = start;
+ *high = end - 1;
+ rc = 0;
+ break;
+ }
+ }
+
+ fclose(f);
+
+ return rc;
+}
+
+static volatile sig_atomic_t took_signal = 0;
+
+static void sigusr1_handler(int sig)
+{
+ took_signal++;
+}
+
+int test_sigreturn_vdso(void)
+{
+ unsigned long low, high, size;
+ struct sigaction act;
+ char *p;
+
+ act.sa_handler = sigusr1_handler;
+ act.sa_flags = 0;
+ sigemptyset(&act.sa_mask);
+
+ 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);
+
+ 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);
+
+ kill(getpid(), SIGUSR1);
+ assert(took_signal == 2);
+ printf("Signal delivered OK with VDSO moved\n");
+
+ assert(munmap((void *)low, size) == 0);
+ printf("Unmapped VDSO\n");
+
+ // Confirm the VDSO is not mapped anymore
+ assert(search_proc_maps("[vdso]", &low, &high) != 0);
+
+ // Make the stack executable
+ assert(search_proc_maps("[stack]", &low, &high) =
