[PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL

[PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL

[Peter Xu]

This is a continuous work of previous discussion on memory transactions [1].
It should be helpful to fail QEMU far earlier if there's misuse of BQL against
the QEMU memory model.

One example is run_on_cpu() during memory commit.  That'll work previously, but
it'll fail with very strange errors (like KVM ioctl failure due to memslot
already existed, and it's not guaranteed to trigger constantly).  Now it'll
directly fail when run_on_cpu() is called.

Please have a look, thanks.

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03205.html

Peter Xu (8):
  memory: Introduce memory_region_transaction_{push|pop}()
  memory: Don't do topology update in memory finalize()
  cpus: Use qemu_cond_wait_iothread() where proper
  cpus: Introduce qemu_cond_timedwait_iothread()
  cpus: Remove the mutex parameter from do_run_on_cpu()
  cpus: Introduce qemu_mutex_unlock_iothread_prepare()
  memory: Assert on no ongoing memory transaction before release BQL
  memory: Delay the transaction pop() until commit completed

 cpus-common.c                  |  5 ++--
 cpus.c                         | 32 ++++++++++++++------
 include/exec/memory-internal.h |  1 +
 include/hw/core/cpu.h          |  4 +--
 include/qemu/main-loop.h       |  7 +++++
 memory.c                       | 53 ++++++++++++++++++++++++++++++----
 6 files changed, 81 insertions(+), 21 deletions(-)

-- 
2.24.1

[PATCH 1/8] memory: Introduce memory_region_transaction_{push|pop}()

[Peter Xu]

memory_region_transaction_{begin|commit}() could be too big when finalizing a
memory region.  E.g., we should never attempt to update address space topology
during the finalize() of a memory region.  Provide helpers for further use.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 601b749906..e5d634d648 100644
--- a/memory.c
+++ b/memory.c
@@ -1054,10 +1054,20 @@ static void address_space_update_topology(AddressSpace *as)
     address_space_set_flatview(as);
 }
 
+static void memory_region_transaction_push(void)
+{
+    memory_region_transaction_depth++;
+}
+
+static void memory_region_transaction_pop(void)
+{
+    memory_region_transaction_depth--;
+}
+
 void memory_region_transaction_begin(void)
 {
     qemu_flush_coalesced_mmio_buffer();
-    ++memory_region_transaction_depth;
+    memory_region_transaction_push();
 }
 
 void memory_region_transaction_commit(void)
@@ -1067,7 +1077,7 @@ void memory_region_transaction_commit(void)
     assert(memory_region_transaction_depth);
     assert(qemu_mutex_iothread_locked());
 
-    --memory_region_transaction_depth;
+    memory_region_transaction_pop();
     if (!memory_region_transaction_depth) {
         if (memory_region_update_pending) {
             flatviews_reset();
-- 
2.24.1

[PATCH 2/8] memory: Don't do topology update in memory finalize()

[Peter Xu]

Topology update could be wrongly triggered in memory region finalize() if
there's bug somewhere else.  It'll be a very confusing stack when it
happens (e.g., sending KVM ioctl within the RCU thread, and we'll observe it
only until it fails!).

Instead of that, we use the push()/pop() helper to avoid memory transaction
commit, at the same time we use assertions to make sure there's no pending
updates or it's a nested transaction, so it could fail even earlier and in a
more explicit way.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index e5d634d648..fea427f43f 100644
--- a/memory.c
+++ b/memory.c
@@ -171,6 +171,12 @@ struct MemoryRegionIoeventfd {
     EventNotifier *e;
 };
 
+/* Returns whether there's any pending memory updates */
+static bool memory_region_has_pending_update(void)
+{
+    return memory_region_update_pending || ioeventfd_update_pending;
+}
+
 static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
                                            MemoryRegionIoeventfd *b)
 {
@@ -1730,12 +1736,25 @@ static void memory_region_finalize(Object *obj)
      * and cause an infinite loop.
      */
     mr->enabled = false;
-    memory_region_transaction_begin();
+
+    /*
+     * Use push()/pop() instead of begin()/commit() to make sure below block
+     * won't trigger any topology update (which should never happen, but it's
+     * still a safety belt).
+     */
+    memory_region_transaction_push();
     while (!QTAILQ_EMPTY(&mr->subregions)) {
         MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
         memory_region_del_subregion(mr, subregion);
     }
-    memory_region_transaction_commit();
+    memory_region_transaction_pop();
+
+    /*
+     * Make sure we're either in a nested transaction or there must have no
+     * pending updates due to memory_region_del_subregion() above.
+     */
+    assert(memory_region_transaction_depth ||
+           !memory_region_has_pending_update());
 
     mr->destructor(mr);
     memory_region_clear_coalescing(mr);
-- 
2.24.1

[PATCH 3/8] cpus: Use qemu_cond_wait_iothread() where proper

[Peter Xu]

The helper is introduced but we've still got plenty of places that are directly
referencing the qemu_global_mutex itself.  Spread the usage.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index ef441bdf62..00f6e361af 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1181,7 +1181,7 @@ static void qemu_tcg_rr_wait_io_event(void)
 
     while (all_cpu_threads_idle()) {
         stop_tcg_kick_timer();
-        qemu_cond_wait(first_cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(first_cpu->halt_cond);
     }
 
     start_tcg_kick_timer();
@@ -1200,7 +1200,7 @@ static void qemu_wait_io_event(CPUState *cpu)
             slept = true;
             qemu_plugin_vcpu_idle_cb(cpu);
         }
-        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(cpu->halt_cond);
     }
     if (slept) {
         qemu_plugin_vcpu_resume_cb(cpu);
@@ -1456,7 +1456,7 @@ static void *qemu_tcg_rr_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) {
@@ -1657,7 +1657,7 @@ static void *qemu_whpx_cpu_thread_fn(void *arg)
             }
         }
         while (cpu_thread_is_idle(cpu)) {
-            qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+            qemu_cond_wait_iothread(cpu->halt_cond);
         }
         qemu_wait_io_event_common(cpu);
     } while (!cpu->unplug || cpu_can_run(cpu));
