QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL
@ 2020-04-21 16:21 Peter Xu
  2020-04-21 16:21 ` [PATCH 1/8] memory: Introduce memory_region_transaction_{push|pop}() Peter Xu
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/8] memory: Introduce memory_region_transaction_{push|pop}()
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-04-21 16:21 ` [PATCH 2/8] memory: Don't do topology update in memory finalize() Peter Xu
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/8] memory: Don't do topology update in memory finalize()
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
  2020-04-21 16:21 ` [PATCH 1/8] memory: Introduce memory_region_transaction_{push|pop}() Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-04-21 16:21 ` [PATCH 3/8] cpus: Use qemu_cond_wait_iothread() where proper Peter Xu
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/8] cpus: Use qemu_cond_wait_iothread() where proper
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
  2020-04-21 16:21 ` [PATCH 1/8] memory: Introduce memory_region_transaction_{push|pop}() Peter Xu
  2020-04-21 16:21 ` [PATCH 2/8] memory: Don't do topology update in memory finalize() Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-04-21 16:21 ` [PATCH 4/8] cpus: Introduce qemu_cond_timedwait_iothread() Peter Xu
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 4/8] cpus: Introduce qemu_cond_timedwait_iothread()
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
                   ` (2 preceding siblings ...)
  2020-04-21 16:21 ` [PATCH 3/8] cpus: Use qemu_cond_wait_iothread() where proper Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-04-21 16:21 ` [PATCH 5/8] cpus: Remove the mutex parameter from do_run_on_cpu() Peter Xu
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 5/8] cpus: Remove the mutex parameter from do_run_on_cpu()
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
                   ` (3 preceding siblings ...)
  2020-04-21 16:21 ` [PATCH 4/8] cpus: Introduce qemu_cond_timedwait_iothread() Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-04-21 16:21 ` [PATCH 6/8] cpus: Introduce qemu_mutex_unlock_iothread_prepare() Peter Xu
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 6/8] cpus: Introduce qemu_mutex_unlock_iothread_prepare()
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
                   ` (4 preceding siblings ...)
  2020-04-21 16:21 ` [PATCH 5/8] cpus: Remove the mutex parameter from do_run_on_cpu() Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-04-21 16:21 ` [PATCH 7/8] memory: Assert on no ongoing memory transaction before release BQL Peter Xu
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 7/8] memory: Assert on no ongoing memory transaction before release BQL
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
                   ` (5 preceding siblings ...)
  2020-04-21 16:21 ` [PATCH 6/8] cpus: Introduce qemu_mutex_unlock_iothread_prepare() Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-04-21 16:21 ` [PATCH 8/8] memory: Delay the transaction pop() until commit completed Peter Xu
  2020-05-23 20:30 ` [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 8/8] memory: Delay the transaction pop() until commit completed
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
                   ` (6 preceding siblings ...)
  2020-04-21 16:21 ` [PATCH 7/8] memory: Assert on no ongoing memory transaction before release BQL Peter Xu
@ 2020-04-21 16:21 ` Peter Xu
  2020-05-23 20:30 ` [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-04-21 16:21 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, peterx, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL
  2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
                   ` (7 preceding siblings ...)
  2020-04-21 16:21 ` [PATCH 8/8] memory: Delay the transaction pop() until commit completed Peter Xu
@ 2020-05-23 20:30 ` Peter Xu
  8 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2020-05-23 20:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 16:21 [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu
2020-04-21 16:21 ` [PATCH 1/8] memory: Introduce memory_region_transaction_{push|pop}() Peter Xu
2020-04-21 16:21 ` [PATCH 2/8] memory: Don't do topology update in memory finalize() Peter Xu
2020-04-21 16:21 ` [PATCH 3/8] cpus: Use qemu_cond_wait_iothread() where proper Peter Xu
2020-04-21 16:21 ` [PATCH 4/8] cpus: Introduce qemu_cond_timedwait_iothread() Peter Xu
2020-04-21 16:21 ` [PATCH 5/8] cpus: Remove the mutex parameter from do_run_on_cpu() Peter Xu
2020-04-21 16:21 ` [PATCH 6/8] cpus: Introduce qemu_mutex_unlock_iothread_prepare() Peter Xu
2020-04-21 16:21 ` [PATCH 7/8] memory: Assert on no ongoing memory transaction before release BQL Peter Xu
2020-04-21 16:21 ` [PATCH 8/8] memory: Delay the transaction pop() until commit completed Peter Xu
2020-05-23 20:30 ` [PATCH 0/8] memory: Sanity checks memory transaction when releasing BQL Peter Xu

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git