Skip to contentSkip to conversation

qemu_mutex_iothread_locked not correctly synchronized

PATCH In review David Engraf 3 participants 12 messages Nov 24, 2015 – Nov 26, 2015

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 qemu_mutex_iothread_locked not correctly synchronized

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function
qemu_mutex_iothread_locked to avoid recursive locking of the iothread lock. The iothread_locked variable uses the __thread attribute which results in a per thread storage location whereas the qemu_global_mutex is not thread specific. This leads to race conditions because the variable is not in sync between threads. I triggered this problem reproducible on a Windows machine whereas Linux works fine.

After removing the __thread attribute, iothread_locked may still return an invalid state because some functions directly use qemu_cond_wait which will unlock and lock the qemu_global_mutex based on a condition. These calls must be synced with iothread_locked as well.

The patch removes the __thread flag from iothread_locked and adds a function called qemu_cond_wait_iothread to keep iothread_locked in sync.

---
 cpus.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)
cpus.c +18 -9
index 877bd70..d7cdd11 100644
@@ -70,6 +70,8 @@ static CPUState *next_cpu;
 int64_t max_delay;
 int64_t max_advance;
 
+static void qemu_cond_wait_iothread(QemuCond *cond);
+
 /* vcpu throttling controls */
 static QEMUTimer *throttle_timer;
 static unsigned int throttle_percentage;
@@ -920,7 +922,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     while (!atomic_mb_read(&wi.done)) {
         CPUState *self_cpu = current_cpu;
 
-        qemu_cond_wait(&qemu_work_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_work_cond);
         current_cpu = self_cpu;
     }
 }
@@ -998,11 +1000,11 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
        /* Start accounting real time to the virtual clock if the CPUs
           are idle.  */
         qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
-        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(cpu->halt_cond);
     }
 
     while (iothread_requesting_mutex) {
-        qemu_cond_wait(&qemu_io_proceeded_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_io_proceeded_cond);
     }
 
     CPU_FOREACH(cpu) {
@@ -1013,7 +1015,7 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
 static void qemu_kvm_wait_io_event(CPUState *cpu)
 {
     while (cpu_thread_is_idle(cpu)) {
-        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(cpu->halt_cond);
     }
 
     qemu_kvm_eat_signals(cpu);
@@ -1123,7 +1125,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     /* wait for initial kick-off after machine start */
     while (first_cpu->stopped) {
-        qemu_cond_wait(first_cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(first_cpu->halt_cond);
 
         /* process any pending work */
         CPU_FOREACH(cpu) {
@@ -1208,13 +1210,20 @@ bool qemu_in_vcpu_thread(void)
     return current_cpu && qemu_cpu_is_self(current_cpu);
 }
 
-static __thread bool iothread_locked = false;
+static bool iothread_locked;
 
 bool qemu_mutex_iothread_locked(void)
 {
     return iothread_locked;
 }
 
+static void qemu_cond_wait_iothread(QemuCond *cond)
+{
+    iothread_locked = false;
+    qemu_cond_wait(cond, &qemu_global_mutex);
+    iothread_locked = true;
+}
+
 void qemu_mutex_lock_iothread(void)
 {
     atomic_inc(&iothread_requesting_mutex);
@@ -1277,7 +1286,7 @@ void pause_all_vcpus(void)
     }
 
     while (!all_vcpus_paused()) {
-        qemu_cond_wait(&qemu_pause_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_pause_cond);
         CPU_FOREACH(cpu) {
             qemu_cpu_kick(cpu);
         }
@@ -1326,7 +1335,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
         cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif
         while (!cpu->created) {
-            qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+            qemu_cond_wait_iothread(&qemu_cpu_cond);
         }
         tcg_cpu_thread = cpu->thread;
     } else {
@@ -1347,7 +1356,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
     qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
                        cpu, QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
-        qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_cpu_cond);
     }
 }
 
@@ -1363,7 +1372,7 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
     qemu_thread_create(cpu->thread, thread_name, 

qemu_dummy_cpu_thread_fn, cpu,

                        QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
-        qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_cpu_cond);
     }

}

