[PATCH] tests/tcg/multiarch: add vma-pthread.c

[PATCH] tests/tcg/multiarch: add vma-pthread.c

[Ilya Leoshkevich]

Add a test that locklessly changes and exercises page protection bits
from various threads. This helps catch race conditions in the VMA
handling.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/Makefile.target  |   3 +
 tests/tcg/multiarch/munmap-pthread.c |  16 +--
 tests/tcg/multiarch/nop_func.h       |  25 ++++
 tests/tcg/multiarch/vma-pthread.c    | 185 +++++++++++++++++++++++++++
 4 files changed, 214 insertions(+), 15 deletions(-)
 create mode 100644 tests/tcg/multiarch/nop_func.h
 create mode 100644 tests/tcg/multiarch/vma-pthread.c

diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 5f0fee1aadb..e7213af4925 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -39,6 +39,9 @@ signals: LDFLAGS+=-lrt -lpthread
 munmap-pthread: CFLAGS+=-pthread
 munmap-pthread: LDFLAGS+=-pthread
 
+vma-pthread: CFLAGS+=-pthread
+vma-pthread: LDFLAGS+=-pthread
+
 # We define the runner for test-mmap after the individual
 # architectures have defined their supported pages sizes. If no
 # additional page sizes are defined we only run the default test.
diff --git a/tests/tcg/multiarch/munmap-pthread.c b/tests/tcg/multiarch/munmap-pthread.c
index d7143b00d5f..1c79005846d 100644
--- a/tests/tcg/multiarch/munmap-pthread.c
+++ b/tests/tcg/multiarch/munmap-pthread.c
@@ -7,21 +7,7 @@
 #include <sys/mman.h>
 #include <unistd.h>
 
