Patchwork [v2,02/11] memory: destroy phys_sections one by one

login
register
mail settings
Submitter Paolo Bonzini
Date June 28, 2013, 4:58 p.m.
Message ID <1372438702-20491-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/255479/
State New
Headers show

Comments

Paolo Bonzini - June 28, 2013, 4:58 p.m.
phys_sections_clear is invoked after the dispatch tree has been
destroyed.  This leaves a window where phys_sections_nb > 0 but the
subpages are not valid anymore, which is a recipe for use-after-free
bugs.

Move the destruction of subpages in phys_sections_clear.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
Jan Kiszka - July 1, 2013, 2:14 p.m.
On 2013-06-28 18:58, Paolo Bonzini wrote:
> phys_sections_clear is invoked after the dispatch tree has been
> destroyed.

You mean destroy_all_mappings called by mem_begin vs.
phys_sections_clear called by core_begin, right?

>  This leaves a window where phys_sections_nb > 0 but the
> subpages are not valid anymore, which is a recipe for use-after-free
> bugs.
> 
> Move the destruction of subpages in phys_sections_clear.

What ensures that we still destroy the subpages when an address space is
cleaned up (address_space_destroy_dispatch)? The fact that an address
space has to be / is de facto empty when destroying it? That question
also applies on the next patch.

Jan

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 1118587..e7eadf5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -762,17 +762,6 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
>  static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
>                               uint16_t section);
>  static subpage_t *subpage_init(AddressSpace *as, hwaddr base);
> -static void destroy_page_desc(uint16_t section_index)
> -{
> -    MemoryRegionSection *section = &phys_sections[section_index];
> -    MemoryRegion *mr = section->mr;
> -
> -    if (mr->subpage) {
> -        subpage_t *subpage = container_of(mr, subpage_t, iomem);
> -        memory_region_destroy(&subpage->iomem);
> -        g_free(subpage);
> -    }
> -}
>  
>  static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
>  {
> @@ -787,8 +776,6 @@ static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
>      for (i = 0; i < L2_SIZE; ++i) {
>          if (!p[i].is_leaf) {
>              destroy_l2_mapping(&p[i], level - 1);
> -        } else {
> -            destroy_page_desc(p[i].ptr);
>          }
>      }
>      lp->is_leaf = 0;
> @@ -818,9 +805,21 @@ static uint16_t phys_section_add(MemoryRegionSection *section)
>      return phys_sections_nb++;
>  }
>  
> +static void phys_section_destroy(MemoryRegion *mr)
> +{
> +    if (mr->subpage) {
> +        subpage_t *subpage = container_of(mr, subpage_t, iomem);
> +        memory_region_destroy(&subpage->iomem);
> +        g_free(subpage);
> +    }
> +}
> +
>  static void phys_sections_clear(void)
>  {
> -    phys_sections_nb = 0;
> +    while (phys_sections_nb > 0) {
> +        MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
> +        phys_section_destroy(section->mr);
> +    }
>  }
>  
>  static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
>
Paolo Bonzini - July 1, 2013, 2:32 p.m.
Il 01/07/2013 16:14, Jan Kiszka ha scritto:
> On 2013-06-28 18:58, Paolo Bonzini wrote:
>> phys_sections_clear is invoked after the dispatch tree has been
>> destroyed.
> 
> You mean destroy_all_mappings called by mem_begin vs.
> phys_sections_clear called by core_begin, right?
> 
>>  This leaves a window where phys_sections_nb > 0 but the
>> subpages are not valid anymore, which is a recipe for use-after-free
>> bugs.
>>
>> Move the destruction of subpages in phys_sections_clear.
> 
> What ensures that we still destroy the subpages when an address space is
> cleaned up (address_space_destroy_dispatch)? The fact that an address
> space has to be / is de facto empty when destroying it?

Yes, the address space is emptied at destruction time.  See

    memory_region_transaction_begin();
    as->root = NULL;
    memory_region_transaction_commit();