@@ -1877,7 +1877,7 @@ void pause_all_vcpus(void)
     replay_mutex_unlock();
 
     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);
         }
@@ -2087,7 +2087,7 @@ void qemu_init_vcpu(CPUState *cpu)
     }
 
     while (!cpu->created) {
-        qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+        qemu_cond_wait_iothread(&qemu_cpu_cond);
     }
 }
 
-- 
2.24.1

[PATCH 4/8] cpus: Introduce qemu_cond_timedwait_iothread()

[Peter Xu]

This is the sister function of qemu_cond_wait_iothread().  Use it at the only
place where we do cond_timedwait() on BQL.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus.c                   | 9 +++++++--
 include/qemu/main-loop.h | 7 +++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 00f6e361af..263eddc8b0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -726,8 +726,8 @@ static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
     endtime_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + sleeptime_ns;
     while (sleeptime_ns > 0 && !cpu->stop) {
         if (sleeptime_ns > SCALE_MS) {
-            qemu_cond_timedwait(cpu->halt_cond, &qemu_global_mutex,
-                                sleeptime_ns / SCALE_MS);
+            qemu_cond_timedwait_iothread(cpu->halt_cond,
+                                         sleeptime_ns / SCALE_MS);
         } else {
             qemu_mutex_unlock_iothread();
             g_usleep(sleeptime_ns / SCALE_US);
@@ -1844,6 +1844,11 @@ void qemu_cond_wait_iothread(QemuCond *cond)
     qemu_cond_wait(cond, &qemu_global_mutex);
 }
 
+void qemu_cond_timedwait_iothread(QemuCond *cond, int ms)
+{
+    qemu_cond_timedwait(cond, &qemu_global_mutex, ms);
+}
+
 static bool all_vcpus_paused(void)
 {
     CPUState *cpu;
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index a6d20b0719..a0e59c5563 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -303,6 +303,13 @@ void qemu_mutex_unlock_iothread(void);
  */
 void qemu_cond_wait_iothread(QemuCond *cond);
 
+/*
+ * qemu_cond_timedwait_iothread: Wait on condition for the main loop mutex
+ *
+ * This is the same as qemu_cond_wait_iothread() but allows a timeout.
+ */
+void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
+
 /* internal interfaces */
 
 void qemu_fd_register(int fd);
-- 
2.24.1

[PATCH 5/8] cpus: Remove the mutex parameter from do_run_on_cpu()

[Peter Xu]

We must use the BQL for do_run_on_cpu() without much choice, it means the
parameter is helpless.  Remove it.  Meanwhile use the newly introduced
qemu_cond_wait_iothread() in do_run_on_cpu().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus-common.c         | 5 ++---
 cpus.c                | 2 +-
 include/hw/core/cpu.h | 4 +---
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/cpus-common.c b/cpus-common.c
index eaf590cb38..4fc11b4e1c 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -121,8 +121,7 @@ static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
     qemu_cpu_kick(cpu);
 }
 
-void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
-                   QemuMutex *mutex)
+void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
 {
     struct qemu_work_item wi;
 
@@ -141,7 +140,7 @@ void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
     while (!atomic_mb_read(&wi.done)) {
         CPUState *self_cpu = current_cpu;
 
-        qemu_cond_wait(&qemu_work_cond, mutex);
+        qemu_cond_wait_iothread(&qemu_work_cond);
         current_cpu = self_cpu;
     }
 }
