[Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode

linzhecheng posted 1 patch 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1516434868-24776-1-git-send-email-linzc@zju.edu.cn
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppc passed
Test s390x passed
[Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
Posted by linzhecheng 6 years, 2 months ago
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


Re: [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
Posted by Paolo Bonzini 6 years, 2 months ago
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

Re: [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
Posted by CheneyLin 6 years, 1 month ago


> -----原始邮件-----
> 发件人: "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?
Re: [Qemu-devel] [PATCH] vcpu: create vcpu thread with QEMU_THREAD_DETACHED mode
Posted by Paolo Bonzini 6 years, 1 month ago
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