[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] exec: Stop using memory after free
From: |
Don Slutz |
Subject: |
Re: [Qemu-devel] [PATCH] exec: Stop using memory after free |
Date: |
Wed, 2 Dec 2015 17:01:41 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
>