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

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

[Don Slutz]

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.

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);
-- 
1.8.3.1

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

[Paolo Bonzini]

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.

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);
> 

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

[Gonglei (Arei)]

> 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);
> >

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

[Gonglei (Arei)]

> 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);
> > >

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

[Paolo Bonzini]

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

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 <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
> 

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

[Don Slutz]

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);
>>