Signature · 1 line
1.9.1
#1 lore raw
Quoted 6 lines · David Engraf

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function qemu_mutex_iothread_locked to avoid recursive locking of the iothread lock. The iothread_locked variable uses the __thread attribute which results in a per thread storage location whereas the qemu_global_mutex is not thread specific. This leads to race conditions because the variable is not in sync between threads.

Frankly, this makes no sense. You're modifying the qemu_mutex_iothread_locked function to return whether _some_ thread has taken the mutex, but the function tells you whether _the calling_ thread has taken the mutex (that's the point about recursive locking). This breaks the callers of qemu_mutex_iothread_locked completely.

The variable need not be in sync between the different threads; each thread only needs to know about itself. The current code works because:

1) the iothread mutex is not locked before qemu_mutex_lock_iothread

2) the iothread mutex is not locked after qemu_mutex_lock_iothread

3) qemu_cond_wait doesn't matter because the thread does not run during a qemu_cond_wait.

Quoted 1 line

I triggered this problem reproducible on a Windows machine whereas Linux works fine.

What are the symptoms, and what is your Windows compiler?

Paolo

#2 lore raw

Hi Paolo,

Am 24.11.2015 um 16:54 schrieb Paolo Bonzini:

Quoted 17 lines · David Engraf
Quoted 6 lines

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function qemu_mutex_iothread_locked to avoid recursive locking of the iothread lock. The iothread_locked variable uses the __thread attribute which results in a per thread storage location whereas the qemu_global_mutex is not thread specific. This leads to race conditions because the variable is not in sync between threads.

Frankly, this makes no sense. You're modifying the qemu_mutex_iothread_locked function to return whether _some_ thread has taken the mutex, but the function tells you whether _the calling_ thread has taken the mutex (that's the point about recursive locking). This breaks the callers of qemu_mutex_iothread_locked completely.

The variable need not be in sync between the different threads; each thread only needs to know about itself. The current code works because:

Quoted 6 lines

1) the iothread mutex is not locked before qemu_mutex_lock_iothread

2) the iothread mutex is not locked after qemu_mutex_lock_iothread

3) qemu_cond_wait doesn't matter because the thread does not run during a qemu_cond_wait.

But this is my issue on Windows:

qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event
-> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec
-> cpu_exec ends up in calling prepare_mmio_access
prepare_mmio_access uses qemu_mutex_iothread_locked to check if the lock is already held for this thread, but it returns 0 because qemu_cond_wait doesn't set iothread_locked but the mutex is locked. Thus the function calls qemu_mutex_lock_iothread which tries to acquire the mutex again. On Windows this results in an assertion:

Assertion failed: mutex->owner == 0, file util/qemu-thread-win32.c, line 60

I'm using a self compiled, cross gcc-5.2.0 mingw compiler.

David

#3 lore raw

Hi Paolo,

please check the new version. I removed changing the iothread_locked variable. But I still need to set the correct value of iothread_locked when using qemu_cond_wait. This will fix my race condition on Windows when prepare_mmio_access is called and checks if the lock is already held by the current thread.

David

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function qemu_mutex_iothread_locked to avoid recursive locking of the iothread lock. But the function qemu_cond_wait directly acquires the lock without modifying iothread_locked. This leads to condition where qemu_tcg_wait_io_event acquires the mutex by using qemu_cond_wait and later calls tcg_exec_all > tcg_cpu_exec -> cpu_exec > prepare_mmio_access checking if the current thread already holds the lock. This is done by checking the variable iothread_locked which is still 0, thus the function tries to acquire the mutex again.

The patch adds a function called qemu_cond_wait_iothread to keep iothread_locked in sync.

---
 cpus.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)
cpus.c +17 -8
index 877bd70..e4d2d4b 100644
@@ -70,6 +70,8 @@ static CPUState *next_cpu;
 int64_t max_delay;
 int64_t max_advance;
 
+static void qemu_cond_wait_iothread(QemuCond *cond);
+
 /* vcpu throttling controls */
 static QEMUTimer *throttle_timer;
 static unsigned int throttle_percentage;
