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>
---
 softmmu/memory.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 1a3e9ff8ad..dfce4a2bda 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -170,6 +170,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)
 {
@@ -1756,12 +1762,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.31.1

On 23.07.21 21:34, Peter Xu wrote:
> 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>
> ---
>   softmmu/memory.c | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 1a3e9ff8ad..dfce4a2bda 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -170,6 +170,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)
>   {
> @@ -1756,12 +1762,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).
> +     */

Hmm, I wonder if we can just keep the begin/end semantics and just do an 
assertion before doing the commit? Does anything speak against that?

-- 
Thanks,

David / dhildenb