-static const char nop_func[] = {
-#if defined(__aarch64__)
-    0xc0, 0x03, 0x5f, 0xd6,     /* ret */
-#elif defined(__alpha__)
-    0x01, 0x80, 0xFA, 0x6B,     /* ret */
-#elif defined(__arm__)
-    0x1e, 0xff, 0x2f, 0xe1,     /* bx lr */
-#elif defined(__riscv)
-    0x67, 0x80, 0x00, 0x00,     /* ret */
-#elif defined(__s390__)
-    0x07, 0xfe,                 /* br %r14 */
-#elif defined(__i386__) || defined(__x86_64__)
-    0xc3,                       /* ret */
-#endif
-};
+#include "nop_func.h"
 
 static void *thread_mmap_munmap(void *arg)
 {
diff --git a/tests/tcg/multiarch/nop_func.h b/tests/tcg/multiarch/nop_func.h
new file mode 100644
index 00000000000..f714d210000
--- /dev/null
+++ b/tests/tcg/multiarch/nop_func.h
@@ -0,0 +1,25 @@
+/*
+ * No-op functions that can be safely copied.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef NOP_FUNC_H
+#define NOP_FUNC_H
+
+static const char nop_func[] = {
+#if defined(__aarch64__)
+    0xc0, 0x03, 0x5f, 0xd6,     /* ret */
+#elif defined(__alpha__)
+    0x01, 0x80, 0xFA, 0x6B,     /* ret */
+#elif defined(__arm__)
+    0x1e, 0xff, 0x2f, 0xe1,     /* bx lr */
+#elif defined(__riscv)
+    0x67, 0x80, 0x00, 0x00,     /* ret */
+#elif defined(__s390__)
+    0x07, 0xfe,                 /* br %r14 */
+#elif defined(__i386__) || defined(__x86_64__)
+    0xc3,                       /* ret */
+#endif
+};
+
+#endif
diff --git a/tests/tcg/multiarch/vma-pthread.c b/tests/tcg/multiarch/vma-pthread.c
new file mode 100644
index 00000000000..c405cd46329
--- /dev/null
+++ b/tests/tcg/multiarch/vma-pthread.c
@@ -0,0 +1,185 @@
+/*
+ * Test that VMA updates do not race.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Map a contiguous chunk of RWX memory. Split it into 8 equally sized
+ * regions, each of which is guaranteed to have a certain combination of
+ * protection bits set.
+ *
+ * Reader, writer and executor threads perform the respective operations on
+ * pages, which are guaranteed to have the respective protection bit set.
+ * Two mutator threads change the non-fixed protection bits randomly.
+ */
+#include <assert.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include "nop_func.h"
+
+#define PAGE_IDX_BITS 10
+#define PAGE_COUNT (1 << PAGE_IDX_BITS)
+#define PAGE_IDX_MASK (PAGE_COUNT - 1)
+#define REGION_IDX_BITS 3
+#define PAGE_IDX_R_MASK (1 << 7)
+#define PAGE_IDX_W_MASK (1 << 8)
+#define PAGE_IDX_X_MASK (1 << 9)
+#define REGION_MASK (PAGE_IDX_R_MASK | PAGE_IDX_W_MASK | PAGE_IDX_X_MASK)
+#define PAGES_PER_REGION (1 << (PAGE_IDX_BITS - REGION_IDX_BITS))
+
+struct context {
+    int pagesize;
+    char *ptr;
+    int dev_null_fd;
+    volatile int mutator_count;
+};
+
+static void *thread_read(void *arg)
+{
+    struct context *ctx = arg;
+    ssize_t sret;
+    size_t i, j;
+    int ret;
+
+    for (i = 0; ctx->mutator_count; i++) {
+        j = (i & PAGE_IDX_MASK) | PAGE_IDX_R_MASK;
+        /* Read directly. */
+        ret = memcmp(&ctx->ptr[j * ctx->pagesize], nop_func, sizeof(nop_func));
+        assert(ret == 0);
+        /* Read indirectly. */
+        sret = write(ctx->dev_null_fd, &ctx->ptr[j * ctx->pagesize], 1);
+        assert(sret == 1);
+    }
+
+    return NULL;
+}
+
+static void *thread_write(void *arg)
+{
+    struct context *ctx = arg;
+    struct timespec *ts;
+    size_t i, j;
+    int ret;
+
+    for (i = 0; ctx->mutator_count; i++) {
+        j = (i & PAGE_IDX_MASK) | PAGE_IDX_W_MASK;
+        /* Write directly. */
+        memcpy(&ctx->ptr[j * ctx->pagesize], nop_func, sizeof(nop_func));
+        /* Write using a syscall. */
+        ts = (struct timespec *)(&ctx->ptr[(j + 1) * ctx->pagesize] -
+                                 sizeof(struct timespec));
+        ret = clock_gettime(CLOCK_REALTIME, ts);
+        assert(ret == 0);
+    }
+
+    return NULL;
+}
+
+static void *thread_execute(void *arg)
+{
+    struct context *ctx = arg;
+    size_t i, j;
+
+    for (i = 0; ctx->mutator_count; i++) {
+        j = (i & PAGE_IDX_MASK) | PAGE_IDX_X_MASK;
+        ((void(*)(void))&ctx->ptr[j * ctx->pagesize])();
+    }
+
+    return NULL;
+}
+
+static void *thread_mutate(void *arg)
+{
+    size_t i, start_idx, end_idx, page_idx, tmp;
+    struct context *ctx = arg;
+    unsigned int seed;
+    int prot, ret;
+
+    seed = (unsigned int)time(NULL);
+    for (i = 0; i < 50000; i++) {
+        start_idx = rand_r(&seed) & PAGE_IDX_MASK;
+        end_idx = rand_r(&seed) & PAGE_IDX_MASK;
+        if (start_idx > end_idx) {
+            tmp = start_idx;
+            start_idx = end_idx;
+            end_idx = tmp;
+        }
+        prot = rand_r(&seed) & (PROT_READ | PROT_WRITE | PROT_EXEC);
+        for (page_idx = start_idx & REGION_MASK; page_idx <= end_idx;
+             page_idx += PAGES_PER_REGION) {
+            if (page_idx & PAGE_IDX_R_MASK) {
+                prot |= PROT_READ;
+            }
+            if (page_idx & PAGE_IDX_W_MASK) {
+                prot |= PROT_WRITE;
+            }
+            if (page_idx & PAGE_IDX_X_MASK) {
+                prot |= PROT_EXEC;
+            }
+        }
+        ret = mprotect(&ctx->ptr[start_idx * ctx->pagesize],
+                       (end_idx - start_idx + 1) * ctx->pagesize, prot);
+        assert(ret == 0);
+    }
+
+    __atomic_fetch_sub(&ctx->mutator_count, 1, __ATOMIC_SEQ_CST);
+
+    return NULL;
+}
+
+int main(void)
+{
+    pthread_t threads[5];
+    struct context ctx;
+    size_t i;
+    int ret;
+
+    /* Without a template, nothing to test. */
+    if (sizeof(nop_func) == 0) {
+        return EXIT_SUCCESS;
+    }
+
+    /* Initialize memory chunk. */
+    ctx.pagesize = getpagesize();
+    ctx.ptr = mmap(NULL, PAGE_COUNT * ctx.pagesize,
+                   PROT_READ | PROT_WRITE | PROT_EXEC,
+                   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    assert(ctx.ptr != MAP_FAILED);
+    for (i = 0; i < PAGE_COUNT; i++) {
+        memcpy(&ctx.ptr[i * ctx.pagesize], nop_func, sizeof(nop_func));
+    }
+    ctx.dev_null_fd = open("/dev/null", O_WRONLY);
+    assert(ctx.dev_null_fd >= 0);
+    ctx.mutator_count = 2;
+
+    /* Start threads. */
+    ret = pthread_create(&threads[0], NULL, thread_read, &ctx);
+    assert(ret == 0);
+    ret = pthread_create(&threads[1], NULL, thread_write, &ctx);
+    assert(ret == 0);
+    ret = pthread_create(&threads[2], NULL, thread_execute, &ctx);
+    assert(ret == 0);
+    for (i = 3; i <= 4; i++) {
+        ret = pthread_create(&threads[i], NULL, thread_mutate, &ctx);
+        assert(ret == 0);
+    }
+
+    /* Wait for threads to stop. */
+    for (i = 0; i < sizeof(threads) / sizeof(threads[0]); i++) {
+        ret = pthread_join(threads[i], NULL);
+        assert(ret == 0);
+    }
+
+    /* Destroy memory chunk. */
+    ret = close(ctx.dev_null_fd);
+    assert(ret == 0);
+    ret = munmap(ctx.ptr, PAGE_COUNT * ctx.pagesize);
+    assert(ret == 0);
+
+    return EXIT_SUCCESS;
+}
-- 
2.38.1

Re: [PATCH] tests/tcg/multiarch: add vma-pthread.c

[Ilya Leoshkevich]

On Fri, 2022-12-23 at 13:02 +0100, Ilya Leoshkevich wrote:
> Add a test that locklessly changes and exercises page protection bits
> from various threads. This helps catch race conditions in the VMA
> handling.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tests/tcg/multiarch/Makefile.target  |   3 +
>  tests/tcg/multiarch/munmap-pthread.c |  16 +--
>  tests/tcg/multiarch/nop_func.h       |  25 ++++
>  tests/tcg/multiarch/vma-pthread.c    | 185
> +++++++++++++++++++++++++++
>  4 files changed, 214 insertions(+), 15 deletions(-)
>  create mode 100644 tests/tcg/multiarch/nop_func.h
>  create mode 100644 tests/tcg/multiarch/vma-pthread.c

This was meant to be a reply to the bug report for [1], but apparently
I forgot to Cc: the mailing list. Copying the original message here:

---
Hi,

Wasmtime testsuite started failing randomly, complaining that
clock_gettime() returns -EFAULT. Bisect points to this commit.

I could not see anything obviously wrong here with the manual review,
and the failure was not reproducible when running individual testcases
or using strace. So I wrote a stress test (which I will post shortly),
which runs fine on the host, but reproduces the issue with qemu-user.

When run with -strace, it also triggers an assertion:

    qemu-x86_64: ../accel/tcg/tb-maint.c:595:
tb_invalidate_phys_page_unwind: Assertion `pc != 0' failed.
    qemu-x86_64: /home/iii/qemu/include/qemu/rcu.h:102:
rcu_read_unlock: Assertion `p_rcu_reader->depth != 0' failed.

I haven't tried analyzing what is causing all this yet, but at least
now the reproducer is small (~200LOC) and fails faster than 1s.

Best regards,
Ilya
---

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg03615.html

Re: [PATCH] tests/tcg/multiarch: add vma-pthread.c

[Richard Henderson]

On 12/23/22 04:02, Ilya Leoshkevich wrote:
> +static void *thread_write(void *arg)
> +{
> +    struct context *ctx = arg;
> +    struct timespec *ts;
> +    size_t i, j;
> +    int ret;
> +
> +    for (i = 0; ctx->mutator_count; i++) {
> +        j = (i & PAGE_IDX_MASK) | PAGE_IDX_W_MASK;
> +        /* Write directly. */
> +        memcpy(&ctx->ptr[j * ctx->pagesize], nop_func, sizeof(nop_func));
> +        /* Write using a syscall. */
> +        ts = (struct timespec *)(&ctx->ptr[(j + 1) * ctx->pagesize] -
> +                                 sizeof(struct timespec));
> +        ret = clock_gettime(CLOCK_REALTIME, ts);
> +        assert(ret == 0);

With the 3 issues that you pointed out in the other email, this is the only remaining 
failure.  This happens because of two issues:

(1) When checking for writability, we actually check for both read+write:

#define VERIFY_WRITE (PAGE_READ | PAGE_WRITE)

This is very likely a bug, but we'd need to audit all uses to find out where we might 
really want read+write.

> +static void *thread_mutate(void *arg)
> +{
> +    size_t i, start_idx, end_idx, page_idx, tmp;
> +    struct context *ctx = arg;
> +    unsigned int seed;
> +    int prot, ret;
> +
> +    seed = (unsigned int)time(NULL);
> +    for (i = 0; i < 50000; i++) {
> +        start_idx = rand_r(&seed) & PAGE_IDX_MASK;
> +        end_idx = rand_r(&seed) & PAGE_IDX_MASK;
> +        if (start_idx > end_idx) {
> +            tmp = start_idx;
> +            start_idx = end_idx;
> +            end_idx = tmp;
> +        }
> +        prot = rand_r(&seed) & (PROT_READ | PROT_WRITE | PROT_EXEC);
> +        for (page_idx = start_idx & REGION_MASK; page_idx <= end_idx;
> +             page_idx += PAGES_PER_REGION) {
> +            if (page_idx & PAGE_IDX_R_MASK) {
> +                prot |= PROT_READ;
> +            }
> +            if (page_idx & PAGE_IDX_W_MASK) {
> +                prot |= PROT_WRITE;
> +            }
> +            if (page_idx & PAGE_IDX_X_MASK) {
> +                prot |= PROT_EXEC;
> +            }

... and here we can wind up with write-only pages.

(2) Certain hardware, like x86_64, does not support write-only pages -- writable implies 
readable -- so the testcase runs on some hardware.  It is a bug that we don't model this 
as well.


r~