@@ -920,7 +922,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     while (!atomic_mb_read(&wi.done)) {
         CPUState *self_cpu = current_cpu;
 
-        qemu_cond_wait(&qemu_work_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_work_cond);
         current_cpu = self_cpu;
     }
 }
@@ -998,11 +1000,11 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
        /* Start accounting real time to the virtual clock if the CPUs
           are idle.  */
         qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
-        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(cpu->halt_cond);
     }
 
     while (iothread_requesting_mutex) {
-        qemu_cond_wait(&qemu_io_proceeded_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_io_proceeded_cond);
     }
 
     CPU_FOREACH(cpu) {
@@ -1013,7 +1015,7 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)
 static void qemu_kvm_wait_io_event(CPUState *cpu)
 {
     while (cpu_thread_is_idle(cpu)) {
-        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(cpu->halt_cond);
     }
 
     qemu_kvm_eat_signals(cpu);
@@ -1123,7 +1125,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     /* wait for initial kick-off after machine start */
     while (first_cpu->stopped) {
-        qemu_cond_wait(first_cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(first_cpu->halt_cond);
 
         /* process any pending work */
         CPU_FOREACH(cpu) {
@@ -1215,6 +1217,13 @@ bool qemu_mutex_iothread_locked(void)
     return iothread_locked;
 }
 
+static void qemu_cond_wait_iothread(QemuCond *cond)
+{
+    iothread_locked = false;
+    qemu_cond_wait(cond, &qemu_global_mutex);
+    iothread_locked = true;
+}
+
 void qemu_mutex_lock_iothread(void)
 {
     atomic_inc(&iothread_requesting_mutex);
@@ -1277,7 +1286,7 @@ void pause_all_vcpus(void)
     }
 
     while (!all_vcpus_paused()) {
-        qemu_cond_wait(&qemu_pause_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_pause_cond);
         CPU_FOREACH(cpu) {
             qemu_cpu_kick(cpu);
         }
@@ -1326,7 +1335,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
         cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif
         while (!cpu->created) {
-            qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+            qemu_cond_wait_iothread(&qemu_cpu_cond);
         }
         tcg_cpu_thread = cpu->thread;
     } else {
@@ -1347,7 +1356,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
     qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
                        cpu, QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
-        qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_cpu_cond);
     }
 }
 
@@ -1363,7 +1372,7 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
     qemu_thread_create(cpu->thread, thread_name, 

qemu_dummy_cpu_thread_fn, cpu,

                        QEMU_THREAD_JOINABLE);
     while (!cpu->created) {
-        qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_cpu_cond);
     }

}

Signature · 5 lines
1.9.1



Am 25.11.2015 um 10:08 schrieb David Engraf:
Quoted 48 lines

Hi Paolo,

Am 24.11.2015 um 16:54 schrieb Paolo Bonzini:

Quoted 17 lines · David Engraf
Quoted 6 lines

Commit afbe70535ff1a8a7a32910cc15ebecc0ba92e7da introduced the function qemu_mutex_iothread_locked to avoid recursive locking of the iothread lock. The iothread_locked variable uses the __thread attribute which results in a per thread storage location whereas the qemu_global_mutex is not thread specific. This leads to race conditions because the variable is not in sync between threads.

Frankly, this makes no sense. You're modifying the qemu_mutex_iothread_locked function to return whether _some_ thread has taken the mutex, but the function tells you whether _the calling_ thread has taken the mutex (that's the point about recursive locking). This breaks the callers of qemu_mutex_iothread_locked completely.

The variable need not be in sync between the different threads; each thread only needs to know about itself. The current code works because:

Quoted 6 lines

1) the iothread mutex is not locked before qemu_mutex_lock_iothread

2) the iothread mutex is not locked after qemu_mutex_lock_iothread

3) qemu_cond_wait doesn't matter because the thread does not run during a qemu_cond_wait.

But this is my issue on Windows:

qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event

    -> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec
