* Re: [Qemu-devel] [PATCH] exec: Stop using memory after free
2015-12-01 9:52 ` Paolo Bonzini
@ 2015-12-01 13:05 ` Gonglei (Arei)
2015-12-02 7:59 ` Gonglei (Arei)
2015-12-02 22:04 ` Don Slutz
2 siblings, 0 replies; 7+ messages in thread
From: Gonglei (Arei) @ 2015-12-01 13:05 UTC (permalink / raw)
To: Paolo Bonzini, Don Slutz, qemu-devel@nongnu.org
Cc: Richard Henderson, Peter Crosthwaite
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>
> On 30/11/2015 23:11, Don Slutz wrote:
> > memory_region_unref(mr) can free memory.
> >
> > For example I got:
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7f43280d4700 (LWP 4462)]
> > 0x00007f43323283c0 in phys_section_destroy (mr=0x7f43259468b0)
> > at /home/don/xen/tools/qemu-xen-dir/exec.c:1023
> > 1023 if (mr->subpage) {
> > (gdb) bt
> > at /home/don/xen/tools/qemu-xen-dir/exec.c:1023
> > at /home/don/xen/tools/qemu-xen-dir/exec.c:1034
> > at /home/don/xen/tools/qemu-xen-dir/exec.c:2205
> > (gdb) p mr
> > $1 = (MemoryRegion *) 0x7f43259468b0
> >
> > And this change prevents this.
>
> Great, thanks! I think this fixes also the problem that Gonglei was seeing a
> few months ago. I'll queue it for 2.5.
>
> BTW, since I have your attention, have you noticed my refresh/rewrite of your
> SAS1068 patches? A review would be welcome.
>
Nice catch! I will check the issue tomorrow.
Thanks for reminding and CC'ing me, Paolo.
Regards,
-Gonglei
> Paolo
>
> > Signed-off-by: Don Slutz <Don.Slutz@Gmail.com>
> > ---
> > exec.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/exec.c b/exec.c
> > index de1cf19..0bf0a6e 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1064,9 +1064,11 @@ static uint16_t phys_section_add(PhysPageMap
> > *map,
> >
> > 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);
> > object_unref(OBJECT(&subpage->iomem));
> > g_free(subpage);
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] exec: Stop using memory after free
2015-12-01 9:52 ` Paolo Bonzini
2015-12-01 13:05 ` Gonglei (Arei)
@ 2015-12-02 7:59 ` Gonglei (Arei)
2015-12-02 9:47 ` Paolo Bonzini
2015-12-02 22:04 ` Don Slutz
2 siblings, 1 reply; 7+ messages in thread
From: Gonglei (Arei) @ 2015-12-02 7:59 UTC (permalink / raw)
To: Gonglei (Arei), Paolo Bonzini, Don Slutz, qemu-devel@nongnu.org
Cc: Richard Henderson, Peter Crosthwaite
> Subject: RE: [PATCH] exec: Stop using memory after free
>
> > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >
> > On 30/11/2015 23:11, Don Slutz wrote:
> > > memory_region_unref(mr) can free memory.
> > >
> > > For example I got:
> > >
> > > Program received signal SIGSEGV, Segmentation fault.
> > > [Switching to Thread 0x7f43280d4700 (LWP 4462)]
> > > 0x00007f43323283c0 in phys_section_destroy (mr=0x7f43259468b0)
> > > at /home/don/xen/tools/qemu-xen-dir/exec.c:1023
> > > 1023 if (mr->subpage) {
> > > (gdb) bt
> > > at /home/don/xen/tools/qemu-xen-dir/exec.c:1023
> > > at /home/don/xen/tools/qemu-xen-dir/exec.c:1034
> > > at /home/don/xen/tools/qemu-xen-dir/exec.c:2205
> > > (gdb) p mr
> > > $1 = (MemoryRegion *) 0x7f43259468b0
> > >
> > > And this change prevents this.
> >
> > Great, thanks! I think this fixes also the problem that Gonglei was
> > seeing a few months ago. I'll queue it for 2.5.
> >
> > BTW, since I have your attention, have you noticed my refresh/rewrite
> > of your
> > SAS1068 patches? A review would be welcome.
> >
>
> Nice catch! I will check the issue tomorrow.
>
> Thanks for reminding and CC'ing me, Paolo.
>
> Regards,
> -Gonglei
>
> > Paolo
> >
> > > Signed-off-by: Don Slutz <Don.Slutz@Gmail.com>
> > > ---
> > > exec.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/exec.c b/exec.c
> > > index de1cf19..0bf0a6e 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1064,9 +1064,11 @@ static uint16_t
> phys_section_add(PhysPageMap
> > > *map,
> > >
> > > 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?
IMO we should invoke memory_region_unref(mr) after the if check.
Regards,
-Gonglei
> > > object_unref(OBJECT(&subpage->iomem));
> > > g_free(subpage);
> > >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] exec: Stop using memory after free
2015-12-02 7:59 ` Gonglei (Arei)
@ 2015-12-02 9:47 ` Paolo Bonzini
2015-12-02 22:01 ` Don Slutz
0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2015-12-02 9:47 UTC (permalink / raw)
To: Gonglei (Arei), Don Slutz, qemu-devel@nongnu.org
Cc: Peter Crosthwaite, Richard Henderson
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.
> IMO we should invoke memory_region_unref(mr) after the if check.
That's also possible.
Paolo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] exec: Stop using memory after free
2015-12-02 9:47 ` Paolo Bonzini
@ 2015-12-02 22:01 ` Don Slutz
0 siblings, 0 replies; 7+ messages in thread
From: Don Slutz @ 2015-12-02 22:01 UTC (permalink / raw)
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 <dslutz@verizon.com>
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 <g_free@plt>, 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 <dslutz@verizon.com>
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
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: Stop using memory after free
2015-12-01 9:52 ` Paolo Bonzini
2015-12-01 13:05 ` Gonglei (Arei)
2015-12-02 7:59 ` Gonglei (Arei)
@ 2015-12-02 22:04 ` Don Slutz
2 siblings, 0 replies; 7+ messages in thread
From: Don Slutz @ 2015-12-02 22:04 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Richard Henderson, Gonglei, Peter Crosthwaite
On 12/01/15 04:52, Paolo Bonzini wrote:
>
>
> On 30/11/2015 23:11, Don Slutz wrote:
>> memory_region_unref(mr) can free memory.
>>
>> For example I got:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> [Switching to Thread 0x7f43280d4700 (LWP 4462)]
>> 0x00007f43323283c0 in phys_section_destroy (mr=0x7f43259468b0)
>> at /home/don/xen/tools/qemu-xen-dir/exec.c:1023
>> 1023 if (mr->subpage) {
>> (gdb) bt
>> at /home/don/xen/tools/qemu-xen-dir/exec.c:1023
>> at /home/don/xen/tools/qemu-xen-dir/exec.c:1034
>> at /home/don/xen/tools/qemu-xen-dir/exec.c:2205
>> (gdb) p mr
>> $1 = (MemoryRegion *) 0x7f43259468b0
>>
>> And this change prevents this.
>
> Great, thanks! I think this fixes also the problem that Gonglei was
> seeing a few months ago. I'll queue it for 2.5.
>
Thanks.
> BTW, since I have your attention, have you noticed my refresh/rewrite of
> your SAS1068 patches? A review would be welcome.
>
It has been on the list of things to do. Since I no longer work for
Verizon, it is now a non-work time event. I also not longer have access
to the testing machines that I had used.
-Don Slutz
> Paolo
>
>> Signed-off-by: Don Slutz <Don.Slutz@Gmail.com>
>> ---
>> exec.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index de1cf19..0bf0a6e 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1064,9 +1064,11 @@ static uint16_t phys_section_add(PhysPageMap *map,
>>
>> 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);
>> object_unref(OBJECT(&subpage->iomem));
>> g_free(subpage);
>>
^ permalink raw reply [flat|nested] 7+ messages in thread