[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 02/11] memory: destroy phys_sections one by o
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 02/11] memory: destroy phys_sections one by one |
Date: |
Mon, 01 Jul 2013 16:32:12 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
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 <address@hidden>
>> ---
>> 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)
>>
>