| Message ID | 20221224151821.464455-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Fixes for user-only page tracking | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > From: Ilya Leoshkevich <iii@linux.ibm.com> > > 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> > Message-Id: <20221223120252.513319-1-iii@linux.ibm.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Acked-by: Alex Bennée <alex.bennee@linaro.org>
On Sat, 24 Dec 2022 at 15:19, Richard Henderson <richard.henderson@linaro.org> wrote: > > From: Ilya Leoshkevich <iii@linux.ibm.com> > > 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> > Message-Id: <20221223120252.513319-1-iii@linux.ibm.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> I've noticed that this newly added vma-pthread test seems to be flaky. Here's an example from a clang-user job: https://gitlab.com/qemu-project/qemu/-/jobs/3600385176 TEST vma-pthread-with-libbb.so on aarch64 fail indirect write 0x5500b1eff0 (Bad address) timeout: the monitored command dumped core Aborted make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libbb.so] Error 134 and another from a few days earlier: https://gitlab.com/qemu-project/qemu/-/jobs/3572970612 TEST vma-pthread-with-libsyscall.so on s390x fail indirect read 0x4000999000 (Bad address) timeout: the monitored command dumped core Aborted make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libsyscall.so] Error 134 thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Sat, 24 Dec 2022 at 15:19, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> From: Ilya Leoshkevich <iii@linux.ibm.com> >> >> 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> >> Message-Id: <20221223120252.513319-1-iii@linux.ibm.com> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > I've noticed that this newly added vma-pthread test seems to > be flaky. Here's an example from a clang-user job: > https://gitlab.com/qemu-project/qemu/-/jobs/3600385176 > > TEST vma-pthread-with-libbb.so on aarch64 > fail indirect write 0x5500b1eff0 (Bad address) > timeout: the monitored command dumped core > Aborted > make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libbb.so] Error 134 > > and another from a few days earlier: > https://gitlab.com/qemu-project/qemu/-/jobs/3572970612 > > TEST vma-pthread-with-libsyscall.so on s390x > fail indirect read 0x4000999000 (Bad address) > timeout: the monitored command dumped core > Aborted > make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libsyscall.so] Error 134 > > thanks > -- PMM Interesting those are both with plugins. I wonder if the tsan plugin fixes in my latest tree help?
On 13/1/23 18:10, Alex Bennée wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Sat, 24 Dec 2022 at 15:19, Richard Henderson >> <richard.henderson@linaro.org> wrote: >>> >>> From: Ilya Leoshkevich <iii@linux.ibm.com> >>> >>> 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> >>> Message-Id: <20221223120252.513319-1-iii@linux.ibm.com> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> >> I've noticed that this newly added vma-pthread test seems to >> be flaky. Here's an example from a clang-user job: >> https://gitlab.com/qemu-project/qemu/-/jobs/3600385176 >> >> TEST vma-pthread-with-libbb.so on aarch64 >> fail indirect write 0x5500b1eff0 (Bad address) >> timeout: the monitored command dumped core >> Aborted >> make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libbb.so] Error 134 >> >> and another from a few days earlier: >> https://gitlab.com/qemu-project/qemu/-/jobs/3572970612 >> >> TEST vma-pthread-with-libsyscall.so on s390x >> fail indirect read 0x4000999000 (Bad address) >> timeout: the monitored command dumped core >> Aborted >> make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libsyscall.so] Error 134 Yet again: https://gitlab.com/qemu-project/qemu/-/jobs/3608436731 > Interesting those are both with plugins. I wonder if the tsan plugin > fixes in my latest tree help? >
On Mon, 16 Jan 2023 at 12:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 13/1/23 18:10, Alex Bennée wrote: > > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > >> On Sat, 24 Dec 2022 at 15:19, Richard Henderson > >> <richard.henderson@linaro.org> wrote: > >>> > >>> From: Ilya Leoshkevich <iii@linux.ibm.com> > >>> > >>> 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> > >>> Message-Id: <20221223120252.513319-1-iii@linux.ibm.com> > >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> > >> I've noticed that this newly added vma-pthread test seems to > >> be flaky. Here's an example from a clang-user job: > >> https://gitlab.com/qemu-project/qemu/-/jobs/3600385176 > >> > >> TEST vma-pthread-with-libbb.so on aarch64 > >> fail indirect write 0x5500b1eff0 (Bad address) > >> timeout: the monitored command dumped core > >> Aborted > >> make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libbb.so] Error 134 > >> > >> and another from a few days earlier: > >> https://gitlab.com/qemu-project/qemu/-/jobs/3572970612 > >> > >> TEST vma-pthread-with-libsyscall.so on s390x > >> fail indirect read 0x4000999000 (Bad address) > >> timeout: the monitored command dumped core > >> Aborted > >> make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libsyscall.so] Error 134 > > Yet again: > https://gitlab.com/qemu-project/qemu/-/jobs/3608436731 Yep. Could somebody write a patch to disable this test while we figure out why it's flaky, please? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 16 Jan 2023 at 12:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> On 13/1/23 18:10, Alex Bennée wrote: >> > >> > Peter Maydell <peter.maydell@linaro.org> writes: >> > >> >> On Sat, 24 Dec 2022 at 15:19, Richard Henderson >> >> <richard.henderson@linaro.org> wrote: >> >>> >> >>> From: Ilya Leoshkevich <iii@linux.ibm.com> >> >>> >> >>> 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> >> >>> Message-Id: <20221223120252.513319-1-iii@linux.ibm.com> >> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> >> >> >> I've noticed that this newly added vma-pthread test seems to >> >> be flaky. Here's an example from a clang-user job: >> >> https://gitlab.com/qemu-project/qemu/-/jobs/3600385176 >> >> >> >> TEST vma-pthread-with-libbb.so on aarch64 >> >> fail indirect write 0x5500b1eff0 (Bad address) >> >> timeout: the monitored command dumped core >> >> Aborted >> >> make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libbb.so] Error 134 >> >> >> >> and another from a few days earlier: >> >> https://gitlab.com/qemu-project/qemu/-/jobs/3572970612 >> >> >> >> TEST vma-pthread-with-libsyscall.so on s390x >> >> fail indirect read 0x4000999000 (Bad address) >> >> timeout: the monitored command dumped core >> >> Aborted >> >> make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libsyscall.so] Error 134 >> >> Yet again: >> https://gitlab.com/qemu-project/qemu/-/jobs/3608436731 > > Yep. Could somebody write a patch to disable this test while > we figure out why it's flaky, please? I don't think the test is flaky - I think it is triggering a race in QEMU code. I have not however been able to replicate it in anything other than CI. Although looking at the test I'm beginning to wonder what the sync point is between the mutator and the read/write threads? > > thanks > -- PMM
On Mon, 16 Jan 2023 at 16:33, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Mon, 16 Jan 2023 at 12:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> > >> On 13/1/23 18:10, Alex Bennée wrote: > > Yep. Could somebody write a patch to disable this test while > > we figure out why it's flaky, please? > > I don't think the test is flaky - I think it is triggering a race in > QEMU code. I have not however been able to replicate it in anything other > than CI. My definition of "flaky" here is "the CI fails randomly". Unless you have a patch to fix the underlying bug ready to go right now, please can we disable this so we don't keep getting CI failures trying to test merges? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 16 Jan 2023 at 16:33, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >> > On Mon, 16 Jan 2023 at 12:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> >> >> On 13/1/23 18:10, Alex Bennée wrote: >> > Yep. Could somebody write a patch to disable this test while >> > we figure out why it's flaky, please? >> >> I don't think the test is flaky - I think it is triggering a race in >> QEMU code. I have not however been able to replicate it in anything other >> than CI. > > My definition of "flaky" here is "the CI fails randomly". > Unless you have a patch to fix the underlying bug ready to > go right now, please can we disable this so we don't keep > getting CI failures trying to test merges? Yes - just testing the patch now. > > thanks > -- PMM
On 1/16/23 06:27, Alex Bennée wrote: > Although looking at the test I'm beginning to wonder what the sync point > is between the mutator and the read/write threads? What do you mean? There is no explicit sync, because the mutator always leaves each page with the (one) permission that the read/write/execute thread requires. Within qemu... the sync point is primarily the syscall for read/write, and the tb invalidate (during the mprotect) or the mmap lock for new translation for execute. r~
On Fri, 13 Jan 2023 at 17:10, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Sat, 24 Dec 2022 at 15:19, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> From: Ilya Leoshkevich <iii@linux.ibm.com> > >> > >> 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> > >> Message-Id: <20221223120252.513319-1-iii@linux.ibm.com> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > > > I've noticed that this newly added vma-pthread test seems to > > be flaky. Here's an example from a clang-user job: > > https://gitlab.com/qemu-project/qemu/-/jobs/3600385176 > > > > TEST vma-pthread-with-libbb.so on aarch64 > > fail indirect write 0x5500b1eff0 (Bad address) > > timeout: the monitored command dumped core > > Aborted > > make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libbb.so] Error 134 > > > > and another from a few days earlier: > > https://gitlab.com/qemu-project/qemu/-/jobs/3572970612 > > > > TEST vma-pthread-with-libsyscall.so on s390x > > fail indirect read 0x4000999000 (Bad address) > > timeout: the monitored command dumped core > > Aborted > > make[1]: *** [Makefile:173: run-plugin-vma-pthread-with-libsyscall.so] Error 134 > > > > thanks > > -- PMM > > Interesting those are both with plugins. I wonder if the tsan plugin > fixes in my latest tree help? I think this is a failure in the non-plugin case: https://gitlab.com/qemu-project/qemu/-/jobs/3636082364 -- PMM
diff --git a/tests/tcg/multiarch/nop_func.h b/tests/tcg/multiarch/nop_func.h new file mode 100644 index 0000000000..f714d21000 --- /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/munmap-pthread.c b/tests/tcg/multiarch/munmap-pthread.c index d7143b00d5..1c79005846 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/vma-pthread.c b/tests/tcg/multiarch/vma-pthread.c new file mode 100644 index 0000000000..35e63cd6cd --- /dev/null +++ b/tests/tcg/multiarch/vma-pthread.c @@ -0,0 +1,207 @@ +/* + * 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 <stdio.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++) { + char *p; + + j = (i & PAGE_IDX_MASK) | PAGE_IDX_R_MASK; + p = &ctx->ptr[j * ctx->pagesize]; + + /* Read directly. */ + ret = memcmp(p, nop_func, sizeof(nop_func)); + if (ret != 0) { + fprintf(stderr, "fail direct read %p\n", p); + abort(); + } + + /* Read indirectly. */ + sret = write(ctx->dev_null_fd, p, 1); + if (sret != 1) { + if (sret < 0) { + fprintf(stderr, "fail indirect read %p (%m)\n", p); + } else { + fprintf(stderr, "fail indirect read %p (%zd)\n", p, sret); + } + abort(); + } + } + + 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); + if (ret != 0) { + fprintf(stderr, "fail indirect write %p (%m)\n", ts); + abort(); + } + } + + 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) { + /* FIXME: qemu syscalls check for both read+write. */ + prot |= PROT_WRITE | PROT_READ; + } + 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; +} diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target index 5f0fee1aad..e7213af492 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.