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);
}
--
1.8.3.1
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
© 2016 - 2024 Red Hat, Inc.