Re: [Qemu-devel] [PATCH v2 03/17] memory: add ref/unref calls

[Paolo Bonzini <pbonzini@redhat.com>]

Il 14/06/2013 12:09, Alexey Kardashevskiy ha scritto:
> Hi.
> 
> Ok. Back to the bug with this patch. The initial problem with this patch is
> that "make check" fails.
> 
> Please help with subpages.
> 
> It turned out that tests use MALLOC_PERTURB_ which is normally off. Who
> does not know - this is a way to tell glibc to fill released memory with
> some value and then debug accesses to released memory. Some bright mind
> made it random what confuses a lot (and btw valgrind found nothing :-/ ).
> So I spend some time before figured out how to reproduce it outside of the
> qtest thingy.
> 
> The tree is qemu.org/master "bd5c51e Michael Roth qemu-char: don't issue
> CHR_EVENT_OPEN in a BH" + replayed patches till the one from $subj on top
> of it. QEMU is configured as "configure --target-list=x86_64-softmmu".
> 
> The magic is:
> 
> export MALLOC_PERTURB_=123
> nc -l -U ~/qtest-16318.sock &
> nc -l -U ~/qtest-16318.qmp &
> x86_64-softmmu/qemu-system-x86_64  \
> 	-qtest unix:/home/alexey/qtest-16318.sock,nowait \
> 	-qtest-log /dev/null \
> 	-qmp unix:/home/alexey/qtest-16318.qmp,nowait \
> 	-pidfile ~/qtest-16318.pid -machine accel=qtest -vnc none
> 
> Immediate crash at (the very last backtrace in this mail is that crash).

It's a use-after-free, strange that valgrind found nothing.  The 
subpage is freed in destroy_page_desc before being used in 
phys_sections_clear.

The fix is to move freeing the phys_sections in phys_sections_clear,
which is also cleaner.

diff --git a/exec.c b/exec.c
index fa5de88..93907e9 100644
--- a/exec.c
+++ b/exec.c
@@ -762,17 +762,6 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
 static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
                              uint16_t section);
 static subpage_t *subpage_init(AddressSpace *as, hwaddr base);
-static void destroy_page_desc(uint16_t section_index)
-{
-    MemoryRegionSection *section = &phys_sections[section_index];
-    MemoryRegion *mr = section->mr;
-
-    if (mr->subpage) {
-        subpage_t *subpage = container_of(mr, subpage_t, iomem);
-        memory_region_destroy(&subpage->iomem);
-        g_free(subpage);
-    }
-}
 
 static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
 {
@@ -787,8 +776,6 @@ static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
     for (i = 0; i < L2_SIZE; ++i) {
         if (!p[i].is_leaf) {
             destroy_l2_mapping(&p[i], level - 1);
-        } else {
-            destroy_page_desc(p[i].ptr);
         }
     }
     lp->is_leaf = 0;
@@ -819,11 +806,20 @@ static uint16_t phys_section_add(MemoryRegionSection *section)
     return phys_sections_nb++;
 }
 
+static void phys_section_destroy(MemoryRegion *mr)
+{
+    memory_region_unref(mr);
+
+    if (mr->subpage) {
+        subpage_t *subpage = container_of(mr, subpage_t, iomem);
+        memory_region_destroy(&subpage->iomem);
+        g_free(subpage);
+    }
+}
+
 static void phys_sections_clear(void)
 {
     while (phys_sections_nb > 0) {
         MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
-        memory_region_unref(section->mr);
+        phys_section_destroy(section->mr);
     }
 }