-> cpu_exec ends up in calling prepare_mmio_access
prepare_mmio_access uses qemu_mutex_iothread_locked to check if the lock is already held for this thread, but it returns 0 because qemu_cond_wait doesn't set iothread_locked but the mutex is locked. Thus the function calls qemu_mutex_lock_iothread which tries to acquire the mutex again. On Windows this results in an assertion:

Assertion failed: mutex->owner == 0, file util/qemu-thread-win32.c, line 60

I'm using a self compiled, cross gcc-5.2.0 mingw compiler.

David

Signature · 16 lines
Mit freundlichen Grüßen/Best regards,

David Engraf
Product Engineer

SYSGO AG
Office Mainz
Am Pfaffenstein 14 / D-55270 Klein-Winternheim / Germany
Phone: +49-6136-9948-0 / Fax: +49-6136-9948-10
VoIP: SIP:[email protected]
E-mail: [email protected] / Web: http://www.sysgo.com

Handelsregister/Commercial Registry: HRB Mainz 90 HRB 8066
Vorstand/Executive Board: Knut Degen (CEO), Kai Sablotny (COO)
Aufsichtsratsvorsitzender/Supervisory Board Chairman: Pascal Bouchiat
USt-Id-Nr./VAT-Id-No.: DE 149062328
#4 lore raw
Quoted 5 lines · David Engraf

Hi Paolo,

please check the new version. I removed changing the iothread_locked variable. But I still need to set the correct value of iothread_locked when using qemu_cond_wait.

No, you don't. Who is reading iothread_locked during qemu_cond_wait_iothread? No one, because it is a thread-local variable whose address is never taken.

Paolo

Quoted 6 lines
cpus.c +6 -0
+static void qemu_cond_wait_iothread(QemuCond *cond)
+{
+    iothread_locked = false;
+    qemu_cond_wait(cond, &qemu_global_mutex);
+    iothread_locked = true;
+}
#5 lore raw

Am 25.11.2015 um 14:26 schrieb Paolo Bonzini:

Quoted 12 lines · David Engraf
Quoted 5 lines

Hi Paolo,

please check the new version. I removed changing the iothread_locked variable. But I still need to set the correct value of iothread_locked when using qemu_cond_wait.

No, you don't. Who is reading iothread_locked during qemu_cond_wait_iothread? No one, because it is a thread-local variable whose address is never taken.

prepare_mmio_access is reading iothread_locked by using qemu_mutex_iothread_locked after qemu_tcg_wait_io_event calls qemu_cond_wait. All one the same thread.

qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event
-> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec -> cpu_exec ends up in calling prepare_mmio_access

David

#6 lore raw
Quoted 9 lines · David Engraf
Quoted 5 lines
Quoted 1 line

No, you don't. Who is reading iothread_locked during qemu_cond_wait_iothread? No one, because it is a thread-local variable whose address is never taken.

prepare_mmio_access is reading iothread_locked by using qemu_mutex_iothread_locked after qemu_tcg_wait_io_event calls qemu_cond_wait. All one the same thread.

Sure, but who has set iothread_locked to false during the execution of qemu_cond_wait? No one, because it's a thread-local variable. If it's true before qemu_cond_wait, it will be true after qemu_cond_wait and you don't need qemu_cond_wait_iothread... unless your compiler is broken and doesn't generate TLS properly.

Can you compile cpus.c with -S and attach it?

Paolo

Quoted 7 lines

qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event
-> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec -> cpu_exec ends up in calling prepare_mmio_access

#7 lore raw

Am 25.11.2015 um 15:36 schrieb Paolo Bonzini:

Quoted 13 lines · David Engraf
Quoted 9 lines
Quoted 5 lines
Quoted 1 line

No, you don't. Who is reading iothread_locked during qemu_cond_wait_iothread? No one, because it is a thread-local variable whose address is never taken.

prepare_mmio_access is reading iothread_locked by using qemu_mutex_iothread_locked after qemu_tcg_wait_io_event calls qemu_cond_wait. All one the same thread.

Quoted 5 lines

Sure, but who has set iothread_locked to false during the execution of qemu_cond_wait? No one, because it's a thread-local variable. If it's true before qemu_cond_wait, it will be true after qemu_cond_wait and you don't need qemu_cond_wait_iothread... unless your compiler is broken and doesn't generate TLS properly.

