Message ID | 1516434868-24776-1-git-send-email-linzc@zju.edu.cn |
---|---|
State | New |
Headers | show |
Series | vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode | expand |
On 20/01/2018 08:54, linzhecheng wrote: > 1. If we create vcpu thread with QEMU_THREAD_JOINABLE mode, > we will get memory leak when vcpu thread exits, which will happen > when hot-unplug vcpus. > 2. We should use QLIST_FOREACH_SAFE instead of QLIST_FOREACH > if we need to remove the entry in QLIST. > > Signed-off-by: linzhecheng <linzc@zju.edu.cn> > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 071f4f5..fd95b18 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -282,9 +282,9 @@ err: > > static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) > { > - struct KVMParkedVcpu *cpu; > + struct KVMParkedVcpu *cpu, *next_cpu; > > - QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) { > + QLIST_FOREACH_SAFE(cpu, &s->kvm_parked_vcpus, node, next_cpu) { > if (cpu->vcpu_id == vcpu_id) { > int kvm_fd; > > diff --git a/cpus.c b/cpus.c > index 2cb0af9..de3a96b 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -1113,6 +1113,9 @@ static void qemu_kvm_destroy_vcpu(CPUState *cpu) > error_report("kvm_destroy_vcpu failed"); > exit(EXIT_FAILURE); > } > + g_free(cpu->thread); > + g_free(cpu->halt_cond); > + g_free(cpu->cpu_ases); > } > > static void qemu_tcg_destroy_vcpu(CPUState *cpu) > @@ -1205,6 +1208,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg) > cpu->created = false; > qemu_cond_signal(&qemu_cpu_cond); > qemu_mutex_unlock_iothread(); > + rcu_unregister_thread(); > return NULL; > } > > @@ -1850,7 +1854,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu) > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM", > cpu->cpu_index); > qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, > - cpu, QEMU_THREAD_JOINABLE); > + cpu, QEMU_THREAD_DETACHED); > while (!cpu->created) { > qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); > } > This is dangerous, it risks introducing use-after-free bugs in the vCPU thread. Can you instead add a qemu_thread_join call where the vCPU goes away (e.g. unrealize, I'm not sure)? Thanks, Paolo
> -----原始邮件----- > 发件人: "Paolo Bonzini" <pbonzini@redhat.com> > 发送时间: 2018-01-25 17:59:03 (星期四) > 收件人: linzhecheng <linzc@zju.edu.cn>, qemu-devel@nongnu.org > 抄送: crosthwaite.peter@gmail.com, rth@twiddle.net > 主题: Re: [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode > > On 20/01/2018 08:54, linzhecheng wrote: > > 1. If we create vcpu thread with QEMU_THREAD_JOINABLE mode, > > we will get memory leak when vcpu thread exits, which will happen > > when hot-unplug vcpus. > > 2. We should use QLIST_FOREACH_SAFE instead of QLIST_FOREACH > > if we need to remove the entry in QLIST. > > > > Signed-off-by: linzhecheng <linzc@zju.edu.cn> > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index 071f4f5..fd95b18 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -282,9 +282,9 @@ err: > > > > static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) > > { > > - struct KVMParkedVcpu *cpu; > > + struct KVMParkedVcpu *cpu, *next_cpu; > > > > - QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) { > > + QLIST_FOREACH_SAFE(cpu, &s->kvm_parked_vcpus, node, next_cpu) { > > if (cpu->vcpu_id == vcpu_id) { > > int kvm_fd; > > > > diff --git a/cpus.c b/cpus.c > > index 2cb0af9..de3a96b 100644 > > --- a/cpus.c > > +++ b/cpus.c > > @@ -1113,6 +1113,9 @@ static void qemu_kvm_destroy_vcpu(CPUState *cpu) > > error_report("kvm_destroy_vcpu failed"); > > exit(EXIT_FAILURE); > > } > > + g_free(cpu->thread); > > + g_free(cpu->halt_cond); > > + g_free(cpu->cpu_ases); > > } > > > > static void qemu_tcg_destroy_vcpu(CPUState *cpu) > > @@ -1205,6 +1208,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg) > > cpu->created = false; > > qemu_cond_signal(&qemu_cpu_cond); > > qemu_mutex_unlock_iothread(); > > + rcu_unregister_thread(); > > return NULL; > > } > > > > @@ -1850,7 +1854,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu) > > snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM", > > cpu->cpu_index); > > qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, > > - cpu, QEMU_THREAD_JOINABLE); > > + cpu, QEMU_THREAD_DETACHED); > > while (!cpu->created) { > > qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); > > } > > > > This is dangerous, it risks introducing use-after-free bugs in the vCPU > thread. Can you instead add a qemu_thread_join call where the vCPU goes > away (e.g. unrealize, I'm not sure)? > > Thanks, > > Paolo 1. If another thread calls qemu_thread_join, it will block until vcpu thread exit. 2. As vcpu exits, its resources should be freed ,which will not be used any more(e.g. user space stack), how can we get use-after-free bugs?
On 28/01/2018 05:14, CheneyLin wrote: >> This is dangerous, it risks introducing use-after-free bugs in the vCPU >> thread. Can you instead add a qemu_thread_join call where the vCPU goes >> away (e.g. unrealize, I'm not sure)? > > 1. If another thread calls qemu_thread_join, it will block until vcpu thread exit. Sure, but that's not a problem. If the code is written correctly, it will only block for a very short time. In particular, in this case we'll block anyway in cpu_remove_sync. The fix is just to change that function from qemu_cond_wait to qemu_thread_join. > 2. As vcpu exits, its resources should be freed ,which will not be used any more(e.g. user space stack), how can we get use-after-free bugs? Use-after-free bugs happen in the vCPU thread if the vCPU resources are freed just before the vCPU thread exits. Paolo
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 071f4f5..fd95b18 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -282,9 +282,9 @@ err: static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) { - struct KVMParkedVcpu *cpu; + struct KVMParkedVcpu *cpu, *next_cpu; - QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) { + QLIST_FOREACH_SAFE(cpu, &s->kvm_parked_vcpus, node, next_cpu) { if (cpu->vcpu_id == vcpu_id) { int kvm_fd; diff --git a/cpus.c b/cpus.c index 2cb0af9..de3a96b 100644 --- a/cpus.c +++ b/cpus.c @@ -1113,6 +1113,9 @@ static void qemu_kvm_destroy_vcpu(CPUState *cpu) error_report("kvm_destroy_vcpu failed"); exit(EXIT_FAILURE); } + g_free(cpu->thread); + g_free(cpu->halt_cond); + g_free(cpu->cpu_ases); } static void qemu_tcg_destroy_vcpu(CPUState *cpu) @@ -1205,6 +1208,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg) cpu->created = false; qemu_cond_signal(&qemu_cpu_cond); qemu_mutex_unlock_iothread(); + rcu_unregister_thread(); return NULL; } @@ -1850,7 +1854,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu) snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM", cpu->cpu_index); qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn, - cpu, QEMU_THREAD_JOINABLE); + cpu, QEMU_THREAD_DETACHED); while (!cpu->created) { qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); }
1. If we create vcpu thread with QEMU_THREAD_JOINABLE mode, we will get memory leak when vcpu thread exits, which will happen when hot-unplug vcpus. 2. We should use QLIST_FOREACH_SAFE instead of QLIST_FOREACH if we need to remove the entry in QLIST. Signed-off-by: linzhecheng <linzc@zju.edu.cn>