| Message ID | 1448921464-21845-1-git-send-email-Don.Slutz@Gmail.com |
|---|---|
| State | New |
| Headers | show |
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); >
> 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); > >
> 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); > > >
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
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); >>
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);
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(-)