Indeed, TLS handling is broken. The address of iothread_locked is always the same between threads and I can see that a different thread sets iothread_locked to false, thus my current thread uses an invalid state. I will have to check why my compiler produces invalid TLS code.

David

Quoted 11 lines

Can you compile cpus.c with -S and attach it?

Paolo

Quoted 7 lines

qemu_tcg_cpu_thread_fn
-> qemu_tcg_wait_io_event

    -> qemu_cond_wait acquires the mutex

qemu_tcg_cpu_thread_fn
-> tcg_exec_all -> tcg_cpu_exec -> cpu_exec -> cpu_exec ends up in calling prepare_mmio_access

#8 lore raw
Quoted 5 lines · David Engraf

Indeed, TLS handling is broken. The address of iothread_locked is always the same between threads and I can see that a different thread sets iothread_locked to false, thus my current thread uses an invalid state. I will have to check why my compiler produces invalid TLS code.

That rings a bell, I think there are different CRT libraries or something like that. Stefan, what would break TLS under Windows?

Paolo

#9 lore raw

Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:

Quoted 11 lines · David Engraf
Quoted 5 lines

Indeed, TLS handling is broken. The address of iothread_locked is always the same between threads and I can see that a different thread sets iothread_locked to false, thus my current thread uses an invalid state. I will have to check why my compiler produces invalid TLS code.

That rings a bell, I think there are different CRT libraries or something like that. Stefan, what would break TLS under Windows?

"--extra-cflags=-mthreads" is the solution. iothread_locked is unique for each thread now. I saw that this is already mentioned in the Documentation [1], but why isn't that enabled by default if QEMU requires this option on Windows? Without this option, even the latest version of gcc doesn't produce working TLS code.

Many thanks for your support!

David

[1] http://wiki.qemu.org/Hosts/W32

#10 lore raw

Am 26.11.2015 um 10:12 schrieb David Engraf:

Quoted 20 lines

Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:

Quoted 12 lines · David Engraf
Quoted 6 lines

Indeed, TLS handling is broken. The address of iothread_locked is always
the same between threads and I can see that a different thread sets iothread_locked to false, thus my current thread uses an invalid state. I will have to check why my compiler produces invalid TLS code.

That rings a bell, I think there are different CRT libraries or something like that. Stefan, what would break TLS under Windows?

"--extra-cflags=-mthreads" is the solution. iothread_locked is unique for each thread now. I saw that this is already mentioned in the Documentation [1], but why isn't that enabled by default if QEMU requires this option on Windows? Without this option, even the latest version of gcc doesn't produce working TLS code.

Quoted 5 lines

Many thanks for your support!

David

[1] http://wiki.qemu.org/Hosts/W32

Hi David,

I just prepared a patch which will enable that option by default for all compilations targeting Windows.

Thanks for your report!

Regards,
Stefan

#11 lore raw

Am 26.11.2015 um 12:25 schrieb Stefan Weil:

Quoted 25 lines

Am 26.11.2015 um 10:12 schrieb David Engraf:

Quoted 24 lines

Am 25.11.2015 um 17:16 schrieb Paolo Bonzini:

Quoted 12 lines · David Engraf
Quoted 6 lines

Indeed, TLS handling is broken. The address of iothread_locked is always
the same between threads and I can see that a different thread sets iothread_locked to false, thus my current thread uses an invalid state. I will have to check why my compiler produces invalid TLS code.

That rings a bell, I think there are different CRT libraries or something like that. Stefan, what would break TLS under Windows?

"--extra-cflags=-mthreads" is the solution. iothread_locked is unique for each thread now. I saw that this is already mentioned in the Documentation [1], but why isn't that enabled by default if QEMU requires this option on Windows? Without this option, even the latest version of gcc doesn't produce working TLS code.

Many thanks for your support!

David

Quoted 8 lines
Quoted 1 line

[1] http://wiki.qemu.org/Hosts/W32

Hi David,

I just prepared a patch which will enable that option by default for all compilations targeting Windows.

Thanks for your report!

Thank you.

David