diff --git a/cpus.c b/cpus.c
index 263eddc8b0..5cbc3f30de 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1140,7 +1140,7 @@ void qemu_init_cpu_loop(void)
 
 void run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data)
 {
-    do_run_on_cpu(cpu, func, data, &qemu_global_mutex);
+    do_run_on_cpu(cpu, func, data);
 }
 
 static void qemu_kvm_destroy_vcpu(CPUState *cpu)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5bf94d28cf..b26fdb5ab8 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -734,12 +734,10 @@ bool cpu_is_stopped(CPUState *cpu);
  * @cpu: The vCPU to run on.
  * @func: The function to be executed.
  * @data: Data to pass to the function.
- * @mutex: Mutex to release while waiting for @func to run.
  *
  * Used internally in the implementation of run_on_cpu.
  */
-void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
-                   QemuMutex *mutex);
+void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data);
 
 /**
  * run_on_cpu:
-- 
2.24.1

[PATCH 6/8] cpus: Introduce qemu_mutex_unlock_iothread_prepare()

[Peter Xu]

The prepare function before unlocking BQL.  There're only three places that can
release the BQL: unlock(), cond_wait() or cond_timedwait().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/cpus.c b/cpus.c
index 5cbc3f30de..48aa295fea 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1832,20 +1832,27 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line)
     iothread_locked = true;
 }
 
+static void qemu_mutex_unlock_iothread_prepare(void)
+{
+}
+
 void qemu_mutex_unlock_iothread(void)
 {
     g_assert(qemu_mutex_iothread_locked());
     iothread_locked = false;
+    qemu_mutex_unlock_iothread_prepare();
     qemu_mutex_unlock(&qemu_global_mutex);
 }
 
 void qemu_cond_wait_iothread(QemuCond *cond)
 {
+    qemu_mutex_unlock_iothread_prepare();
     qemu_cond_wait(cond, &qemu_global_mutex);
 }
 
 void qemu_cond_timedwait_iothread(QemuCond *cond, int ms)
 {
+    qemu_mutex_unlock_iothread_prepare();
     qemu_cond_timedwait(cond, &qemu_global_mutex, ms);
 }
 
-- 
2.24.1

[PATCH 7/8] memory: Assert on no ongoing memory transaction before release BQL

[Peter Xu]

Make sure we don't have any more ongoing memory transaction when releasing the
BQL.  This will trigger an abort if we misuse the QEMU memory model, e.g., when
calling run_on_cpu() during a memory commit.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 cpus.c                         | 2 ++
 include/exec/memory-internal.h | 1 +
 memory.c                       | 6 ++++++
 3 files changed, 9 insertions(+)

diff --git a/cpus.c b/cpus.c
index 48aa295fea..8c82556ff4 100644
--- a/cpus.c
+++ b/cpus.c
@@ -43,6 +43,7 @@
 #include "sysemu/hvf.h"
 #include "sysemu/whpx.h"
 #include "exec/exec-all.h"
+#include "exec/memory-internal.h"
 
 #include "qemu/thread.h"
 #include "qemu/plugin.h"
@@ -1834,6 +1835,7 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line)
 
 static void qemu_mutex_unlock_iothread_prepare(void)
 {
+    assert(!memory_region_has_pending_transaction());
 }
 
 void qemu_mutex_unlock_iothread(void)
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 9fcc2af25c..3124b91c4b 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -35,6 +35,7 @@ static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
 
 FlatView *address_space_get_flatview(AddressSpace *as);
 void flatview_unref(FlatView *view);
+bool memory_region_has_pending_transaction(void);
 
 extern const MemoryRegionOps unassigned_mem_ops;
 
diff --git a/memory.c b/memory.c
index fea427f43f..2f8dc9721f 100644
--- a/memory.c
+++ b/memory.c
@@ -177,6 +177,12 @@ static bool memory_region_has_pending_update(void)
     return memory_region_update_pending || ioeventfd_update_pending;
 }
 
+bool memory_region_has_pending_transaction(void)
+{
+    return memory_region_transaction_depth ||
+        memory_region_has_pending_update();
+}
+
 static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
                                            MemoryRegionIoeventfd *b)
 {
-- 
2.24.1

[PATCH 8/8] memory: Delay the transaction pop() until commit completed

[Peter Xu]

This should be functionally the same as before, but this allows the
memory_region_transaction_depth to be non-zero during commit, which can help us
to do sanity check on misuses.

Since at it, fix an indentation issue on the bracket.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/memory.c b/memory.c
index 2f8dc9721f..357f7276ee 100644
--- a/memory.c
+++ b/memory.c
@@ -1089,8 +1089,7 @@ void memory_region_transaction_commit(void)
     assert(memory_region_transaction_depth);
     assert(qemu_mutex_iothread_locked());
 
-    memory_region_transaction_pop();
-    if (!memory_region_transaction_depth) {
+    if (memory_region_transaction_depth == 1) {
         if (memory_region_update_pending) {
             flatviews_reset();
 
@@ -1109,7 +1108,14 @@ void memory_region_transaction_commit(void)
             }
             ioeventfd_update_pending = false;
         }
-   }
+    }
+
+    /*
+     * Pop the depth at last, so that memory_region_transaction_depth will
+     * still be non-zero during committing.  This can help us to do some sanity
+     * check within the process of committing.
+     */
+    memory_region_transaction_pop();
 }
 
 static void memory_region_destructor_none(MemoryRegion *mr)
-- 
2.24.1

Re: [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL

[Peter Xu]

On Tue, Apr 21, 2020 at 12:21:00PM -0400, Peter Xu wrote:
> This is a continuous work of previous discussion on memory transactions [1].
> It should be helpful to fail QEMU far earlier if there's misuse of BQL against
> the QEMU memory model.
> 
> One example is run_on_cpu() during memory commit.  That'll work previously, but
> it'll fail with very strange errors (like KVM ioctl failure due to memslot
> already existed, and it's not guaranteed to trigger constantly).  Now it'll
> directly fail when run_on_cpu() is called.
> 
> Please have a look, thanks.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03205.html

Still think it as something good to have...  Any comments?  Thanks,

-- 
Peter Xu