Re: [Qemu-devel] [PATCH] exec: Stop using memory after free

[Don Slutz]

On 12/02/15 04:47, Paolo Bonzini wrote:
> 
> 
> On 02/12/2015 08:59, Gonglei (Arei) wrote:
>>>>>  static void phys_section_destroy(MemoryRegion *mr)  {
>>>>> +    bool have_sub_page = mr->subpage;
>>>>> +
>>>>>      memory_region_unref(mr);
>>>>>
>>>>> -    if (mr->subpage) {
>>>>> +    if (have_sub_page) {
>>>>>          subpage_t *subpage = container_of(mr, subpage_t, iomem);
>>
>> Can we use the *mr* here again?
> 
> Yes, in the subpage case the memory is allocated by exec.c.  Accessing
> mr->subpage is only problematic if memory_region_unref destroys a device.
> 

My memory of debugging this in June, was that when ever subpage is true
the reference count on the mr was greater then (>2?) so that after the
call on memory_region_unref(mr), the reference count on the mr was still
non-zero and so g_free() was not called.

>> IMO we should invoke memory_region_unref(mr) after the if check.
> 
> That's also possible.

My attempts to do so all failed, maybe coding bugs.  At that time I came
up with the patch:

commit 475d53a44933171222516689ae00e7fad5a8bf69
Author: Don Slutz <address@hidden>
Date:   Sat Jun 6 10:13:08 2015 -0400

    exec: Do not use MemoryRegion after free

    Here is gdb output that shows this happening:

      Breakpoint 3, object_finalize (data=0x7fdf32a14010) at
qom/object.c:417
      417             obj->free(obj);
      (gdb) bt
      #0  object_finalize (data=0x7fdf32a14010) at qom/object.c:417
      #1  0x00000000007329d4 in object_unref (obj=0x7fdf32a14010) at
qom/object.c:720
      #2  0x0000000000468a65 in memory_region_unref (mr=0x7fdf32a168b0)
at /home/don/xen/tools/qemu-xen-dir/memory.c:1359
      #3  0x000000000040eb52 in phys_section_destroy (mr=0x7fdf32a168b0)
at /home/don/xen/tools/qemu-xen-dir/exec.c:960
      #4  0x000000000040ec0a in phys_sections_free (map=0x3e51fc8) at
/home/don/xen/tools/qemu-xen-dir/exec.c:973
      #5  0x0000000000411cc9 in address_space_dispatch_free
(d=0x3e51fb0) at /home/don/xen/tools/qemu-xen-dir/exec.c:2133
      #6  0x0000000000840ae2 in call_rcu_thread (opaque=0x0) at
util/rcu.c:256
      #7  0x00000032fdc07d14 in start_thread (arg=0x7fdf34866700) at
pthread_create.c:309
      #8  0x00000032fd4f168d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:115
      (gdb) p obj
      $5 = (Object *) 0x7fdf32a14010
      (gdb) p *obj
      $6 = {class = 0x302f380, free = 0x40a1e0 <address@hidden>, properties
= {tqh_first = 0x0, tqh_last = 0x7fdf32a14020},
      ref = 0, parent = 0x0}
      (gdb) up
      #1  0x00000000007329d4 in object_unref (obj=0x7fdf32a14010) at
qom/object.c:720
      720             object_finalize(obj);
      (gdb) up
      #2  0x0000000000468a65 in memory_region_unref (mr=0x7fdf32a168b0)
at /home/don/xen/tools/qemu-xen-dir/memory.c:1359
      1359            object_unref(obj->parent);
      (gdb) up
      #3  0x000000000040eb52 in phys_section_destroy (mr=0x7fdf32a168b0)
at /home/don/xen/tools/qemu-xen-dir/exec.c:960
      960         memory_region_unref(mr);
      (gdb) l
      955         return map->sections_nb++;
      956     }
      957
      958     static void phys_section_destroy(MemoryRegion *mr)
      959     {
      960         memory_region_unref(mr);
      961
      962         if (mr->subpage) {
      963             subpage_t *subpage = container_of(mr, subpage_t,
iomem);
      964             object_unref(OBJECT(&subpage->iomem));
      (gdb) p mr
      $7 = (MemoryRegion *) 0x7fdf32a168b0
      (gdb) p mr->subpage
      $9 = false
      (gdb) n
      419     }
      (gdb) n
      object_unref (obj=0x7fdf32a14010) at qom/object.c:722
      722     }
      (gdb) n
      memory_region_unref (mr=0x7fdf32a168b0) at
/home/don/xen/tools/qemu-xen-dir/memory.c:1363
      1363    }
      (gdb) n
      phys_section_destroy (mr=0x7fdf32a168b0) at
/home/don/xen/tools/qemu-xen-dir/exec.c:962
      962         if (mr->subpage) {
      (gdb) p mr
      $10 = (MemoryRegion *) 0x7fdf32a168b0
      (gdb) p *mr
      Cannot access memory at address 0x7fdf32a168b0

    Signed-off-by: Don Slutz <address@hidden>

diff --git a/exec.c b/exec.c
index f7883d2..b4be5d9 100644
--- a/exec.c
+++ b/exec.c
@@ -958,10 +958,14 @@ static uint16_t phys_section_add(PhysPageMap *map,

 static void phys_section_destroy(MemoryRegion *mr)
 {
-    memory_region_unref(mr);
+    subpage_t *subpage = NULL;

     if (mr->subpage) {
-        subpage_t *subpage = container_of(mr, subpage_t, iomem);
+        subpage = container_of(mr, subpage_t, iomem);
+    }
+    memory_region_unref(mr);
+
+    if (subpage) {
         object_unref(OBJECT(&subpage->iomem));
         g_free(subpage);
     }

which never got posted do to external events.  When I hit this again, I
was able to remember enough to generate this patch from scratch.  I only
found this patch when reading this email thread.

   -Don Slutz
P.S. I no longer work for verizon, and so the email address above no
longer works.


> 
> Paolo
>