in address_space_destroy.

Paolo

> Jan
> 
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  exec.c | 27 +++++++++++++--------------
>>  1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 1118587..e7eadf5 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -762,17 +762,6 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
>>  static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
>>                               uint16_t section);
>>  static subpage_t *subpage_init(AddressSpace *as, hwaddr base);
>> -static void destroy_page_desc(uint16_t section_index)
>> -{
>> -    MemoryRegionSection *section = &phys_sections[section_index];
>> -    MemoryRegion *mr = section->mr;
>> -
>> -    if (mr->subpage) {
>> -        subpage_t *subpage = container_of(mr, subpage_t, iomem);
>> -        memory_region_destroy(&subpage->iomem);
>> -        g_free(subpage);
>> -    }
>> -}
>>  
>>  static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
>>  {
>> @@ -787,8 +776,6 @@ static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
>>      for (i = 0; i < L2_SIZE; ++i) {
>>          if (!p[i].is_leaf) {
>>              destroy_l2_mapping(&p[i], level - 1);
>> -        } else {
>> -            destroy_page_desc(p[i].ptr);
>>          }
>>      }
>>      lp->is_leaf = 0;
>> @@ -818,9 +805,21 @@ static uint16_t phys_section_add(MemoryRegionSection *section)
>>      return phys_sections_nb++;
>>  }
>>  
>> +static void phys_section_destroy(MemoryRegion *mr)
>> +{
>> +    if (mr->subpage) {
>> +        subpage_t *subpage = container_of(mr, subpage_t, iomem);
>> +        memory_region_destroy(&subpage->iomem);
>> +        g_free(subpage);
>> +    }
>> +}
>> +
>>  static void phys_sections_clear(void)
>>  {
>> -    phys_sections_nb = 0;
>> +    while (phys_sections_nb > 0) {
>> +        MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
>> +        phys_section_destroy(section->mr);
>> +    }
>>  }
>>  
>>  static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)
>>
>

Patch

diff --git a/exec.c b/exec.c
index 1118587..e7eadf5 100644
--- a/exec.c
+++ b/exec.c
@@ -762,17 +762,6 @@  hwaddr memory_region_section_get_iotlb(CPUArchState *env,
 static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
                              uint16_t section);
 static subpage_t *subpage_init(AddressSpace *as, hwaddr base);
-static void destroy_page_desc(uint16_t section_index)
-{
-    MemoryRegionSection *section = &phys_sections[section_index];
-    MemoryRegion *mr = section->mr;
-
-    if (mr->subpage) {
-        subpage_t *subpage = container_of(mr, subpage_t, iomem);
-        memory_region_destroy(&subpage->iomem);
-        g_free(subpage);
-    }
-}
 
 static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
 {
@@ -787,8 +776,6 @@  static void destroy_l2_mapping(PhysPageEntry *lp, unsigned level)
     for (i = 0; i < L2_SIZE; ++i) {
         if (!p[i].is_leaf) {
             destroy_l2_mapping(&p[i], level - 1);
-        } else {
-            destroy_page_desc(p[i].ptr);
         }
     }
     lp->is_leaf = 0;
@@ -818,9 +805,21 @@  static uint16_t phys_section_add(MemoryRegionSection *section)
     return phys_sections_nb++;
 }
 
+static void phys_section_destroy(MemoryRegion *mr)
+{
+    if (mr->subpage) {
+        subpage_t *subpage = container_of(mr, subpage_t, iomem);
+        memory_region_destroy(&subpage->iomem);
+        g_free(subpage);
+    }
+}
+
 static void phys_sections_clear(void)
 {
-    phys_sections_nb = 0;
+    while (phys_sections_nb > 0) {
+        MemoryRegionSection *section = &phys_sections[--phys_sections_nb];
+        phys_section_destroy(section->mr);
+    }
 }
 
 static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *section)