From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60567) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4FTA-0006R8-4Z for qemu-devel@nongnu.org; Wed, 02 Dec 2015 17:01:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a4FT6-0003Hl-3y for qemu-devel@nongnu.org; Wed, 02 Dec 2015 17:01:48 -0500 Received: from mail-qg0-x232.google.com ([2607:f8b0:400d:c04::232]:34881) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4FT5-0003Hf-VH for qemu-devel@nongnu.org; Wed, 02 Dec 2015 17:01:44 -0500 Received: by qgec40 with SMTP id c40so45628937qge.2 for ; Wed, 02 Dec 2015 14:01:43 -0800 (PST) From: Don Slutz References: <1448921464-21845-1-git-send-email-Don.Slutz@Gmail.com> <565D6DCE.5010707@redhat.com> <33183CC9F5247A488A2544077AF19020B02AC190@SZXEMA503-MBS.china.huawei.com> <565EBE31.7010304@redhat.com> Message-ID: <565F6A45.2080900@Gmail.com> Date: Wed, 2 Dec 2015 17:01:41 -0500 MIME-Version: 1.0 In-Reply-To: <565EBE31.7010304@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] exec: Stop using memory after free List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , "Gonglei (Arei)" , "qemu-devel@nongnu.org" Cc: Peter Crosthwaite , Richard Henderson 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 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 , 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 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 >