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

Ilya Leoshkevich posted 2 patches 3 years, 3 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>
[PATCH 1/2] accel/tcg: Fix concurrent pthread_create() and munmap()
Posted by Ilya Leoshkevich 3 years, 3 months ago
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
Re: [PATCH 1/2] accel/tcg: Fix concurrent pthread_create() and munmap()
Posted by Richard Henderson 3 years, 3 months ago
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~