[PATCH] kvm: dirty-ring: Fix race with vcpu creation

[PATCH] kvm: dirty-ring: Fix race with vcpu creation

[Peter Xu]

It's possible that we want to reap a dirty ring on a vcpu that is during
creation, because the vcpu is put onto list (CPU_FOREACH visible) before
initialization of the structures.  In this case:

qemu_init_vcpu
    x86_cpu_realizefn
        cpu_exec_realizefn
            cpu_list_add      <---- can be probed by CPU_FOREACH
        qemu_init_vcpu
            cpus_accel->create_vcpu_thread(cpu);
                kvm_init_vcpu
                    map kvm_dirty_gfns  <--- kvm_dirty_gfns valid

Don't try to reap dirty ring on vcpus during creation or it'll crash.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124756
Reported-by: Xiaohui Li <xiaohli@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/kvm/kvm-all.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 5acab1767f..df5fabd3a8 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -757,6 +757,15 @@ static uint32_t kvm_dirty_ring_reap_one(KVMState *s, CPUState *cpu)
     uint32_t ring_size = s->kvm_dirty_ring_size;
     uint32_t count = 0, fetch = cpu->kvm_fetch_index;
 
+    /*
+     * It's possible that we race with vcpu creation code where the vcpu is
+     * put onto the vcpus list but not yet initialized the dirty ring
+     * structures.  If so, skip it.
+     */
+    if (!cpu->created) {
+        return 0;
+    }
+
     assert(dirty_gfns && ring_size);
     trace_kvm_dirty_ring_reap_vcpu(cpu->cpu_index);
 
-- 
2.37.3

Re: [PATCH] kvm: dirty-ring: Fix race with vcpu creation

[Peter Xu]

Ping?

Re: [PATCH] kvm: dirty-ring: Fix race with vcpu creation

[Peter Xu]

Ping

On Tue, Sep 27, 2022 at 11:46:53AM -0400, Peter Xu wrote:
> It's possible that we want to reap a dirty ring on a vcpu that is during
> creation, because the vcpu is put onto list (CPU_FOREACH visible) before
> initialization of the structures.  In this case:
> 
> qemu_init_vcpu
>     x86_cpu_realizefn
>         cpu_exec_realizefn
>             cpu_list_add      <---- can be probed by CPU_FOREACH
>         qemu_init_vcpu
>             cpus_accel->create_vcpu_thread(cpu);
>                 kvm_init_vcpu
>                     map kvm_dirty_gfns  <--- kvm_dirty_gfns valid
> 
> Don't try to reap dirty ring on vcpus during creation or it'll crash.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124756
> Reported-by: Xiaohui Li <xiaohli@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 5acab1767f..df5fabd3a8 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -757,6 +757,15 @@ static uint32_t kvm_dirty_ring_reap_one(KVMState *s, CPUState *cpu)
>      uint32_t ring_size = s->kvm_dirty_ring_size;
>      uint32_t count = 0, fetch = cpu->kvm_fetch_index;
>  
> +    /*
> +     * It's possible that we race with vcpu creation code where the vcpu is
> +     * put onto the vcpus list but not yet initialized the dirty ring
> +     * structures.  If so, skip it.
> +     */
> +    if (!cpu->created) {
> +        return 0;
> +    }
> +
>      assert(dirty_gfns && ring_size);
>      trace_kvm_dirty_ring_reap_vcpu(cpu->cpu_index);
>  
> -- 
> 2.37.3
> 

-- 
Peter Xu

Re: [PATCH] kvm: dirty-ring: Fix race with vcpu creation

[Gavin Shan]

On 2/6/23 10:00 AM, Peter Xu wrote:
> On Tue, Sep 27, 2022 at 11:46:53AM -0400, Peter Xu wrote:
>> It's possible that we want to reap a dirty ring on a vcpu that is during
>> creation, because the vcpu is put onto list (CPU_FOREACH visible) before
>> initialization of the structures.  In this case:
>>
>> qemu_init_vcpu
>>      x86_cpu_realizefn
>>          cpu_exec_realizefn
>>              cpu_list_add      <---- can be probed by CPU_FOREACH
>>          qemu_init_vcpu
>>              cpus_accel->create_vcpu_thread(cpu);
>>                  kvm_init_vcpu
>>                      map kvm_dirty_gfns  <--- kvm_dirty_gfns valid
>>
>> Don't try to reap dirty ring on vcpus during creation or it'll crash.
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2124756
>> Reported-by: Xiaohui Li <xiaohli@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>   accel/kvm/kvm-all.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 5acab1767f..df5fabd3a8 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -757,6 +757,15 @@ static uint32_t kvm_dirty_ring_reap_one(KVMState *s, CPUState *cpu)
>>       uint32_t ring_size = s->kvm_dirty_ring_size;
>>       uint32_t count = 0, fetch = cpu->kvm_fetch_index;
>>   
>> +    /*
>> +     * It's possible that we race with vcpu creation code where the vcpu is
>> +     * put onto the vcpus list but not yet initialized the dirty ring
>> +     * structures.  If so, skip it.
>> +     */
>> +    if (!cpu->created) {
>> +        return 0;
>> +    }
>> +
>>       assert(dirty_gfns && ring_size);
>>       trace_kvm_dirty_ring_reap_vcpu(cpu->cpu_index);
>>   

Reviewed-by: Gavin Shan <gshan@redhat.com>