[PATCH 0/2] accel/tcg: Fix concurrent pthread_create() and munmap()

[PATCH 0/2] accel/tcg: Fix concurrent pthread_create() and munmap()

[Ilya Leoshkevich]

Hi,

This is a fix for the issue reported in [1].
Patch 1 is the fix itself, patch 2 is the test/reproducer.

By the way, I noticed that there is no code to free tb_jmp_cache, and
object_finalize(CPUState) is never called - in case of system emulation
this is fine, but what about linux-user?

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

Best regards,
Ilya

Ilya Leoshkevich (2):
  accel/tcg: Fix concurrent pthread_create() and munmap()
  tests/tcg/multiarch: Add munmap-pthread.c

 accel/tcg/tb-maint.c                 |  5 ++
 tests/tcg/multiarch/Makefile.target  |  3 ++
 tests/tcg/multiarch/munmap-pthread.c | 71 ++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 tests/tcg/multiarch/munmap-pthread.c

-- 
2.37.2

[PATCH 1/2] accel/tcg: Fix concurrent pthread_create() and munmap()

[Ilya Leoshkevich]

munmap() flushes jump cache on all CPUs if the unmapped range contains
a translation block. This currently includes new CPUs (i.e. qemu-user
threads), for which there is no jump cache yet, which leads to a null
pointer dereference.

Fix by skipping new CPUs.

Fixes: a976a99a2975 ("include/hw/core: Create struct CPUJumpCache")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 accel/tcg/tb-maint.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index c8e921089df..2a063f91aa6 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -241,6 +241,11 @@ static void tb_jmp_cache_inval_tb(TranslationBlock *tb)
         CPU_FOREACH(cpu) {
             CPUJumpCache *jc = cpu->tb_jmp_cache;
 
+            if (unlikely(!jc)) {
+                /* This is a new CPU that is not initialized yet. */
+                continue;
+            }
+
             if (qatomic_read(&jc->array[h].tb) == tb) {
                 qatomic_set(&jc->array[h].tb, NULL);
             }
-- 
2.37.2

[PATCH 2/2] tests/tcg/multiarch: Add munmap-pthread.c

[Ilya Leoshkevich]

Add a test to detect races between munmap() and creating new threads.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/Makefile.target  |  3 ++
 tests/tcg/multiarch/munmap-pthread.c | 71 ++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 tests/tcg/multiarch/munmap-pthread.c

diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 78104f9bbb7..5f0fee1aadb 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -36,6 +36,9 @@ threadcount: LDFLAGS+=-lpthread
 
 signals: LDFLAGS+=-lrt -lpthread
 
+munmap-pthread: CFLAGS+=-pthread
+munmap-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
new file mode 100644
index 00000000000..d2ac9aae604
--- /dev/null
+++ b/tests/tcg/multiarch/munmap-pthread.c
@@ -0,0 +1,71 @@
+/* Test that munmap() and thread creation do not race. */
+#include <assert.h>
+#include <pthread.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+static const char nop_func[] = {
+#if defined(__s390__)
+    /* br %r14 */
+    0x07, 0xfe,
+#elif defined(__i386__) || defined(__x86_64__)
+    /* ret */
+    0xc3,
+#endif
+};
+
+static void *thread_mmap_munmap(void *arg)
+{
+    volatile bool *run = arg;
+    char *p;
+    int ret;
+
+    while (*run) {
+        p = mmap(NULL, getpagesize(), PROT_READ | PROT_WRITE | PROT_EXEC,
+                 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+        assert(p != MAP_FAILED);
+        if (sizeof(nop_func) != 0) {
+            /*
+             * Create a small translation block if we have a template for the
+             * current architecture.
+             */
+            memcpy(p, nop_func, sizeof(nop_func));
+            ((void(*)(void))p)();
+        }
+        ret = munmap(p, getpagesize());
+        assert(ret == 0);
+    }
+
+    return NULL;
+}
+
+static void *thread_dummy(void *arg)
+{
+    return NULL;
+}
+
+int main(void)
+{
+    pthread_t mmap_munmap, dummy;
+    volatile bool run = true;
+    int i, ret;
+
+    ret = pthread_create(&mmap_munmap, NULL, thread_mmap_munmap, (void *)&run);
+    assert(ret == 0);
+
+    for (i = 0; i < 1000; i++) {
+        ret = pthread_create(&dummy, NULL, thread_dummy, NULL);
+        assert(ret == 0);
+        ret = pthread_join(dummy, NULL);
+        assert(ret == 0);
+    }
+
+    run = false;
+    ret = pthread_join(mmap_munmap, NULL);
+    assert(ret == 0);
+
+    return EXIT_SUCCESS;
+}
-- 
2.37.2

Re: [PATCH 0/2] accel/tcg: Fix concurrent pthread_create() and munmap()

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Hi,
>
> This is a fix for the issue reported in [1].
> Patch 1 is the fix itself, patch 2 is the test/reproducer.
>
> By the way, I noticed that there is no code to free tb_jmp_cache, and
> object_finalize(CPUState) is never called - in case of system emulation
> this is fine, but what about linux-user?

We definitely have a memory leak here. The last attempt to properly
finalize CPUState failed because we have duplicate pointers to the
cpregs structures that end up in generated code. See:

  https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02819.html

>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-10/msg05181.html
>
> Best regards,
> Ilya
>
> Ilya Leoshkevich (2):
>   accel/tcg: Fix concurrent pthread_create() and munmap()
>   tests/tcg/multiarch: Add munmap-pthread.c
>
>  accel/tcg/tb-maint.c                 |  5 ++
>  tests/tcg/multiarch/Makefile.target  |  3 ++
>  tests/tcg/multiarch/munmap-pthread.c | 71 ++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 tests/tcg/multiarch/munmap-pthread.c


-- 
Alex Bennée

Re: [PATCH 1/2] accel/tcg: Fix concurrent pthread_create() and munmap()

[Richard Henderson]

On 10/28/22 23:42, Ilya Leoshkevich wrote:
> munmap() flushes jump cache on all CPUs if the unmapped range contains
> a translation block. This currently includes new CPUs (i.e. qemu-user
> threads), for which there is no jump cache yet, which leads to a null
> pointer dereference.
> 
> Fix by skipping new CPUs.
> 
> Fixes: a976a99a2975 ("include/hw/core: Create struct CPUJumpCache")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   accel/tcg/tb-maint.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
> index c8e921089df..2a063f91aa6 100644
> --- a/accel/tcg/tb-maint.c
> +++ b/accel/tcg/tb-maint.c
> @@ -241,6 +241,11 @@ static void tb_jmp_cache_inval_tb(TranslationBlock *tb)
>           CPU_FOREACH(cpu) {
>               CPUJumpCache *jc = cpu->tb_jmp_cache;
>   
> +            if (unlikely(!jc)) {
> +                /* This is a new CPU that is not initialized yet. */
> +                continue;
> +            }
> +

Interesting that the new cpu gets linked before realize.  I wonder if cpu_list_add should 
be moved from the top of cpu_exec_realizefn to the end.

Thanks for the test case.  I'll have a closer look.


r~