* [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
@ 2018-01-20 7:54 linzhecheng
2018-01-25 9:59 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: linzhecheng @ 2018-01-20 7:54 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, crosthwaite.peter, rth, linzhecheng
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
2018-01-20 7:54 [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode linzhecheng
@ 2018-01-25 9:59 ` Paolo Bonzini
2018-01-28 4:14 ` CheneyLin
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2018-01-25 9:59 UTC (permalink / raw)
To: linzhecheng, qemu-devel; +Cc: rth, crosthwaite.peter
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
2018-01-25 9:59 ` Paolo Bonzini
@ 2018-01-28 4:14 ` CheneyLin
2018-01-30 23:02 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: CheneyLin @ 2018-01-28 4:14 UTC (permalink / raw)
To: paolo bonzini; +Cc: qemu-devel, crosthwaite.peter, rth
> -----原始邮件-----
> 发件人: "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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
2018-01-28 4:14 ` CheneyLin
@ 2018-01-30 23:02 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2018-01-30 23:02 UTC (permalink / raw)
To: CheneyLin; +Cc: qemu-devel, crosthwaite.peter, rth
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-01-30 23:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-20 7:54 [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode linzhecheng
2018-01-25 9:59 ` Paolo Bonzini
2018-01-28 4:14 ` CheneyLin
2018-01-30 23:02 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.